Re: [PATCH v2 2/9] drm/i915: Send GCP infoframes for deep color HDMI sinks

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

On Mon, May 25, 2015 at 04:09:57PM +0300, Ander Conselvan De Oliveira wrote:
> On Mon, 2015-05-25 at 15:44 +0300, Ville Syrjälä wrote:
> > On Mon, May 25, 2015 at 03:32:52PM +0300, Ander Conselvan De Oliveira wrote:
> > > On Tue, 2015-05-05 at 17:06 +0300, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > GCP infoframes are required to inform the HDMI sink about the color
> > > > depth.
> > > > 
> > > > Send the GCP infoframe whenever the sink supports any deep color modes
> > > > since such sinks must anyway be capable of receiving them. For sinks
> > > > that don't support deep color let's skip the GCP in case it might
> > > > confuse the sink, although HDMI 1.4 spec does say all sinks must be
> > > > capable of reciving them. In theory we could skip the GCP infoframe
> > > > for deep color sinks in 8bpc mode as well since sinks must fall back to
> > > > 8bpc whenever GCP isn't received for some time.
> > > > 
> > > > BSpec says we should disable GCP after disabling the port, so do that as
> > > > well.
> > > > 
> > > > v2: s/intel_set_gcp_infoframe/intel_hdmi_set_gcp_infoframe/
> > > >     Rebased due to crtc->config changes
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_reg.h   |  3 ++
> > > >  drivers/gpu/drm/i915/intel_hdmi.c | 74 +++++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 77 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index e619e41..dcd93b5 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -6010,6 +6010,9 @@ enum skl_disp_power_wells {
> > > >  #define _VIDEO_DIP_CTL_A         0xe0200
> > > >  #define _VIDEO_DIP_DATA_A        0xe0208
> > > >  #define _VIDEO_DIP_GCP_A         0xe0210
> > > > +#define  GCP_COLOR_INDICATION		(1 << 2)
> > > > +#define  GCP_DEFAULT_PHASE_ENABLE	(1 << 1)
> > > > +#define  GCP_AV_MUTE			(1 << 0)
> > > >  
> > > >  #define _VIDEO_DIP_CTL_B         0xe1200
> > > >  #define _VIDEO_DIP_DATA_B        0xe1208
> > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > index 79cf445..87c4905 100644
> > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > @@ -541,6 +541,66 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
> > > >  	intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode);
> > > >  }
> > > >  
> > > > +static bool hdmi_sink_is_deep_color(struct drm_encoder *encoder)
> > > > +{
> > > > +	struct drm_device *dev = encoder->dev;
> > > > +	struct drm_connector *connector;
> > > > +
> > > > +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> > > > +
> > > > +	/*
> > > > +	 * HDMI cloning is only supported on g4x which doesn't
> > > > +	 * support deep color or GCP infoframes anyway so no
> > > > +	 * need to worry about multiple HDMI sinks here.
> > > > +	 */
> > > > +	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> > > > +		if (connector->encoder == encoder)
> > > > +			return connector->display_info.bpc > 8;
> > > > +
> > > > +	return false;
> > > > +}
> > > > +
> > > > +static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
> > > > +	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> > > > +	u32 reg, val = 0;
> > > > +
> > > > +	if (HAS_DDI(dev_priv))
> > > > +		reg = HSW_TVIDEO_DIP_GCP(crtc->config->cpu_transcoder);
> > > > +	else if (IS_VALLEYVIEW(dev_priv))
> > > > +		reg = VLV_TVIDEO_DIP_GCP(crtc->pipe);
> > > > +	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > > > +		reg = TVIDEO_DIP_GCP(crtc->pipe);
> > > > +	else
> > > > +		return false;
> > > > +
> > > > +	/* Indicate color depth wheneven the sink supports deep color */
> > > > +	if (hdmi_sink_is_deep_color(encoder))
> > > > +		val |= GCP_COLOR_INDICATION;
> > > > +
> > > > +	I915_WRITE(reg, val);
> > > > +
> > > > +	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)
> > > > @@ -581,6 +641,9 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
> > > >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > >  		 VIDEO_DIP_ENABLE_GCP);
> > > >  
> > > > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > > +		val |= VIDEO_DIP_ENABLE_GCP;
> > > > +
> > > >  	I915_WRITE(reg, val);
> > > >  	POSTING_READ(reg);
> > > >  
> > > > @@ -618,6 +681,9 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
> > > >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > >  		 VIDEO_DIP_ENABLE_GCP);
> > > >  
> > > > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > > +		val |= VIDEO_DIP_ENABLE_GCP;
> > > > +
> > > >  	I915_WRITE(reg, val);
> > > >  	POSTING_READ(reg);
> > > >  
> > > > @@ -666,6 +732,9 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
> > > >  	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
> > > >  		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
> > > >  
> > > > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > > +		val |= VIDEO_DIP_ENABLE_GCP;
> > > > +
> > > >  	I915_WRITE(reg, val);
> > > >  	POSTING_READ(reg);
> > > >  
> > > > @@ -695,6 +764,9 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
> > > >  	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;
> > > > +
> > > >  	I915_WRITE(reg, val);
> > > >  	POSTING_READ(reg);
> > > >  
> > > > @@ -986,6 +1058,8 @@ 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));
> > > 
> > > BSpec says this should be disabled after disabling TRANS_DDI_FUNC_CTL,
> > > so shouldn't this go in post_disable?
> > 
> > intel_disable_hdmi() isn't used on DDI platforms. I've not looked at the
> > DDI code too much, but I expect it could have similar issues with the
> > disable sequence like the earlier PCH platforms had.
> 
> Ah, right. So if the GCP would be disabled that would only be done in
> ->set_infoframes() called from ->pre_enable(), that is called while
> TRANS_DDI_FUNC_CTL is disabled. So no issue there.

Perhaps. As stated I didn't really look at DDI. I do think we should
change it do things the same way as everyone else, but for the time
being I have enough things on my plate so I'll not take on another
project.

> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> 
> 

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