[PATCH 0/3] GEN8 SRM changes

[PATCH 0/3] GEN8 SRM changes

[Damien Lespiau]

STORE_REGISTER_MEM has now one extra dword on gen8. This series is not exactly
well tested.

-- 
Damien

Damien Lespiau (3):
  drm/i915: Protect the argument expansion in LRI and SRM macros
  drm/i915/bdw: Provide a gen8 version of SRM
  drm/i915/bdw: Use the GEN8 SRM when qeueing a flip

 drivers/gpu/drm/i915/i915_reg.h      |  5 +++--
 drivers/gpu/drm/i915/intel_display.c | 22 +++++++++++++++++++---
 2 files changed, 22 insertions(+), 5 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] drm/i915: Protect the argument expansion in LRI and SRM macros

[Damien Lespiau]

It seems like it wouldn't be too unlikely to be wanting to use a an
expression in the macro argument and things could go very wrong.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2315366..22d8b14 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -265,8 +265,8 @@
  * - One can actually load arbitrary many arbitrary registers: Simply issue x
  *   address/value pairs. Don't overdue it, though, x <= 2^4 must hold!
  */
-#define MI_LOAD_REGISTER_IMM(x)	MI_INSTR(0x22, 2*x-1)
-#define MI_STORE_REGISTER_MEM(x) MI_INSTR(0x24, 2*x-1)
+#define MI_LOAD_REGISTER_IMM(x)	MI_INSTR(0x22, 2*(x)-1)
+#define MI_STORE_REGISTER_MEM(x) MI_INSTR(0x24, 2*(x)-1)
 #define   MI_SRM_LRM_GLOBAL_GTT		(1<<22)
 #define MI_FLUSH_DW		MI_INSTR(0x26, 1) /* for GEN6 */
 #define   MI_FLUSH_DW_STORE_INDEX	(1<<21)
-- 
1.8.3.1

Re: [PATCH 1/3] drm/i915: Protect the argument expansion in LRI and SRM macros

[Ben Widawsky]

On Mon, Apr 07, 2014 at 08:24:32PM +0100, Damien Lespiau wrote:
> It seems like it wouldn't be too unlikely to be wanting to use a an
> expression in the macro argument and things could go very wrong.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2315366..22d8b14 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -265,8 +265,8 @@
>   * - One can actually load arbitrary many arbitrary registers: Simply issue x
>   *   address/value pairs. Don't overdue it, though, x <= 2^4 must hold!
>   */
> -#define MI_LOAD_REGISTER_IMM(x)	MI_INSTR(0x22, 2*x-1)
> -#define MI_STORE_REGISTER_MEM(x) MI_INSTR(0x24, 2*x-1)
> +#define MI_LOAD_REGISTER_IMM(x)	MI_INSTR(0x22, 2*(x)-1)
> +#define MI_STORE_REGISTER_MEM(x) MI_INSTR(0x24, 2*(x)-1)
>  #define   MI_SRM_LRM_GLOBAL_GTT		(1<<22)
>  #define MI_FLUSH_DW		MI_INSTR(0x26, 1) /* for GEN6 */
>  #define   MI_FLUSH_DW_STORE_INDEX	(1<<21)

I was just looking at this macro myself recently (well, LRI, not SRM). I
think it would be good to WARN_ON values of X which are too large. (I'd
rather BUG, but we don't have to go that route). Any thoughts on that?

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>


-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/3] drm/i915: Protect the argument expansion in LRI and SRM macros

[Daniel Vetter]

On Mon, Apr 07, 2014 at 01:47:05PM -0700, Ben Widawsky wrote:
> On Mon, Apr 07, 2014 at 08:24:32PM +0100, Damien Lespiau wrote:
> > It seems like it wouldn't be too unlikely to be wanting to use a an
> > expression in the macro argument and things could go very wrong.
> > 
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 2315366..22d8b14 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -265,8 +265,8 @@
> >   * - One can actually load arbitrary many arbitrary registers: Simply issue x
> >   *   address/value pairs. Don't overdue it, though, x <= 2^4 must hold!
> >   */
> > -#define MI_LOAD_REGISTER_IMM(x)	MI_INSTR(0x22, 2*x-1)
> > -#define MI_STORE_REGISTER_MEM(x) MI_INSTR(0x24, 2*x-1)
> > +#define MI_LOAD_REGISTER_IMM(x)	MI_INSTR(0x22, 2*(x)-1)
> > +#define MI_STORE_REGISTER_MEM(x) MI_INSTR(0x24, 2*(x)-1)
> >  #define   MI_SRM_LRM_GLOBAL_GTT		(1<<22)
> >  #define MI_FLUSH_DW		MI_INSTR(0x26, 1) /* for GEN6 */
> >  #define   MI_FLUSH_DW_STORE_INDEX	(1<<21)
> 
> I was just looking at this macro myself recently (well, LRI, not SRM). I
> think it would be good to WARN_ON values of X which are too large. (I'd
> rather BUG, but we don't have to go that route). Any thoughts on that?
> 
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 2/3] drm/i915/bdw: Provide a gen8 version of SRM

[Damien Lespiau]

GEN8 now has a qword to code for 48bit addresses.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 22d8b14..bc5ec33 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -267,6 +267,7 @@
  */
 #define MI_LOAD_REGISTER_IMM(x)	MI_INSTR(0x22, 2*(x)-1)
 #define MI_STORE_REGISTER_MEM(x) MI_INSTR(0x24, 2*(x)-1)
+#define MI_STORE_REGISTER_MEM_GEN8(x) MI_INSTR(0x24, 3*(x)-1)
 #define   MI_SRM_LRM_GLOBAL_GTT		(1<<22)
 #define MI_FLUSH_DW		MI_INSTR(0x26, 1) /* for GEN6 */
 #define   MI_FLUSH_DW_STORE_INDEX	(1<<21)
-- 
1.8.3.1

[PATCH 3/3] drm/i915/bdw: Use the GEN8 SRM when qeueing a flip

[Damien Lespiau]

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3697433..a646ed4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8821,8 +8821,16 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
 	}
 
 	len = 4;
-	if (ring->id == RCS)
+	if (ring->id == RCS) {
 		len += 6;
+		/*
+		 * On Gen 8, SRM is now taking an extra dword to accommodate
+		 * 48bits addresses, and we need a NOOP for the batch size to
+		 * stay even.
+		 */
+		if (IS_GEN8(dev))
+			len += 2;
+	}
 
 	/*
 	 * BSpec MI_DISPLAY_FLIP for IVB:
@@ -8857,10 +8865,18 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
 		intel_ring_emit(ring, ~(DERRMR_PIPEA_PRI_FLIP_DONE |
 					DERRMR_PIPEB_PRI_FLIP_DONE |
 					DERRMR_PIPEC_PRI_FLIP_DONE));
-		intel_ring_emit(ring, MI_STORE_REGISTER_MEM(1) |
-				MI_SRM_LRM_GLOBAL_GTT);
+		if (IS_GEN8(dev))
+			intel_ring_emit(ring, MI_STORE_REGISTER_MEM_GEN8(1) |
+					      MI_SRM_LRM_GLOBAL_GTT);
+		else
+			intel_ring_emit(ring, MI_STORE_REGISTER_MEM(1) |
+					      MI_SRM_LRM_GLOBAL_GTT);
 		intel_ring_emit(ring, DERRMR);
 		intel_ring_emit(ring, ring->scratch.gtt_offset + 256);
+		if (IS_GEN8(dev)) {
+			intel_ring_emit(ring, 0);
+			intel_ring_emit(ring, MI_NOOP);
+		}
 	}
 
 	intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 | plane_bit);
-- 
1.8.3.1

Re: [PATCH 3/3] drm/i915/bdw: Use the GEN8 SRM when qeueing a flip

[Ben Widawsky]

On Mon, Apr 07, 2014 at 08:24:34PM +0100, Damien Lespiau wrote:
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3697433..a646ed4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8821,8 +8821,16 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
>  	}
>  
>  	len = 4;
> -	if (ring->id == RCS)
> +	if (ring->id == RCS) {
>  		len += 6;
> +		/*
> +		 * On Gen 8, SRM is now taking an extra dword to accommodate
> +		 * 48bits addresses, and we need a NOOP for the batch size to
> +		 * stay even.
> +		 */
> +		if (IS_GEN8(dev))
> +			len += 2;
> +	}
>  
>  	/*
>  	 * BSpec MI_DISPLAY_FLIP for IVB:
> @@ -8857,10 +8865,18 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
>  		intel_ring_emit(ring, ~(DERRMR_PIPEA_PRI_FLIP_DONE |
>  					DERRMR_PIPEB_PRI_FLIP_DONE |
>  					DERRMR_PIPEC_PRI_FLIP_DONE));
> -		intel_ring_emit(ring, MI_STORE_REGISTER_MEM(1) |
> -				MI_SRM_LRM_GLOBAL_GTT);
> +		if (IS_GEN8(dev))
> +			intel_ring_emit(ring, MI_STORE_REGISTER_MEM_GEN8(1) |
> +					      MI_SRM_LRM_GLOBAL_GTT);
> +		else
> +			intel_ring_emit(ring, MI_STORE_REGISTER_MEM(1) |
> +					      MI_SRM_LRM_GLOBAL_GTT);
>  		intel_ring_emit(ring, DERRMR);
>  		intel_ring_emit(ring, ring->scratch.gtt_offset + 256);
> +		if (IS_GEN8(dev)) {
> +			intel_ring_emit(ring, 0);
> +			intel_ring_emit(ring, MI_NOOP);
> +		}
>  	}
>  
>  	intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 | plane_bit);

Cool. This explains the bad DERRMR values I was seeing in in error
states. I'm honestly didn't check if we actually need an SRM for BDW
still, but I'll assume you did check.

I also think it's worth to make a intel_gen8_queue_flip, but since I
don't touch this code much, I'll leave it to you to decide.

The series is:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

minus the above mentioned fact of whether it's needed.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 3/3] drm/i915/bdw: Use the GEN8 SRM when qeueing a flip

[Damien Lespiau]

On Mon, Apr 07, 2014 at 01:59:17PM -0700, Ben Widawsky wrote:
> Cool. This explains the bad DERRMR values I was seeing in in error
> states. I'm honestly didn't check if we actually need an SRM for BDW
> still, but I'll assume you did check.

Just checked, the LRI command still mentions that we need the SRM after
writes to the display engine.

> I also think it's worth to make a intel_gen8_queue_flip, but since I
> don't touch this code much, I'll leave it to you to decide.

Don't have a strong opinion at all, so I'll leave it at that for now.

-- 
Damien

Re: [PATCH 3/3] drm/i915/bdw: Use the GEN8 SRM when qeueing a flip

[Chris Wilson]

On Mon, Apr 07, 2014 at 11:20:14PM +0100, Damien Lespiau wrote:
> On Mon, Apr 07, 2014 at 01:59:17PM -0700, Ben Widawsky wrote:
> > Cool. This explains the bad DERRMR values I was seeing in in error
> > states. I'm honestly didn't check if we actually need an SRM for BDW
> > still, but I'll assume you did check.
> 
> Just checked, the LRI command still mentions that we need the SRM after
> writes to the display engine.

It shouldn't explain the DERRMR values being incorrect though aiui. The
SRM is to prevent system hangs from two concurrent writes, which never
satisfied me as to how that prevents two different MMIO paths from
accessing the same register cacheline simultaneously. Magic.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] drm/i915/bdw: Use the GEN8 SRM when qeueing a flip

[Damien Lespiau]

On Tue, Apr 08, 2014 at 07:24:23AM +0100, Chris Wilson wrote:
> On Mon, Apr 07, 2014 at 11:20:14PM +0100, Damien Lespiau wrote:
> > On Mon, Apr 07, 2014 at 01:59:17PM -0700, Ben Widawsky wrote:
> > > Cool. This explains the bad DERRMR values I was seeing in in error
> > > states. I'm honestly didn't check if we actually need an SRM for BDW
> > > still, but I'll assume you did check.
> > 
> > Just checked, the LRI command still mentions that we need the SRM after
> > writes to the display engine.
> 
> It shouldn't explain the DERRMR values being incorrect though aiui. The
> SRM is to prevent system hangs from two concurrent writes, which never
> satisfied me as to how that prevents two different MMIO paths from
> accessing the same register cacheline simultaneously. Magic.

I've seen, elsewhere, this LRI not landing and DE_RRMR being the same
value as the reset value after the first flip post boot. Hence my IRC
question about the need to touch DE_RRMR if we're not waiting
explicitely for an event in the CS. This story may not end here!

-- 
Damien

Re: [PATCH 3/3] drm/i915/bdw: Use the GEN8 SRM when qeueing a flip

[Daniel Vetter]

On Mon, Apr 07, 2014 at 01:59:17PM -0700, Ben Widawsky wrote:
> On Mon, Apr 07, 2014 at 08:24:34PM +0100, Damien Lespiau wrote:
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3697433..a646ed4 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8821,8 +8821,16 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
> >  	}
> >  
> >  	len = 4;
> > -	if (ring->id == RCS)
> > +	if (ring->id == RCS) {
> >  		len += 6;
> > +		/*
> > +		 * On Gen 8, SRM is now taking an extra dword to accommodate
> > +		 * 48bits addresses, and we need a NOOP for the batch size to
> > +		 * stay even.
> > +		 */
> > +		if (IS_GEN8(dev))
> > +			len += 2;
> > +	}
> >  
> >  	/*
> >  	 * BSpec MI_DISPLAY_FLIP for IVB:
> > @@ -8857,10 +8865,18 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
> >  		intel_ring_emit(ring, ~(DERRMR_PIPEA_PRI_FLIP_DONE |
> >  					DERRMR_PIPEB_PRI_FLIP_DONE |
> >  					DERRMR_PIPEC_PRI_FLIP_DONE));
> > -		intel_ring_emit(ring, MI_STORE_REGISTER_MEM(1) |
> > -				MI_SRM_LRM_GLOBAL_GTT);
> > +		if (IS_GEN8(dev))
> > +			intel_ring_emit(ring, MI_STORE_REGISTER_MEM_GEN8(1) |
> > +					      MI_SRM_LRM_GLOBAL_GTT);
> > +		else
> > +			intel_ring_emit(ring, MI_STORE_REGISTER_MEM(1) |
> > +					      MI_SRM_LRM_GLOBAL_GTT);
> >  		intel_ring_emit(ring, DERRMR);
> >  		intel_ring_emit(ring, ring->scratch.gtt_offset + 256);
> > +		if (IS_GEN8(dev)) {
> > +			intel_ring_emit(ring, 0);
> > +			intel_ring_emit(ring, MI_NOOP);
> > +		}
> >  	}
> >  
> >  	intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 | plane_bit);
> 
> Cool. This explains the bad DERRMR values I was seeing in in error
> states. I'm honestly didn't check if we actually need an SRM for BDW
> still, but I'll assume you did check.
> 
> I also think it's worth to make a intel_gen8_queue_flip, but since I
> don't touch this code much, I'll leave it to you to decide.

Yeah, gen8_queue_flip would kinda look neat. Anyway pull in the entire
series, thanks.
-Daniel

> 
> The series is:
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> minus the above mentioned fact of whether it's needed.
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch