> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Ville Syrjala > Sent: Wednesday, September 29, 2021 12:21 AM > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: [PATCH] drm/i915: Stop force enabling pipe bottom color > gammma/csc > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > 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@xxxxxxxxx> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3534 > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > --- > 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
