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

[Matthew Auld]

On Tue, 28 Jun 2022 at 22:11, Robert Beckett <bob.beckett at collabora.com> wrote:
>
>
>
> On 14/06/2022 18:55, Matthew Auld wrote:
> > 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.
>
> given the commit "068b1bd09253 drm/i915: stop setting cache_dirty on
> discrete"
> with it's justification of cache_dirty should not be set on discreet as
> it is not needed, I think this patch should change to set
>
> obj->cache_dirty = !IS_DGFX(to_i915(obj->base.dev));

Yeah, seems reasonable to me.

>
> along with the assignment in flush_write_domain()

I think this one should already be covered by the check in
gpu_write_needs_flush().

>
> and should be considered a fix for that patch.
>
> It should keep the asignment for integrated as it's original purpose
> still holds there.
>
>
>
> >
> >> -       }
> >>
> >>          /* The cache-level will be applied when each vma is rebound. */
> >>          return i915_gem_object_unbind(obj,
> >> --
> >> 2.36.1
> >>