diff mbox

[v2,6/9] drm/i915: Disable all infoframes when turning off the HDMI port

Message ID 1430834787-10255-7-git-send-email-ville.syrjala@linux.intel.com
State New, archived
Headers show

Commit Message

Ville Syrjälä May 5, 2015, 2:06 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we just disable the GCP infoframe when turning off the port.
That means if the same transcoder is used on a DP port next, we might
end up pushing infoframes over DP, which isn't intended. Just disable
all the infoframes when turning off the port.

Also protect against two ports stomping on each other on g4x due to
the single video DIP instance. Now only the first port to enable
gets to send infoframes.

v2: Rebase

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 85 ++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 45 deletions(-)

Comments

Chandra Konduru June 1, 2015, 10:48 p.m. UTC | #1
> -----Original Message-----

> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of

> ville.syrjala@linux.intel.com

> Sent: Tuesday, May 05, 2015 7:06 AM

> To: intel-gfx@lists.freedesktop.org

> Subject: [Intel-gfx] [PATCH v2 6/9] drm/i915: Disable all infoframes when

> turning off the HDMI port

> 

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 

> Currently we just disable the GCP infoframe when turning off the port.

> That means if the same transcoder is used on a DP port next, we might

> end up pushing infoframes over DP, which isn't intended. Just disable


Wonder how it is working. May be it is ok, or never hit using a previously 
used transcoder for HDMI port for DP.

By the way, you mean end up pushing "other" infoframes over DP?

> all the infoframes when turning off the port.

> 

> Also protect against two ports stomping on each other on g4x due to

> the single video DIP instance. Now only the first port to enable

> gets to send infoframes.


So is, 2nd port doesn't gets to send infoframes, the expected behavior 
when two ports trying with a single DIP?

Seems like a corner case to test thoroughly. Is this path exercised in your testing?

> 

> v2: Rebase

> 

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---

>  drivers/gpu/drm/i915/intel_hdmi.c | 85 ++++++++++++++++++---------------------

>  1 file changed, 40 insertions(+), 45 deletions(-)

> 

> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c

> b/drivers/gpu/drm/i915/intel_hdmi.c

> index 766bdb1..03b4759 100644

> --- a/drivers/gpu/drm/i915/intel_hdmi.c

> +++ b/drivers/gpu/drm/i915/intel_hdmi.c

> @@ -514,7 +514,13 @@ static void g4x_set_infoframes(struct drm_encoder

> *encoder,

>  	if (!enable) {

>  		if (!(val & VIDEO_DIP_ENABLE))

>  			return;

> -		val &= ~VIDEO_DIP_ENABLE;

> +		if (port != (val & VIDEO_DIP_PORT_MASK)) {

> +			DRM_DEBUG_KMS("video DIP still enabled on port

> %c\n",

> +				      (val & VIDEO_DIP_PORT_MASK) >> 29);

> +			return;

> +		}

> +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |

> +			 VIDEO_DIP_ENABLE_VENDOR |

> VIDEO_DIP_ENABLE_SPD);

>  		I915_WRITE(reg, val);

>  		POSTING_READ(reg);

>  		return;

> @@ -522,16 +528,17 @@ static void g4x_set_infoframes(struct drm_encoder

> *encoder,

> 

>  	if (port != (val & VIDEO_DIP_PORT_MASK)) {

>  		if (val & VIDEO_DIP_ENABLE) {

> -			val &= ~VIDEO_DIP_ENABLE;

> -			I915_WRITE(reg, val);

> -			POSTING_READ(reg);

> +			DRM_DEBUG_KMS("video DIP already enabled on port

> %c\n",

> +				      (val & VIDEO_DIP_PORT_MASK) >> 29);

> +			return;

>  		}

>  		val &= ~VIDEO_DIP_PORT_MASK;

>  		val |= port;

>  	}

> 

>  	val |= VIDEO_DIP_ENABLE;

> -	val &= ~VIDEO_DIP_ENABLE_VENDOR;

> +	val &= ~(VIDEO_DIP_ENABLE_AVI |

> +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_SPD);

> 

>  	I915_WRITE(reg, val);

>  	POSTING_READ(reg);

> @@ -632,23 +639,6 @@ static bool intel_hdmi_set_gcp_infoframe(struct

> drm_encoder *encoder)

>  	return val != 0;

>  }

> 

> -static void intel_disable_gcp_infoframe(struct intel_crtc *crtc)

> -{

> -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;

> -	u32 reg;

> -

> -	if (HAS_DDI(dev_priv))

> -		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);

> -	else if (IS_VALLEYVIEW(dev_priv))

> -		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);

> -	else if (HAS_PCH_SPLIT(dev_priv->dev))

> -		reg = TVIDEO_DIP_CTL(crtc->pipe);

> -	else

> -		return;

> -

> -	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP);

> -}

> -

>  static void ibx_set_infoframes(struct drm_encoder *encoder,

>  			       bool enable,

>  			       struct drm_display_mode *adjusted_mode)

> @@ -669,25 +659,26 @@ static void ibx_set_infoframes(struct drm_encoder

> *encoder,

>  	if (!enable) {

>  		if (!(val & VIDEO_DIP_ENABLE))

>  			return;

> -		val &= ~VIDEO_DIP_ENABLE;

> +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |

> +			 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |

> +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);

>  		I915_WRITE(reg, val);

>  		POSTING_READ(reg);

>  		return;

>  	}

> 

>  	if (port != (val & VIDEO_DIP_PORT_MASK)) {

> -		if (val & VIDEO_DIP_ENABLE) {

> -			val &= ~VIDEO_DIP_ENABLE;

> -			I915_WRITE(reg, val);

> -			POSTING_READ(reg);

> -		}

> +		WARN(val & VIDEO_DIP_ENABLE,

> +		     "DIP already enabled on port %c\n",

> +		     (val & VIDEO_DIP_PORT_MASK) >> 29);

>  		val &= ~VIDEO_DIP_PORT_MASK;

>  		val |= port;

>  	}

> 

>  	val |= VIDEO_DIP_ENABLE;

> -	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |

> -		 VIDEO_DIP_ENABLE_GCP);

> +	val &= ~(VIDEO_DIP_ENABLE_AVI |

> +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |

> +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);

> 

>  	if (intel_hdmi_set_gcp_infoframe(encoder))

>  		val |= VIDEO_DIP_ENABLE_GCP;

> @@ -718,7 +709,9 @@ static void cpt_set_infoframes(struct drm_encoder

> *encoder,

>  	if (!enable) {

>  		if (!(val & VIDEO_DIP_ENABLE))

>  			return;

> -		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI);

> +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |

> +			 VIDEO_DIP_ENABLE_VENDOR |

> VIDEO_DIP_ENABLE_GAMUT |

> +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);

>  		I915_WRITE(reg, val);

>  		POSTING_READ(reg);

>  		return;

> @@ -727,7 +720,7 @@ static void cpt_set_infoframes(struct drm_encoder

> *encoder,

>  	/* Set both together, unset both together: see the spec. */

>  	val |= VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI;

>  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |

> -		 VIDEO_DIP_ENABLE_GCP);

> +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);

> 

>  	if (intel_hdmi_set_gcp_infoframe(encoder))

>  		val |= VIDEO_DIP_ENABLE_GCP;

> @@ -760,25 +753,26 @@ static void vlv_set_infoframes(struct drm_encoder

> *encoder,

>  	if (!enable) {

>  		if (!(val & VIDEO_DIP_ENABLE))

>  			return;

> -		val &= ~VIDEO_DIP_ENABLE;

> +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |

> +			 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |

> +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);

>  		I915_WRITE(reg, val);

>  		POSTING_READ(reg);

>  		return;

>  	}

> 

>  	if (port != (val & VIDEO_DIP_PORT_MASK)) {

> -		if (val & VIDEO_DIP_ENABLE) {

> -			val &= ~VIDEO_DIP_ENABLE;

> -			I915_WRITE(reg, val);

> -			POSTING_READ(reg);

> -		}

> +		WARN(val & VIDEO_DIP_ENABLE,

> +		     "DIP already enabled on port %c\n",

> +		     (val & VIDEO_DIP_PORT_MASK) >> 29);

>  		val &= ~VIDEO_DIP_PORT_MASK;

>  		val |= port;

>  	}

> 

>  	val |= VIDEO_DIP_ENABLE;

> -	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |

> -		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);

> +	val &= ~(VIDEO_DIP_ENABLE_AVI |

> +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |

> +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);

> 

>  	if (intel_hdmi_set_gcp_infoframe(encoder))

>  		val |= VIDEO_DIP_ENABLE_GCP;

> @@ -803,15 +797,16 @@ static void hsw_set_infoframes(struct drm_encoder

> *encoder,

> 

>  	assert_hdmi_port_disabled(intel_hdmi);

> 

> +	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_AVI_HSW |

> +		 VIDEO_DIP_ENABLE_GCP_HSW | VIDEO_DIP_ENABLE_VS_HSW |

> +		 VIDEO_DIP_ENABLE_GMP_HSW | VIDEO_DIP_ENABLE_SPD_HSW);

> +

>  	if (!enable) {

> -		I915_WRITE(reg, 0);

> +		I915_WRITE(reg, val);

>  		POSTING_READ(reg);

>  		return;

>  	}

> 

> -	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_GCP_HSW |

> -		 VIDEO_DIP_ENABLE_VS_HSW | VIDEO_DIP_ENABLE_GMP_HSW);

> -

>  	if (intel_hdmi_set_gcp_infoframe(encoder))

>  		val |= VIDEO_DIP_ENABLE_GCP_HSW;

> 

> @@ -1133,7 +1128,7 @@ static void intel_disable_hdmi(struct intel_encoder

> *encoder)

>  	if (IS_CHERRYVIEW(dev))

>  		chv_powergate_phy_lanes(encoder, 0xf);

> 

> -	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));

> +	intel_hdmi->set_infoframes(&encoder->base, false, NULL);

>  }

> 

>  static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)

> --

> 2.0.5

> 

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org

> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä June 2, 2015, 11:11 a.m. UTC | #2
On Mon, Jun 01, 2015 at 10:48:03PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> > ville.syrjala@linux.intel.com
> > Sent: Tuesday, May 05, 2015 7:06 AM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH v2 6/9] drm/i915: Disable all infoframes when
> > turning off the HDMI port
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently we just disable the GCP infoframe when turning off the port.
> > That means if the same transcoder is used on a DP port next, we might
> > end up pushing infoframes over DP, which isn't intended. Just disable
> 
> Wonder how it is working. May be it is ok, or never hit using a previously 
> used transcoder for HDMI port for DP.
> 
> By the way, you mean end up pushing "other" infoframes over DP?

We don't send infoframes over DP at all currently. Or I should say we
never intended to send them. After this patch that should even be true.
Well, unless the BIOS already enables them and we just fire up a DP port
using the transcoder in question. So I suppose we should really have the
DP code clear the infoframe settings explicitly, or we should clear them
during the sanitize phase.

> 
> > all the infoframes when turning off the port.
> > 
> > Also protect against two ports stomping on each other on g4x due to
> > the single video DIP instance. Now only the first port to enable
> > gets to send infoframes.
> 
> So is, 2nd port doesn't gets to send infoframes, the expected behavior 
> when two ports trying with a single DIP?

I'm not sure what's the best behaviour here. Either we somehow pick one
of the ports to send infoframes (first come first serve in this patch),
or we could just disable infoframes entirely for cloned cases. But it's
probably a rare configuration anyway since HDMI cloning is only allowed
on g4x.

> 
> Seems like a corner case to test thoroughly. Is this path exercised in your testing?

I tested it long ago. Although I must admit that the patch looked a bit
different back then.

> 
> > 
> > v2: Rebase
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_hdmi.c | 85 ++++++++++++++++++---------------------
> >  1 file changed, 40 insertions(+), 45 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 766bdb1..03b4759 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -514,7 +514,13 @@ static void g4x_set_infoframes(struct drm_encoder
> > *encoder,
> >  	if (!enable) {
> >  		if (!(val & VIDEO_DIP_ENABLE))
> >  			return;
> > -		val &= ~VIDEO_DIP_ENABLE;
> > +		if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > +			DRM_DEBUG_KMS("video DIP still enabled on port
> > %c\n",
> > +				      (val & VIDEO_DIP_PORT_MASK) >> 29);
> > +			return;
> > +		}
> > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > +			 VIDEO_DIP_ENABLE_VENDOR |
> > VIDEO_DIP_ENABLE_SPD);
> >  		I915_WRITE(reg, val);
> >  		POSTING_READ(reg);
> >  		return;
> > @@ -522,16 +528,17 @@ static void g4x_set_infoframes(struct drm_encoder
> > *encoder,
> > 
> >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> >  		if (val & VIDEO_DIP_ENABLE) {
> > -			val &= ~VIDEO_DIP_ENABLE;
> > -			I915_WRITE(reg, val);
> > -			POSTING_READ(reg);
> > +			DRM_DEBUG_KMS("video DIP already enabled on port
> > %c\n",
> > +				      (val & VIDEO_DIP_PORT_MASK) >> 29);
> > +			return;
> >  		}
> >  		val &= ~VIDEO_DIP_PORT_MASK;
> >  		val |= port;
> >  	}
> > 
> >  	val |= VIDEO_DIP_ENABLE;
> > -	val &= ~VIDEO_DIP_ENABLE_VENDOR;
> > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_SPD);
> > 
> >  	I915_WRITE(reg, val);
> >  	POSTING_READ(reg);
> > @@ -632,23 +639,6 @@ static bool intel_hdmi_set_gcp_infoframe(struct
> > drm_encoder *encoder)
> >  	return val != 0;
> >  }
> > 
> > -static void intel_disable_gcp_infoframe(struct intel_crtc *crtc)
> > -{
> > -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > -	u32 reg;
> > -
> > -	if (HAS_DDI(dev_priv))
> > -		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
> > -	else if (IS_VALLEYVIEW(dev_priv))
> > -		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
> > -	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > -		reg = TVIDEO_DIP_CTL(crtc->pipe);
> > -	else
> > -		return;
> > -
> > -	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP);
> > -}
> > -
> >  static void ibx_set_infoframes(struct drm_encoder *encoder,
> >  			       bool enable,
> >  			       struct drm_display_mode *adjusted_mode)
> > @@ -669,25 +659,26 @@ static void ibx_set_infoframes(struct drm_encoder
> > *encoder,
> >  	if (!enable) {
> >  		if (!(val & VIDEO_DIP_ENABLE))
> >  			return;
> > -		val &= ~VIDEO_DIP_ENABLE;
> > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > +			 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> >  		I915_WRITE(reg, val);
> >  		POSTING_READ(reg);
> >  		return;
> >  	}
> > 
> >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > -		if (val & VIDEO_DIP_ENABLE) {
> > -			val &= ~VIDEO_DIP_ENABLE;
> > -			I915_WRITE(reg, val);
> > -			POSTING_READ(reg);
> > -		}
> > +		WARN(val & VIDEO_DIP_ENABLE,
> > +		     "DIP already enabled on port %c\n",
> > +		     (val & VIDEO_DIP_PORT_MASK) >> 29);
> >  		val &= ~VIDEO_DIP_PORT_MASK;
> >  		val |= port;
> >  	}
> > 
> >  	val |= VIDEO_DIP_ENABLE;
> > -	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > -		 VIDEO_DIP_ENABLE_GCP);
> > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > 
> >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> >  		val |= VIDEO_DIP_ENABLE_GCP;
> > @@ -718,7 +709,9 @@ static void cpt_set_infoframes(struct drm_encoder
> > *encoder,
> >  	if (!enable) {
> >  		if (!(val & VIDEO_DIP_ENABLE))
> >  			return;
> > -		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI);
> > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > +			 VIDEO_DIP_ENABLE_VENDOR |
> > VIDEO_DIP_ENABLE_GAMUT |
> > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> >  		I915_WRITE(reg, val);
> >  		POSTING_READ(reg);
> >  		return;
> > @@ -727,7 +720,7 @@ static void cpt_set_infoframes(struct drm_encoder
> > *encoder,
> >  	/* Set both together, unset both together: see the spec. */
> >  	val |= VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI;
> >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > -		 VIDEO_DIP_ENABLE_GCP);
> > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > 
> >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> >  		val |= VIDEO_DIP_ENABLE_GCP;
> > @@ -760,25 +753,26 @@ static void vlv_set_infoframes(struct drm_encoder
> > *encoder,
> >  	if (!enable) {
> >  		if (!(val & VIDEO_DIP_ENABLE))
> >  			return;
> > -		val &= ~VIDEO_DIP_ENABLE;
> > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > +			 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> >  		I915_WRITE(reg, val);
> >  		POSTING_READ(reg);
> >  		return;
> >  	}
> > 
> >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > -		if (val & VIDEO_DIP_ENABLE) {
> > -			val &= ~VIDEO_DIP_ENABLE;
> > -			I915_WRITE(reg, val);
> > -			POSTING_READ(reg);
> > -		}
> > +		WARN(val & VIDEO_DIP_ENABLE,
> > +		     "DIP already enabled on port %c\n",
> > +		     (val & VIDEO_DIP_PORT_MASK) >> 29);
> >  		val &= ~VIDEO_DIP_PORT_MASK;
> >  		val |= port;
> >  	}
> > 
> >  	val |= VIDEO_DIP_ENABLE;
> > -	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
> > -		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
> > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > 
> >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> >  		val |= VIDEO_DIP_ENABLE_GCP;
> > @@ -803,15 +797,16 @@ static void hsw_set_infoframes(struct drm_encoder
> > *encoder,
> > 
> >  	assert_hdmi_port_disabled(intel_hdmi);
> > 
> > +	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_AVI_HSW |
> > +		 VIDEO_DIP_ENABLE_GCP_HSW | VIDEO_DIP_ENABLE_VS_HSW |
> > +		 VIDEO_DIP_ENABLE_GMP_HSW | VIDEO_DIP_ENABLE_SPD_HSW);
> > +
> >  	if (!enable) {
> > -		I915_WRITE(reg, 0);
> > +		I915_WRITE(reg, val);
> >  		POSTING_READ(reg);
> >  		return;
> >  	}
> > 
> > -	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_GCP_HSW |
> > -		 VIDEO_DIP_ENABLE_VS_HSW | VIDEO_DIP_ENABLE_GMP_HSW);
> > -
> >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> >  		val |= VIDEO_DIP_ENABLE_GCP_HSW;
> > 
> > @@ -1133,7 +1128,7 @@ static void intel_disable_hdmi(struct intel_encoder
> > *encoder)
> >  	if (IS_CHERRYVIEW(dev))
> >  		chv_powergate_phy_lanes(encoder, 0xf);
> > 
> > -	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));
> > +	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
> >  }
> > 
> >  static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
> > --
> > 2.0.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chandra Konduru June 2, 2015, 6:18 p.m. UTC | #3
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Tuesday, June 02, 2015 4:11 AM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v2 6/9] drm/i915: Disable all infoframes when
> turning off the HDMI port
> 
> On Mon, Jun 01, 2015 at 10:48:03PM +0000, Konduru, Chandra wrote:
> >
> >
> > > -----Original Message-----
> > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of
> > > ville.syrjala@linux.intel.com
> > > Sent: Tuesday, May 05, 2015 7:06 AM
> > > To: intel-gfx@lists.freedesktop.org
> > > Subject: [Intel-gfx] [PATCH v2 6/9] drm/i915: Disable all infoframes when
> > > turning off the HDMI port
> > >
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Currently we just disable the GCP infoframe when turning off the port.
> > > That means if the same transcoder is used on a DP port next, we might
> > > end up pushing infoframes over DP, which isn't intended. Just disable
> >
> > Wonder how it is working. May be it is ok, or never hit using a previously
> > used transcoder for HDMI port for DP.
> >
> > By the way, you mean end up pushing "other" infoframes over DP?
> 
> We don't send infoframes over DP at all currently. Or I should say we
> never intended to send them. After this patch that should even be true.
> Well, unless the BIOS already enables them and we just fire up a DP port
> using the transcoder in question. So I suppose we should really have the
> DP code clear the infoframe settings explicitly, or we should clear them
> during the sanitize phase.

Agree that DP code path should clear the infoframe settings explicitly.
As you are already touching this code, will you plan to handle it?
Once it is taken care, it gets
Reviewed-by: Chandra Konduru <Chandra.konduru@intel.com>

> 
> >
> > > all the infoframes when turning off the port.
> > >
> > > Also protect against two ports stomping on each other on g4x due to
> > > the single video DIP instance. Now only the first port to enable
> > > gets to send infoframes.
> >
> > So is, 2nd port doesn't gets to send infoframes, the expected behavior
> > when two ports trying with a single DIP?
> 
> I'm not sure what's the best behaviour here. Either we somehow pick one
> of the ports to send infoframes (first come first serve in this patch),
> or we could just disable infoframes entirely for cloned cases. But it's
> probably a rare configuration anyway since HDMI cloning is only allowed
> on g4x.

I am fine with what you outlined above. If anything come up, this can be
revisited.

> 
> >
> > Seems like a corner case to test thoroughly. Is this path exercised in your
> testing?
> 
> I tested it long ago. Although I must admit that the patch looked a bit
> different back then.
> 
> >
> > >
> > > v2: Rebase
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_hdmi.c | 85 ++++++++++++++++++-----------------
> ----
> > >  1 file changed, 40 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > > b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index 766bdb1..03b4759 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -514,7 +514,13 @@ static void g4x_set_infoframes(struct drm_encoder
> > > *encoder,
> > >  	if (!enable) {
> > >  		if (!(val & VIDEO_DIP_ENABLE))
> > >  			return;
> > > -		val &= ~VIDEO_DIP_ENABLE;
> > > +		if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > > +			DRM_DEBUG_KMS("video DIP still enabled on port
> > > %c\n",
> > > +				      (val & VIDEO_DIP_PORT_MASK) >> 29);
> > > +			return;
> > > +		}
> > > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > +			 VIDEO_DIP_ENABLE_VENDOR |
> > > VIDEO_DIP_ENABLE_SPD);
> > >  		I915_WRITE(reg, val);
> > >  		POSTING_READ(reg);
> > >  		return;
> > > @@ -522,16 +528,17 @@ static void g4x_set_infoframes(struct
> drm_encoder
> > > *encoder,
> > >
> > >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > >  		if (val & VIDEO_DIP_ENABLE) {
> > > -			val &= ~VIDEO_DIP_ENABLE;
> > > -			I915_WRITE(reg, val);
> > > -			POSTING_READ(reg);
> > > +			DRM_DEBUG_KMS("video DIP already enabled on port
> > > %c\n",
> > > +				      (val & VIDEO_DIP_PORT_MASK) >> 29);
> > > +			return;
> > >  		}
> > >  		val &= ~VIDEO_DIP_PORT_MASK;
> > >  		val |= port;
> > >  	}
> > >
> > >  	val |= VIDEO_DIP_ENABLE;
> > > -	val &= ~VIDEO_DIP_ENABLE_VENDOR;
> > > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_SPD);
> > >
> > >  	I915_WRITE(reg, val);
> > >  	POSTING_READ(reg);
> > > @@ -632,23 +639,6 @@ static bool intel_hdmi_set_gcp_infoframe(struct
> > > drm_encoder *encoder)
> > >  	return val != 0;
> > >  }
> > >
> > > -static void intel_disable_gcp_infoframe(struct intel_crtc *crtc)
> > > -{
> > > -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > > -	u32 reg;
> > > -
> > > -	if (HAS_DDI(dev_priv))
> > > -		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
> > > -	else if (IS_VALLEYVIEW(dev_priv))
> > > -		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
> > > -	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > > -		reg = TVIDEO_DIP_CTL(crtc->pipe);
> > > -	else
> > > -		return;
> > > -
> > > -	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP);
> > > -}
> > > -
> > >  static void ibx_set_infoframes(struct drm_encoder *encoder,
> > >  			       bool enable,
> > >  			       struct drm_display_mode *adjusted_mode)
> > > @@ -669,25 +659,26 @@ static void ibx_set_infoframes(struct
> drm_encoder
> > > *encoder,
> > >  	if (!enable) {
> > >  		if (!(val & VIDEO_DIP_ENABLE))
> > >  			return;
> > > -		val &= ~VIDEO_DIP_ENABLE;
> > > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > +			 VIDEO_DIP_ENABLE_VENDOR |
> VIDEO_DIP_ENABLE_GAMUT |
> > > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > >  		I915_WRITE(reg, val);
> > >  		POSTING_READ(reg);
> > >  		return;
> > >  	}
> > >
> > >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > > -		if (val & VIDEO_DIP_ENABLE) {
> > > -			val &= ~VIDEO_DIP_ENABLE;
> > > -			I915_WRITE(reg, val);
> > > -			POSTING_READ(reg);
> > > -		}
> > > +		WARN(val & VIDEO_DIP_ENABLE,
> > > +		     "DIP already enabled on port %c\n",
> > > +		     (val & VIDEO_DIP_PORT_MASK) >> 29);
> > >  		val &= ~VIDEO_DIP_PORT_MASK;
> > >  		val |= port;
> > >  	}
> > >
> > >  	val |= VIDEO_DIP_ENABLE;
> > > -	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > -		 VIDEO_DIP_ENABLE_GCP);
> > > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > >
> > >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> > >  		val |= VIDEO_DIP_ENABLE_GCP;
> > > @@ -718,7 +709,9 @@ static void cpt_set_infoframes(struct drm_encoder
> > > *encoder,
> > >  	if (!enable) {
> > >  		if (!(val & VIDEO_DIP_ENABLE))
> > >  			return;
> > > -		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI);
> > > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > +			 VIDEO_DIP_ENABLE_VENDOR |
> > > VIDEO_DIP_ENABLE_GAMUT |
> > > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > >  		I915_WRITE(reg, val);
> > >  		POSTING_READ(reg);
> > >  		return;
> > > @@ -727,7 +720,7 @@ static void cpt_set_infoframes(struct drm_encoder
> > > *encoder,
> > >  	/* Set both together, unset both together: see the spec. */
> > >  	val |= VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI;
> > >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > -		 VIDEO_DIP_ENABLE_GCP);
> > > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > >
> > >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> > >  		val |= VIDEO_DIP_ENABLE_GCP;
> > > @@ -760,25 +753,26 @@ static void vlv_set_infoframes(struct
> drm_encoder
> > > *encoder,
> > >  	if (!enable) {
> > >  		if (!(val & VIDEO_DIP_ENABLE))
> > >  			return;
> > > -		val &= ~VIDEO_DIP_ENABLE;
> > > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > +			 VIDEO_DIP_ENABLE_VENDOR |
> VIDEO_DIP_ENABLE_GAMUT |
> > > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > >  		I915_WRITE(reg, val);
> > >  		POSTING_READ(reg);
> > >  		return;
> > >  	}
> > >
> > >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > > -		if (val & VIDEO_DIP_ENABLE) {
> > > -			val &= ~VIDEO_DIP_ENABLE;
> > > -			I915_WRITE(reg, val);
> > > -			POSTING_READ(reg);
> > > -		}
> > > +		WARN(val & VIDEO_DIP_ENABLE,
> > > +		     "DIP already enabled on port %c\n",
> > > +		     (val & VIDEO_DIP_PORT_MASK) >> 29);
> > >  		val &= ~VIDEO_DIP_PORT_MASK;
> > >  		val |= port;
> > >  	}
> > >
> > >  	val |= VIDEO_DIP_ENABLE;
> > > -	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
> > > -		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
> > > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > >
> > >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> > >  		val |= VIDEO_DIP_ENABLE_GCP;
> > > @@ -803,15 +797,16 @@ static void hsw_set_infoframes(struct
> drm_encoder
> > > *encoder,
> > >
> > >  	assert_hdmi_port_disabled(intel_hdmi);
> > >
> > > +	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW |
> VIDEO_DIP_ENABLE_AVI_HSW |
> > > +		 VIDEO_DIP_ENABLE_GCP_HSW |
> VIDEO_DIP_ENABLE_VS_HSW |
> > > +		 VIDEO_DIP_ENABLE_GMP_HSW |
> VIDEO_DIP_ENABLE_SPD_HSW);
> > > +
> > >  	if (!enable) {
> > > -		I915_WRITE(reg, 0);
> > > +		I915_WRITE(reg, val);
> > >  		POSTING_READ(reg);
> > >  		return;
> > >  	}
> > >
> > > -	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW |
> VIDEO_DIP_ENABLE_GCP_HSW |
> > > -		 VIDEO_DIP_ENABLE_VS_HSW |
> VIDEO_DIP_ENABLE_GMP_HSW);
> > > -
> > >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> > >  		val |= VIDEO_DIP_ENABLE_GCP_HSW;
> > >
> > > @@ -1133,7 +1128,7 @@ static void intel_disable_hdmi(struct
> intel_encoder
> > > *encoder)
> > >  	if (IS_CHERRYVIEW(dev))
> > >  		chv_powergate_phy_lanes(encoder, 0xf);
> > >
> > > -	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));
> > > +	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
> > >  }
> > >
> > >  static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool
> respect_dvi_limit)
> > > --
> > > 2.0.5
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel OTC
Ville Syrjälä June 3, 2015, 9:21 a.m. UTC | #4
On Tue, Jun 02, 2015 at 06:18:16PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Tuesday, June 02, 2015 4:11 AM
> > To: Konduru, Chandra
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH v2 6/9] drm/i915: Disable all infoframes when
> > turning off the HDMI port
> > 
> > On Mon, Jun 01, 2015 at 10:48:03PM +0000, Konduru, Chandra wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> > Of
> > > > ville.syrjala@linux.intel.com
> > > > Sent: Tuesday, May 05, 2015 7:06 AM
> > > > To: intel-gfx@lists.freedesktop.org
> > > > Subject: [Intel-gfx] [PATCH v2 6/9] drm/i915: Disable all infoframes when
> > > > turning off the HDMI port
> > > >
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > Currently we just disable the GCP infoframe when turning off the port.
> > > > That means if the same transcoder is used on a DP port next, we might
> > > > end up pushing infoframes over DP, which isn't intended. Just disable
> > >
> > > Wonder how it is working. May be it is ok, or never hit using a previously
> > > used transcoder for HDMI port for DP.
> > >
> > > By the way, you mean end up pushing "other" infoframes over DP?
> > 
> > We don't send infoframes over DP at all currently. Or I should say we
> > never intended to send them. After this patch that should even be true.
> > Well, unless the BIOS already enables them and we just fire up a DP port
> > using the transcoder in question. So I suppose we should really have the
> > DP code clear the infoframe settings explicitly, or we should clear them
> > during the sanitize phase.
> 
> Agree that DP code path should clear the infoframe settings explicitly.
> As you are already touching this code, will you plan to handle it?

I don't think I'll go there now. It would require quite a bit more work
to expose the infoframe stuff to the DP code. DP + infoframes in general
sounds like a good project for someone else to figure out. A quick
glance at the DP spec suggests that we should at least send the audio
infoframe.

> Once it is taken care, it gets
> Reviewed-by: Chandra Konduru <Chandra.konduru@intel.com>
> 
> > 
> > >
> > > > all the infoframes when turning off the port.
> > > >
> > > > Also protect against two ports stomping on each other on g4x due to
> > > > the single video DIP instance. Now only the first port to enable
> > > > gets to send infoframes.
> > >
> > > So is, 2nd port doesn't gets to send infoframes, the expected behavior
> > > when two ports trying with a single DIP?
> > 
> > I'm not sure what's the best behaviour here. Either we somehow pick one
> > of the ports to send infoframes (first come first serve in this patch),
> > or we could just disable infoframes entirely for cloned cases. But it's
> > probably a rare configuration anyway since HDMI cloning is only allowed
> > on g4x.
> 
> I am fine with what you outlined above. If anything come up, this can be
> revisited.
> 
> > 
> > >
> > > Seems like a corner case to test thoroughly. Is this path exercised in your
> > testing?
> > 
> > I tested it long ago. Although I must admit that the patch looked a bit
> > different back then.
> > 
> > >
> > > >
> > > > v2: Rebase
> > > >
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_hdmi.c | 85 ++++++++++++++++++-----------------
> > ----
> > > >  1 file changed, 40 insertions(+), 45 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > index 766bdb1..03b4759 100644
> > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > @@ -514,7 +514,13 @@ static void g4x_set_infoframes(struct drm_encoder
> > > > *encoder,
> > > >  	if (!enable) {
> > > >  		if (!(val & VIDEO_DIP_ENABLE))
> > > >  			return;
> > > > -		val &= ~VIDEO_DIP_ENABLE;
> > > > +		if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > > > +			DRM_DEBUG_KMS("video DIP still enabled on port
> > > > %c\n",
> > > > +				      (val & VIDEO_DIP_PORT_MASK) >> 29);
> > > > +			return;
> > > > +		}
> > > > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > > +			 VIDEO_DIP_ENABLE_VENDOR |
> > > > VIDEO_DIP_ENABLE_SPD);
> > > >  		I915_WRITE(reg, val);
> > > >  		POSTING_READ(reg);
> > > >  		return;
> > > > @@ -522,16 +528,17 @@ static void g4x_set_infoframes(struct
> > drm_encoder
> > > > *encoder,
> > > >
> > > >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > > >  		if (val & VIDEO_DIP_ENABLE) {
> > > > -			val &= ~VIDEO_DIP_ENABLE;
> > > > -			I915_WRITE(reg, val);
> > > > -			POSTING_READ(reg);
> > > > +			DRM_DEBUG_KMS("video DIP already enabled on port
> > > > %c\n",
> > > > +				      (val & VIDEO_DIP_PORT_MASK) >> 29);
> > > > +			return;
> > > >  		}
> > > >  		val &= ~VIDEO_DIP_PORT_MASK;
> > > >  		val |= port;
> > > >  	}
> > > >
> > > >  	val |= VIDEO_DIP_ENABLE;
> > > > -	val &= ~VIDEO_DIP_ENABLE_VENDOR;
> > > > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > > > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_SPD);
> > > >
> > > >  	I915_WRITE(reg, val);
> > > >  	POSTING_READ(reg);
> > > > @@ -632,23 +639,6 @@ static bool intel_hdmi_set_gcp_infoframe(struct
> > > > drm_encoder *encoder)
> > > >  	return val != 0;
> > > >  }
> > > >
> > > > -static void intel_disable_gcp_infoframe(struct intel_crtc *crtc)
> > > > -{
> > > > -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > > > -	u32 reg;
> > > > -
> > > > -	if (HAS_DDI(dev_priv))
> > > > -		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
> > > > -	else if (IS_VALLEYVIEW(dev_priv))
> > > > -		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
> > > > -	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > > > -		reg = TVIDEO_DIP_CTL(crtc->pipe);
> > > > -	else
> > > > -		return;
> > > > -
> > > > -	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP);
> > > > -}
> > > > -
> > > >  static void ibx_set_infoframes(struct drm_encoder *encoder,
> > > >  			       bool enable,
> > > >  			       struct drm_display_mode *adjusted_mode)
> > > > @@ -669,25 +659,26 @@ static void ibx_set_infoframes(struct
> > drm_encoder
> > > > *encoder,
> > > >  	if (!enable) {
> > > >  		if (!(val & VIDEO_DIP_ENABLE))
> > > >  			return;
> > > > -		val &= ~VIDEO_DIP_ENABLE;
> > > > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > > +			 VIDEO_DIP_ENABLE_VENDOR |
> > VIDEO_DIP_ENABLE_GAMUT |
> > > > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > > >  		I915_WRITE(reg, val);
> > > >  		POSTING_READ(reg);
> > > >  		return;
> > > >  	}
> > > >
> > > >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > > > -		if (val & VIDEO_DIP_ENABLE) {
> > > > -			val &= ~VIDEO_DIP_ENABLE;
> > > > -			I915_WRITE(reg, val);
> > > > -			POSTING_READ(reg);
> > > > -		}
> > > > +		WARN(val & VIDEO_DIP_ENABLE,
> > > > +		     "DIP already enabled on port %c\n",
> > > > +		     (val & VIDEO_DIP_PORT_MASK) >> 29);
> > > >  		val &= ~VIDEO_DIP_PORT_MASK;
> > > >  		val |= port;
> > > >  	}
> > > >
> > > >  	val |= VIDEO_DIP_ENABLE;
> > > > -	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > > -		 VIDEO_DIP_ENABLE_GCP);
> > > > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > > > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > > >
> > > >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > >  		val |= VIDEO_DIP_ENABLE_GCP;
> > > > @@ -718,7 +709,9 @@ static void cpt_set_infoframes(struct drm_encoder
> > > > *encoder,
> > > >  	if (!enable) {
> > > >  		if (!(val & VIDEO_DIP_ENABLE))
> > > >  			return;
> > > > -		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI);
> > > > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > > +			 VIDEO_DIP_ENABLE_VENDOR |
> > > > VIDEO_DIP_ENABLE_GAMUT |
> > > > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > > >  		I915_WRITE(reg, val);
> > > >  		POSTING_READ(reg);
> > > >  		return;
> > > > @@ -727,7 +720,7 @@ static void cpt_set_infoframes(struct drm_encoder
> > > > *encoder,
> > > >  	/* Set both together, unset both together: see the spec. */
> > > >  	val |= VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI;
> > > >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > > -		 VIDEO_DIP_ENABLE_GCP);
> > > > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > > >
> > > >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > >  		val |= VIDEO_DIP_ENABLE_GCP;
> > > > @@ -760,25 +753,26 @@ static void vlv_set_infoframes(struct
> > drm_encoder
> > > > *encoder,
> > > >  	if (!enable) {
> > > >  		if (!(val & VIDEO_DIP_ENABLE))
> > > >  			return;
> > > > -		val &= ~VIDEO_DIP_ENABLE;
> > > > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > > +			 VIDEO_DIP_ENABLE_VENDOR |
> > VIDEO_DIP_ENABLE_GAMUT |
> > > > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > > >  		I915_WRITE(reg, val);
> > > >  		POSTING_READ(reg);
> > > >  		return;
> > > >  	}
> > > >
> > > >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > > > -		if (val & VIDEO_DIP_ENABLE) {
> > > > -			val &= ~VIDEO_DIP_ENABLE;
> > > > -			I915_WRITE(reg, val);
> > > > -			POSTING_READ(reg);
> > > > -		}
> > > > +		WARN(val & VIDEO_DIP_ENABLE,
> > > > +		     "DIP already enabled on port %c\n",
> > > > +		     (val & VIDEO_DIP_PORT_MASK) >> 29);
> > > >  		val &= ~VIDEO_DIP_PORT_MASK;
> > > >  		val |= port;
> > > >  	}
> > > >
> > > >  	val |= VIDEO_DIP_ENABLE;
> > > > -	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
> > > > -		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
> > > > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > > > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > > >
> > > >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > >  		val |= VIDEO_DIP_ENABLE_GCP;
> > > > @@ -803,15 +797,16 @@ static void hsw_set_infoframes(struct
> > drm_encoder
> > > > *encoder,
> > > >
> > > >  	assert_hdmi_port_disabled(intel_hdmi);
> > > >
> > > > +	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW |
> > VIDEO_DIP_ENABLE_AVI_HSW |
> > > > +		 VIDEO_DIP_ENABLE_GCP_HSW |
> > VIDEO_DIP_ENABLE_VS_HSW |
> > > > +		 VIDEO_DIP_ENABLE_GMP_HSW |
> > VIDEO_DIP_ENABLE_SPD_HSW);
> > > > +
> > > >  	if (!enable) {
> > > > -		I915_WRITE(reg, 0);
> > > > +		I915_WRITE(reg, val);
> > > >  		POSTING_READ(reg);
> > > >  		return;
> > > >  	}
> > > >
> > > > -	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW |
> > VIDEO_DIP_ENABLE_GCP_HSW |
> > > > -		 VIDEO_DIP_ENABLE_VS_HSW |
> > VIDEO_DIP_ENABLE_GMP_HSW);
> > > > -
> > > >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > >  		val |= VIDEO_DIP_ENABLE_GCP_HSW;
> > > >
> > > > @@ -1133,7 +1128,7 @@ static void intel_disable_hdmi(struct
> > intel_encoder
> > > > *encoder)
> > > >  	if (IS_CHERRYVIEW(dev))
> > > >  		chv_powergate_phy_lanes(encoder, 0xf);
> > > >
> > > > -	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));
> > > > +	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
> > > >  }
> > > >
> > > >  static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool
> > respect_dvi_limit)
> > > > --
> > > > 2.0.5
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > --
> > Ville Syrjälä
> > Intel OTC
Chandra Konduru June 3, 2015, 11:24 p.m. UTC | #5
> > > > > Currently we just disable the GCP infoframe when turning off the port.
> > > > > That means if the same transcoder is used on a DP port next, we might
> > > > > end up pushing infoframes over DP, which isn't intended. Just disable
> > > >
> > > > Wonder how it is working. May be it is ok, or never hit using a previously
> > > > used transcoder for HDMI port for DP.
> > > >
> > > > By the way, you mean end up pushing "other" infoframes over DP?
> > >
> > > We don't send infoframes over DP at all currently. Or I should say we
> > > never intended to send them. After this patch that should even be true.
> > > Well, unless the BIOS already enables them and we just fire up a DP port
> > > using the transcoder in question. So I suppose we should really have the
> > > DP code clear the infoframe settings explicitly, or we should clear them
> > > during the sanitize phase.
> >
> > Agree that DP code path should clear the infoframe settings explicitly.
> > As you are already touching this code, will you plan to handle it?
> 
> I don't think I'll go there now. It would require quite a bit more work
> to expose the infoframe stuff to the DP code. DP + infoframes in general
> sounds like a good project for someone else to figure out. A quick
> glance at the DP spec suggests that we should at least send the audio
> infoframe.

OK, may be binned for future.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 766bdb1..03b4759 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -514,7 +514,13 @@  static void g4x_set_infoframes(struct drm_encoder *encoder,
 	if (!enable) {
 		if (!(val & VIDEO_DIP_ENABLE))
 			return;
-		val &= ~VIDEO_DIP_ENABLE;
+		if (port != (val & VIDEO_DIP_PORT_MASK)) {
+			DRM_DEBUG_KMS("video DIP still enabled on port %c\n",
+				      (val & VIDEO_DIP_PORT_MASK) >> 29);
+			return;
+		}
+		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
+			 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_SPD);
 		I915_WRITE(reg, val);
 		POSTING_READ(reg);
 		return;
@@ -522,16 +528,17 @@  static void g4x_set_infoframes(struct drm_encoder *encoder,
 
 	if (port != (val & VIDEO_DIP_PORT_MASK)) {
 		if (val & VIDEO_DIP_ENABLE) {
-			val &= ~VIDEO_DIP_ENABLE;
-			I915_WRITE(reg, val);
-			POSTING_READ(reg);
+			DRM_DEBUG_KMS("video DIP already enabled on port %c\n",
+				      (val & VIDEO_DIP_PORT_MASK) >> 29);
+			return;
 		}
 		val &= ~VIDEO_DIP_PORT_MASK;
 		val |= port;
 	}
 
 	val |= VIDEO_DIP_ENABLE;
-	val &= ~VIDEO_DIP_ENABLE_VENDOR;
+	val &= ~(VIDEO_DIP_ENABLE_AVI |
+		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_SPD);
 
 	I915_WRITE(reg, val);
 	POSTING_READ(reg);
@@ -632,23 +639,6 @@  static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder)
 	return val != 0;
 }
 
-static void intel_disable_gcp_infoframe(struct intel_crtc *crtc)
-{
-	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
-	u32 reg;
-
-	if (HAS_DDI(dev_priv))
-		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
-	else if (IS_VALLEYVIEW(dev_priv))
-		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
-	else if (HAS_PCH_SPLIT(dev_priv->dev))
-		reg = TVIDEO_DIP_CTL(crtc->pipe);
-	else
-		return;
-
-	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP);
-}
-
 static void ibx_set_infoframes(struct drm_encoder *encoder,
 			       bool enable,
 			       struct drm_display_mode *adjusted_mode)
@@ -669,25 +659,26 @@  static void ibx_set_infoframes(struct drm_encoder *encoder,
 	if (!enable) {
 		if (!(val & VIDEO_DIP_ENABLE))
 			return;
-		val &= ~VIDEO_DIP_ENABLE;
+		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
+			 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
+			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
 		I915_WRITE(reg, val);
 		POSTING_READ(reg);
 		return;
 	}
 
 	if (port != (val & VIDEO_DIP_PORT_MASK)) {
-		if (val & VIDEO_DIP_ENABLE) {
-			val &= ~VIDEO_DIP_ENABLE;
-			I915_WRITE(reg, val);
-			POSTING_READ(reg);
-		}
+		WARN(val & VIDEO_DIP_ENABLE,
+		     "DIP already enabled on port %c\n",
+		     (val & VIDEO_DIP_PORT_MASK) >> 29);
 		val &= ~VIDEO_DIP_PORT_MASK;
 		val |= port;
 	}
 
 	val |= VIDEO_DIP_ENABLE;
-	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
-		 VIDEO_DIP_ENABLE_GCP);
+	val &= ~(VIDEO_DIP_ENABLE_AVI |
+		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
+		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
 
 	if (intel_hdmi_set_gcp_infoframe(encoder))
 		val |= VIDEO_DIP_ENABLE_GCP;
@@ -718,7 +709,9 @@  static void cpt_set_infoframes(struct drm_encoder *encoder,
 	if (!enable) {
 		if (!(val & VIDEO_DIP_ENABLE))
 			return;
-		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI);
+		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
+			 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
+			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
 		I915_WRITE(reg, val);
 		POSTING_READ(reg);
 		return;
@@ -727,7 +720,7 @@  static void cpt_set_infoframes(struct drm_encoder *encoder,
 	/* Set both together, unset both together: see the spec. */
 	val |= VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI;
 	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
-		 VIDEO_DIP_ENABLE_GCP);
+		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
 
 	if (intel_hdmi_set_gcp_infoframe(encoder))
 		val |= VIDEO_DIP_ENABLE_GCP;
@@ -760,25 +753,26 @@  static void vlv_set_infoframes(struct drm_encoder *encoder,
 	if (!enable) {
 		if (!(val & VIDEO_DIP_ENABLE))
 			return;
-		val &= ~VIDEO_DIP_ENABLE;
+		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
+			 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
+			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
 		I915_WRITE(reg, val);
 		POSTING_READ(reg);
 		return;
 	}
 
 	if (port != (val & VIDEO_DIP_PORT_MASK)) {
-		if (val & VIDEO_DIP_ENABLE) {
-			val &= ~VIDEO_DIP_ENABLE;
-			I915_WRITE(reg, val);
-			POSTING_READ(reg);
-		}
+		WARN(val & VIDEO_DIP_ENABLE,
+		     "DIP already enabled on port %c\n",
+		     (val & VIDEO_DIP_PORT_MASK) >> 29);
 		val &= ~VIDEO_DIP_PORT_MASK;
 		val |= port;
 	}
 
 	val |= VIDEO_DIP_ENABLE;
-	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
-		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
+	val &= ~(VIDEO_DIP_ENABLE_AVI |
+		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
+		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
 
 	if (intel_hdmi_set_gcp_infoframe(encoder))
 		val |= VIDEO_DIP_ENABLE_GCP;
@@ -803,15 +797,16 @@  static void hsw_set_infoframes(struct drm_encoder *encoder,
 
 	assert_hdmi_port_disabled(intel_hdmi);
 
+	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_AVI_HSW |
+		 VIDEO_DIP_ENABLE_GCP_HSW | VIDEO_DIP_ENABLE_VS_HSW |
+		 VIDEO_DIP_ENABLE_GMP_HSW | VIDEO_DIP_ENABLE_SPD_HSW);
+
 	if (!enable) {
-		I915_WRITE(reg, 0);
+		I915_WRITE(reg, val);
 		POSTING_READ(reg);
 		return;
 	}
 
-	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_GCP_HSW |
-		 VIDEO_DIP_ENABLE_VS_HSW | VIDEO_DIP_ENABLE_GMP_HSW);
-
 	if (intel_hdmi_set_gcp_infoframe(encoder))
 		val |= VIDEO_DIP_ENABLE_GCP_HSW;
 
@@ -1133,7 +1128,7 @@  static void intel_disable_hdmi(struct intel_encoder *encoder)
 	if (IS_CHERRYVIEW(dev))
 		chv_powergate_phy_lanes(encoder, 0xf);
 
-	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));
+	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
 }
 
 static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)