Finish gen8 legacy semaphores

Finish gen8 legacy semaphores

[Chris Wilson]

Semaphores for gen8 were almost finished and only required a couple of
tweaks to pass the most stressful igt I could write, as well as the
existing inter-engine stress tests.

But what's the point you might ask when legacy ringbuffer submission is
already turned off by default? gen8 still serves as our test platform on
which we can directly compare the alternative submission modes and for
the purposes of testing, we want it to be as feature complete as
possible.
-Chris

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/4] drm/i915: Fix gen8 semaphores id for legacy mode

[Chris Wilson]

With the introduction of a distinct engine->id vs the hardware id, we need
to fix up the value we use for selecting the target engine when signaling
a semaphore. Note that these values can be merged with engine->guc_id.

Fixes: de1add360522c876c25ef2bbbbab1c94bdb509ab
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 9 +++++++--
 drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 6b4952031e30..556924ee47f9 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1307,7 +1307,7 @@ static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req,
 		intel_ring_emit(signaller, seqno);
 		intel_ring_emit(signaller, 0);
 		intel_ring_emit(signaller, MI_SEMAPHORE_SIGNAL |
-					   MI_SEMAPHORE_TARGET(waiter->id));
+					   MI_SEMAPHORE_TARGET(waiter->hw_id));
 		intel_ring_emit(signaller, 0);
 	}
 
@@ -1347,7 +1347,7 @@ static int gen8_xcs_signal(struct drm_i915_gem_request *signaller_req,
 		intel_ring_emit(signaller, upper_32_bits(gtt_offset));
 		intel_ring_emit(signaller, seqno);
 		intel_ring_emit(signaller, MI_SEMAPHORE_SIGNAL |
-					   MI_SEMAPHORE_TARGET(waiter->id));
+					   MI_SEMAPHORE_TARGET(waiter->hw_id));
 		intel_ring_emit(signaller, 0);
 	}
 
@@ -2793,6 +2793,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 	engine->name = "render ring";
 	engine->id = RCS;
 	engine->exec_id = I915_EXEC_RENDER;
+	engine->hw_id = 0;
 	engine->mmio_base = RENDER_RING_BASE;
 
 	if (INTEL_INFO(dev)->gen >= 8) {
@@ -2942,6 +2943,7 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
 	engine->name = "bsd ring";
 	engine->id = VCS;
 	engine->exec_id = I915_EXEC_BSD;
+	engine->hw_id = 1;
 
 	engine->write_tail = ring_write_tail;
 	if (INTEL_INFO(dev)->gen >= 6) {
@@ -3019,6 +3021,7 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev)
 	engine->name = "bsd2 ring";
 	engine->id = VCS2;
 	engine->exec_id = I915_EXEC_BSD;
+	engine->hw_id = 4;
 
 	engine->write_tail = ring_write_tail;
 	engine->mmio_base = GEN8_BSD2_RING_BASE;
@@ -3050,6 +3053,7 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
 	engine->name = "blitter ring";
 	engine->id = BCS;
 	engine->exec_id = I915_EXEC_BLT;
+	engine->hw_id = 2;
 
 	engine->mmio_base = BLT_RING_BASE;
 	engine->write_tail = ring_write_tail;
@@ -3108,6 +3112,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
 	engine->name = "video enhancement ring";
 	engine->id = VECS;
 	engine->exec_id = I915_EXEC_VEBOX;
+	engine->hw_id = 3;
 
 	engine->mmio_base = VEBOX_RING_BASE;
 	engine->write_tail = ring_write_tail;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 98eadfa79116..6efa47389276 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -155,7 +155,8 @@ struct  intel_engine_cs {
 #define I915_NUM_ENGINES 5
 #define _VCS(n) (VCS + (n))
 	unsigned int exec_id;
-	unsigned int guc_id;
+	unsigned int hw_id;
+	unsigned int guc_id; /* XXX same as hw_id? */
 	u32		mmio_base;
 	struct		drm_device *dev;
 	struct intel_ringbuffer *buffer;
-- 
2.8.0.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Fix gen8 semaphores id for legacy mode

[Ville Syrjälä]

On Sat, Apr 09, 2016 at 11:14:43AM +0100, Chris Wilson wrote:
> With the introduction of a distinct engine->id vs the hardware id, we need
> to fix up the value we use for selecting the target engine when signaling
> a semaphore. Note that these values can be merged with engine->guc_id.
> 
> Fixes: de1add360522c876c25ef2bbbbab1c94bdb509ab
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 9 +++++++--
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++-
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 6b4952031e30..556924ee47f9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1307,7 +1307,7 @@ static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req,
>  		intel_ring_emit(signaller, seqno);
>  		intel_ring_emit(signaller, 0);
>  		intel_ring_emit(signaller, MI_SEMAPHORE_SIGNAL |
> -					   MI_SEMAPHORE_TARGET(waiter->id));
> +					   MI_SEMAPHORE_TARGET(waiter->hw_id));
>  		intel_ring_emit(signaller, 0);
>  	}
>  
> @@ -1347,7 +1347,7 @@ static int gen8_xcs_signal(struct drm_i915_gem_request *signaller_req,
>  		intel_ring_emit(signaller, upper_32_bits(gtt_offset));
>  		intel_ring_emit(signaller, seqno);
>  		intel_ring_emit(signaller, MI_SEMAPHORE_SIGNAL |
> -					   MI_SEMAPHORE_TARGET(waiter->id));
> +					   MI_SEMAPHORE_TARGET(waiter->hw_id));
>  		intel_ring_emit(signaller, 0);
>  	}
>  
> @@ -2793,6 +2793,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
>  	engine->name = "render ring";
>  	engine->id = RCS;
>  	engine->exec_id = I915_EXEC_RENDER;
> +	engine->hw_id = 0;
>  	engine->mmio_base = RENDER_RING_BASE;
>  
>  	if (INTEL_INFO(dev)->gen >= 8) {
> @@ -2942,6 +2943,7 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
>  	engine->name = "bsd ring";
>  	engine->id = VCS;
>  	engine->exec_id = I915_EXEC_BSD;
> +	engine->hw_id = 1;
>  
>  	engine->write_tail = ring_write_tail;
>  	if (INTEL_INFO(dev)->gen >= 6) {
> @@ -3019,6 +3021,7 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev)
>  	engine->name = "bsd2 ring";
>  	engine->id = VCS2;
>  	engine->exec_id = I915_EXEC_BSD;
> +	engine->hw_id = 4;
>  
>  	engine->write_tail = ring_write_tail;
>  	engine->mmio_base = GEN8_BSD2_RING_BASE;
> @@ -3050,6 +3053,7 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
>  	engine->name = "blitter ring";
>  	engine->id = BCS;
>  	engine->exec_id = I915_EXEC_BLT;
> +	engine->hw_id = 2;
>  
>  	engine->mmio_base = BLT_RING_BASE;
>  	engine->write_tail = ring_write_tail;
> @@ -3108,6 +3112,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
>  	engine->name = "video enhancement ring";
>  	engine->id = VECS;
>  	engine->exec_id = I915_EXEC_VEBOX;
> +	engine->hw_id = 3;
>  
>  	engine->mmio_base = VEBOX_RING_BASE;
>  	engine->write_tail = ring_write_tail;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 98eadfa79116..6efa47389276 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -155,7 +155,8 @@ struct  intel_engine_cs {
>  #define I915_NUM_ENGINES 5
>  #define _VCS(n) (VCS + (n))
>  	unsigned int exec_id;
> -	unsigned int guc_id;
> +	unsigned int hw_id;
> +	unsigned int guc_id; /* XXX same as hw_id? */
>  	u32		mmio_base;
>  	struct		drm_device *dev;
>  	struct intel_ringbuffer *buffer;
> -- 
> 2.8.0.rc3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Chris Wilson]

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);
 		intel_ring_emit(signaller, lower_32_bits(gtt_offset));
 		intel_ring_emit(signaller, upper_32_bits(gtt_offset));
 		intel_ring_emit(signaller, seqno);
@@ -1454,7 +1454,6 @@ gen8_ring_sync(struct drm_i915_gem_request *waiter_req,
 
 	intel_ring_emit(waiter, MI_SEMAPHORE_WAIT |
 				MI_SEMAPHORE_GLOBAL_GTT |
-				MI_SEMAPHORE_POLL |
 				MI_SEMAPHORE_SAD_GTE_SDD);
 	intel_ring_emit(waiter, seqno);
 	intel_ring_emit(waiter,
-- 
2.8.0.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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?)

Unfortunately it looks like the hardware wouldn't like it if we used
the option to signal the semaphore as a post-sync operation.

>  		intel_ring_emit(signaller, lower_32_bits(gtt_offset));
>  		intel_ring_emit(signaller, upper_32_bits(gtt_offset));
>  		intel_ring_emit(signaller, seqno);
> @@ -1454,7 +1454,6 @@ gen8_ring_sync(struct drm_i915_gem_request *waiter_req,
>  
>  	intel_ring_emit(waiter, MI_SEMAPHORE_WAIT |
>  				MI_SEMAPHORE_GLOBAL_GTT |
> -				MI_SEMAPHORE_POLL |
>  				MI_SEMAPHORE_SAD_GTE_SDD);
>  	intel_ring_emit(waiter, seqno);
>  	intel_ring_emit(waiter,
> -- 
> 2.8.0.rc3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Chris Wilson]

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.

The syndrome when it is wrong is exactly like the missed interrupt one -
the waiter blocks forever as it is woken up before the sempahore value
is updated. Currently the code workarounds around this bug by the waiter
polling that value every N microseconds.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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

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

[Ville Syrjälä]

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

Oh and based on that I guess I can say
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> 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

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

Re: [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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915: Reload PD tables after semaphore wait on gen8

[Chris Wilson]

When the engine idles waiting upon a semaphore, it loses its
pagetables and we must reload them before executing the batch.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 62d09cf2ea8f..a4bab6466388 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1446,6 +1446,7 @@ gen8_ring_sync(struct drm_i915_gem_request *waiter_req,
 {
 	struct intel_engine_cs *waiter = waiter_req->engine;
 	struct drm_i915_private *dev_priv = waiter->dev->dev_private;
+	struct i915_hw_ppgtt *ppgtt;
 	int ret;
 
 	ret = intel_ring_begin(waiter_req, 4);
@@ -1461,6 +1462,15 @@ gen8_ring_sync(struct drm_i915_gem_request *waiter_req,
 	intel_ring_emit(waiter,
 			upper_32_bits(GEN8_WAIT_OFFSET(waiter, signaller->id)));
 	intel_ring_advance(waiter);
+
+	/* When the engine idles waiting upon a semaphore, it loses its
+	 * pagetables and we must reload them before executing the batch.
+	 * We do this on the i915_switch_context() following the wait and
+	 * before the dispatch.
+	 */
+	ppgtt = waiter_req->ctx->ppgtt;
+	if (ppgtt)
+		ppgtt->pd_dirty_rings |= intel_engine_flag(waiter_req->engine);
 	return 0;
 }
 
-- 
2.8.0.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Reload PD tables after semaphore wait on gen8

[Ville Syrjälä]

On Sat, Apr 09, 2016 at 11:14:45AM +0100, Chris Wilson wrote:
> When the engine idles waiting upon a semaphore, it loses its
> pagetables and we must reload them before executing the batch.

RCS shouldn't lose them IIRC, other engines yes. As I recall the
other way to avoid this would be to prevent the engine from
entering some power save mode during the wait, but I suppose
that might come at a power cost.

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 62d09cf2ea8f..a4bab6466388 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1446,6 +1446,7 @@ gen8_ring_sync(struct drm_i915_gem_request *waiter_req,
>  {
>  	struct intel_engine_cs *waiter = waiter_req->engine;
>  	struct drm_i915_private *dev_priv = waiter->dev->dev_private;
> +	struct i915_hw_ppgtt *ppgtt;
>  	int ret;
>  
>  	ret = intel_ring_begin(waiter_req, 4);
> @@ -1461,6 +1462,15 @@ gen8_ring_sync(struct drm_i915_gem_request *waiter_req,
>  	intel_ring_emit(waiter,
>  			upper_32_bits(GEN8_WAIT_OFFSET(waiter, signaller->id)));
>  	intel_ring_advance(waiter);
> +
> +	/* When the engine idles waiting upon a semaphore, it loses its
> +	 * pagetables and we must reload them before executing the batch.
> +	 * We do this on the i915_switch_context() following the wait and
> +	 * before the dispatch.
> +	 */
> +	ppgtt = waiter_req->ctx->ppgtt;
> +	if (ppgtt)
> +		ppgtt->pd_dirty_rings |= intel_engine_flag(waiter_req->engine);
>  	return 0;
>  }
>  
> -- 
> 2.8.0.rc3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 3/4] drm/i915: Reload PD tables after semaphore wait on gen8

[Chris Wilson]

On Mon, Apr 11, 2016 at 10:34:14AM +0300, Ville Syrjälä wrote:
> On Sat, Apr 09, 2016 at 11:14:45AM +0100, Chris Wilson wrote:
> > When the engine idles waiting upon a semaphore, it loses its
> > pagetables and we must reload them before executing the batch.
> 
> RCS shouldn't lose them IIRC, other engines yes. As I recall the
> other way to avoid this would be to prevent the engine from
> entering some power save mode during the wait, but I suppose
> that might come at a power cost.

Right, this only affects !RCS.
Just changing to if (ppgtt && waiter_req->engine->id != RCS)
would be enough. Care to give a r-b with that in place since you've
checked bspec? :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Reload PD tables after semaphore wait on gen8

[Ville Syrjälä]

On Mon, Apr 11, 2016 at 09:16:21AM +0100, Chris Wilson wrote:
> On Mon, Apr 11, 2016 at 10:34:14AM +0300, Ville Syrjälä wrote:
> > On Sat, Apr 09, 2016 at 11:14:45AM +0100, Chris Wilson wrote:
> > > When the engine idles waiting upon a semaphore, it loses its
> > > pagetables and we must reload them before executing the batch.
> > 
> > RCS shouldn't lose them IIRC, other engines yes. As I recall the
> > other way to avoid this would be to prevent the engine from
> > entering some power save mode during the wait, but I suppose
> > that might come at a power cost.
> 
> Right, this only affects !RCS.
> Just changing to if (ppgtt && waiter_req->engine->id != RCS)
> would be enough. Care to give a r-b with that in place since you've
> checked bspec? :)

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

[PATCH 4/4] drm/i915: Enable semaphores for legacy submission on gen8

[Chris Wilson]

We have sufficient evidence from igt to support that semaphores are in
a working state. Enabling semaphores now for legacy provides a better
comparison of execlists against legacy ring submission.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 29b4e79c85a6..98f24474bc04 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -542,10 +542,6 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
 	if (i915.enable_execlists)
 		return false;
 
-	/* Until we get further testing... */
-	if (IS_GEN8(dev))
-		return false;
-
 #ifdef CONFIG_INTEL_IOMMU
 	/* Enable semaphores on SNB when IO remapping is off */
 	if (INTEL_INFO(dev)->gen == 6 && intel_iommu_gfx_mapped)
-- 
2.8.0.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for series starting with [1/4] drm/i915: Fix gen8 semaphores id for legacy mode

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Fix gen8 semaphores id for legacy mode
URL   : https://patchwork.freedesktop.org/series/5486/
State : warning

== Summary ==

Series 5486v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5486/revisions/1/mbox/

Test gem_exec_basic:
        Subgroup basic-bsd:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test gem_exec_suspend:
        Subgroup basic-s3:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test gem_sync:
        Subgroup basic-bsd:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (bsw-nuc-2)

bdw-nuci7        total:196  pass:184  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:196  pass:175  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:196  pass:158  dwarn:1   dfail:0   fail:0   skip:37 
byt-nuc          total:196  pass:161  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:196  pass:174  dwarn:0   dfail:0   fail:0   skip:22 
ilk-hp8440p      total:196  pass:132  dwarn:0   dfail:0   fail:0   skip:64 
ivb-t430s        total:196  pass:171  dwarn:0   dfail:0   fail:0   skip:25 
skl-i7k-2        total:196  pass:173  dwarn:0   dfail:0   fail:0   skip:23 
snb-dellxps      total:196  pass:162  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:196  pass:162  dwarn:0   dfail:0   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1854/

949884a57b51aa158e3ae9afe1f08130cdb7a3ef drm-intel-nightly: 2016y-04m-08d-10h-45m-28s UTC integration manifest
927d2ad1d9dc07c247b8982be63364dd7a783a86 drm/i915: Enable semaphores for legacy submission on gen8
cca81731cca2772d61ce2def44fcb9e9d3ee173e drm/i915: Reload PD tables after semaphore wait on gen8
c9e1c1a170cb57d0c9192388717a1fba4c632921 drm/i915: Fix serialisation of pipecontrol write vs semaphore signal
5123a722cfc43c931126b372a4c3ee55e9bcd662 drm/i915: Fix gen8 semaphores id for legacy mode

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx