drm/i915: Make CHICKEN_TRANS reg not depend on enum value

Submitted by Imre Deak on Nov. 23, 2018, 4:03 p.m.

Details

Message ID 20181123160357.GB6775@ideak-desk.fi.intel.com
State New
Headers show
Series "drm/i915: Make CHICKEN_TRANS reg not depend on enum value" ( rev: 3 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Imre Deak Nov. 23, 2018, 4:03 p.m.
On Fri, Nov 23, 2018 at 02:03:18PM +0200, Jani Nikula wrote:
> On Mon, 19 Nov 2018, Imre Deak <imre.deak@intel.com> wrote:
> > Depending on the transcoder enum values to translate from transcoder to the
> > corresponding CHICKEN_TRANS register can easily break if we add a new
> > transcoder. Add an explicit mapping instead, by using helpers to look up the
> > register instance either by transcoder or port (since unconveniently the
> > registers have both port and transcoder specific bits).
> >
> > While at it also check for the correctness of GEN, port, transcoder. I wasn't
> > sure if psr2_enabled can only be set for GEN9+, but that seems to be the case
> > indeed (see setting of sink_psr2_support in intel_psr_init_dpcd()).
> >
> > 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  |  7 +++---
> >  drivers/gpu/drm/i915/intel_ddi.c | 53 +++++++++++++++++++++++++++++++---------
> >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> >  drivers/gpu/drm/i915/intel_psr.c |  6 +++--
> >  4 files changed, 51 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 25b069175c2a..1beed39de303 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7399,9 +7399,10 @@ enum {
> >  #define  BDW_DPRS_MASK_VBLANK_SRD	(1 << 0)
> >  #define CHICKEN_PIPESL_1(pipe) _MMIO_PIPE(pipe, _CHICKEN_PIPESL_1_A, _CHICKEN_PIPESL_1_B)
> >  
> > -#define CHICKEN_TRANS_A         0x420c0
> > -#define CHICKEN_TRANS_B         0x420c4
> > -#define CHICKEN_TRANS(trans) _MMIO_TRANS(trans, CHICKEN_TRANS_A, CHICKEN_TRANS_B)
> > +#define CHICKEN_TRANS_A		_MMIO(0x420c0)
> > +#define CHICKEN_TRANS_B		_MMIO(0x420c4)
> > +#define CHICKEN_TRANS_C		_MMIO(0x420c8)
> > +#define CHICKEN_TRANS_EDP	_MMIO(0x420cc)
> >  #define  VSC_DATA_SEL_SOFTWARE_CONTROL	(1 << 25) /* GLK and CNL+ */
> >  #define  DDI_TRAINING_OVERRIDE_ENABLE	(1 << 19)
> >  #define  DDI_TRAINING_OVERRIDE_VALUE	(1 << 18)
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 040483c96029..126e6aac335d 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -3380,6 +3380,42 @@ static void intel_enable_ddi_dp(struct intel_encoder *encoder,
> >  		intel_audio_codec_enable(encoder, crtc_state, conn_state);
> >  }
> >  
> > +i915_reg_t gen9_chicken_trans_reg(struct drm_i915_private *dev_priv,
> > +				  enum transcoder trans)
> > +{
> > +	static const i915_reg_t regs[] = {
> > +		[TRANSCODER_A] = CHICKEN_TRANS_A,
> > +		[TRANSCODER_B] = CHICKEN_TRANS_B,
> > +		[TRANSCODER_C] = CHICKEN_TRANS_C,
> > +		[TRANSCODER_EDP] = CHICKEN_TRANS_EDP,
> > +	};
> > +
> > +	WARN_ON(INTEL_GEN(dev_priv) < 9);
> > +
> > +	if (WARN_ON(trans >= ARRAY_SIZE(regs) || !regs[trans].reg))
> > +		trans = TRANSCODER_A;
> > +
> > +	return regs[trans];
> > +}
> 
> I'm late to the party, but it kind of makes me sad that we're now
> introducing yet another way to choose registers based on transcoder (or
> some other index). And this one is hand-rolled, with local functions
> instead of putting it to i915_reg.h.

Didn't occur to me that _PICK could be used with named initializers.. We
would still miss the checks which I added here for incorrect indices,
but that's a separate issue from what I wanted to solve in this patch.
So the following would work too:

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 47baf2fe8f71..dfa547832dae 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7399,10 +7399,31 @@  enum {
 #define  BDW_DPRS_MASK_VBLANK_SRD	(1 << 0)
 #define CHICKEN_PIPESL_1(pipe) _MMIO_PIPE(pipe, _CHICKEN_PIPESL_1_A, _CHICKEN_PIPESL_1_B)
 
-#define CHICKEN_TRANS_A		_MMIO(0x420c0)
-#define CHICKEN_TRANS_B		_MMIO(0x420c4)
-#define CHICKEN_TRANS_C		_MMIO(0x420c8)
-#define CHICKEN_TRANS_EDP	_MMIO(0x420cc)
+#define _CHICKEN_TRANS_A		0x420c0
+#define _CHICKEN_TRANS_B		0x420c4
+#define _CHICKEN_TRANS_C		0x420c8
+#define _CHICKEN_TRANS_EDP		0x420cc
+#define CHICKEN_TRANS(trans)		_MMIO(_PICK(trans, \
+						    [TRANSCODER_A] = \
+							_CHICKEN_TRANS_A, \
+						    [TRANSCODER_B] = \
+							_CHICKEN_TRANS_B, \
+						    [TRANSCODER_C] = \
+							_CHICKEN_TRANS_C, \
+						    [TRANSCODER_EDP] = \
+							_CHICKEN_TRANS_EDP))
+
+#define CHICKEN_TRANS_BYPORT(port)	_MMIO(_PICK(port, \
+						    [PORT_A] = \
+							_CHICKEN_TRANS_EDP, \
+						    [PORT_B] = \
+							_CHICKEN_TRANS_A, \
+						    [PORT_C] = \
+							_CHICKEN_TRANS_B, \
+						    [PORT_D] = \
+							_CHICKEN_TRANS_C, \
+						    [PORT_E] = \
+							_CHICKEN_TRANS_A))
 #define  VSC_DATA_SEL_SOFTWARE_CONTROL	(1 << 25) /* GLK and CNL+ */
 #define  DDI_TRAINING_OVERRIDE_ENABLE	(1 << 19)
 #define  DDI_TRAINING_OVERRIDE_VALUE	(1 << 18)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ad11540ac436..c261320330b6 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3380,26 +3380,6 @@  static void intel_enable_ddi_dp(struct intel_encoder *encoder,
 		intel_audio_codec_enable(encoder, crtc_state, conn_state);
 }
 
-static i915_reg_t
-gen9_chicken_trans_reg_by_port(struct drm_i915_private *dev_priv,
-			       enum port port)
-{
-	static const i915_reg_t regs[] = {
-		[PORT_A] = CHICKEN_TRANS_EDP,
-		[PORT_B] = CHICKEN_TRANS_A,
-		[PORT_C] = CHICKEN_TRANS_B,
-		[PORT_D] = CHICKEN_TRANS_C,
-		[PORT_E] = CHICKEN_TRANS_A,
-	};
-
-	WARN_ON(INTEL_GEN(dev_priv) < 9);
-
-	if (WARN_ON(port < PORT_A || port > PORT_E))
-		port = PORT_A;
-
-	return regs[port];
-}
-
 static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
 				  const struct intel_crtc_state *crtc_state,
 				  const struct drm_connector_state *conn_state)
@@ -3423,10 +3403,7 @@  static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
 		 * the bits affect a specific DDI port rather than
 		 * a specific transcoder.
 		 */
-		i915_reg_t reg = gen9_chicken_trans_reg_by_port(dev_priv, port);
-		u32 val;
-
-		val = I915_READ(reg);
+		u32 val = I915_READ(CHICKEN_TRANS_BYPORT(port));
 
 		if (port == PORT_E)
 			val |= DDIE_TRAINING_OVERRIDE_ENABLE |
@@ -3435,8 +3412,8 @@  static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
 			val |= DDI_TRAINING_OVERRIDE_ENABLE |
 				DDI_TRAINING_OVERRIDE_VALUE;
 
-		I915_WRITE(reg, val);
-		POSTING_READ(reg);
+		I915_WRITE(CHICKEN_TRANS_BYPORT(port), val);
+		POSTING_READ(CHICKEN_TRANS_BYPORT(port));
 
 		udelay(1);
 
@@ -3447,7 +3424,7 @@  static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
 			val &= ~(DDI_TRAINING_OVERRIDE_ENABLE |
 				 DDI_TRAINING_OVERRIDE_VALUE);
 
-		I915_WRITE(reg, val);
+		I915_WRITE(CHICKEN_TRANS_BYPORT(port), val);
 	}
 
 	/* In HDMI/DVI mode, the port width, and swing/emphasis values
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 54fa17a5596a..76667c5d7800 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -577,25 +577,6 @@  static void intel_psr_activate(struct intel_dp *intel_dp)
 	dev_priv->psr.active = true;
 }
 
-static i915_reg_t gen9_chicken_trans_reg(struct drm_i915_private *dev_priv,
-					 enum transcoder cpu_transcoder)
-{
-	static const i915_reg_t regs[] = {
-		[TRANSCODER_A] = CHICKEN_TRANS_A,
-		[TRANSCODER_B] = CHICKEN_TRANS_B,
-		[TRANSCODER_C] = CHICKEN_TRANS_C,
-		[TRANSCODER_EDP] = CHICKEN_TRANS_EDP,
-	};
-
-	WARN_ON(INTEL_GEN(dev_priv) < 9);
-
-	if (WARN_ON(cpu_transcoder >= ARRAY_SIZE(regs) ||
-		    !regs[cpu_transcoder].reg))
-		cpu_transcoder = TRANSCODER_A;
-
-	return regs[cpu_transcoder];
-}
-
 static void intel_psr_enable_source(struct intel_dp *intel_dp,
 				    const struct intel_crtc_state *crtc_state)
 {
@@ -610,9 +591,10 @@  static void intel_psr_enable_source(struct intel_dp *intel_dp,
 		hsw_psr_setup_aux(intel_dp);
 
 	if (dev_priv->psr.psr2_enabled) {
-		i915_reg_t reg = gen9_chicken_trans_reg(dev_priv,
-							cpu_transcoder);
-		u32 chicken = I915_READ(reg);
+		u32 chicken;
+
+		WARN_ON(INTEL_GEN(dev_priv) < 9);
+		chicken = I915_READ(CHICKEN_TRANS(cpu_transcoder));
 
 		if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv))
 			chicken |= (PSR2_VSC_ENABLE_PROG_HEADER
@@ -620,7 +602,8 @@  static void intel_psr_enable_source(struct intel_dp *intel_dp,
 
 		else
 			chicken &= ~VSC_DATA_SEL_SOFTWARE_CONTROL;
-		I915_WRITE(reg, chicken);
+
+		I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken);
 	}
 
 	/*

Comments

On Fri, 23 Nov 2018, Imre Deak <imre.deak@intel.com> wrote:
> On Fri, Nov 23, 2018 at 02:03:18PM +0200, Jani Nikula wrote:
>> On Mon, 19 Nov 2018, Imre Deak <imre.deak@intel.com> wrote:
>> > Depending on the transcoder enum values to translate from transcoder to the
>> > corresponding CHICKEN_TRANS register can easily break if we add a new
>> > transcoder. Add an explicit mapping instead, by using helpers to look up the
>> > register instance either by transcoder or port (since unconveniently the
>> > registers have both port and transcoder specific bits).
>> >
>> > While at it also check for the correctness of GEN, port, transcoder. I wasn't
>> > sure if psr2_enabled can only be set for GEN9+, but that seems to be the case
>> > indeed (see setting of sink_psr2_support in intel_psr_init_dpcd()).
>> >
>> > 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  |  7 +++---
>> >  drivers/gpu/drm/i915/intel_ddi.c | 53 +++++++++++++++++++++++++++++++---------
>> >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>> >  drivers/gpu/drm/i915/intel_psr.c |  6 +++--
>> >  4 files changed, 51 insertions(+), 17 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> > index 25b069175c2a..1beed39de303 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -7399,9 +7399,10 @@ enum {
>> >  #define  BDW_DPRS_MASK_VBLANK_SRD	(1 << 0)
>> >  #define CHICKEN_PIPESL_1(pipe) _MMIO_PIPE(pipe, _CHICKEN_PIPESL_1_A, _CHICKEN_PIPESL_1_B)
>> >  
>> > -#define CHICKEN_TRANS_A         0x420c0
>> > -#define CHICKEN_TRANS_B         0x420c4
>> > -#define CHICKEN_TRANS(trans) _MMIO_TRANS(trans, CHICKEN_TRANS_A, CHICKEN_TRANS_B)
>> > +#define CHICKEN_TRANS_A		_MMIO(0x420c0)
>> > +#define CHICKEN_TRANS_B		_MMIO(0x420c4)
>> > +#define CHICKEN_TRANS_C		_MMIO(0x420c8)
>> > +#define CHICKEN_TRANS_EDP	_MMIO(0x420cc)
>> >  #define  VSC_DATA_SEL_SOFTWARE_CONTROL	(1 << 25) /* GLK and CNL+ */
>> >  #define  DDI_TRAINING_OVERRIDE_ENABLE	(1 << 19)
>> >  #define  DDI_TRAINING_OVERRIDE_VALUE	(1 << 18)
>> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> > index 040483c96029..126e6aac335d 100644
>> > --- a/drivers/gpu/drm/i915/intel_ddi.c
>> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> > @@ -3380,6 +3380,42 @@ static void intel_enable_ddi_dp(struct intel_encoder *encoder,
>> >  		intel_audio_codec_enable(encoder, crtc_state, conn_state);
>> >  }
>> >  
>> > +i915_reg_t gen9_chicken_trans_reg(struct drm_i915_private *dev_priv,
>> > +				  enum transcoder trans)
>> > +{
>> > +	static const i915_reg_t regs[] = {
>> > +		[TRANSCODER_A] = CHICKEN_TRANS_A,
>> > +		[TRANSCODER_B] = CHICKEN_TRANS_B,
>> > +		[TRANSCODER_C] = CHICKEN_TRANS_C,
>> > +		[TRANSCODER_EDP] = CHICKEN_TRANS_EDP,
>> > +	};
>> > +
>> > +	WARN_ON(INTEL_GEN(dev_priv) < 9);
>> > +
>> > +	if (WARN_ON(trans >= ARRAY_SIZE(regs) || !regs[trans].reg))
>> > +		trans = TRANSCODER_A;
>> > +
>> > +	return regs[trans];
>> > +}
>> 
>> I'm late to the party, but it kind of makes me sad that we're now
>> introducing yet another way to choose registers based on transcoder (or
>> some other index). And this one is hand-rolled, with local functions
>> instead of putting it to i915_reg.h.
>
> Didn't occur to me that _PICK could be used with named initializers.. We
> would still miss the checks which I added here for incorrect indices,
> but that's a separate issue from what I wanted to solve in this patch.
> So the following would work too:

Ha! It didn't occur to me either you could use designated initializers
with _PICK. It's kind of neat... in a scary way. :)

We do miss the checks for incorrect indices throughout the code base. So
I'm not that worried about it in this particular case. As you say, a
separate issue.

I'd go with this change.

BR,
Jani.


>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>b/drivers/gpu/drm/i915/i915_reg.h index 47baf2fe8f71..dfa547832dae
>100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++
>b/drivers/gpu/drm/i915/i915_reg.h @@ -7399,10 +7399,31 @@ enum {
>#define BDW_DPRS_MASK_VBLANK_SRD (1 << 0) #define
>CHICKEN_PIPESL_1(pipe) _MMIO_PIPE(pipe, _CHICKEN_PIPESL_1_A,
>_CHICKEN_PIPESL_1_B)
>  
> -#define CHICKEN_TRANS_A		_MMIO(0x420c0)
> -#define CHICKEN_TRANS_B		_MMIO(0x420c4)
> -#define CHICKEN_TRANS_C		_MMIO(0x420c8)
> -#define CHICKEN_TRANS_EDP	_MMIO(0x420cc)
> +#define _CHICKEN_TRANS_A		0x420c0
> +#define _CHICKEN_TRANS_B		0x420c4
> +#define _CHICKEN_TRANS_C		0x420c8
> +#define _CHICKEN_TRANS_EDP		0x420cc
> +#define CHICKEN_TRANS(trans)		_MMIO(_PICK(trans, \
> +						    [TRANSCODER_A] = \
> +							_CHICKEN_TRANS_A, \
> +						    [TRANSCODER_B] = \
> +							_CHICKEN_TRANS_B, \
> +						    [TRANSCODER_C] = \
> +							_CHICKEN_TRANS_C, \
> +						    [TRANSCODER_EDP] = \
> +							_CHICKEN_TRANS_EDP))
> +
> +#define CHICKEN_TRANS_BYPORT(port)	_MMIO(_PICK(port, \
> +						    [PORT_A] = \
> +							_CHICKEN_TRANS_EDP, \
> +						    [PORT_B] = \
> +							_CHICKEN_TRANS_A, \
> +						    [PORT_C] = \
> +							_CHICKEN_TRANS_B, \
> +						    [PORT_D] = \
> +							_CHICKEN_TRANS_C, \
> +						    [PORT_E] = \
> +							_CHICKEN_TRANS_A))
>  #define  VSC_DATA_SEL_SOFTWARE_CONTROL	(1 << 25) /* GLK and CNL+ */
>  #define  DDI_TRAINING_OVERRIDE_ENABLE	(1 << 19)
>  #define  DDI_TRAINING_OVERRIDE_VALUE	(1 << 18)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index ad11540ac436..c261320330b6 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3380,26 +3380,6 @@ static void intel_enable_ddi_dp(struct intel_encoder *encoder,
>  		intel_audio_codec_enable(encoder, crtc_state, conn_state);
>  }
>  
> -static i915_reg_t
> -gen9_chicken_trans_reg_by_port(struct drm_i915_private *dev_priv,
> -			       enum port port)
> -{
> -	static const i915_reg_t regs[] = {
> -		[PORT_A] = CHICKEN_TRANS_EDP,
> -		[PORT_B] = CHICKEN_TRANS_A,
> -		[PORT_C] = CHICKEN_TRANS_B,
> -		[PORT_D] = CHICKEN_TRANS_C,
> -		[PORT_E] = CHICKEN_TRANS_A,
> -	};
> -
> -	WARN_ON(INTEL_GEN(dev_priv) < 9);
> -
> -	if (WARN_ON(port < PORT_A || port > PORT_E))
> -		port = PORT_A;
> -
> -	return regs[port];
> -}
> -
>  static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
>  				  const struct intel_crtc_state *crtc_state,
>  				  const struct drm_connector_state *conn_state)
> @@ -3423,10 +3403,7 @@ static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
>  		 * the bits affect a specific DDI port rather than
>  		 * a specific transcoder.
>  		 */
> -		i915_reg_t reg = gen9_chicken_trans_reg_by_port(dev_priv, port);
> -		u32 val;
> -
> -		val = I915_READ(reg);
> +		u32 val = I915_READ(CHICKEN_TRANS_BYPORT(port));
>  
>  		if (port == PORT_E)
>  			val |= DDIE_TRAINING_OVERRIDE_ENABLE |
> @@ -3435,8 +3412,8 @@ static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
>  			val |= DDI_TRAINING_OVERRIDE_ENABLE |
>  				DDI_TRAINING_OVERRIDE_VALUE;
>  
> -		I915_WRITE(reg, val);
> -		POSTING_READ(reg);
> +		I915_WRITE(CHICKEN_TRANS_BYPORT(port), val);
> +		POSTING_READ(CHICKEN_TRANS_BYPORT(port));
>  
>  		udelay(1);
>  
> @@ -3447,7 +3424,7 @@ static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
>  			val &= ~(DDI_TRAINING_OVERRIDE_ENABLE |
>  				 DDI_TRAINING_OVERRIDE_VALUE);
>  
> -		I915_WRITE(reg, val);
> +		I915_WRITE(CHICKEN_TRANS_BYPORT(port), val);
>  	}
>  
>  	/* In HDMI/DVI mode, the port width, and swing/emphasis values
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 54fa17a5596a..76667c5d7800 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -577,25 +577,6 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
>  	dev_priv->psr.active = true;
>  }
>  
> -static i915_reg_t gen9_chicken_trans_reg(struct drm_i915_private *dev_priv,
> -					 enum transcoder cpu_transcoder)
> -{
> -	static const i915_reg_t regs[] = {
> -		[TRANSCODER_A] = CHICKEN_TRANS_A,
> -		[TRANSCODER_B] = CHICKEN_TRANS_B,
> -		[TRANSCODER_C] = CHICKEN_TRANS_C,
> -		[TRANSCODER_EDP] = CHICKEN_TRANS_EDP,
> -	};
> -
> -	WARN_ON(INTEL_GEN(dev_priv) < 9);
> -
> -	if (WARN_ON(cpu_transcoder >= ARRAY_SIZE(regs) ||
> -		    !regs[cpu_transcoder].reg))
> -		cpu_transcoder = TRANSCODER_A;
> -
> -	return regs[cpu_transcoder];
> -}
> -
>  static void intel_psr_enable_source(struct intel_dp *intel_dp,
>  				    const struct intel_crtc_state *crtc_state)
>  {
> @@ -610,9 +591,10 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
>  		hsw_psr_setup_aux(intel_dp);
>  
>  	if (dev_priv->psr.psr2_enabled) {
> -		i915_reg_t reg = gen9_chicken_trans_reg(dev_priv,
> -							cpu_transcoder);
> -		u32 chicken = I915_READ(reg);
> +		u32 chicken;
> +
> +		WARN_ON(INTEL_GEN(dev_priv) < 9);
> +		chicken = I915_READ(CHICKEN_TRANS(cpu_transcoder));
>  
>  		if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv))
>  			chicken |= (PSR2_VSC_ENABLE_PROG_HEADER
> @@ -620,7 +602,8 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
>  
>  		else
>  			chicken &= ~VSC_DATA_SEL_SOFTWARE_CONTROL;
> -		I915_WRITE(reg, chicken);
> +
> +		I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken);
>  	}
>  
>  	/*
>
On Mon, Nov 26, 2018 at 09:33:13AM +0200, Jani Nikula wrote:
> On Fri, 23 Nov 2018, Imre Deak <imre.deak@intel.com> wrote:
> > On Fri, Nov 23, 2018 at 02:03:18PM +0200, Jani Nikula wrote:
> >> On Mon, 19 Nov 2018, Imre Deak <imre.deak@intel.com> wrote:
> >> > Depending on the transcoder enum values to translate from transcoder to the
> >> > corresponding CHICKEN_TRANS register can easily break if we add a new
> >> > transcoder. Add an explicit mapping instead, by using helpers to look up the
> >> > register instance either by transcoder or port (since unconveniently the
> >> > registers have both port and transcoder specific bits).
> >> >
> >> > While at it also check for the correctness of GEN, port, transcoder. I wasn't
> >> > sure if psr2_enabled can only be set for GEN9+, but that seems to be the case
> >> > indeed (see setting of sink_psr2_support in intel_psr_init_dpcd()).
> >> >
> >> > 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  |  7 +++---
> >> >  drivers/gpu/drm/i915/intel_ddi.c | 53 +++++++++++++++++++++++++++++++---------
> >> >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> >> >  drivers/gpu/drm/i915/intel_psr.c |  6 +++--
> >> >  4 files changed, 51 insertions(+), 17 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> > index 25b069175c2a..1beed39de303 100644
> >> > --- a/drivers/gpu/drm/i915/i915_reg.h
> >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> > @@ -7399,9 +7399,10 @@ enum {
> >> >  #define  BDW_DPRS_MASK_VBLANK_SRD	(1 << 0)
> >> >  #define CHICKEN_PIPESL_1(pipe) _MMIO_PIPE(pipe, _CHICKEN_PIPESL_1_A, _CHICKEN_PIPESL_1_B)
> >> >  
> >> > -#define CHICKEN_TRANS_A         0x420c0
> >> > -#define CHICKEN_TRANS_B         0x420c4
> >> > -#define CHICKEN_TRANS(trans) _MMIO_TRANS(trans, CHICKEN_TRANS_A, CHICKEN_TRANS_B)
> >> > +#define CHICKEN_TRANS_A		_MMIO(0x420c0)
> >> > +#define CHICKEN_TRANS_B		_MMIO(0x420c4)
> >> > +#define CHICKEN_TRANS_C		_MMIO(0x420c8)
> >> > +#define CHICKEN_TRANS_EDP	_MMIO(0x420cc)
> >> >  #define  VSC_DATA_SEL_SOFTWARE_CONTROL	(1 << 25) /* GLK and CNL+ */
> >> >  #define  DDI_TRAINING_OVERRIDE_ENABLE	(1 << 19)
> >> >  #define  DDI_TRAINING_OVERRIDE_VALUE	(1 << 18)
> >> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> > index 040483c96029..126e6aac335d 100644
> >> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> > @@ -3380,6 +3380,42 @@ static void intel_enable_ddi_dp(struct intel_encoder *encoder,
> >> >  		intel_audio_codec_enable(encoder, crtc_state, conn_state);
> >> >  }
> >> >  
> >> > +i915_reg_t gen9_chicken_trans_reg(struct drm_i915_private *dev_priv,
> >> > +				  enum transcoder trans)
> >> > +{
> >> > +	static const i915_reg_t regs[] = {
> >> > +		[TRANSCODER_A] = CHICKEN_TRANS_A,
> >> > +		[TRANSCODER_B] = CHICKEN_TRANS_B,
> >> > +		[TRANSCODER_C] = CHICKEN_TRANS_C,
> >> > +		[TRANSCODER_EDP] = CHICKEN_TRANS_EDP,
> >> > +	};
> >> > +
> >> > +	WARN_ON(INTEL_GEN(dev_priv) < 9);
> >> > +
> >> > +	if (WARN_ON(trans >= ARRAY_SIZE(regs) || !regs[trans].reg))
> >> > +		trans = TRANSCODER_A;
> >> > +
> >> > +	return regs[trans];
> >> > +}
> >> 
> >> I'm late to the party, but it kind of makes me sad that we're now
> >> introducing yet another way to choose registers based on transcoder (or
> >> some other index). And this one is hand-rolled, with local functions
> >> instead of putting it to i915_reg.h.
> >
> > Didn't occur to me that _PICK could be used with named initializers.. We
> > would still miss the checks which I added here for incorrect indices,
> > but that's a separate issue from what I wanted to solve in this patch.
> > So the following would work too:
> 
> Ha! It didn't occur to me either you could use designated initializers
> with _PICK. It's kind of neat... in a scary way. :)

Do you mean it starts to look like some python code?:)

> 
> We do miss the checks for incorrect indices throughout the code base. So
> I'm not that worried about it in this particular case. As you say, a
> separate issue.
> 
> I'd go with this change.

Ok, will follow up after checking the generated code.

> 
> BR,
> Jani.
> 
> 
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >b/drivers/gpu/drm/i915/i915_reg.h index 47baf2fe8f71..dfa547832dae
> >100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++
> >b/drivers/gpu/drm/i915/i915_reg.h @@ -7399,10 +7399,31 @@ enum {
> >#define BDW_DPRS_MASK_VBLANK_SRD (1 << 0) #define
> >CHICKEN_PIPESL_1(pipe) _MMIO_PIPE(pipe, _CHICKEN_PIPESL_1_A,
> >_CHICKEN_PIPESL_1_B)
> >  
> > -#define CHICKEN_TRANS_A		_MMIO(0x420c0)
> > -#define CHICKEN_TRANS_B		_MMIO(0x420c4)
> > -#define CHICKEN_TRANS_C		_MMIO(0x420c8)
> > -#define CHICKEN_TRANS_EDP	_MMIO(0x420cc)
> > +#define _CHICKEN_TRANS_A		0x420c0
> > +#define _CHICKEN_TRANS_B		0x420c4
> > +#define _CHICKEN_TRANS_C		0x420c8
> > +#define _CHICKEN_TRANS_EDP		0x420cc
> > +#define CHICKEN_TRANS(trans)		_MMIO(_PICK(trans, \
> > +						    [TRANSCODER_A] = \
> > +							_CHICKEN_TRANS_A, \
> > +						    [TRANSCODER_B] = \
> > +							_CHICKEN_TRANS_B, \
> > +						    [TRANSCODER_C] = \
> > +							_CHICKEN_TRANS_C, \
> > +						    [TRANSCODER_EDP] = \
> > +							_CHICKEN_TRANS_EDP))
> > +
> > +#define CHICKEN_TRANS_BYPORT(port)	_MMIO(_PICK(port, \
> > +						    [PORT_A] = \
> > +							_CHICKEN_TRANS_EDP, \
> > +						    [PORT_B] = \
> > +							_CHICKEN_TRANS_A, \
> > +						    [PORT_C] = \
> > +							_CHICKEN_TRANS_B, \
> > +						    [PORT_D] = \
> > +							_CHICKEN_TRANS_C, \
> > +						    [PORT_E] = \
> > +							_CHICKEN_TRANS_A))
> >  #define  VSC_DATA_SEL_SOFTWARE_CONTROL	(1 << 25) /* GLK and CNL+ */
> >  #define  DDI_TRAINING_OVERRIDE_ENABLE	(1 << 19)
> >  #define  DDI_TRAINING_OVERRIDE_VALUE	(1 << 18)
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index ad11540ac436..c261320330b6 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -3380,26 +3380,6 @@ static void intel_enable_ddi_dp(struct intel_encoder *encoder,
> >  		intel_audio_codec_enable(encoder, crtc_state, conn_state);
> >  }
> >  
> > -static i915_reg_t
> > -gen9_chicken_trans_reg_by_port(struct drm_i915_private *dev_priv,
> > -			       enum port port)
> > -{
> > -	static const i915_reg_t regs[] = {
> > -		[PORT_A] = CHICKEN_TRANS_EDP,
> > -		[PORT_B] = CHICKEN_TRANS_A,
> > -		[PORT_C] = CHICKEN_TRANS_B,
> > -		[PORT_D] = CHICKEN_TRANS_C,
> > -		[PORT_E] = CHICKEN_TRANS_A,
> > -	};
> > -
> > -	WARN_ON(INTEL_GEN(dev_priv) < 9);
> > -
> > -	if (WARN_ON(port < PORT_A || port > PORT_E))
> > -		port = PORT_A;
> > -
> > -	return regs[port];
> > -}
> > -
> >  static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
> >  				  const struct intel_crtc_state *crtc_state,
> >  				  const struct drm_connector_state *conn_state)
> > @@ -3423,10 +3403,7 @@ static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
> >  		 * the bits affect a specific DDI port rather than
> >  		 * a specific transcoder.
> >  		 */
> > -		i915_reg_t reg = gen9_chicken_trans_reg_by_port(dev_priv, port);
> > -		u32 val;
> > -
> > -		val = I915_READ(reg);
> > +		u32 val = I915_READ(CHICKEN_TRANS_BYPORT(port));
> >  
> >  		if (port == PORT_E)
> >  			val |= DDIE_TRAINING_OVERRIDE_ENABLE |
> > @@ -3435,8 +3412,8 @@ static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
> >  			val |= DDI_TRAINING_OVERRIDE_ENABLE |
> >  				DDI_TRAINING_OVERRIDE_VALUE;
> >  
> > -		I915_WRITE(reg, val);
> > -		POSTING_READ(reg);
> > +		I915_WRITE(CHICKEN_TRANS_BYPORT(port), val);
> > +		POSTING_READ(CHICKEN_TRANS_BYPORT(port));
> >  
> >  		udelay(1);
> >  
> > @@ -3447,7 +3424,7 @@ static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
> >  			val &= ~(DDI_TRAINING_OVERRIDE_ENABLE |
> >  				 DDI_TRAINING_OVERRIDE_VALUE);
> >  
> > -		I915_WRITE(reg, val);
> > +		I915_WRITE(CHICKEN_TRANS_BYPORT(port), val);
> >  	}
> >  
> >  	/* In HDMI/DVI mode, the port width, and swing/emphasis values
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 54fa17a5596a..76667c5d7800 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -577,25 +577,6 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
> >  	dev_priv->psr.active = true;
> >  }
> >  
> > -static i915_reg_t gen9_chicken_trans_reg(struct drm_i915_private *dev_priv,
> > -					 enum transcoder cpu_transcoder)
> > -{
> > -	static const i915_reg_t regs[] = {
> > -		[TRANSCODER_A] = CHICKEN_TRANS_A,
> > -		[TRANSCODER_B] = CHICKEN_TRANS_B,
> > -		[TRANSCODER_C] = CHICKEN_TRANS_C,
> > -		[TRANSCODER_EDP] = CHICKEN_TRANS_EDP,
> > -	};
> > -
> > -	WARN_ON(INTEL_GEN(dev_priv) < 9);
> > -
> > -	if (WARN_ON(cpu_transcoder >= ARRAY_SIZE(regs) ||
> > -		    !regs[cpu_transcoder].reg))
> > -		cpu_transcoder = TRANSCODER_A;
> > -
> > -	return regs[cpu_transcoder];
> > -}
> > -
> >  static void intel_psr_enable_source(struct intel_dp *intel_dp,
> >  				    const struct intel_crtc_state *crtc_state)
> >  {
> > @@ -610,9 +591,10 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
> >  		hsw_psr_setup_aux(intel_dp);
> >  
> >  	if (dev_priv->psr.psr2_enabled) {
> > -		i915_reg_t reg = gen9_chicken_trans_reg(dev_priv,
> > -							cpu_transcoder);
> > -		u32 chicken = I915_READ(reg);
> > +		u32 chicken;
> > +
> > +		WARN_ON(INTEL_GEN(dev_priv) < 9);
> > +		chicken = I915_READ(CHICKEN_TRANS(cpu_transcoder));
> >  
> >  		if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv))
> >  			chicken |= (PSR2_VSC_ENABLE_PROG_HEADER
> > @@ -620,7 +602,8 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
> >  
> >  		else
> >  			chicken &= ~VSC_DATA_SEL_SOFTWARE_CONTROL;
> > -		I915_WRITE(reg, chicken);
> > +
> > +		I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken);
> >  	}
> >  
> >  	/*
> >
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center