Re: [Intel-gfx] [PATCH v2 05/12] drm/i915: Never set limited_color_range=true for YCbCr output

["Mun, Gwan-gyeong" <gwan-gyeong.mun@intel.com>]

On Thu, 2019-07-18 at 19:45 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> crtc_state->limited_color_range only applies to RGB output but
> we're currently setting it even for YCbCr output. That will
> lead to conflicting MSA and PIPECONF settings which can mess
> up the image. Let's make sure limited_color_range stays unset
> with YCbCr output.
> 
> Also WARN if we end up with such a bogus combination when
> programming the MSA MISC bits as it's impossible to even
> indicate quantization rangle for YCbCr via MSA MISC. YCbCr
> output is simply assumed to be limited range always. Note
> that VSC SDP does provide a mechanism for full range YCbCr,
> so in the future we may want to rethink how we compute/store
> this state.
> 
> And for good measure we add the same WARN to the HDMI path.
> 
> v2: s/==/!=/ in the HDMI WARN
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c  | 10 +++++++---
>  drivers/gpu/drm/i915/display/intel_dp.c   | 10 ++++++++++
>  drivers/gpu/drm/i915/display/intel_hdmi.c | 20 +++++++++++++++++---
>  3 files changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 157c5851a688..7dd54f573f35 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1706,9 +1706,6 @@ void intel_ddi_set_pipe_settings(const struct
> intel_crtc_state *crtc_state)
>  
>  	temp = TRANS_MSA_SYNC_CLK;
>  
> -	if (crtc_state->limited_color_range)
> -		temp |= TRANS_MSA_CEA_RANGE;
> -
>  	switch (crtc_state->pipe_bpp) {
>  	case 18:
>  		temp |= TRANS_MSA_6_BPC;
> @@ -1727,6 +1724,13 @@ void intel_ddi_set_pipe_settings(const struct
> intel_crtc_state *crtc_state)
>  		break;
>  	}
>  
> +	/* nonsense combination */
> +	WARN_ON(crtc_state->limited_color_range &&
> +		crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB);
> +
> +	if (crtc_state->limited_color_range)
> +		temp |= TRANS_MSA_CEA_RANGE;
> +
>  	/*
>  	 * As per DP 1.2 spec section 2.3.4.3 while sending
>  	 * YCBCR 444 signals we should program MSA MISC1/0 fields with
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 0eb5d66f87a7..84d2724f0854 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2126,6 +2126,16 @@ bool intel_dp_limited_color_range(const struct
> intel_crtc_state *crtc_state,
>  	const struct drm_display_mode *adjusted_mode =
>  		&crtc_state->base.adjusted_mode;
>  
> +	/*
> +	 * Our YCbCr output is always limited range.
> +	 * crtc_state->limited_color_range only applies to RGB,
> +	 * and it must never be set for YCbCr or we risk setting
> +	 * some conflicting bits in PIPECONF which will mess up
> +	 * the colors on the monitor.
> +	 */
> +	if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
> +		return false;
> +
>  	if (intel_conn_state->broadcast_rgb ==
> INTEL_BROADCAST_RGB_AUTO) {
>  		/*
>  		 * See:
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index ca377ba3a15e..325abd462a46 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -724,6 +724,10 @@ intel_hdmi_compute_avi_infoframe(struct
> intel_encoder *encoder,
>  
>  	drm_hdmi_avi_infoframe_colorspace(frame, conn_state);
>  
> +	/* nonsense combination */
> +	WARN_ON(crtc_state->limited_color_range &&
> +		crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB);
> +
>  	if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_RGB) {
>  		drm_hdmi_avi_infoframe_quant_range(frame, connector,
>  						   adjusted_mode,
> @@ -2305,6 +2309,16 @@ static bool
> intel_hdmi_limited_color_range(const struct intel_crtc_state *crtc_s
>  	const struct drm_display_mode *adjusted_mode =
>  		&crtc_state->base.adjusted_mode;
>  
> +	/*
> +	 * Our YCbCr output is always limited range.
> +	 * crtc_state->limited_color_range only applies to RGB,
> +	 * and it must never be set for YCbCr or we risk setting
> +	 * some conflicting bits in PIPECONF which will mess up
> +	 * the colors on the monitor.
> +	 */
> +	if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
> +		return false;
> +
>  	if (intel_conn_state->broadcast_rgb ==
> INTEL_BROADCAST_RGB_AUTO) {
>  		/* See CEA-861-E - 5.1 Default Encoding Parameters */
>  		return crtc_state->has_hdmi_sink &&
> @@ -2341,9 +2355,6 @@ int intel_hdmi_compute_config(struct
> intel_encoder *encoder,
>  	if (pipe_config->has_hdmi_sink)
>  		pipe_config->has_infoframe = true;
>  
> -	pipe_config->limited_color_range =
> -		intel_hdmi_limited_color_range(pipe_config,
> conn_state);
> -
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) {
>  		pipe_config->pixel_multiplier = 2;
>  		clock_8bpc *= 2;
> @@ -2360,6 +2371,9 @@ int intel_hdmi_compute_config(struct
> intel_encoder *encoder,
>  		}
>  	}
>  
> +	pipe_config->limited_color_range =
> +		intel_hdmi_limited_color_range(pipe_config,
> conn_state);
> +
>  	if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv))
>  		pipe_config->has_pch_encoder = true;
>  
The changes look good to me.
Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel