[PATCH 0/2] Couple of patches on top of the SKL latency retrieval

[PATCH 0/2] Couple of patches on top of the SKL latency retrieval

[Damien Lespiau]

Ville had 2 comments on the SKL memory latency patch that are better addressed
as separate patches on top.

Patch 1: SKL unifies the plane hw and their latencies to fech data from memory
is identical. So SKL only needs a single array. We save some room by using
anymous union/structs. However, last time I tried that (shared DPLL state),
Daniel didn't like it. So separate that into its own patch so it can be easily
rejected.

Patch 2: While it shouldn't cause any harm and DATA1 is likely unused by the
pcode firmware, we don't actually know. So it's better to have as a separate
patch, just in case a revert is needed (for testing or otherwise).

-- 
Damien

Damien Lespiau (2):
  drm/i915: Use anonymous union/struct to save space taken by latency
    values
  drm/i915: Clear PCODE_DATA1 on SNB+

 drivers/gpu/drm/i915/i915_drv.h | 38 +++++++++++++++++++++-----------------
 drivers/gpu/drm/i915/i915_reg.h |  2 +-
 drivers/gpu/drm/i915/intel_pm.c |  3 +--
 3 files changed, 23 insertions(+), 20 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] drm/i915: Use anonymous union/struct to save space taken by latency values

[Damien Lespiau]

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0baf7f3..8471d12 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1730,23 +1730,27 @@ struct drm_i915_private {
 	struct vlv_s0ix_state vlv_s0ix_state;
 
 	struct {
-		/*
-		 * Raw watermark latency values:
-		 * in 0.1us units for WM0,
-		 * in 0.5us units for WM1+.
-		 */
-		/* primary */
-		uint16_t pri_latency[5];
-		/* sprite */
-		uint16_t spr_latency[5];
-		/* cursor */
-		uint16_t cur_latency[5];
-		/*
-		 * Raw watermark memory latency values
-		 * for SKL for all 8 levels
-		 * in 1us units.
-		 */
-		uint16_t skl_latency[8];
+		union {
+			/*
+			 * Raw watermark latency values:
+			 * in 0.1us units for WM0,
+			 * in 0.5us units for WM1+.
+			 */
+			struct {
+				/* primary */
+				uint16_t pri_latency[5];
+				/* sprite */
+				uint16_t spr_latency[5];
+				/* cursor */
+				uint16_t cur_latency[5];
+			};
+
+			/*
+			 * Raw watermark memory latency values for SKL for all
+			 * 8 levels. In 1us units.
+			 */
+			uint16_t skl_latency[8];
+		};
 
 		/*
 		 * The skl_wm_values structure is a bit too big for stack
-- 
1.8.3.1

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

Re: [PATCH 1/2] drm/i915: Use anonymous union/struct to save space taken by latency values

[Ville Syrjälä]

On Fri, Sep 05, 2014 at 01:53:11PM +0100, Damien Lespiau wrote:
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

I think I'll punt on reviewing this for now. I need to go through
the other skl wm stuff first, and then I probaly need to stare at the
result a bit before I can think about unification.

> ---
>  drivers/gpu/drm/i915/i915_drv.h | 38 +++++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0baf7f3..8471d12 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1730,23 +1730,27 @@ struct drm_i915_private {
>  	struct vlv_s0ix_state vlv_s0ix_state;
>  
>  	struct {
> -		/*
> -		 * Raw watermark latency values:
> -		 * in 0.1us units for WM0,
> -		 * in 0.5us units for WM1+.
> -		 */
> -		/* primary */
> -		uint16_t pri_latency[5];
> -		/* sprite */
> -		uint16_t spr_latency[5];
> -		/* cursor */
> -		uint16_t cur_latency[5];
> -		/*
> -		 * Raw watermark memory latency values
> -		 * for SKL for all 8 levels
> -		 * in 1us units.
> -		 */
> -		uint16_t skl_latency[8];
> +		union {
> +			/*
> +			 * Raw watermark latency values:
> +			 * in 0.1us units for WM0,
> +			 * in 0.5us units for WM1+.
> +			 */
> +			struct {
> +				/* primary */
> +				uint16_t pri_latency[5];
> +				/* sprite */
> +				uint16_t spr_latency[5];
> +				/* cursor */
> +				uint16_t cur_latency[5];
> +			};
> +
> +			/*
> +			 * Raw watermark memory latency values for SKL for all
> +			 * 8 levels. In 1us units.
> +			 */
> +			uint16_t skl_latency[8];
> +		};
>  
>  		/*
>  		 * The skl_wm_values structure is a bit too big for stack
> -- 
> 1.8.3.1

-- 
Ville Syrjälä
Intel OTC

[PATCH 2/2] drm/i915: Clear PCODE_DATA1 on SNB+

[Damien Lespiau]

Ville found out that the DATA1 register exists since SNB with some
scarce apparitions in the specs throughout the times. In his own words:

  Also according to Bspec the mailbox data1 register already existed
  since snb.  The hsw cdclk change sequence also mentions that it should
  be set to 0, but eg. the bdw IPS sequence doesn't mention it. I guess
  in theory some pcode command might cause it to be clobbered, so I'm
  thinking we should just explicitly set it to 0 for all platforms in
  the pcode read/write functions

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 2 +-
 drivers/gpu/drm/i915/intel_pm.c | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5a7adb1..56cccde 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5952,8 +5952,8 @@ enum skl_disp_power_wells {
 #define GEN6_PCODE_DATA				0x138128
 #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
 #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
+#define GEN6_PCODE_DATA1			0x13812C
 
-#define GEN9_PCODE_DATA1			0x13812C
 #define   GEN9_PCODE_READ_MEM_LATENCY		0x6
 #define   GEN9_MEM_LATENCY_LEVEL_MASK		0xFF
 #define   GEN9_MEM_LATENCY_LEVEL_1_5_SHIFT	8
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3f69f9a..7bc8f73 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -8716,8 +8716,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
 	}
 
 	I915_WRITE(GEN6_PCODE_DATA, *val);
-	if (INTEL_INFO(dev_priv)->gen >= 9)
-		I915_WRITE(GEN9_PCODE_DATA1, 0);
+	I915_WRITE(GEN6_PCODE_DATA1, 0);
 	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
 
 	if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
-- 
1.8.3.1

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

Re: [PATCH 2/2] drm/i915: Clear PCODE_DATA1 on SNB+

[Ville Syrjälä]

On Fri, Sep 05, 2014 at 01:53:12PM +0100, Damien Lespiau wrote:
> Ville found out that the DATA1 register exists since SNB with some
> scarce apparitions in the specs throughout the times. In his own words:
> 
>   Also according to Bspec the mailbox data1 register already existed
>   since snb.  The hsw cdclk change sequence also mentions that it should
>   be set to 0, but eg. the bdw IPS sequence doesn't mention it. I guess
>   in theory some pcode command might cause it to be clobbered, so I'm
>   thinking we should just explicitly set it to 0 for all platforms in
>   the pcode read/write functions
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

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

> ---
>  drivers/gpu/drm/i915/i915_reg.h | 2 +-
>  drivers/gpu/drm/i915/intel_pm.c | 3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5a7adb1..56cccde 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5952,8 +5952,8 @@ enum skl_disp_power_wells {
>  #define GEN6_PCODE_DATA				0x138128
>  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
>  #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
> +#define GEN6_PCODE_DATA1			0x13812C
>  
> -#define GEN9_PCODE_DATA1			0x13812C
>  #define   GEN9_PCODE_READ_MEM_LATENCY		0x6
>  #define   GEN9_MEM_LATENCY_LEVEL_MASK		0xFF
>  #define   GEN9_MEM_LATENCY_LEVEL_1_5_SHIFT	8
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3f69f9a..7bc8f73 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -8716,8 +8716,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
>  	}
>  
>  	I915_WRITE(GEN6_PCODE_DATA, *val);
> -	if (INTEL_INFO(dev_priv)->gen >= 9)
> -		I915_WRITE(GEN9_PCODE_DATA1, 0);
> +	I915_WRITE(GEN6_PCODE_DATA1, 0);
>  	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
>  
>  	if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
> -- 
> 1.8.3.1

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 0/2] Couple of patches on top of the SKL latency retrieval

[Rodrigo Vivi]

just a warn/heads-up: I had listed this one for -collector but got a
conflict because skl latency retrieval didn't get merged yet.

On Fri, Sep 5, 2014 at 5:53 AM, Damien Lespiau <damien.lespiau@intel.com> wrote:
> Ville had 2 comments on the SKL memory latency patch that are better addressed
> as separate patches on top.
>
> Patch 1: SKL unifies the plane hw and their latencies to fech data from memory
> is identical. So SKL only needs a single array. We save some room by using
> anymous union/structs. However, last time I tried that (shared DPLL state),
> Daniel didn't like it. So separate that into its own patch so it can be easily
> rejected.
>
> Patch 2: While it shouldn't cause any harm and DATA1 is likely unused by the
> pcode firmware, we don't actually know. So it's better to have as a separate
> patch, just in case a revert is needed (for testing or otherwise).
>
> --
> Damien
>
> Damien Lespiau (2):
>   drm/i915: Use anonymous union/struct to save space taken by latency
>     values
>   drm/i915: Clear PCODE_DATA1 on SNB+
>
>  drivers/gpu/drm/i915/i915_drv.h | 38 +++++++++++++++++++++-----------------
>  drivers/gpu/drm/i915/i915_reg.h |  2 +-
>  drivers/gpu/drm/i915/intel_pm.c |  3 +--
>  3 files changed, 23 insertions(+), 20 deletions(-)
>
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/2] Couple of patches on top of the SKL latency retrieval

[Damien Lespiau]

On Tue, Nov 04, 2014 at 11:00:46AM -0800, Rodrigo Vivi wrote:
> just a warn/heads-up: I had listed this one for -collector but got a
> conflict because skl latency retrieval didn't get merged yet.

I carry those in my SKL branch and will resend them as part of stage1
upstreaming, I think you can drop them from -collector.

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