[Intel-gfx] [PATCH 4/4] drm/i915: Flush stale cachelines on set-cache-level

[Daniel Vetter]

On Tue, Nov 12, 2019 at 10:43 AM Chris Wilson <chris at chris-wilson.co.uk> wrote:
>
> Quoting Daniel Vetter (2019-11-12 09:09:06)
> > On Thu, Jul 18, 2019 at 4:54 PM Chris Wilson <chris at chris-wilson.co.uk> wrote:
> > >
> > > Ensure that we flush any cache dirt out to main memory before the user
> > > changes the cache-level as they may elect to bypass the cache (even after
> > > declaring their access cache-coherent) via use of unprivileged MOCS.
> > >
> > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > > Cc: stable at vger.kernel.org
> > > ---
> > >  drivers/gpu/drm/i915/gem/i915_gem_domain.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > index 2e3ce2a69653..5d41e769a428 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > @@ -277,6 +277,11 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > >
> > >         list_for_each_entry(vma, &obj->vma.list, obj_link)
> > >                 vma->node.color = cache_level;
> > > +
> > > +       /* Flush any previous cache dirt in case of cache bypass */
> > > +       if (obj->cache_dirty & ~obj->cache_coherent)
> > > +               i915_gem_clflush_object(obj, I915_CLFLUSH_SYNC);
> >
> > I think writing out the bit logic instead of implicitly relying on the
> > #defines would be much better, i.e. && !(cache_coherent &
> > COHERENT_FOR_READ). Plus I think we only need to set cache_dirty =
> > true if we don't flush here already, to avoid double flushing?
>
> No. The mask is being updated, so you need to flush before you lose
> track. The cache is then cleared of the dirty bit so won't be flushed
> again until dirty and no longer coherent. We need to flag that the page
> is no longer coherent at the end of its lifetime (passing back to the
> system) to force the flush then.

Hm I think I overlooked that we only clear cache_dirty in
i915_gem_clflush_object when it's a coherent mode.

I also spotted more cases for (obj->cache_dirty
&~obj->cache_coherent), so that obscure/fragile pattern is
pre-existing :-/ One of them also checks outside of the object lock,
which I think is how these states are supposed to be protected. Smells
a bit fishy still, would be good to make a bit clearer.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch