Re: [PATCH 6/7] drm/i915: Remove mostly duplicated video DIP handling from PSR code

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

On Mon, Jun 13, 2016 at 05:39:47PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> 
> On 6/3/2016 1:25 AM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Now that the infoframe hooks are part of the intel_dig_port, we can use
> > the normal .write_infoframe() hook to update the VSC SDP. We do need to
> > deal with the size difference between the VSC DIP and the others though.
> >
> > Another minor snag is that the compiler will complain to use if we keep
> > using enum hdmi_infoframe_type type and passing in the DP define instead,
> > so et's just change to unsigned int all over for the inforframe type.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_drv.h  |  2 +-
> >   drivers/gpu/drm/i915/intel_hdmi.c | 26 +++++++++++++++----------
> >   drivers/gpu/drm/i915/intel_psr.c  | 41 ++++++++-------------------------------
> >   3 files changed, 25 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 4c8451e3d8f1..5dcaa14ff90d 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -900,7 +900,7 @@ struct intel_digital_port {
> >   	/* for communication with audio component; protected by av_mutex */
> >   	const struct drm_connector *audio_connector;
> >   	void (*write_infoframe)(struct drm_encoder *encoder,
> > -				enum hdmi_infoframe_type type,
> > +				unsigned int type,
> >   				const void *frame, ssize_t len);
> >   	void (*set_infoframes)(struct drm_encoder *encoder,
> >   			       bool enable,
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 637b17baf798..600a58210450 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -68,7 +68,7 @@ static struct intel_hdmi *intel_attached_hdmi(struct drm_connector *connector)
> >   	return enc_to_intel_hdmi(&intel_attached_encoder(connector)->base);
> >   }
> >
> > -static u32 g4x_infoframe_index(enum hdmi_infoframe_type type)
> > +static u32 g4x_infoframe_index(unsigned int type)
> >   {
> >   	switch (type) {
> >   	case HDMI_INFOFRAME_TYPE_AVI:
> > @@ -83,7 +83,7 @@ static u32 g4x_infoframe_index(enum hdmi_infoframe_type type)
> >   	}
> >   }
> >
> > -static u32 g4x_infoframe_enable(enum hdmi_infoframe_type type)
> > +static u32 g4x_infoframe_enable(unsigned int type)
> >   {
> >   	switch (type) {
> >   	case HDMI_INFOFRAME_TYPE_AVI:
> > @@ -98,9 +98,11 @@ static u32 g4x_infoframe_enable(enum hdmi_infoframe_type type)
> >   	}
> >   }
> >
> > -static u32 hsw_infoframe_enable(enum hdmi_infoframe_type type)
> > +static u32 hsw_infoframe_enable(unsigned int type)
> >   {
> >   	switch (type) {
> > +	case DP_SDP_VSC:
> Why do we need a new field like this ? Would it make sense to set the 
> type itself as "HDMI_INFOFRAME_TYPE_AVI" ?

We want to enable/write the VSC, not the AVI.

> > +		return VIDEO_DIP_ENABLE_VSC_HSW;
> >   	case HDMI_INFOFRAME_TYPE_AVI:
> >   		return VIDEO_DIP_ENABLE_AVI_HSW;
> >   	case HDMI_INFOFRAME_TYPE_SPD:
> > @@ -116,10 +118,12 @@ static u32 hsw_infoframe_enable(enum hdmi_infoframe_type type)
> >   static i915_reg_t
> >   hsw_dip_data_reg(struct drm_i915_private *dev_priv,
> >   		 enum transcoder cpu_transcoder,
> > -		 enum hdmi_infoframe_type type,
> > +		 unsigned int type,
> >   		 int i)
> >   {
> >   	switch (type) {
> > +	case DP_SDP_VSC:
> > +		return HSW_TVIDEO_DIP_VSC_DATA(cpu_transcoder, i);
> >   	case HDMI_INFOFRAME_TYPE_AVI:
> >   		return HSW_TVIDEO_DIP_AVI_DATA(cpu_transcoder, i);
> >   	case HDMI_INFOFRAME_TYPE_SPD:
> > @@ -133,7 +137,7 @@ hsw_dip_data_reg(struct drm_i915_private *dev_priv,
> >   }
> >
> >   static void g4x_write_infoframe(struct drm_encoder *encoder,
> > -				enum hdmi_infoframe_type type,
> > +				unsigned int type,
> >   				const void *frame, ssize_t len)
> >   {
> >   	const uint32_t *data = frame;
> > @@ -187,7 +191,7 @@ static bool g4x_infoframe_enabled(struct drm_encoder *encoder,
> >   }
> >
> >   static void ibx_write_infoframe(struct drm_encoder *encoder,
> > -				enum hdmi_infoframe_type type,
> > +				unsigned int type,
> >   				const void *frame, ssize_t len)
> >   {
> >   	const uint32_t *data = frame;
> > @@ -246,7 +250,7 @@ static bool ibx_infoframe_enabled(struct drm_encoder *encoder,
> >   }
> >
> >   static void cpt_write_infoframe(struct drm_encoder *encoder,
> > -				enum hdmi_infoframe_type type,
> > +				unsigned int type,
> >   				const void *frame, ssize_t len)
> >   {
> >   	const uint32_t *data = frame;
> > @@ -303,7 +307,7 @@ static bool cpt_infoframe_enabled(struct drm_encoder *encoder,
> >   }
> >
> >   static void vlv_write_infoframe(struct drm_encoder *encoder,
> > -				enum hdmi_infoframe_type type,
> > +				unsigned int type,
> >   				const void *frame, ssize_t len)
> >   {
> >   	const uint32_t *data = frame;
> > @@ -361,7 +365,7 @@ static bool vlv_infoframe_enabled(struct drm_encoder *encoder,
> >   }
> >
> >   static void hsw_write_infoframe(struct drm_encoder *encoder,
> > -				enum hdmi_infoframe_type type,
> > +				unsigned int type,
> >   				const void *frame, ssize_t len)
> >   {
> >   	const uint32_t *data = frame;
> > @@ -371,6 +375,8 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
> >   	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
> >   	i915_reg_t ctl_reg = HSW_TVIDEO_DIP_CTL(cpu_transcoder);
> >   	i915_reg_t data_reg;
> > +	int data_size = type == DP_SDP_VSC ?
> Ok, I think may be this is the reason why we need a separate filed for 
> DP_SDP_VSC, is it so ?

No, we need to regarless of the size of the buffer.

> 
> - Shashank
> > +		VIDEO_DIP_VSC_DATA_SIZE : VIDEO_DIP_DATA_SIZE;
> >   	int i;
> >   	u32 val = I915_READ(ctl_reg);
> >
> > @@ -386,7 +392,7 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
> >   		data++;
> >   	}
> >   	/* Write every possible data byte to force correct ECC calculation. */
> > -	for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
> > +	for (; i < data_size; i += 4)
> >   		I915_WRITE(hsw_dip_data_reg(dev_priv, cpu_transcoder,
> >   					    type, i >> 2), 0);
> >   	mmiowb();
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 29a09bf6bd18..904994dd1c9e 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -72,37 +72,6 @@ static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
> >   	       (val == VLV_EDP_PSR_ACTIVE_SF_UPDATE);
> >   }
> >
> > -static void intel_psr_write_vsc(struct intel_dp *intel_dp,
> > -				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);
> > -	enum transcoder cpu_transcoder = crtc->config->cpu_transcoder;
> > -	i915_reg_t ctl_reg = HSW_TVIDEO_DIP_CTL(cpu_transcoder);
> > -	uint32_t *data = (uint32_t *) vsc_psr;
> > -	unsigned int i;
> > -
> > -	/* As per BSPec (Pipe Video Data Island Packet), we need to disable
> > -	   the video DIP being updated before program video DIP data buffer
> > -	   registers for DIP being updated. */
> > -	I915_WRITE(ctl_reg, 0);
> > -	POSTING_READ(ctl_reg);
> > -
> > -	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);
> > -}
> > -
> >   static void vlv_psr_setup_vsc(struct intel_dp *intel_dp)
> >   {
> >   	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > @@ -121,6 +90,7 @@ static void vlv_psr_setup_vsc(struct intel_dp *intel_dp)
> >
> >   static void skl_psr_setup_su_vsc(struct intel_dp *intel_dp)
> >   {
> > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> >   	struct edp_vsc_psr psr_vsc;
> >
> >   	/* Prepare VSC Header for SU as per EDP 1.4 spec, Table 6.11 */
> > @@ -129,11 +99,14 @@ static void skl_psr_setup_su_vsc(struct intel_dp *intel_dp)
> >   	psr_vsc.sdp_header.HB1 = 0x7;
> >   	psr_vsc.sdp_header.HB2 = 0x3;
> >   	psr_vsc.sdp_header.HB3 = 0xb;
> > -	intel_psr_write_vsc(intel_dp, &psr_vsc);
> > +
> > +	intel_dig_port->write_infoframe(&intel_dig_port->base.base,
> > +					DP_SDP_VSC, &psr_vsc, sizeof(psr_vsc));
> >   }
> >
> >   static void hsw_psr_setup_vsc(struct intel_dp *intel_dp)
> >   {
> > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> >   	struct edp_vsc_psr psr_vsc;
> >
> >   	/* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
> > @@ -142,7 +115,9 @@ static void hsw_psr_setup_vsc(struct intel_dp *intel_dp)
> >   	psr_vsc.sdp_header.HB1 = 0x7;
> >   	psr_vsc.sdp_header.HB2 = 0x2;
> >   	psr_vsc.sdp_header.HB3 = 0x8;
> > -	intel_psr_write_vsc(intel_dp, &psr_vsc);
> > +
> > +	intel_dig_port->write_infoframe(&intel_dig_port->base.base,
> > +					DP_SDP_VSC, &psr_vsc, sizeof(psr_vsc));
> >   }
> >
> >   static void vlv_psr_enable_sink(struct intel_dp *intel_dp)
> >

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