| Message ID | 20191001063128.10566-1-swati2.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/color: fix broken display in icl+ | expand |
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));
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).
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.
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
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.
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));
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(-)