On Thu, 2019-07-18 at 19:45 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxx>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
