Re: [PATCH 9/9] drm/i915: Flush caches for scanout during cpu->gtt move

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Nov 25, 2013 at 04:40:22PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 25, 2013 at 11:04:48AM +0000, Chris Wilson wrote:
> > On Mon, Nov 25, 2013 at 09:47:28AM +0100, Daniel Vetter wrote:
> > > On Thu, Nov 21, 2013 at 11:20:58PM +0000, Chris Wilson wrote:
> > > > On Thu, Nov 21, 2013 at 09:29:53PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > Flush the caches when moving a scanout buffer from CPU to GTT domain.
> > > > > This allows us to move a scanout buffer to CPU write domain, do some
> > > > > writes, and move it back to the GTT read domain. The display will then
> > > > > see the correct data. In addition we still need to do the dirtyfb
> > > > > ioctl to nuke FBC if that's enabled.
> > > > > 
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > 
> > > Isn't this what sw_finish is for?
> > 
> > I was more concerned about making sure the code was reasonably
> > self-consistent in applying our own coherency rules. As you point out,
> > it may be userspace making a mistake, but so many paths can end up here,
> > and almost never have pin_display, that it would seem to be preferrable
> > to do the extra flush rather than have the discrepancy.
> 
> If we don't do the flush in CPU->GTT move, we might miss it completely.
> Eg. if we do things in this order:
> 
> set_domain(CPU,CPU)
> write some data
> set_domain(GTT,0)

The question is: Who's doing the set_domain? Atm the sw_finish is done as
part of the libdrm unmap interface, so if userspace is wreaking havoc by
interleaving a set_domain here then that's Just a Bug.

If otoh the kernel is doing a set_domain then that means userspace is
racing access with some other thread concurrently and again gets to keep
both pieces.

We obviously need to keep things working, but beding over backwards to
make even insane cases work is imo not a good approach: The resulting
overkill tends to duct-tape over real issues which then hurt us
long-term. I much prefer a simple model for all the bo access paths we
have (and want to support) and shrug all the insane cases off with a
"you're doing it wrong".

Cheers, Daniel

> sw_finish()
> 
> sw_finish would not do the flush since the object is no longer in the
> CPU write domain.
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch