diff mbox series

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

Message ID 20181024222840.25683-22-manasi.d.navare@intel.com (mailing list archive)
State New, archived
Headers show
Series Display Stream Compression enabling on eDP/DP | expand

Commit Message

Navare, Manasi Oct. 24, 2018, 10:28 p.m. UTC
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(-)

Comments

Ville Syrjala Oct. 25, 2018, 2:08 p.m. UTC | #1
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?

>  	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.

>  	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.

> +
>  	val &= ~hsw_infoframe_enable(type);
>  	I915_WRITE(ctl_reg, val);
>  
> -- 
> 2.18.0
Navare, Manasi Oct. 29, 2018, 7:24 p.m. UTC | #2
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
diff mbox series

Patch

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;
 	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;
 	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);
+
 	val &= ~hsw_infoframe_enable(type);
 	I915_WRITE(ctl_reg, val);