Re: [Intel-gfx] [PATCH] drm/i915: set DP Main Stream Attribute for color range on DDI platforms

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Aug 14, 2018 at 09:00:01AM +0300, Jani Nikula wrote:
> Since Haswell we have no color range indication either in the pipe or
> port registers for DP. Instead, there's a separate register for setting
> the DP Main Stream Attributes (MSA) directly. The MSA register
> definition makes no references to colorimetry, just a vague reference to
> the DP spec. The connection to the color range was lost.
> 
> Apparently we've failed to set the proper MSA bit for limited, or CEA,
> range ever since the first DDI platforms. We've started setting other
> MSA parameters since commit dae847991a43 ("drm/i915: add
> intel_ddi_set_pipe_settings").
> 
> Without the crucial bit of information, the DP sink has no way of
> knowing the source is actually transmitting limited range RGB, leading
> to "washed out" colors. With the colorimetry information, compliant
> sinks should be able to handle the limited range properly. Native
> (i.e. non-LSPCON) HDMI was not affected because we do pass the color
> range via AVI infoframes.
> 
> Though not the root cause, the problem was made worse for DDI platforms
> with commit 55bc60db5988 ("drm/i915: Add "Automatic" mode for the
> "Broadcast RGB" property"), which selects limited range RGB
> automatically based on the mode, as per the DP, HDMI and CEA specs.
> 
> After all these years, the fix boils down to flipping one bit.
> 
> [Per testing reports, this fixes DP sinks, but not the LSPCON. My
>  educated guess is that the LSPCON fails to turn the CEA range MSA into
>  AVI infoframes for HDMI.]
> 
> Reported-by: Michał Kopeć <mkopec12@gmail.com>
> Reported-by: N. W. <nw9165-3201@yahoo.com>
> Reported-by: Nicholas Stommel <nicholas.stommel@gmail.com>
> Reported-by: Tom Yan <tom.ty89@gmail.com>
> Tested-by: Nicholas Stommel <nicholas.stommel@gmail.com>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=100023
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107476
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=94921
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v3.9+
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

wow! and that was hard to review as well...

I finally found the bit defined and set for this case on
"Table 2-96: MISC1 and MISC0 Fields for Pixel Encoding/Colorimetry Format Indication"

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


> ---
>  drivers/gpu/drm/i915/i915_reg.h  | 1 +
>  drivers/gpu/drm/i915/intel_ddi.c | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 17575cfc22b5..0c9f03dda569 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9246,6 +9246,7 @@ enum skl_power_gate {
>  #define  TRANS_MSA_10_BPC		(2 << 5)
>  #define  TRANS_MSA_12_BPC		(3 << 5)
>  #define  TRANS_MSA_16_BPC		(4 << 5)
> +#define  TRANS_MSA_CEA_RANGE		(1 << 3)
>  
>  /* LCPLL Control */
>  #define LCPLL_CTL			_MMIO(0x130040)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 0adc043529f2..6f7be066c8f2 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1685,6 +1685,10 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
>  	WARN_ON(transcoder_is_dsi(cpu_transcoder));
>  
>  	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;
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx