Re: [Intel-gfx] [PATCH] drm/i915: Stop force enabling pipe bottom color gammma/csc

["Shankar, Uma" <uma.shankar@intel.com>]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjala
> Sent: Wednesday, September 29, 2021 12:21 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH] drm/i915: Stop force enabling pipe bottom color
> gammma/csc
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> While sanitizing the hardware state we're currently forcing the pipe bottom color
> legacy csc/gamma bits on. That is not a good idea as BIOSen are likely to leave
> gabage in the LUTs and so doing this causes ugly visual glitches if and when the
> planes covering the background get disabled. This was exactly the case on this Dell
> Precision 5560 tgl laptop.
> 
> On icl+ we don't normally even use these legacy bits anymore and instead use their
> GAMMA_MODE counterparts.
> On earlier platforms the bits are used, but we still shouldn't force them on without
> knowing what's in the LUT.
> 
> So two options, get rid of the whole thing, or do what
> intel_color_commit() does to make sure the bottom color state matches whatever
> out hardware readout produced. I chose the latter since it'll match what happens on
> older platforms when the primary plane gets turned off. In fact let's just call
> intel_color_commit(). It'll also do some CSC programming but since we don't have
> readout for that it'll actually just set to all zeros. So in the unlikely case of CSC
> actually being enabld by the BIOS we'll end up with all black until the first atomic
> commit happens.
> 
> Still not totally sure what we should do about color management features here in
> general. Probably the safest  thing would be to force everything off exactly at the
> same time when we disable the primary plane as there is no guarantees that
> whatever the LUTs/CSCs contain make any sense whatsoever without the specific
> pixel data in the BIOS fb. And if we preserve the primary plane then we should
> disable the color management features exactly when the primary plane fb contents
> first changes since the new content assumes more or less no transformations. But of
> course synchronizing front buffer rendering with anything else is a bit hard...
> 

A nice catch.

Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3534
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index f27c294beb92..17ce51576ed2 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -11991,13 +11991,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>  				intel_plane_disable_noatomic(crtc, plane);
>  		}
> 
> -		/*
> -		 * Disable any background color set by the BIOS, but enable the
> -		 * gamma and CSC to match how we program our planes.
> -		 */
> -		if (DISPLAY_VER(dev_priv) >= 9)
> -			intel_de_write(dev_priv, SKL_BOTTOM_COLOR(crtc->pipe),
> -				       SKL_BOTTOM_COLOR_GAMMA_ENABLE |
> SKL_BOTTOM_COLOR_CSC_ENABLE);
> +		/* Disable any background color/etc. set by the BIOS */
> +		intel_color_commit(crtc_state);
>  	}
> 
>  	/* Adjust the state of the output pipe according to whether we
> --
> 2.32.0