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

From: Daniel Vetter <daniel@ffwll.ch>
To: Eero Tamminen <eero.t.tamminen@intel.com>
Cc: Michael T Frederick <michael.t.frederick@intel.com>,
	Valtteri Rantala <valtteri.rantala@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 2/2] drm/i915/bxt: Fix inadvertent CPU snooping due to incorrect MOCS config
Date: Tue, 26 Apr 2016 16:30:05 +0200	[thread overview]
Message-ID: <20160426143005.GT8291@phenom.ffwll.local> (raw)
In-Reply-To: <571F7AA3.8080509@intel.com>

On Tue, Apr 26, 2016 at 05:26:43PM +0300, Eero Tamminen wrote:
> Hi,
> 
> On 26.04.2016 16:23, 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.
> >
> >>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.
> >
> >>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?
> 
> What this kernel ABI (index entry #2) has been agreed & documented to
> provide?
> 
> I thought this entry is supposed to replace the writeback LLC/eLLC cache
> MOCS setting Mesa is using on (e.g. BDW) to speed up accesses to a memory
> area which it knows always to be accessed so that it can be cached.
> 
> If app runs on HW where LLC/eLLC is missing, giving the app extra slowdown
> instead of potential speedup sounds like failed HW abstraction. :-)

Well mesa needs to know llc vs. !llc anyway to not totally suck, and
defining entry #2 as "coherent, always" makes sense. I thought entry 0 was
the reaonable default aka pte passthrough and hence managed by kernel?

If mesa asks for nonsense, the kernel is happy to oblige.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx