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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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 == 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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx