diff mbox

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

Message ID 1430834787-10255-3-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>

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(+)

Comments

Ander Conselvan de Oliveira May 25, 2015, 12:32 p.m. UTC | #1
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?

Ander

>  }
>  
>  static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
Ville Syrjälä May 25, 2015, 12:44 p.m. UTC | #2
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.
Ander Conselvan de Oliveira May 25, 2015, 1:09 p.m. UTC | #3
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.

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
Ville Syrjälä May 25, 2015, 1:14 p.m. UTC | #4
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>
> 
>
Chandra Konduru June 1, 2015, 9:49 p.m. UTC | #5
> -----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 2/9] drm/i915: Send GCP infoframes for deep

> color HDMI sinks

> 

> 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) {


Some static functions are prefixed but this isn't. Can you add prefix?

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

> +	 */


There isn't any code specific for HDMI cloning or multiple HDMI sinks here, 
any relevance of above comment. Or am I missing something 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 */


Typo

> +	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;

> +


There is a comment in the code that 
"Note that g4x/vlv don't support 12bpc hdmi outputs."
Is above call still required for vlv? Or need an update to that comment.

>  	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));

>  }

> 

>  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, 12:58 p.m. UTC | #6
On Mon, Jun 01, 2015 at 09:49:40PM +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 2/9] drm/i915: Send GCP infoframes for deep
> > color HDMI sinks
> > 
> > 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) {
> 
> Some static functions are prefixed but this isn't. Can you add prefix?

Is there any benefit from a prefix?

> 
> > +	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.
> > +	 */
> 
> There isn't any code specific for HDMI cloning or multiple HDMI sinks here, 
> any relevance of above comment. Or am I missing something here?

In a cloned setup we'd feed multiple sinks from the same transcoder, so
we'd need to basically check that connector->encoder->crtc matches our crtc
here, and only return true if all the relevant connectors were deep color
capable. But since we can ignore cloning we can stop looking once we
find the connector for our current encoder (ie. HDMI port).

> 
> > +	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 */
> 
> Typo
> 
> > +	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;
> > +
> 
> There is a comment in the code that 
> "Note that g4x/vlv don't support 12bpc hdmi outputs."
> Is above call still required for vlv? Or need an update to that comment.

VLV/CHV support the GCP infoframe even though they don't support
12bpc output. The way I decided to do things is to send the GCP
infoframe whenever there's a deep color sink hooked up, even if
we feed it 8bpc data.

> 
> >  	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));
> >  }
> > 
> >  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, 7:07 p.m. UTC | #7
> > > @@ -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) {
> >
> > Some static functions are prefixed but this isn't. Can you add prefix?
> 
> Is there any benefit from a prefix?

Personally, I am fine either way. But based on review feedback seen so far,
this is being looked at and asked to follow. From that perspective I mentioned that. 
If this is something optional and tied to benefit, then no issue here.

> 
> >
> > > +	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.
> > > +	 */
> >
> > There isn't any code specific for HDMI cloning or multiple HDMI sinks here,
> > any relevance of above comment. Or am I missing something here?
> 
> In a cloned setup we'd feed multiple sinks from the same transcoder, so
> we'd need to basically check that connector->encoder->crtc matches our crtc
> here, and only return true if all the relevant connectors were deep color
> capable. But since we can ignore cloning we can stop looking once we
> find the connector for our current encoder (ie. HDMI port).

OK, makes sense. Thanks for the background info.

Not sure if you are spinning another version to fix the typo below or prefix above,
otherwise, this gets

Reviewed-by: Chandra Konduru <Chandra.konduru@intel.com>


> 
> >
> > > +	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 */
> >
> > Typo
> >
> > > +	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;
> > > +
> >
> > There is a comment in the code that
> > "Note that g4x/vlv don't support 12bpc hdmi outputs."
> > Is above call still required for vlv? Or need an update to that comment.
> 
> VLV/CHV support the GCP infoframe even though they don't support
> 12bpc output. The way I decided to do things is to send the GCP
> infoframe whenever there's a deep color sink hooked up, even if
> we feed it 8bpc data.

Got it. Should be ok then.
diff mbox

Patch

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));
 }
 
 static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)