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

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

On Mon, Oct 12, 2015 at 08:54:56AM -0700, Jesse Barnes wrote:
> 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:

I had it both ways during development, but then I figured we should
try to do it the same way always. All other registers like this were
already indexed in dwords, and both the AUX code and the rest of the
infoframe code were counting the data in bytes, so I stuck to the
same approach in the end.

> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

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