[Intel-gfx] [PATCH] drm/i915: GFX_MODE Flush TLB Invalidate Mode must be '1' for scanline waits

[Chris Wilson]

On Sun, Jan 20, 2013 at 07:36:39PM -0800, Ben Widawsky wrote:
> On Sun, Jan 20, 2013 at 04:33:32PM +0000, Chris Wilson wrote:
> > On SNB, if bit 13 of GFX_MODE, Flush TLB Invalidate Mode, is not set to 1,
> > the hardware can not program the scanline values. Those scanline values
> > then control when the signal is sent from the display engine to the render
> > ring for MI_WAIT_FOR_EVENTs. Note setting this bit means that TLB
> > invalidations must be performed explicitly through the appropriate bits
> > being set in PIPE_CONTROL.
> > 
> > References: https://bugzilla.kernel.org/show_bug.cgi?id=52311
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: stable at vger.kernel.org
> 
> This all sounds a bit hand-wavy to me. First, it's not clear from the
> commit message if this actually fixes anything. Is that connection
> between bit 13 of GFX_MODE and the scanline updates documented, or was
> it just "found." If it was found we should get a doc update, or
> clarification, because I can't find that in the docs, and perhaps more
> importantly, I can't even come up with a theory why the TLB would have
> any effect on the problem.

It was copied almost verbatim from a tiny little note in the updated
SNB bspecs for the magic DISPLAY LOAD SCANLINES register.
 
> OTOH, I've always disliked that this bit wasn't explicitly set. As a
> note there, we have a context workaround you could update as a result of
> this patch.
> 
> My only real concern is over old userspace with this patch. Were there
> any released ddx, or mesa which didn't set the bit on the PIPE_CONTROL?
> Does anyone care? It'd be nice if we had some way to observe the TLBs
> weren't being invalidated as needed.

No, TLB invalidation is always a kernel problem as it is only required on
moving a buffer into the GPU domain after PTE updates. Userspace is only
concerned about ensuring that the contents of the render and texture
caches are coherent within a batch.
 
> If you can address my concerns over breaking old userspace, and don't
> feel like addressing my other comments, it is (and the next patch):
> Reviewed-by: Ben Widawsky <ben at bwidawsk.net>

Hopefully cleared that up,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre