ACK/cmnt: [PATCH V2 1/2][SRU][H/OEM-5.10] UBUNTU: SAUCE: drm/i915: Stop force enabling pipe bottom color gammma/csc

[Kleber Souza]

On 04.10.21 07:48, Koba Ko wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1945932
> 
> 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...
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3534
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Ref: https://patchwork.freedesktop.org/series/95171/
> Signed-off-by: Koba Ko <koba.ko at canonical.com>

As Tim mentioned, this patch has been applied to linux-next so we
should use the provenance block from it and have something like:

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3534
Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20210928185105.3030-1-ville.syrjala@linux.intel.com
Reviewed-by: Uma Shankar <uma.shankar at intel.com>
(backported from f22f4e5be89c4296d76eaa9ba83dda46bdf11134 linux-next)
Signed-off-by: Koba Ko <koba.ko at canonical.com>

Note that it should be "backported" instead of "cherry picked" as
the original patch can't be applied with 'git am' due to context
differences.

The patch subject should also have "UBUNTU: SAUCE:" removed.


Acked-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>

Thanks


> ---
>   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 c55e3c504c6d0..a35462277637c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -18392,13 +18392,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 (INTEL_GEN(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
>