Re: [PATCH] drm/i915: Make CHICKEN_TRANS reg not depend on enum value

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 19, 2018 at 06:33:26PM +0200, Imre Deak 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)

s/trans/cpu_transcoder/ perhaps

> +{
> +	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];
> +}
> +
> +static i915_reg_t
> +gen9_chicken_trans_reg_by_port(struct drm_i915_private *dev_priv,
> +			       enum port port)
> +{
> +	static const enum transcoder port_to_transcoder[] = {
> +		[PORT_A] = TRANSCODER_EDP,
> +		[PORT_B] = TRANSCODER_A,
> +		[PORT_C] = TRANSCODER_B,
> +		[PORT_D] = TRANSCODER_C,
> +		[PORT_E] = TRANSCODER_A,
> +	};
> +
> +	if (WARN_ON(port < PORT_A || port > PORT_E))
> +		port = PORT_A;
> +
> +	return gen9_chicken_trans_reg(dev_priv, port_to_transcoder[port]);

Or we could just return the correct reg directly based on the port,
and  then gen9_chicken_trans_reg() can be moved into intel_psr.c
and made static.

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

> +}
> +
>  static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
>  				  const struct intel_crtc_state *crtc_state,
>  				  const struct drm_connector_state *conn_state)
> @@ -3403,17 +3439,10 @@ static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
>  		 * the bits affect a specific DDI port rather than
>  		 * a specific transcoder.
>  		 */
> -		static const enum transcoder port_to_transcoder[] = {
> -			[PORT_A] = TRANSCODER_EDP,
> -			[PORT_B] = TRANSCODER_A,
> -			[PORT_C] = TRANSCODER_B,
> -			[PORT_D] = TRANSCODER_C,
> -			[PORT_E] = TRANSCODER_A,
> -		};
> -		enum transcoder transcoder = port_to_transcoder[port];
> +		i915_reg_t reg = gen9_chicken_trans_reg_by_port(dev_priv, port);
>  		u32 val;
>  
> -		val = I915_READ(CHICKEN_TRANS(transcoder));
> +		val = I915_READ(reg);
>  
>  		if (port == PORT_E)
>  			val |= DDIE_TRAINING_OVERRIDE_ENABLE |
> @@ -3422,8 +3451,8 @@ static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
>  			val |= DDI_TRAINING_OVERRIDE_ENABLE |
>  				DDI_TRAINING_OVERRIDE_VALUE;
>  
> -		I915_WRITE(CHICKEN_TRANS(transcoder), val);
> -		POSTING_READ(CHICKEN_TRANS(transcoder));
> +		I915_WRITE(reg, val);
> +		POSTING_READ(reg);
>  
>  		udelay(1);
>  
> @@ -3434,7 +3463,7 @@ static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
>  			val &= ~(DDI_TRAINING_OVERRIDE_ENABLE |
>  				 DDI_TRAINING_OVERRIDE_VALUE);
>  
> -		I915_WRITE(CHICKEN_TRANS(transcoder), val);
> +		I915_WRITE(reg, val);
>  	}
>  
>  	/* In HDMI/DVI mode, the port width, and swing/emphasis values
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f575ba2a59da..0d064b71002b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1526,6 +1526,8 @@ void icl_sanitize_encoder_pll_mapping(struct intel_encoder *encoder);
>  
>  unsigned int intel_fb_align_height(const struct drm_framebuffer *fb,
>  				   int color_plane, unsigned int height);
> +i915_reg_t gen9_chicken_trans_reg(struct drm_i915_private *dev_priv,
> +				  enum transcoder trans);
>  
>  /* intel_audio.c */
>  void intel_init_audio_hooks(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 5fdc2f196045..b4d811c62473 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -606,7 +606,9 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
>  		hsw_psr_setup_aux(intel_dp);
>  
>  	if (dev_priv->psr.psr2_enabled) {
> -		u32 chicken = I915_READ(CHICKEN_TRANS(cpu_transcoder));
> +		i915_reg_t reg = gen9_chicken_trans_reg(dev_priv,
> +							cpu_transcoder);
> +		u32 chicken = I915_READ(reg);
>  
>  		if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv))
>  			chicken |= (PSR2_VSC_ENABLE_PROG_HEADER
> @@ -614,7 +616,7 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
>  
>  		else
>  			chicken &= ~VSC_DATA_SEL_SOFTWARE_CONTROL;
> -		I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken);
> +		I915_WRITE(reg, chicken);
>  	}
>  
>  	/*
> -- 
> 2.13.2

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