[v2,2/3] drm/i915: Make EDP PSR flags not depend on enum values

Submitted by Imre Deak on Nov. 19, 2018, 8:46 p.m.

Details

Message ID 20181119204637.6748-1-imre.deak@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 2 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Imre Deak Nov. 19, 2018, 8:46 p.m.
Depending on the transcoder enum values to translate from transcoder
to EDP PSR flags can easily break if we add a new transcoder. So remove
the dependency by using an explicit mapping.

While at it also add a WARN for unexpected trancoders.

v2:
- Simplify things by defining flag shift values instead of indices.
- s/trans/cpu_transcoder/ (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Mika Kahola <mika.kahola@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  | 10 +++++---
 drivers/gpu/drm/i915/intel_psr.c | 55 +++++++++++++++++++++++++++-------------
 2 files changed, 44 insertions(+), 21 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index edb58af1e903..f80b16e3ff33 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4150,9 +4150,13 @@  enum {
 /* Bspec claims those aren't shifted but stay at 0x64800 */
 #define EDP_PSR_IMR				_MMIO(0x64834)
 #define EDP_PSR_IIR				_MMIO(0x64838)
-#define   EDP_PSR_ERROR(trans)			(1 << (((trans) * 8 + 10) & 31))
-#define   EDP_PSR_POST_EXIT(trans)		(1 << (((trans) * 8 + 9) & 31))
-#define   EDP_PSR_PRE_ENTRY(trans)		(1 << (((trans) * 8 + 8) & 31))
+#define   EDP_PSR_ERROR(shift)			(4 << (shift))
+#define   EDP_PSR_POST_EXIT(shift)		(2 << (shift))
+#define   EDP_PSR_PRE_ENTRY(shift)		(1 << (shift))
+#define   EDP_PSR_TRANSCODER_C_SHIFT		24
+#define   EDP_PSR_TRANSCODER_B_SHIFT		16
+#define   EDP_PSR_TRANSCODER_A_SHIFT		8
+#define   EDP_PSR_TRANSCODER_EDP_SHIFT		0
 
 #define EDP_PSR_AUX_CTL				_MMIO(dev_priv->psr_mmio_base + 0x10)
 #define   EDP_PSR_AUX_CTL_TIME_OUT_MASK		(3 << 26)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 48df16a02fac..26292961d693 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -83,25 +83,42 @@  static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
 	}
 }
 
+static int edp_psr_shift(enum transcoder cpu_transcoder)
+{
+	switch (cpu_transcoder) {
+	case TRANSCODER_A:
+		return EDP_PSR_TRANSCODER_A_SHIFT;
+	case TRANSCODER_B:
+		return EDP_PSR_TRANSCODER_B_SHIFT;
+	case TRANSCODER_C:
+		return EDP_PSR_TRANSCODER_C_SHIFT;
+	default:
+		MISSING_CASE(cpu_transcoder);
+		/* fallthrough */
+	case TRANSCODER_EDP:
+		return EDP_PSR_TRANSCODER_EDP_SHIFT;
+	}
+}
+
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32 debug)
 {
 	u32 debug_mask, mask;
+	enum transcoder cpu_transcoder;
+	u32 transcoders = BIT(TRANSCODER_EDP);
 
-	mask = EDP_PSR_ERROR(TRANSCODER_EDP);
-	debug_mask = EDP_PSR_POST_EXIT(TRANSCODER_EDP) |
-		     EDP_PSR_PRE_ENTRY(TRANSCODER_EDP);
-
-	if (INTEL_GEN(dev_priv) >= 8) {
-		mask |= EDP_PSR_ERROR(TRANSCODER_A) |
-			EDP_PSR_ERROR(TRANSCODER_B) |
-			EDP_PSR_ERROR(TRANSCODER_C);
-
-		debug_mask |= EDP_PSR_POST_EXIT(TRANSCODER_A) |
-			      EDP_PSR_PRE_ENTRY(TRANSCODER_A) |
-			      EDP_PSR_POST_EXIT(TRANSCODER_B) |
-			      EDP_PSR_PRE_ENTRY(TRANSCODER_B) |
-			      EDP_PSR_POST_EXIT(TRANSCODER_C) |
-			      EDP_PSR_PRE_ENTRY(TRANSCODER_C);
+	if (INTEL_GEN(dev_priv) >= 8)
+		transcoders |= BIT(TRANSCODER_A) |
+			       BIT(TRANSCODER_B) |
+			       BIT(TRANSCODER_C);
+
+	debug_mask = 0;
+	mask = 0;
+	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
+		int shift = edp_psr_shift(cpu_transcoder);
+
+		mask |= EDP_PSR_ERROR(shift);
+		debug_mask |= EDP_PSR_POST_EXIT(shift) |
+			      EDP_PSR_PRE_ENTRY(shift);
 	}
 
 	if (debug & I915_PSR_DEBUG_IRQ)
@@ -159,18 +176,20 @@  void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
 			       BIT(TRANSCODER_C);
 
 	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
+		int shift = edp_psr_shift(cpu_transcoder);
+
 		/* FIXME: Exit PSR and link train manually when this happens. */
-		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
+		if (psr_iir & EDP_PSR_ERROR(shift))
 			DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
 				      transcoder_name(cpu_transcoder));
 
-		if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
+		if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) {
 			dev_priv->psr.last_entry_attempt = time_ns;
 			DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
 				      transcoder_name(cpu_transcoder));
 		}
 
-		if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
+		if (psr_iir & EDP_PSR_POST_EXIT(shift)) {
 			dev_priv->psr.last_exit = time_ns;
 			DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
 				      transcoder_name(cpu_transcoder));

Comments

On Mon, Nov 19, 2018 at 10:46:37PM +0200, Imre Deak wrote:
> Depending on the transcoder enum values to translate from transcoder
> to EDP PSR flags can easily break if we add a new transcoder. So remove
> the dependency by using an explicit mapping.
> 
> While at it also add a WARN for unexpected trancoders.
> 
> v2:
> - Simplify things by defining flag shift values instead of indices.
> - s/trans/cpu_transcoder/ (Ville)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Mika Kahola <mika.kahola@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  | 10 +++++---
>  drivers/gpu/drm/i915/intel_psr.c | 55 +++++++++++++++++++++++++++-------------
>  2 files changed, 44 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index edb58af1e903..f80b16e3ff33 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4150,9 +4150,13 @@ enum {
>  /* Bspec claims those aren't shifted but stay at 0x64800 */
>  #define EDP_PSR_IMR				_MMIO(0x64834)
>  #define EDP_PSR_IIR				_MMIO(0x64838)
> -#define   EDP_PSR_ERROR(trans)			(1 << (((trans) * 8 + 10) & 31))
> -#define   EDP_PSR_POST_EXIT(trans)		(1 << (((trans) * 8 + 9) & 31))
> -#define   EDP_PSR_PRE_ENTRY(trans)		(1 << (((trans) * 8 + 8) & 31))
> +#define   EDP_PSR_ERROR(shift)			(4 << (shift))
> +#define   EDP_PSR_POST_EXIT(shift)		(2 << (shift))
> +#define   EDP_PSR_PRE_ENTRY(shift)		(1 << (shift))

The 1,2,4 suggest that these are values for the same bitfield, which
they are not. So I would prefer 1<<((shift)+n) etc. instead.

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

> +#define   EDP_PSR_TRANSCODER_C_SHIFT		24
> +#define   EDP_PSR_TRANSCODER_B_SHIFT		16
> +#define   EDP_PSR_TRANSCODER_A_SHIFT		8
> +#define   EDP_PSR_TRANSCODER_EDP_SHIFT		0
>  
>  #define EDP_PSR_AUX_CTL				_MMIO(dev_priv->psr_mmio_base + 0x10)
>  #define   EDP_PSR_AUX_CTL_TIME_OUT_MASK		(3 << 26)
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 48df16a02fac..26292961d693 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -83,25 +83,42 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> +static int edp_psr_shift(enum transcoder cpu_transcoder)
> +{
> +	switch (cpu_transcoder) {
> +	case TRANSCODER_A:
> +		return EDP_PSR_TRANSCODER_A_SHIFT;
> +	case TRANSCODER_B:
> +		return EDP_PSR_TRANSCODER_B_SHIFT;
> +	case TRANSCODER_C:
> +		return EDP_PSR_TRANSCODER_C_SHIFT;
> +	default:
> +		MISSING_CASE(cpu_transcoder);
> +		/* fallthrough */
> +	case TRANSCODER_EDP:
> +		return EDP_PSR_TRANSCODER_EDP_SHIFT;
> +	}
> +}
> +
>  void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32 debug)
>  {
>  	u32 debug_mask, mask;
> +	enum transcoder cpu_transcoder;
> +	u32 transcoders = BIT(TRANSCODER_EDP);
>  
> -	mask = EDP_PSR_ERROR(TRANSCODER_EDP);
> -	debug_mask = EDP_PSR_POST_EXIT(TRANSCODER_EDP) |
> -		     EDP_PSR_PRE_ENTRY(TRANSCODER_EDP);
> -
> -	if (INTEL_GEN(dev_priv) >= 8) {
> -		mask |= EDP_PSR_ERROR(TRANSCODER_A) |
> -			EDP_PSR_ERROR(TRANSCODER_B) |
> -			EDP_PSR_ERROR(TRANSCODER_C);
> -
> -		debug_mask |= EDP_PSR_POST_EXIT(TRANSCODER_A) |
> -			      EDP_PSR_PRE_ENTRY(TRANSCODER_A) |
> -			      EDP_PSR_POST_EXIT(TRANSCODER_B) |
> -			      EDP_PSR_PRE_ENTRY(TRANSCODER_B) |
> -			      EDP_PSR_POST_EXIT(TRANSCODER_C) |
> -			      EDP_PSR_PRE_ENTRY(TRANSCODER_C);
> +	if (INTEL_GEN(dev_priv) >= 8)
> +		transcoders |= BIT(TRANSCODER_A) |
> +			       BIT(TRANSCODER_B) |
> +			       BIT(TRANSCODER_C);
> +
> +	debug_mask = 0;
> +	mask = 0;
> +	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> +		int shift = edp_psr_shift(cpu_transcoder);
> +
> +		mask |= EDP_PSR_ERROR(shift);
> +		debug_mask |= EDP_PSR_POST_EXIT(shift) |
> +			      EDP_PSR_PRE_ENTRY(shift);
>  	}
>  
>  	if (debug & I915_PSR_DEBUG_IRQ)
> @@ -159,18 +176,20 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
>  			       BIT(TRANSCODER_C);
>  
>  	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> +		int shift = edp_psr_shift(cpu_transcoder);
> +
>  		/* FIXME: Exit PSR and link train manually when this happens. */
> -		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> +		if (psr_iir & EDP_PSR_ERROR(shift))
>  			DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
>  				      transcoder_name(cpu_transcoder));
>  
> -		if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> +		if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) {
>  			dev_priv->psr.last_entry_attempt = time_ns;
>  			DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
>  				      transcoder_name(cpu_transcoder));
>  		}
>  
> -		if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
> +		if (psr_iir & EDP_PSR_POST_EXIT(shift)) {
>  			dev_priv->psr.last_exit = time_ns;
>  			DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
>  				      transcoder_name(cpu_transcoder));
> -- 
> 2.13.2