Re: [PATCH v2 2/2] drm/i915/bxt: Fix inadvertent CPU snooping due to incorrect MOCS config

[Chris Wilson <chris@chris-wilson.co.uk>]

On Tue, Apr 26, 2016 at 04:43:10PM +0300, Imre Deak wrote:
> On ti, 2016-04-26 at 14:23 +0100, Chris Wilson wrote:
> > On Tue, Apr 26, 2016 at 04:17:55PM +0300, Imre Deak wrote:
> > > On ti, 2016-04-26 at 13:57 +0100, Chris Wilson wrote:
> > > > On Tue, Apr 26, 2016 at 03:44:22PM +0300, Imre Deak wrote:
> > > > > Setting a write-back cache policy in the MOCS entry definition
> > > > > also
> > > > > implies snooping, which has a considerable overhead. This is
> > > > > unexpected for a few reasons:
> > > > 
> > > > If it is snooping, then I don't see why it is undesirable to have
> > > > it
> > > > available in a mocs setting. If it is bogus and the bit is
> > > > undefined,
> > > > then by all means remove it.
> > > 
> > > None of these entries are used alone for coherent surfaces. For
> > > that
> > > the application would have to use entry index#1 or #2 _and_ call
> > > the
> > > set caching IOCTL to set the corresponding buffer to be cached.
> > 
> > No, the application doesn't. There are sufficent interfaces exposed
> > that
> > userspace can bypass the kernel if it so desired.
> > 
> > > The
> > > problem is that without setting the buffer to be cacheable the
> > > expectation is that we won't be snooping and incur the
> > > corresponding
> > > overhead. This is what this patch addresses.
> > 
> > Not true.
> 
> This is what we get running basic tests with current Mesa and UFO
> drivers. They use entry #2 and do not expect snooping and the incurred
> overhead. If they wanted a coherent surface they would have to call the
> set caching IOCTL at least on VLV/CHV since on those platforms you
> don't have a snooping flag in the MOCS, you have to set up the PTE
> accordingly.

That doesn't imply anything about a new interface for bxt. This feature
allows userspace to mix snooping, cached, and uncached access to the
same surface within the same batch. And userspace is able to read that
information coherently without having to tell the kernel its
set-caching.

> > > The bit is also bogus, if we wanted snooping via MOCS we'd use the
> > > dedicated HW flag for that.
> > 
> > But you keep saying this bit *enables* snooping. So either it does or
> > it doesn't.
> 
> It enables snooping but that's just a side effect. WB certainly doesn't
> make sense for BXT since this WB flag controls the cacheability in
> (e)LLC which doesn't exist on BXT. We'd use the 'snooping' HW MOCS flag
> for this purpose.
> 
> > > If we wanted to have a snooping MOCS entry we should add that
> > > separately (as a forth entry), but we'd need this change as a fix
> > > for
> > > current users.
> > 
> > The current users who are getting what they request but don't know
> > what they were requesting?
> 
> They were requesting a non-coherent surface via MOCS entry #2.

But the kernel was advertising a coherent surface via MOCS entry #2. Is
not the code just cut'n'paste from LLC. It's not like mesa has a good
history of supporting !llc at all.

If the patch is make #2 == #0, then seriously there is no point as we
already advertise that mocs setting in #0. I'd rather have the entry
fixed to work as advertised.

At the moment your argument is just that userspace is shooting itself in
the foot.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx