Re: [PATCH 06/13] drm/i915/bdw: implement semaphore signal

[Ben Widawsky <benjamin.widawsky@intel.com>]

On Tue, Feb 11, 2014 at 10:23:38PM +0000, Chris Wilson wrote:
> On Tue, Feb 11, 2014 at 01:48:22PM -0800, Ben Widawsky wrote:
> > On Thu, Jan 30, 2014 at 01:35:41PM +0000, Chris Wilson wrote:
> > > On Thu, Jan 30, 2014 at 02:18:32PM +0100, Daniel Vetter wrote:
> > > > On Thu, Jan 30, 2014 at 1:46 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > > Oh. So they changed how post-sync writes operated - this should be a
> > > > > separate fix for stable I believe (so that batches are not run before we
> > > > > have finished invalidating the TLBs required).
> > > > 
> > > > We have an igt to exercise tlb invalidation stuff, which runs on all
> > > > rings. But it only runs a batch, so only uses the CS tlb. Do we need
> > > > to extend this?
> > > 
> > > So the spec says:
> > > 
> > > Pipe Control Flush Enable (IVB+)
> > > If ENABLED, the PIPE_CONTROL command will wait until all previous writes
> > > of immediate data from post sync circles are complete before executing
> > > the next command.
> > > 
> > > Post Sync Operation
> > > This field specifies an optional action to be taken upon completion of
> > > the synchronization operation.
> > > 
> > > TLB Invalidate
> > > If ENABLED, all TLBs belonging to Render Engine will be invalidated once
> > > the flush operation is complete.
> > > 
> > > Command Streamer Stall Enable
> > > If ENABLED, the sync operation will not occur until all previous flush
> > > operations pending a completion of those previous flushes will complete,
> > > including the flush produced from this command. This enables the command
> > > to act similar to the legacy MI_FLUSH command.
> > > 
> > > Going by that, the order is
> > > 
> > > flush, stall, TLB invalidate / post-sync op, [pipe control flush]
> > > 
> > > Based on my reading of the above (which unless someone has a more
> > > definitive source) says that without the CONTROL_FLUSH_ENABLE, the CS
> > > can continue operations as soon as the flush is complete - in parallel
> > > to the TLB invalidate. Adding CONTROL_FLUSH_ENABLE would then stall the
> > > CS until the post-sync operation completes. That still leaves the
> > > possibility that the TLB invalidate is being performed in parallel and
> > > is itself provides no CS sync.
> > > -Chris
> > > 
> > > -- 
> > > Chris Wilson, Intel Open Source Technology Centre
> > 
> > so.... what the verdict?
> 
> Gut feeling is that it fixes an issue with IVB TLB invalidate.
> (Not yet sure if the bug I was looking at was accidentally fixed at the
> same time as testing this.)
> So cc stable@
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

You still want a separate patch?

-- 
Ben Widawsky, Intel Open Source Technology Center