Re: [Intel-gfx] [PATCH 5/9] drm/i915/sdvo: Implement limited color range for SDVO HDMI properly

[Imre Deak <imre.deak@intel.com>]

On Wed, Jan 08, 2020 at 08:12:38PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The SDVO/HDMI port register limited color range bit can only be used
> with TMDS encoding and not SDVO encoding, ie. to be used only when
> using the port as a HDMI port as opposed to a SDVO port. The SDVO
> spec does have a note that some GMCHs might allow that, but gen4
> bspec vehemently disagrees. I suppose on ILK+ it might work since
> the color range handling is on the CPU side rather than on the PCH
> side, so there is no clear linkage between the TMDS vs. SDVO
> encoding and color range. Alas, I have no hardware to test that
> theory.
> 
> To implement limited color range support for SDVO->HDMI we need to
> ask the SDVO device to do the range compression. Do so, but first
> check if the device even supports the colorimetry selection.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  3 +-
>  drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +-
>  drivers/gpu/drm/i915/display/intel_hdmi.h    |  2 +
>  drivers/gpu/drm/i915/display/intel_sdvo.c    | 62 ++++++++++++--------
>  4 files changed, 45 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 59c375879186..7ef1f209acc4 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -9888,7 +9888,8 @@ static void ilk_set_pipeconf(const struct intel_crtc_state *crtc_state)
>  	WARN_ON(crtc_state->limited_color_range &&
>  		crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB);
>  
> -	if (crtc_state->limited_color_range)
> +	if (crtc_state->limited_color_range &&
> +	    !intel_crtc_has_type(crtc_state, INTEL_OUTPUT_SDVO))
>  		val |= PIPECONF_COLOR_RANGE_SELECT;
>  
>  	if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 1659cff91426..85c5f840a0fc 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2375,8 +2375,8 @@ static int intel_hdmi_compute_clock(struct intel_encoder *encoder,
>  	return 0;
>  }
>  
> -static bool intel_hdmi_limited_color_range(const struct intel_crtc_state *crtc_state,
> -					   const struct drm_connector_state *conn_state)
> +bool intel_hdmi_limited_color_range(const struct intel_crtc_state *crtc_state,
> +				    const struct drm_connector_state *conn_state)
>  {
>  	const struct intel_digital_connector_state *intel_conn_state =
>  		to_intel_digital_connector_state(conn_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.h b/drivers/gpu/drm/i915/display/intel_hdmi.h
> index cf1ea5427639..c5f59c20f1e8 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.h
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.h
> @@ -48,5 +48,7 @@ void intel_read_infoframe(struct intel_encoder *encoder,
>  			  const struct intel_crtc_state *crtc_state,
>  			  enum hdmi_infoframe_type type,
>  			  union hdmi_infoframe *frame);
> +bool intel_hdmi_limited_color_range(const struct intel_crtc_state *crtc_state,
> +				    const struct drm_connector_state *conn_state);
>  
>  #endif /* __INTEL_HDMI_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 2d2c5e1c7e7c..a0bbd728aa54 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -95,6 +95,8 @@ struct intel_sdvo {
>  	 */
>  	struct intel_sdvo_caps caps;
>  
> +	u8 colorimetry_cap;
> +
>  	/* Pixel clock limitations reported by the SDVO device, in kHz */
>  	int pixel_clock_min, pixel_clock_max;
>  
> @@ -1271,6 +1273,18 @@ static bool intel_has_hdmi_sink(struct intel_sdvo *sdvo,
>  		READ_ONCE(to_intel_digital_connector_state(conn_state)->force_audio) != HDMI_AUDIO_OFF_DVI;
>  }
>  
> +static bool intel_sdvo_limited_color_range(struct intel_encoder *encoder,
> +					   const struct intel_crtc_state *crtc_state,
> +					   const struct drm_connector_state *conn_state)
> +{
> +	struct intel_sdvo *intel_sdvo = to_sdvo(encoder);
> +
> +	if ((intel_sdvo->colorimetry_cap & SDVO_COLORIMETRY_RGB220) == 0)
> +		return false;
> +
> +	return intel_hdmi_limited_color_range(crtc_state, conn_state);
> +}
> +
>  static int intel_sdvo_compute_config(struct intel_encoder *encoder,
>  				     struct intel_crtc_state *pipe_config,
>  				     struct drm_connector_state *conn_state)
> @@ -1336,21 +1350,9 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
>  				intel_sdvo_state->base.force_audio == HDMI_AUDIO_ON;
>  	}
>  
> -	if (intel_sdvo_state->base.broadcast_rgb == INTEL_BROADCAST_RGB_AUTO) {
> -		/*
> -		 * See CEA-861-E - 5.1 Default Encoding Parameters
> -		 *
> -		 * FIXME: This bit is only valid when using TMDS encoding and 8
> -		 * bit per color mode.
> -		 */
> -		if (pipe_config->has_hdmi_sink &&
> -		    drm_match_cea_mode(adjusted_mode) > 1)
> -			pipe_config->limited_color_range = true;
> -	} else {
> -		if (pipe_config->has_hdmi_sink &&
> -		    intel_sdvo_state->base.broadcast_rgb == INTEL_BROADCAST_RGB_LIMITED)
> -			pipe_config->limited_color_range = true;
> -	}
> +	pipe_config->limited_color_range =
> +		intel_sdvo_limited_color_range(encoder, pipe_config,
> +					       conn_state);
>  
>  	/* Clock computation needs to happen after pixel multiplier. */
>  	if (IS_TV(intel_sdvo_connector))
> @@ -1487,6 +1489,8 @@ static void intel_sdvo_pre_enable(struct intel_encoder *intel_encoder,
>  	if (crtc_state->has_hdmi_sink) {
>  		intel_sdvo_set_encode(intel_sdvo, SDVO_ENCODE_HDMI);
>  		intel_sdvo_set_colorimetry(intel_sdvo,
> +					   crtc_state->limited_color_range ?
> +					   SDVO_COLORIMETRY_RGB220 :
>  					   SDVO_COLORIMETRY_RGB256);
>  		intel_sdvo_set_avi_infoframe(intel_sdvo, crtc_state);
>  	} else
> @@ -1520,8 +1524,6 @@ static void intel_sdvo_pre_enable(struct intel_encoder *intel_encoder,
>  		/* The real mode polarity is set by the SDVO commands, using
>  		 * struct intel_sdvo_dtd. */
>  		sdvox = SDVO_VSYNC_ACTIVE_HIGH | SDVO_HSYNC_ACTIVE_HIGH;
> -		if (!HAS_PCH_SPLIT(dev_priv) && crtc_state->limited_color_range)
> -			sdvox |= HDMI_COLOR_RANGE_16_235;
>  		if (INTEL_GEN(dev_priv) < 5)
>  			sdvox |= SDVO_BORDER_ENABLE;
>  	} else {
> @@ -1678,8 +1680,11 @@ static void intel_sdvo_get_config(struct intel_encoder *encoder,
>  	     "SDVO pixel multiplier mismatch, port: %i, encoder: %i\n",
>  	     pipe_config->pixel_multiplier, encoder_pixel_multiplier);
>  
> -	if (sdvox & HDMI_COLOR_RANGE_16_235)
> -		pipe_config->limited_color_range = true;
> +	if (intel_sdvo_get_value(intel_sdvo, SDVO_CMD_GET_COLORIMETRY,
> +				 &val, 1)) {
> +		if (val == SDVO_COLORIMETRY_RGB220)
> +			pipe_config->limited_color_range = true;
> +	}
>  
>  	if (intel_sdvo_get_value(intel_sdvo, SDVO_CMD_GET_AUDIO_STAT,
>  				 &val, 1)) {
> @@ -1898,6 +1903,17 @@ static bool intel_sdvo_get_capabilities(struct intel_sdvo *intel_sdvo, struct in
>  	return true;
>  }
>  
> +static u8 intel_sdvo_get_colorimetry_cap(struct intel_sdvo *intel_sdvo)
> +{
> +	u8 cap;
> +
> +	if (!intel_sdvo_get_value(intel_sdvo, SDVO_CMD_GET_COLORIMETRY_CAP,
> +				  &cap, sizeof(cap)))
> +		return SDVO_COLORIMETRY_RGB256;
> +
> +	return cap;
> +}
> +
>  static u16 intel_sdvo_get_hotplug_support(struct intel_sdvo *intel_sdvo)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(intel_sdvo->base.base.dev);
> @@ -2654,12 +2670,9 @@ static void
>  intel_sdvo_add_hdmi_properties(struct intel_sdvo *intel_sdvo,
>  			       struct intel_sdvo_connector *connector)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(connector->base.base.dev);
> -
>  	intel_attach_force_audio_property(&connector->base.base);
> -	if (INTEL_GEN(dev_priv) >= 4 && IS_MOBILE(dev_priv)) {
> +	if (intel_sdvo->colorimetry_cap & SDVO_COLORIMETRY_RGB220)
>  		intel_attach_broadcast_rgb_property(&connector->base.base);
> -	}
>  	intel_attach_aspect_ratio_property(&connector->base.base);
>  }
>  
> @@ -3298,6 +3311,9 @@ bool intel_sdvo_init(struct drm_i915_private *dev_priv,
>  	if (!intel_sdvo_get_capabilities(intel_sdvo, &intel_sdvo->caps))
>  		goto err;
>  
> +	intel_sdvo->colorimetry_cap =
> +		intel_sdvo_get_colorimetry_cap(intel_sdvo);
> +
>  	if (intel_sdvo_output_setup(intel_sdvo,
>  				    intel_sdvo->caps.output_flags) != true) {
>  		DRM_DEBUG_KMS("SDVO output failed to setup on %s\n",
> -- 
> 2.24.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx