drm/i915/color: fix broken display in icl+
diff mbox series

Message ID 20191001063128.10566-1-swati2.sharma@intel.com
State New
Headers show
Series
  • drm/i915/color: fix broken display in icl+
Related show

Commit Message

Swati Sharma Oct. 1, 2019, 6:31 a.m. UTC
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
Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Jani Nikula Oct. 1, 2019, 8:03 a.m. UTC | #1
On Tue, 01 Oct 2019, Swati Sharma <swati2.sharma@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@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().

BR,
Jani.


>  
>  	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
>  	WARN_ON(power_domain_mask & BIT_ULL(power_domain));
Ville Syrjälä Oct. 1, 2019, 2:21 p.m. UTC | #2
On Tue, Oct 01, 2019 at 11:03:08AM +0300, Jani Nikula wrote:
> On Tue, 01 Oct 2019, Swati Sharma <swati2.sharma@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@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).
Swati Sharma Oct. 1, 2019, 2:28 p.m. UTC | #3
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@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@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.
Saarinen, Jani Oct. 1, 2019, 2:31 p.m. UTC | #4
HI, 

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjälä
> Sent: tiistai 1. lokakuuta 2019 17.21
> To: Nikula, Jani <jani.nikula@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/color: fix broken display in icl+
> 
> On Tue, Oct 01, 2019 at 11:03:08AM +0300, Jani Nikula wrote:
> > On Tue, 01 Oct 2019, Swati Sharma <swati2.sharma@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@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).
Could this be also reason that on BSW I was able to see some color issue with BSW on latest drm-tip still with Tapani? 

> 
> --
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Oct. 1, 2019, 2:36 p.m. UTC | #5
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@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@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.

Patch
diff mbox series

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);
 
 	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
 	WARN_ON(power_domain_mask & BIT_ULL(power_domain));