Re: [PATCH 24/43] drm/i915: Parametrize HSW video DIP data registers

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On 09/18/2015 10:03 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h   | 16 ++++++++--------
>  drivers/gpu/drm/i915/intel_hdmi.c | 26 ++++++++++++++------------
>  drivers/gpu/drm/i915/intel_psr.c  | 18 ++++++++++--------
>  3 files changed, 32 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 134b075..b35e24f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6236,16 +6236,16 @@ enum skl_disp_power_wells {
>  
>  #define HSW_TVIDEO_DIP_CTL(trans) \
>  	 _TRANSCODER2(trans, HSW_VIDEO_DIP_CTL_A)
> -#define HSW_TVIDEO_DIP_AVI_DATA(trans) \
> -	 _TRANSCODER2(trans, HSW_VIDEO_DIP_AVI_DATA_A)
> -#define HSW_TVIDEO_DIP_VS_DATA(trans) \
> -	 _TRANSCODER2(trans, HSW_VIDEO_DIP_VS_DATA_A)
> -#define HSW_TVIDEO_DIP_SPD_DATA(trans) \
> -	 _TRANSCODER2(trans, HSW_VIDEO_DIP_SPD_DATA_A)
> +#define HSW_TVIDEO_DIP_AVI_DATA(trans, i) \
> +	(_TRANSCODER2(trans, HSW_VIDEO_DIP_AVI_DATA_A) + (i) * 4)
> +#define HSW_TVIDEO_DIP_VS_DATA(trans, i) \
> +	(_TRANSCODER2(trans, HSW_VIDEO_DIP_VS_DATA_A) + (i) * 4)
> +#define HSW_TVIDEO_DIP_SPD_DATA(trans, i) \
> +	(_TRANSCODER2(trans, HSW_VIDEO_DIP_SPD_DATA_A) + (i) * 4)
>  #define HSW_TVIDEO_DIP_GCP(trans) \
>  	_TRANSCODER2(trans, HSW_VIDEO_DIP_GCP_A)
> -#define HSW_TVIDEO_DIP_VSC_DATA(trans) \
> -	 _TRANSCODER2(trans, HSW_VIDEO_DIP_VSC_DATA_A)
> +#define HSW_TVIDEO_DIP_VSC_DATA(trans, i) \
> +	(_TRANSCODER2(trans, HSW_VIDEO_DIP_VSC_DATA_A) + (i) * 4)
>  
>  #define HSW_STEREO_3D_CTL_A	0x70020
>  #define   S3D_ENABLE		(1<<31)
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index e978c59..6b16292 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -113,17 +113,18 @@ static u32 hsw_infoframe_enable(enum hdmi_infoframe_type type)
>  	}
>  }
>  
> -static u32 hsw_infoframe_data_reg(enum hdmi_infoframe_type type,
> -				  enum transcoder cpu_transcoder,
> -				  struct drm_i915_private *dev_priv)
> +static u32 hsw_dip_data_reg(struct drm_i915_private *dev_priv,
> +			    enum transcoder cpu_transcoder,
> +			    enum hdmi_infoframe_type type,
> +			    int i)
>  {
>  	switch (type) {
>  	case HDMI_INFOFRAME_TYPE_AVI:
> -		return HSW_TVIDEO_DIP_AVI_DATA(cpu_transcoder);
> +		return HSW_TVIDEO_DIP_AVI_DATA(cpu_transcoder, i);
>  	case HDMI_INFOFRAME_TYPE_SPD:
> -		return HSW_TVIDEO_DIP_SPD_DATA(cpu_transcoder);
> +		return HSW_TVIDEO_DIP_SPD_DATA(cpu_transcoder, i);
>  	case HDMI_INFOFRAME_TYPE_VENDOR:
> -		return HSW_TVIDEO_DIP_VS_DATA(cpu_transcoder);
> +		return HSW_TVIDEO_DIP_VS_DATA(cpu_transcoder, i);
>  	default:
>  		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
>  		return 0;
> @@ -365,14 +366,13 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
>  	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> -	u32 ctl_reg = HSW_TVIDEO_DIP_CTL(intel_crtc->config->cpu_transcoder);
> +	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
> +	u32 ctl_reg = HSW_TVIDEO_DIP_CTL(cpu_transcoder);
>  	u32 data_reg;
>  	int i;
>  	u32 val = I915_READ(ctl_reg);
>  
> -	data_reg = hsw_infoframe_data_reg(type,
> -					  intel_crtc->config->cpu_transcoder,
> -					  dev_priv);
> +	data_reg = hsw_dip_data_reg(dev_priv, cpu_transcoder, type, 0);
>  	if (data_reg == 0)
>  		return;
>  
> @@ -381,12 +381,14 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
>  
>  	mmiowb();
>  	for (i = 0; i < len; i += 4) {
> -		I915_WRITE(data_reg + i, *data);
> +		I915_WRITE(hsw_dip_data_reg(dev_priv, cpu_transcoder,
> +					    type, i >> 2), *data);
>  		data++;
>  	}
>  	/* Write every possible data byte to force correct ECC calculation. */
>  	for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
> -		I915_WRITE(data_reg + i, 0);
> +		I915_WRITE(hsw_dip_data_reg(dev_priv, cpu_transcoder,
> +					    type, i >> 2), 0);
>  	mmiowb();
>  
>  	val |= hsw_infoframe_enable(type);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index a04b4dc..213581c 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -73,14 +73,14 @@ static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
>  }
>  
>  static void intel_psr_write_vsc(struct intel_dp *intel_dp,
> -				    struct edp_vsc_psr *vsc_psr)
> +				const struct edp_vsc_psr *vsc_psr)
>  {
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
> -	u32 ctl_reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
> -	u32 data_reg = HSW_TVIDEO_DIP_VSC_DATA(crtc->config->cpu_transcoder);
> +	enum transcoder cpu_transcoder = crtc->config->cpu_transcoder;
> +	u32 ctl_reg = HSW_TVIDEO_DIP_CTL(cpu_transcoder);
>  	uint32_t *data = (uint32_t *) vsc_psr;
>  	unsigned int i;
>  
> @@ -90,12 +90,14 @@ static void intel_psr_write_vsc(struct intel_dp *intel_dp,
>  	I915_WRITE(ctl_reg, 0);
>  	POSTING_READ(ctl_reg);
>  
> -	for (i = 0; i < VIDEO_DIP_VSC_DATA_SIZE; i += 4) {
> -		if (i < sizeof(struct edp_vsc_psr))
> -			I915_WRITE(data_reg + i, *data++);
> -		else
> -			I915_WRITE(data_reg + i, 0);
> +	for (i = 0; i < sizeof(*vsc_psr); i += 4) {
> +		I915_WRITE(HSW_TVIDEO_DIP_VSC_DATA(cpu_transcoder,
> +						   i >> 2), *data);
> +		data++;
>  	}
> +	for (; i < VIDEO_DIP_VSC_DATA_SIZE; i += 4)
> +		I915_WRITE(HSW_TVIDEO_DIP_VSC_DATA(cpu_transcoder,
> +						   i >> 2), 0);
>  
>  	I915_WRITE(ctl_reg, VIDEO_DIP_ENABLE_VSC_HSW);
>  	POSTING_READ(ctl_reg);
> 

Since you fixed the macro to use a *4 for the reg index, it might be
clearer to fix up the loop to just use i++ instead?  I guess you'd then
have to divide the condition, so meh (or maybe we need a DWORDS(bytes)
macro!).  Either way:

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx