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

[Daniel Vetter]

On Tue, Nov 12, 2019 at 11:57 AM Daniel Vetter <daniel at ffwll.ch> wrote:
>
> 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.

Hm, the clear/blt code recently merged doesn't preserve the
->cache_dirty setting for this case, unlike clfush_object. Do we have
a bug there?

> 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