Re: [PATCH v6 21/28] drm/i915/dp: Use the existing write_infoframe() for DSC PPS SDPs

[Manasi Navare <manasi.d.navare@intel.com>]

On Thu, Oct 25, 2018 at 05:08:39PM +0300, Ville Syrjälä wrote:
> On Wed, Oct 24, 2018 at 03:28:33PM -0700, Manasi Navare wrote:
> > Infoframes are used to send secondary data packets. This patch
> > adds support for DSC Picture parameter set secondary data packets
> > in the existing write_infoframe helpers.
> > 
> > v2:
> > * Rebase on drm-tip (Manasi)
> > 
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h   |  1 +
> >  drivers/gpu/drm/i915/intel_hdmi.c | 23 +++++++++++++++++++++--
> >  2 files changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 64cca0a83cf7..0ecdc95f56d8 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4545,6 +4545,7 @@ enum {
> >   * of the infoframe structure specified by CEA-861. */
> >  #define   VIDEO_DIP_DATA_SIZE	32
> >  #define   VIDEO_DIP_VSC_DATA_SIZE	36
> > +#define   VIDEO_DIP_PPS_DATA_SIZE	132
> >  #define VIDEO_DIP_CTL		_MMIO(0x61170)
> >  /* Pre HSW: */
> >  #define   VIDEO_DIP_ENABLE		(1 << 31)
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index d3e653640ce7..02fb54737d92 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -115,6 +115,8 @@ static u32 hsw_infoframe_enable(unsigned int type)
> >  	switch (type) {
> >  	case DP_SDP_VSC:
> >  		return VIDEO_DIP_ENABLE_VSC_HSW;
> > +	case DP_SDP_PPS:
> > +		return VDIP_ENABLE_PPS;
> 
> Hmm. Why is that bit named so differently to the rest?

Will have to address the mess around VDIP_CVTL reg earlier defs in a separate patch set

> 
> >  	case HDMI_INFOFRAME_TYPE_AVI:
> >  		return VIDEO_DIP_ENABLE_AVI_HSW;
> >  	case HDMI_INFOFRAME_TYPE_SPD:
> > @@ -136,6 +138,8 @@ hsw_dip_data_reg(struct drm_i915_private *dev_priv,
> >  	switch (type) {
> >  	case DP_SDP_VSC:
> >  		return HSW_TVIDEO_DIP_VSC_DATA(cpu_transcoder, i);
> > +	case DP_SDP_PPS:
> > +		return ICL_VIDEO_DIP_PPS_DATA(cpu_transcoder, i);
> >  	case HDMI_INFOFRAME_TYPE_AVI:
> >  		return HSW_TVIDEO_DIP_AVI_DATA(cpu_transcoder, i);
> >  	case HDMI_INFOFRAME_TYPE_SPD:
> > @@ -148,6 +152,18 @@ hsw_dip_data_reg(struct drm_i915_private *dev_priv,
> >  	}
> >  }
> >  
> > +static int hsw_dip_data_size(unsigned int type)
> > +{
> > +	switch (type) {
> > +	case DP_SDP_VSC:
> > +		return VIDEO_DIP_VSC_DATA_SIZE;
> > +	case DP_SDP_PPS:
> > +		return VIDEO_DIP_PPS_DATA_SIZE;
> > +	default:
> > +		return VIDEO_DIP_DATA_SIZE;
> > +	}
> > +}
> > +
> >  static void g4x_write_infoframe(struct intel_encoder *encoder,
> >  				const struct intel_crtc_state *crtc_state,
> >  				unsigned int type,
> > @@ -382,11 +398,14 @@ static void hsw_write_infoframe(struct intel_encoder *encoder,
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> >  	i915_reg_t ctl_reg = HSW_TVIDEO_DIP_CTL(cpu_transcoder);
> > -	int data_size = type == DP_SDP_VSC ?
> > -		VIDEO_DIP_VSC_DATA_SIZE : VIDEO_DIP_DATA_SIZE;
> > +	i915_reg_t data_reg;
> > +	int data_size = 0;
> 
> =0 is unnecessary.

This was added to adderss a warning that data_size is uninitialized.
But will double check again if its really needed.

> 
> >  	int i;
> >  	u32 val = I915_READ(ctl_reg);
> >  
> > +	data_size = hsw_dip_data_size(type);
> > +	data_reg = hsw_dip_data_reg(dev_priv, cpu_transcoder, type, 0);
> 
> data_reg is unused.

Yes I think the cleanup patch series that was sent sometime recently now
uses hsw_dip_data_reg directly in I915_WRITE call.
I will remove the data_reg.

Manasi

> 
> > +
> >  	val &= ~hsw_infoframe_enable(type);
> >  	I915_WRITE(ctl_reg, val);
> >  
> > -- 
> > 2.18.0
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx