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

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

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

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