-----Original Message-----
From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of David Gow
Sent: Sunday, August 4, 2024 2:19 AM
To: Christian König <ckoenig.leichtzumerken@xxxxxxxxx>; Somalapuram Amaranath <Amaranath.Somalapuram@xxxxxxx>; Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>; De Marchi, Lucas <lucas.demarchi@xxxxxxxxx>; Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>; Thomas Zimmermann <tzimmermann@xxxxxxx>
Cc: David Gow <david@xxxxxxxxxxxx>; Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>; Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>; Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; Tvrtko Ursulin <tursulin@xxxxxxxxxxx>; Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
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@xxxxxxxxxxxx>
Thank you for the thorough write-up of this issue.
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
-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
>
>
