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

[Konduru, Chandra]

> > > @@ -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 at 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.