On Tue, 14 Jun 2022 at 02:14, Adrian Larumbe <adrian.larumbe@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> > --- > 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 >