[PATCH 0/2] drm/i915: Fix ttm small BAR placement handling

[PATCH 0/2] drm/i915: Fix ttm small BAR placement handling

[David Gow]

From: David Gow <david@ingeniumdigital.com>

As described in [1], there have been a couple of regressions in the TTM
placement handling for i915, which adversely affect DG2 systems with
small BAR.  In particular, performance become very poor when eviction
from the mappable BAR memory is required, as suboptimal placements can
be preferred, leading to thrashing. This often leads to hangs of >10s,
during which even the compositor is unusable.

These regressions were largely introduced during the flag rework in
commit a78a8da51b36 ("drm/ttm: replace busy placement with flags v6").

The first patch has already been sent out[2]. I'm resending it as part
of this series which fixes both known regressions.

Thanks to Justin Brewer for bisecting the issue.

Cheers,
-- David

[1]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11255
[2]: https://lore.kernel.org/dri-devel/20240722074540.15295-1-david@davidgow.net/

---

David Gow (2):
  drm/i915: Allow evicting to use the requested placement
  drm/i915: Attempt to get pages without eviction first

 drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

-- 
2.46.0

[PATCH 1/2] drm/i915: Allow evicting to use the requested placement

[David Gow]

In commit a78a8da51b36 ("drm/ttm: replace busy placement with flags v6"),
the old system of having a separate placement list (for placements
which should be used without eviction) and a 'busy' placement list (for
placements which should be attempted if eviction is required) was
replaced with a new one where placements could be marked 'FALLBACK' (to
be attempted if eviction is required) or 'DESIRED' (to be attempted
first, but not if eviction is required).

i915 had always included the requested placement in the list of
'busy' placements: i.e., the placement could be used either if eviction
is required or not. But when the new system was put in place, the
requested (first) placement was marked 'DESIRED', so would never be used
if eviction became necessary. While a bug in the original commit
prevented this flag from working, when this was fixed in
4a0e7b3c ("drm/i915: fix applying placement flag"), it caused long hangs
on DG2 systems with small BAR.

Don't mark the requested placement DESIRED (or FALLBACK), allowing it to
be used in both situations. This matches the old behaviour, and resolves
the hangs.

Thanks to Justin Brewer for bisecting the issue.

Fixes: a78a8da51b36 ("drm/ttm: replace busy placement with flags v6")
Fixes: 4a0e7b3c3753 ("drm/i915: fix applying placement flag")
Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11255
Signed-off-by: David Gow <david@davidgow.net>
---
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index e6f177183c0f..fb848fd8ba15 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -165,7 +165,6 @@ i915_ttm_placement_from_obj(const struct drm_i915_gem_object *obj,
 	i915_ttm_place_from_region(num_allowed ? obj->mm.placements[0] :
 				   obj->mm.region, &places[0], obj->bo_offset,
 				   obj->base.size, flags);
-	places[0].flags |= TTM_PL_FLAG_DESIRED;
 
 	/* Cache this on object? */
 	for (i = 0; i < num_allowed; ++i) {
-- 
2.46.0

RE: [PATCH 1/2] drm/i915: Allow evicting to use the requested placement

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of David Gow
Sent: Sunday, August 4, 2024 2:19 AM
To: Christian König <ckoenig.leichtzumerken@gmail.com>; Somalapuram Amaranath <Amaranath.Somalapuram@amd.com>; Thomas Hellström <thomas.hellstrom@linux.intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Andi Shyti <andi.shyti@linux.intel.com>; Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Gow <david@davidgow.net>; Jani Nikula <jani.nikula@linux.intel.com>; Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Tvrtko Ursulin <tursulin@ursulin.net>; Ville Syrjälä <ville.syrjala@linux.intel.com>; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org
Subject: [PATCH 1/2] drm/i915: Allow evicting to use the requested placement
> 
> In commit a78a8da51b36 ("drm/ttm: replace busy placement with flags v6"),
> the old system of having a separate placement list (for placements
> which should be used without eviction) and a 'busy' placement list (for
> placements which should be attempted if eviction is required) was
> replaced with a new one where placements could be marked 'FALLBACK' (to
> be attempted if eviction is required) or 'DESIRED' (to be attempted
> first, but not if eviction is required).
> 
> i915 had always included the requested placement in the list of
> 'busy' placements: i.e., the placement could be used either if eviction
> is required or not. But when the new system was put in place, the
> requested (first) placement was marked 'DESIRED', so would never be used
> if eviction became necessary. While a bug in the original commit
> prevented this flag from working, when this was fixed in
> 4a0e7b3c ("drm/i915: fix applying placement flag"), it caused long hangs
> on DG2 systems with small BAR.
> 
> Don't mark the requested placement DESIRED (or FALLBACK), allowing it to
> be used in both situations. This matches the old behaviour, and resolves
> the hangs.
> 
> Thanks to Justin Brewer for bisecting the issue.
> 
> Fixes: a78a8da51b36 ("drm/ttm: replace busy placement with flags v6")
> Fixes: 4a0e7b3c3753 ("drm/i915: fix applying placement flag")
> Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11255
> Signed-off-by: David Gow <david@davidgow.net>

Thank you for the thorough write-up of this issue.
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
-Jonathan Cavitt
 
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index e6f177183c0f..fb848fd8ba15 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -165,7 +165,6 @@ i915_ttm_placement_from_obj(const struct drm_i915_gem_object *obj,
>  	i915_ttm_place_from_region(num_allowed ? obj->mm.placements[0] :
>  				   obj->mm.region, &places[0], obj->bo_offset,
>  				   obj->base.size, flags);
> -	places[0].flags |= TTM_PL_FLAG_DESIRED;
>  
>  	/* Cache this on object? */
>  	for (i = 0; i < num_allowed; ++i) {
> -- 
> 2.46.0
> 
> 

[PATCH 2/2] drm/i915: Attempt to get pages without eviction first

[David Gow]

In commit a78a8da51b36 ("drm/ttm: replace busy placement with flags v6"),
__i915_ttm_get_pages was updated to use flags instead of the separate
'busy' placement list. However, the behaviour was subtly changed.
Originally, the function would attempt to use the preferred placement
without eviction, and give an opportunity to restart the operation
before falling back to allowing eviction.

This was unintentionally changed, as the preferred placement was not
given the TTM_PL_FLAG_DESIRED flag, and so eviction could be triggered
in that first pass. This caused thrashing, and a significant performance
regression on DG2 systems with small BAR. For example, Minecraft and
Team Fortress 2 would drop to single-digit framerates.

Restore the original behaviour by marking the initial placement as
desired on that first attempt. Also, rework this to use a separate
struct ttm_palcement, as the individual placements are marked 'const',
so hot-patching the flags is even more dodgy than before.

Thanks to Justin Brewer for bisecting this.

Fixes: a78a8da51b36 ("drm/ttm: replace busy placement with flags v6")
Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11255
Signed-off-by: David Gow <david@davidgow.net>
---
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index fb848fd8ba15..5c72462d1f57 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -778,13 +778,16 @@ static int __i915_ttm_get_pages(struct drm_i915_gem_object *obj,
 		.interruptible = true,
 		.no_wait_gpu = false,
 	};
-	int real_num_busy;
+	struct ttm_placement initial_placement;
+	struct ttm_place initial_place;
 	int ret;
 
 	/* First try only the requested placement. No eviction. */
-	real_num_busy = placement->num_placement;
-	placement->num_placement = 1;
-	ret = ttm_bo_validate(bo, placement, &ctx);
+	initial_placement.num_placement = 1;
+	memcpy(&initial_place, placement->placement, sizeof(struct ttm_place));
+	initial_place.flags |= TTM_PL_FLAG_DESIRED;
+	initial_placement.placement = &initial_place;
+	ret = ttm_bo_validate(bo, &initial_placement, &ctx);
 	if (ret) {
 		ret = i915_ttm_err_to_gem(ret);
 		/*
@@ -799,7 +802,6 @@ static int __i915_ttm_get_pages(struct drm_i915_gem_object *obj,
 		 * If the initial attempt fails, allow all accepted placements,
 		 * evicting if necessary.
 		 */
-		placement->num_placement = real_num_busy;
 		ret = ttm_bo_validate(bo, placement, &ctx);
 		if (ret)
 			return i915_ttm_err_to_gem(ret);
-- 
2.46.0

RE: [PATCH 2/2] drm/i915: Attempt to get pages without eviction first

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of David Gow
Sent: Sunday, August 4, 2024 2:19 AM
To: Christian König <ckoenig.leichtzumerken@gmail.com>; Somalapuram Amaranath <Amaranath.Somalapuram@amd.com>; Thomas Hellström <thomas.hellstrom@linux.intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Andi Shyti <andi.shyti@linux.intel.com>; Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Gow <david@davidgow.net>; Jani Nikula <jani.nikula@linux.intel.com>; Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Tvrtko Ursulin <tursulin@ursulin.net>; Ville Syrjälä <ville.syrjala@linux.intel.com>; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org
Subject: [PATCH 2/2] drm/i915: Attempt to get pages without eviction first
> 
> In commit a78a8da51b36 ("drm/ttm: replace busy placement with flags v6"),
> __i915_ttm_get_pages was updated to use flags instead of the separate
> 'busy' placement list. However, the behaviour was subtly changed.
> Originally, the function would attempt to use the preferred placement
> without eviction, and give an opportunity to restart the operation
> before falling back to allowing eviction.
> 
> This was unintentionally changed, as the preferred placement was not
> given the TTM_PL_FLAG_DESIRED flag, and so eviction could be triggered
> in that first pass. This caused thrashing, and a significant performance
> regression on DG2 systems with small BAR. For example, Minecraft and
> Team Fortress 2 would drop to single-digit framerates.
> 
> Restore the original behaviour by marking the initial placement as
> desired on that first attempt. Also, rework this to use a separate
> struct ttm_palcement, as the individual placements are marked 'const',
> so hot-patching the flags is even more dodgy than before.
> 
> Thanks to Justin Brewer for bisecting this.
> 
> Fixes: a78a8da51b36 ("drm/ttm: replace busy placement with flags v6")
> Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11255
> Signed-off-by: David Gow <david@davidgow.net>

Thank you for the thorough write-up of this issue.
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
-Jonathan Cavitt

> ---
>  drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index fb848fd8ba15..5c72462d1f57 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -778,13 +778,16 @@ static int __i915_ttm_get_pages(struct drm_i915_gem_object *obj,
>  		.interruptible = true,
>  		.no_wait_gpu = false,
>  	};
> -	int real_num_busy;
> +	struct ttm_placement initial_placement;
> +	struct ttm_place initial_place;
>  	int ret;
>  
>  	/* First try only the requested placement. No eviction. */
> -	real_num_busy = placement->num_placement;
> -	placement->num_placement = 1;
> -	ret = ttm_bo_validate(bo, placement, &ctx);
> +	initial_placement.num_placement = 1;
> +	memcpy(&initial_place, placement->placement, sizeof(struct ttm_place));
> +	initial_place.flags |= TTM_PL_FLAG_DESIRED;
> +	initial_placement.placement = &initial_place;
> +	ret = ttm_bo_validate(bo, &initial_placement, &ctx);
>  	if (ret) {
>  		ret = i915_ttm_err_to_gem(ret);
>  		/*
> @@ -799,7 +802,6 @@ static int __i915_ttm_get_pages(struct drm_i915_gem_object *obj,
>  		 * If the initial attempt fails, allow all accepted placements,
>  		 * evicting if necessary.
>  		 */
> -		placement->num_placement = real_num_busy;
>  		ret = ttm_bo_validate(bo, placement, &ctx);
>  		if (ret)
>  			return i915_ttm_err_to_gem(ret);
> -- 
> 2.46.0
> 
> 

Re: [PATCH 0/2] drm/i915: Fix ttm small BAR placement handling

[Andi Shyti]

Hi David,

thanks for the patches and thanks Justin for debugging the issue.

I reviewed and merged it into drm-intel-gt-next.

Thanks,
Andi

On Sun, Aug 04, 2024 at 05:18:46PM +0800, David Gow wrote:
> From: David Gow <david@ingeniumdigital.com>
> 
> As described in [1], there have been a couple of regressions in the TTM
> placement handling for i915, which adversely affect DG2 systems with
> small BAR.  In particular, performance become very poor when eviction
> from the mappable BAR memory is required, as suboptimal placements can
> be preferred, leading to thrashing. This often leads to hangs of >10s,
> during which even the compositor is unusable.
> 
> These regressions were largely introduced during the flag rework in
> commit a78a8da51b36 ("drm/ttm: replace busy placement with flags v6").
> 
> The first patch has already been sent out[2]. I'm resending it as part
> of this series which fixes both known regressions.
> 
> Thanks to Justin Brewer for bisecting the issue.
> 
> Cheers,
> -- David
> 
> [1]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11255
> [2]: https://lore.kernel.org/dri-devel/20240722074540.15295-1-david@davidgow.net/
> 
> ---
> 
> David Gow (2):
>   drm/i915: Allow evicting to use the requested placement
>   drm/i915: Attempt to get pages without eviction first
> 
>  drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> -- 
> 2.46.0