Re: [Intel-gfx] [PATCH 07/13] drm/i915: Move LUT programming to happen after vblank waits
On Wed, Jan 16, 2019 at 09:38:59AM -0800, Matt Roper wrote:
> On Fri, Jan 11, 2019 at 07:08:17PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > The LUTs are single buffered so we should program them after
> > the double buffered pipe updates have been latched by the
> > hardware.
> >
> > We'll also fix up the IPS vs. split gamma w/a to do the IPS
> > disable like everyone else. Note that this is currently dead
> > code as we don't use the split gamma mode on HSW, but that
> > will be fixed up shortly.
>
> I don't think this is quite dead code...we don't use split gamma
> ourselves, but we could potentially inherit that setup from the BIOS
> (which will stick around until it eventually gets clobbered by the first
> modeset/fastset).
>
> Uma's series added some logic to sanitize the LUT's immediately on boot,
> but that hasn't landed yet.
With the gamma_enable/csc_enable flags properly tracked we shouldn't
need anything like that. Or am I missing something?
>
>
> >
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/i915/intel_color.c | 25 +--
> > drivers/gpu/drm/i915/intel_display.c | 47
> > 2 files changed, 42 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_color.c
> > b/drivers/gpu/drm/i915/intel_color.c
> > index f9e0855162f3..0c0da7ed0fd7 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -361,29 +361,6 @@ static void hsw_color_commit(const struct
> > intel_crtc_state *crtc_state)
> > ilk_load_csc_matrix(crtc_state);
> > }
> >
> > -/* Loads the legacy palette/gamma unit for the CRTC on Haswell. */
> > -static void haswell_load_luts(const struct intel_crtc_state *crtc_state)
> > -{
> > - struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > - bool reenable_ips = false;
> > -
> > - /*
> > -* Workaround : Do not read or write the pipe palette/gamma data while
> > -* GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
> > -*/
> > - if (IS_HASWELL(dev_priv) && crtc_state->ips_enabled &&
> > - (crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)) {
> > - hsw_disable_ips(crtc_state);
> > - reenable_ips = true;
> > - }
> > -
> > - i9xx_load_luts(crtc_state);
> > -
> > - if (reenable_ips)
> > - hsw_enable_ips(crtc_state);
> > -}
> > -
> > static void bdw_load_degamma_lut(const struct intel_crtc_state *crtc_state)
> > {
> > struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > @@ -660,7 +637,7 @@ void intel_color_init(struct intel_crtc *crtc)
> > if (IS_CHERRYVIEW(dev_priv)) {
> > dev_priv->display.load_luts = cherryview_load_luts;
> > } else if (IS_HASWELL(dev_priv)) {
> > - dev_priv->display.load_luts = haswell_load_luts;
> > + dev_priv->display.load_luts = i9xx_load_luts;
> > dev_priv->display.color_commit = hsw_color_commit;
> > } else if (IS_BROADWELL(dev_priv) || IS_GEN9_BC(dev_priv) ||
> >IS_BROXTON(dev_priv)) {
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 96c78566b8e6..1caee4128974 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5299,24 +5299,54 @@ intel_pre_disable_primary_noatomic(struct drm_crtc
> > *crtc)
> > static bool hsw_pre_update_disable_ips(const struct intel_crtc_state
> > *old_crtc_state,
> >const struct intel_crtc_state
> > *new_crtc_state)
> > {
> > + struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
> > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +
> > if (!old_crtc_state->ips_enabled)
> > return false;
> >
> > if (needs_modeset(&new_crtc_state->base))
> > return true;
> >
> > + /*
> > +* Workaround : Do not read or write the pipe palette/gamma data while
> > +* GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
> > +*
> > +* Disable IPS before we program the LUT.
> > +*/
> > + if (IS_HASWELL(dev_priv) &&
> > + (new_crtc_state->base.color_mgmt_changed ||
> > +new_crtc_state->update_pipe) &&
> > + new_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)
>
> Wouldn't we want old_crtc_state for the gamma_mode test? We need to
> disable IPS if we're already in split gamma mode (inherited from BIOS),
> regardless of whether we're moving to non-split gamma.
>
>
> Matt
>
> > + return true;
> > +
> > return !new_crtc_state->ips_enabled;
> > }
> >
> > static bool hsw_post_update_enable_ips(const struct intel_crtc_state
> > *old_crtc_state,
> >const struct intel_crtc_state
> > *new_crtc_state)
> > {
> > + struct intel_cr
Re: [Intel-gfx] [PATCH 07/13] drm/i915: Move LUT programming to happen after vblank waits
On Wed, Jan 16, 2019 at 09:38:59AM -0800, Matt Roper wrote:
> On Fri, Jan 11, 2019 at 07:08:17PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > The LUTs are single buffered so we should program them after
> > the double buffered pipe updates have been latched by the
> > hardware.
> >
> > We'll also fix up the IPS vs. split gamma w/a to do the IPS
> > disable like everyone else. Note that this is currently dead
> > code as we don't use the split gamma mode on HSW, but that
> > will be fixed up shortly.
>
> I don't think this is quite dead code...we don't use split gamma
> ourselves, but we could potentially inherit that setup from the BIOS
> (which will stick around until it eventually gets clobbered by the first
> modeset/fastset).
>
> Uma's series added some logic to sanitize the LUT's immediately on boot,
> but that hasn't landed yet.
>
>
> >
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/i915/intel_color.c | 25 +--
> > drivers/gpu/drm/i915/intel_display.c | 47
> > 2 files changed, 42 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_color.c
> > b/drivers/gpu/drm/i915/intel_color.c
> > index f9e0855162f3..0c0da7ed0fd7 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -361,29 +361,6 @@ static void hsw_color_commit(const struct
> > intel_crtc_state *crtc_state)
> > ilk_load_csc_matrix(crtc_state);
> > }
> >
> > -/* Loads the legacy palette/gamma unit for the CRTC on Haswell. */
> > -static void haswell_load_luts(const struct intel_crtc_state *crtc_state)
> > -{
> > - struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > - bool reenable_ips = false;
> > -
> > - /*
> > -* Workaround : Do not read or write the pipe palette/gamma data while
> > -* GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
> > -*/
> > - if (IS_HASWELL(dev_priv) && crtc_state->ips_enabled &&
> > - (crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)) {
> > - hsw_disable_ips(crtc_state);
> > - reenable_ips = true;
> > - }
> > -
> > - i9xx_load_luts(crtc_state);
> > -
> > - if (reenable_ips)
> > - hsw_enable_ips(crtc_state);
> > -}
> > -
> > static void bdw_load_degamma_lut(const struct intel_crtc_state *crtc_state)
> > {
> > struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > @@ -660,7 +637,7 @@ void intel_color_init(struct intel_crtc *crtc)
> > if (IS_CHERRYVIEW(dev_priv)) {
> > dev_priv->display.load_luts = cherryview_load_luts;
> > } else if (IS_HASWELL(dev_priv)) {
> > - dev_priv->display.load_luts = haswell_load_luts;
> > + dev_priv->display.load_luts = i9xx_load_luts;
> > dev_priv->display.color_commit = hsw_color_commit;
> > } else if (IS_BROADWELL(dev_priv) || IS_GEN9_BC(dev_priv) ||
> >IS_BROXTON(dev_priv)) {
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 96c78566b8e6..1caee4128974 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5299,24 +5299,54 @@ intel_pre_disable_primary_noatomic(struct drm_crtc
> > *crtc)
> > static bool hsw_pre_update_disable_ips(const struct intel_crtc_state
> > *old_crtc_state,
> >const struct intel_crtc_state
> > *new_crtc_state)
> > {
> > + struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
> > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +
> > if (!old_crtc_state->ips_enabled)
> > return false;
> >
> > if (needs_modeset(&new_crtc_state->base))
> > return true;
> >
> > + /*
> > +* Workaround : Do not read or write the pipe palette/gamma data while
> > +* GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
> > +*
> > +* Disable IPS before we program the LUT.
> > +*/
> > + if (IS_HASWELL(dev_priv) &&
> > + (new_crtc_state->base.color_mgmt_changed ||
> > +new_crtc_state->update_pipe) &&
> > + new_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)
>
> Wouldn't we want old_crtc_state for the gamma_mode test? We need to
> disable IPS if we're already in split gamma mode (inherited from BIOS),
> regardless of whether we're moving to non-split gamma.
We're going to update the gamma mode before programming the LUT.
But I think I'll probably change this to disable IPS around all LUT
updates to prevent IPS from using the old LUT during the vblank. That's
assuming IPS does actually prefill during vblank (which would make sense
to me).
>
>
> Matt
>
> > + return true;
> > +
> > return !new_crtc_state->ips_enabled;
> > }
> >
> > static bool hsw_post_update_enable_ips(const
Re: [Intel-gfx] [PATCH 07/13] drm/i915: Move LUT programming to happen after vblank waits
On Fri, Jan 11, 2019 at 07:08:17PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> The LUTs are single buffered so we should program them after
> the double buffered pipe updates have been latched by the
> hardware.
>
> We'll also fix up the IPS vs. split gamma w/a to do the IPS
> disable like everyone else. Note that this is currently dead
> code as we don't use the split gamma mode on HSW, but that
> will be fixed up shortly.
I don't think this is quite dead code...we don't use split gamma
ourselves, but we could potentially inherit that setup from the BIOS
(which will stick around until it eventually gets clobbered by the first
modeset/fastset).
Uma's series added some logic to sanitize the LUT's immediately on boot,
but that hasn't landed yet.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/intel_color.c | 25 +--
> drivers/gpu/drm/i915/intel_display.c | 47
> 2 files changed, 42 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_color.c
> b/drivers/gpu/drm/i915/intel_color.c
> index f9e0855162f3..0c0da7ed0fd7 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -361,29 +361,6 @@ static void hsw_color_commit(const struct
> intel_crtc_state *crtc_state)
> ilk_load_csc_matrix(crtc_state);
> }
>
> -/* Loads the legacy palette/gamma unit for the CRTC on Haswell. */
> -static void haswell_load_luts(const struct intel_crtc_state *crtc_state)
> -{
> - struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> - bool reenable_ips = false;
> -
> - /*
> - * Workaround : Do not read or write the pipe palette/gamma data while
> - * GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
> - */
> - if (IS_HASWELL(dev_priv) && crtc_state->ips_enabled &&
> - (crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)) {
> - hsw_disable_ips(crtc_state);
> - reenable_ips = true;
> - }
> -
> - i9xx_load_luts(crtc_state);
> -
> - if (reenable_ips)
> - hsw_enable_ips(crtc_state);
> -}
> -
> static void bdw_load_degamma_lut(const struct intel_crtc_state *crtc_state)
> {
> struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> @@ -660,7 +637,7 @@ void intel_color_init(struct intel_crtc *crtc)
> if (IS_CHERRYVIEW(dev_priv)) {
> dev_priv->display.load_luts = cherryview_load_luts;
> } else if (IS_HASWELL(dev_priv)) {
> - dev_priv->display.load_luts = haswell_load_luts;
> + dev_priv->display.load_luts = i9xx_load_luts;
> dev_priv->display.color_commit = hsw_color_commit;
> } else if (IS_BROADWELL(dev_priv) || IS_GEN9_BC(dev_priv) ||
> IS_BROXTON(dev_priv)) {
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 96c78566b8e6..1caee4128974 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5299,24 +5299,54 @@ intel_pre_disable_primary_noatomic(struct drm_crtc
> *crtc)
> static bool hsw_pre_update_disable_ips(const struct intel_crtc_state
> *old_crtc_state,
> const struct intel_crtc_state
> *new_crtc_state)
> {
> + struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +
> if (!old_crtc_state->ips_enabled)
> return false;
>
> if (needs_modeset(&new_crtc_state->base))
> return true;
>
> + /*
> + * Workaround : Do not read or write the pipe palette/gamma data while
> + * GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
> + *
> + * Disable IPS before we program the LUT.
> + */
> + if (IS_HASWELL(dev_priv) &&
> + (new_crtc_state->base.color_mgmt_changed ||
> + new_crtc_state->update_pipe) &&
> + new_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)
Wouldn't we want old_crtc_state for the gamma_mode test? We need to
disable IPS if we're already in split gamma mode (inherited from BIOS),
regardless of whether we're moving to non-split gamma.
Matt
> + return true;
> +
> return !new_crtc_state->ips_enabled;
> }
>
> static bool hsw_post_update_enable_ips(const struct intel_crtc_state
> *old_crtc_state,
> const struct intel_crtc_state
> *new_crtc_state)
> {
> + struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +
> if (!new_crtc_state->ips_enabled)
> return false;
>
> if (needs_modeset(&new_crtc_state->base))
> return true;
>
> + /*
> + * Workaround : Do not read or wr
[Intel-gfx] [PATCH 07/13] drm/i915: Move LUT programming to happen after vblank waits
From: Ville Syrjälä
The LUTs are single buffered so we should program them after
the double buffered pipe updates have been latched by the
hardware.
We'll also fix up the IPS vs. split gamma w/a to do the IPS
disable like everyone else. Note that this is currently dead
code as we don't use the split gamma mode on HSW, but that
will be fixed up shortly.
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/intel_color.c | 25 +--
drivers/gpu/drm/i915/intel_display.c | 47
2 files changed, 42 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_color.c
b/drivers/gpu/drm/i915/intel_color.c
index f9e0855162f3..0c0da7ed0fd7 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -361,29 +361,6 @@ static void hsw_color_commit(const struct intel_crtc_state
*crtc_state)
ilk_load_csc_matrix(crtc_state);
}
-/* Loads the legacy palette/gamma unit for the CRTC on Haswell. */
-static void haswell_load_luts(const struct intel_crtc_state *crtc_state)
-{
- struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
- struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
- bool reenable_ips = false;
-
- /*
-* Workaround : Do not read or write the pipe palette/gamma data while
-* GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
-*/
- if (IS_HASWELL(dev_priv) && crtc_state->ips_enabled &&
- (crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)) {
- hsw_disable_ips(crtc_state);
- reenable_ips = true;
- }
-
- i9xx_load_luts(crtc_state);
-
- if (reenable_ips)
- hsw_enable_ips(crtc_state);
-}
-
static void bdw_load_degamma_lut(const struct intel_crtc_state *crtc_state)
{
struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
@@ -660,7 +637,7 @@ void intel_color_init(struct intel_crtc *crtc)
if (IS_CHERRYVIEW(dev_priv)) {
dev_priv->display.load_luts = cherryview_load_luts;
} else if (IS_HASWELL(dev_priv)) {
- dev_priv->display.load_luts = haswell_load_luts;
+ dev_priv->display.load_luts = i9xx_load_luts;
dev_priv->display.color_commit = hsw_color_commit;
} else if (IS_BROADWELL(dev_priv) || IS_GEN9_BC(dev_priv) ||
IS_BROXTON(dev_priv)) {
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 96c78566b8e6..1caee4128974 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5299,24 +5299,54 @@ intel_pre_disable_primary_noatomic(struct drm_crtc
*crtc)
static bool hsw_pre_update_disable_ips(const struct intel_crtc_state
*old_crtc_state,
const struct intel_crtc_state
*new_crtc_state)
{
+ struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
+ struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+
if (!old_crtc_state->ips_enabled)
return false;
if (needs_modeset(&new_crtc_state->base))
return true;
+ /*
+* Workaround : Do not read or write the pipe palette/gamma data while
+* GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
+*
+* Disable IPS before we program the LUT.
+*/
+ if (IS_HASWELL(dev_priv) &&
+ (new_crtc_state->base.color_mgmt_changed ||
+new_crtc_state->update_pipe) &&
+ new_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)
+ return true;
+
return !new_crtc_state->ips_enabled;
}
static bool hsw_post_update_enable_ips(const struct intel_crtc_state
*old_crtc_state,
const struct intel_crtc_state
*new_crtc_state)
{
+ struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
+ struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+
if (!new_crtc_state->ips_enabled)
return false;
if (needs_modeset(&new_crtc_state->base))
return true;
+ /*
+* Workaround : Do not read or write the pipe palette/gamma data while
+* GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
+*
+* Re-enable IPS after the LUT has been programmed.
+*/
+ if (IS_HASWELL(dev_priv) &&
+ (new_crtc_state->base.color_mgmt_changed ||
+new_crtc_state->update_pipe) &&
+ new_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)
+ return true;
+
/*
* We can't read out IPS on broadwell, assume the worst and
* forcibly enable IPS on the first fastset.
@@ -11050,7 +11080,7 @@ static int intel_crtc_atomic_check(struct drm_crtc
*crtc,
return ret;
}
- if (crtc_state
