[Intel-gfx] [PATCH 2/4] drm/i915: Fix serialisation of pipecontrol write vs semaphore signal

[Chris Wilson]

On Thu, Apr 14, 2016 at 05:29:31PM +0300, Ville Syrjälä wrote:
> On Mon, Apr 11, 2016 at 10:06:28AM +0100, Chris Wilson wrote:
> > On Mon, Apr 11, 2016 at 11:34:54AM +0300, Ville Syrjälä wrote:
> > > On Sat, Apr 09, 2016 at 11:14:44AM +0100, Chris Wilson wrote:
> > > > In order for the MI_SEMAPHORE_SIGNAL command to wait until after the
> > > > pipecontrol writing the signal value is complete, we have to pause the
> > > > CS inside the PIPE_CONTROL with the CS_STALL bit.
> > > > 
> > > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > > index 556924ee47f9..62d09cf2ea8f 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > > @@ -1301,7 +1301,7 @@ static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req,
> > > >  		intel_ring_emit(signaller, GFX_OP_PIPE_CONTROL(6));
> > > >  		intel_ring_emit(signaller, PIPE_CONTROL_GLOBAL_GTT_IVB |
> > > >  					   PIPE_CONTROL_QW_WRITE |
> > > > -					   PIPE_CONTROL_FLUSH_ENABLE);
> > > > +					   PIPE_CONTROL_CS_STALL);
> > > 
> > > Doesn't this just stall when parsing the pipe control? Shouldn't
> > > we intead make sure the post-sync write is issued before the semaphore
> > > is signalled? (pipe_control /w post-sync write + second pipe control w/
> > > flush enable?)
> > 
> > No, afaik and can determine experimentally. The stall is after the
> > post-sync write. The pipe-control dosn't emit the write until it has
> > done the flush/invalidate and will not complete until the write is
> > commited (in theory, until it is coherent). The CS stall prevents the
> > command parser advancing until the pipecontrol is finished.
> 
> OK, so I did a quick experiemnt here doing something like:
> 
> for i=0-128:
> 	pipe_control w/ qw write data=0xdead0000+i offset=(i%8)*8
> for i=0-8:
> 	pipe_control w/ qw write data=i offset=(i%8)*8
> 	mi_lrm reg=0x2310+(i%8)*4 offset=(i%8)*8
> 
> and then check the regs afterwards.
> 
> If I use CS stall in the second pipe control, it does seem to wait for
> the post-sync operation initiated by itself. So seems you are right
> about that. So the register values I saw were:
>  (0x00002310): 0x00000001
>  (0x00002314): 0x00000002
>  (0x00002318): 0x00000003
>  (0x0000231c): 0x00000004
>  (0x00002320): 0x00000005
>  (0x00002324): 0x00000006
>  (0x00002328): 0x00000007
>  (0x0000232c): 0x00000008
> 
> I also noticed something about the "pipe control flush" (bit 7). It
> only really seems to wait for already initiated post-sync operations.
> If the pipe control that is going to initiate the post-sync op itself
> is still in the pipeline, bit 7 won't actually wait for it. Or at
> least that's how I would interpret these results:

bit 7 is PIPE_CONTROL_FLUSH_ENABLE which means to wait upon preceeding
pipecontrols to complete. If the earlier pipecontrol doesn't wait for
the write to complete, the second pipecontrol will only wait for the
queue to clear. So that makes sense to me.

> bit 7 == 0:
>  (0x00002310): 0xdead0058
>  (0x00002314): 0xdead0059
>  (0x00002318): 0xdead0062
>  (0x0000231c): 0xdead0063
>  (0x00002320): 0xdead006c
>  (0x00002324): 0xdead006d
>  (0x00002328): 0xdead006e
>  (0x0000232c): 0xdead0077
> 
> bit 7 == 1:
>  (0x00002310): 0xdead0078
>  (0x00002314): 0xdead0079
>  (0x00002318): 0xdead007a
>  (0x0000231c): 0xdead007b
>  (0x00002320): 0xdead007c
>  (0x00002324): 0xdead007d
>  (0x00002328): 0xdead007e
>  (0x0000232c): 0xdead007f
> 
> So even with bit 7 == 1 I didn't see the value the previous pipe control
> would write.
> 
> So not sure what good that bit really is then since you always need a
> CS stall to make sure that previous pipe controls have reached the end
> of the pipe, and CS stall anyway seems to wait for the post-sync
> operations, so adding bit 7 to the mix seems redundant.

Nice analysis. I'll make a mental note on how bit7 is mostly useless (or
speaking positively, mostly harmless).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre