-----Original Message----- From: Intel-gfx <[email protected]> On Behalf Of David Gow Sent: Sunday, August 4, 2024 2:19 AM To: Christian König <[email protected]>; Somalapuram Amaranath <[email protected]>; Thomas Hellström <[email protected]>; De Marchi, Lucas <[email protected]>; Andi Shyti <[email protected]>; Thomas Zimmermann <[email protected]> Cc: David Gow <[email protected]>; Jani Nikula <[email protected]>; Joonas Lahtinen <[email protected]>; Vivi, Rodrigo <[email protected]>; Tvrtko Ursulin <[email protected]>; Ville Syrjälä <[email protected]>; [email protected]; [email protected]; [email protected] 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 <[email protected]>
Thank you for the thorough write-up of this issue. Reviewed-by: Jonathan Cavitt <[email protected]> -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 > >
