[Intel-gfx] [PATCH] drm/i915/color: fix broken display in icl+

[Ville Syrjälä]

On Tue, Oct 01, 2019 at 07:58:39PM +0530, Sharma, Swati2 wrote:
> On 01-Oct-19 7:51 PM, Ville Syrjälä wrote:
> > On Tue, Oct 01, 2019 at 11:03:08AM +0300, Jani Nikula wrote:
> >> On Tue, 01 Oct 2019, Swati Sharma <swati2.sharma at intel.com> wrote:
> >>> Premature gamma lut prepration and loading which was getting
> >>> reflected in first modeset causing different colors on
> >>> screen during boot.
> >>>
> >>> Issue: In BIOS, gamma is disabled by default. However,
> >>> legacy_read_luts() was getting called even before the legacy_load_luts()
> >>> which was setting crtc_state->base.gamma_lut and gamma_lut was
> >>> programmed with junk values which led to visual artifacts (different
> >>> colored screens instead of usual black during boot).
> >>>
> >>> Fix: Calling read_luts() only when gamma is enabled which will happen
> >>> after first modeset.
> >>>
> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111809
> >>
> >> I'm confused. Is there a current problem upstream after the revert
> >> 1b8588741fdc ("Revert "drm/i915/color: Extract icl_read_luts()"")?
> >>
> >> Or does this fix a problem that only occurs in conjunction with the
> >> reverted commit? Then say so.
> >>
> >> Note inline.
> >>
> >>> Signed-off-by: Swati Sharma <swati2.sharma at intel.com>
> >>> ---
> >>>   drivers/gpu/drm/i915/display/intel_display.c | 4 +++-
> >>>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >>> index f1328c08f4ad..f89aa4bb9f42 100644
> >>> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >>> @@ -10528,7 +10528,9 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> >>>   		i9xx_get_pipe_color_config(pipe_config);
> >>>   	}
> >>>   
> >>> -	intel_color_get_config(pipe_config);
> >>> +	if ((INTEL_GEN(dev_priv) >= 11 && (pipe_config->gamma_mode & POST_CSC_GAMMA_ENABLE)) ||
> >>> +	   (INTEL_GEN(dev_priv) >= 9 && (pipe_config->gamma_enable)))
> >>> +			intel_color_get_config(pipe_config);
> >>
> >> Put all of the conditions inside intel_color_get_config().
> > 
> > In fact inside the .read_luts() since these checks are platform
> > specific.
> > 
> > Also this check is wrong for CHV since it has a separate
> > enable knob for the CGM LUT (gamma_enable only deals with the
> > legacy LUT) >
> Inside read_luts() I already have. But the issue is first read_lut() 
> will happen before load_lut() and it will replace 
> crtc_state->base.gamma_lut and gamma_lut will be programmed with junk 
> values which led to multiple colored screens. So we need a check to call
> intel_color_get_config() itself.

That doesn't make sense. If you're already checking these then
adding a second check won't change anything.

Also state readout is meant to just blindly readout the hardware state.
If the LUT used by the BIOS is enabled and not something we want to
use then we need to sanitize it after the readout.

-- 
Ville Syrjälä
Intel