[Intel-gfx] [PATCH 2/3] drm/i915/ttm: don't overwrite cache_dirty after setting coherency

[Matthew Auld]

On Tue, 14 Jun 2022 at 02:14, Adrian Larumbe
<adrian.larumbe at collabora.com> wrote:
>
> When i915_gem_object_set_cache_level sets the GEM object's cache_dirty to
> true, in the case of TTM that will sometimes be overwritten when getting
> the object's pages, more specifically for shmem-placed objects for which
> its ttm structure has just been populated.
>
> This wasn't an issue so far, even though intel_dpt_create was setting the
> object's cache level to 'none', regardless of the platform and memory
> placement of the framebuffer. However, commit 2f0ec95ed20c ("drm/i915/ttm:
> dont trample cache_level overrides during ttm move") makes sure the cache
> level set by older backends soon to be managed by TTM isn't altered after
> their TTM bo ttm structure is populated.
>
> However this led to the 'obj->cache_dirty = true' set in
> i915_gem_object_set_cache_level to stick around rather than being reset
> inside i915_ttm_adjust_gem_after_move after calling ttm_tt_populate in
> __i915_ttm_get_pages, which eventually led to a warning in DGFX platforms.
>
> There also seems to be no need for this statement to be kept in
> i915_gem_object_set_cache_level, since i915_gem_object_set_cache_coherency
> is already taking care of it, and also considering whether it's a discrete
> platform.
>
> Remove statement altogether.
>
> Signed-off-by: Adrian Larumbe <adrian.larumbe at collabora.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_domain.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> index 3e5d6057b3ef..b2c9e16bfa55 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> @@ -273,10 +273,8 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>                 return ret;
>
>         /* Always invalidate stale cachelines */
> -       if (obj->cache_level != cache_level) {
> +       if (obj->cache_level != cache_level)
>                 i915_gem_object_set_cache_coherency(obj, cache_level);
> -               obj->cache_dirty = true;

Maybe ban calling this on dgpu or have it fail silently? At the ioctl
level this should already be banned.

Ignoring dgpu, the cache_dirty handling is quite thorny on non-LLC
platforms. I'm not sure if there are other historical reasons for
having this here, but one big issue is that we are not allowed to
freely set cache_dirty = false, unless we are certain that the pages
underneath have been populated and the potential flush-on-acquire
completed. See the kernel-doc for @cache_dirty for more details.

> -       }
>
>         /* The cache-level will be applied when each vma is rebound. */
>         return i915_gem_object_unbind(obj,
> --
> 2.36.1
>