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

Jesse Barnes
2015-10-12T15:54:09Z

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>