[Intel-gfx] [PATCH] drm/i915: Beef up the IPS vs. CRC workaround
Ville Syrjälä
ville.syrjala at linux.intel.com
Thu Dec 22 12:24:09 UTC 2016
On Wed, Dec 21, 2016 at 03:04:43PM -0200, Paulo Zanoni wrote:
> Em Qua, 2016-12-21 às 11:31 +0200, ville.syrjala at linux.intel.com
> escreveu:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > Oneshot disabling of IPS when CRC capturing is started is
> > insufficient.
> > IPS may get re-enabled by any plane update, and hence tests that keep
> > CRC capturing on across plane updates will start to see inconsistent
> > results as soon as IPS kicks back in. Add a new knob into the crtc
> > state
> > to make sure IPS stays disabled as long as CRC capturing is enabled.
> >
> > Forcing a modeset is the easiest way to handle this since that's
> > already
> > how we do the panel fitter workaround. It's a little heavy handed
> > just
> > for IPS, but seeing as we might already do the panel fitter
> > workaround
> > I think it's better to follow that. We migth want to optimize both
> > cases
> > later if someone gets too upset by the extra delay from the modeset.
> >
>
> Please add a "Testcase:" tag listing the IGTs this patch unbreaks.
The only thin I noticed so far was my kms_plane_blinker which isn't in
yet.
> Maybe also "Bugzilla:" if there's any.
Don't recall any. I can make a quick scan of the list though.
>
>
> > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 5 +++-
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_pipe_crc.c | 43 +++++++++++++++++++----
> > ------------
> > 3 files changed, 28 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index ef5dde5ab1cf..1ce479614f52 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7189,6 +7189,7 @@ static void hsw_compute_ips_config(struct
> > intel_crtc *crtc,
> > struct drm_i915_private *dev_priv = to_i915(dev);
> >
> > pipe_config->ips_enabled = i915.enable_ips &&
> > + !pipe_config->ips_force_disable &&
> > hsw_crtc_supports_ips(crtc) &&
> > pipe_config_supports_ips(dev_priv, pipe_config);
> > }
> > @@ -12958,7 +12959,7 @@ clear_intel_crtc_state(struct
> > intel_crtc_state *crtc_state)
> > struct intel_crtc_scaler_state scaler_state;
> > struct intel_dpll_hw_state dpll_hw_state;
> > struct intel_shared_dpll *shared_dpll;
> > - bool force_thru;
> > + bool force_thru, ips_force_disable;
> >
> > /* FIXME: before the switch to atomic started, a new
> > pipe_config was
> > * kzalloc'd. Code that depends on any field being zero
> > should be
> > @@ -12970,6 +12971,7 @@ clear_intel_crtc_state(struct
> > intel_crtc_state *crtc_state)
> > shared_dpll = crtc_state->shared_dpll;
> > dpll_hw_state = crtc_state->dpll_hw_state;
> > force_thru = crtc_state->pch_pfit.force_thru;
> > + ips_force_disable = crtc_state->ips_force_disable;
> >
> > memset(crtc_state, 0, sizeof *crtc_state);
> >
> > @@ -12978,6 +12980,7 @@ clear_intel_crtc_state(struct
> > intel_crtc_state *crtc_state)
> > crtc_state->shared_dpll = shared_dpll;
> > crtc_state->dpll_hw_state = dpll_hw_state;
> > crtc_state->pch_pfit.force_thru = force_thru;
> > + crtc_state->ips_force_disable = ips_force_disable;
> > }
> >
> > static int
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 025e4c8b3e63..cadba9b92cc9 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -651,6 +651,7 @@ struct intel_crtc_state {
> > struct intel_link_m_n fdi_m_n;
> >
> > bool ips_enabled;
> > + bool ips_force_disable;
> >
> > bool enable_fbc;
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index ef0c0e195164..708cf1d419d4 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -547,8 +547,8 @@ static int ilk_pipe_crc_ctl_reg(enum
> > intel_pipe_crc_source *source,
> > return 0;
> > }
> >
> > -static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private
> > *dev_priv,
> > - bool enable)
> > +static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> > + bool enable)
>
> Now we apply more than one WA. So maybe: hsw_pipe_a_crc_workarounds()
> or just hsw_pipe_crc_workarounds() (or apply_workarounds). Just "was"
> is confusing since it looks like a verb is missing. Also, that capital
> A is not exactly following our coding style.
>
>
> > {
> > struct drm_device *dev = &dev_priv->drm;
> > struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> > PIPE_A);
> > @@ -570,11 +570,23 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct
> > drm_i915_private *dev_priv,
> > goto out;
> > }
> >
> > - pipe_config->pch_pfit.force_thru = enable;
> > - if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> > - pipe_config->pch_pfit.enabled != enable)
> > + /*
> > + * When IPS gets enabled, the pipe CRC changes. Since IPS
> > gets
> > + * enabled and disabled dynamically based on package C
> > states,
> > + * user space can't make reliable use of the CRCs, so let's
> > just
> > + * completely disable it.
> > + */
> > + pipe_config->ips_force_disable = enable;
> > + if (pipe_config->ips_force_disable != enable)
>
> The fix mentioned in your other email makes sense, but I find it easier
> to read "if (pipe_config->ips_enable == pipe_config-
> >ips_force_disable)".
>
> > pipe_config->base.connectors_changed = true;
> >
> > + if (IS_HASWELL(dev_priv)) {
> > + pipe_config->pch_pfit.force_thru = enable;
> > + if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> > + pipe_config->pch_pfit.enabled != enable)
> > + pipe_config->base.connectors_changed = true;
> > + }
> > +
> > ret = drm_atomic_commit(state);
> > out:
> > WARN(ret, "Toggling workaround to %i returns %i\n", enable,
> > ret);
> > @@ -598,8 +610,9 @@ static int ivb_pipe_crc_ctl_reg(struct
> > drm_i915_private *dev_priv,
> > *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
> > break;
> > case INTEL_PIPE_CRC_SOURCE_PF:
> > - if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> > - hsw_trans_edp_pipe_A_crc_wa(dev_priv, true);
> > + if ((IS_HASWELL(dev_priv) ||
> > + IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> > + hsw_pipe_A_crc_wa(dev_priv, true);
>
> I know this problem is not caused by your patch, but in case you want:
> I wonder if there's a better place to call this. Maybe outside this
> function. Calling it here sounds like an unrelated side-effect of a
> function that's supposed to just return a value for a register.
Yeah, the disable call happens from a higher level IIRC, so moving this
might make sense from a consistency POV as well. I'll take another look
at it.
>
>
> >
> > *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
> > break;
> > @@ -618,7 +631,6 @@ static int pipe_crc_set_source(struct
> > drm_i915_private *dev_priv,
> > enum intel_pipe_crc_source source)
> > {
> > struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> > - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> > pipe);
> > enum intel_display_power_domain power_domain;
> > u32 val = 0; /* shut up gcc */
> > int ret;
> > @@ -665,14 +677,6 @@ static int pipe_crc_set_source(struct
> > drm_i915_private *dev_priv,
> > goto out;
> > }
> >
> > - /*
> > - * When IPS gets enabled, the pipe CRC changes.
> > Since IPS gets
> > - * enabled and disabled dynamically based on package
> > C states,
> > - * user space can't make reliable use of the CRCs,
> > so let's just
> > - * completely disable it.
> > - */
> > - hsw_disable_ips(crtc);
> > -
> > spin_lock_irq(&pipe_crc->lock);
> > kfree(pipe_crc->entries);
> > pipe_crc->entries = entries;
> > @@ -713,10 +717,9 @@ static int pipe_crc_set_source(struct
> > drm_i915_private *dev_priv,
> > g4x_undo_pipe_scramble_reset(dev_priv,
> > pipe);
> > else if (IS_VALLEYVIEW(dev_priv) ||
> > IS_CHERRYVIEW(dev_priv))
> > vlv_undo_pipe_scramble_reset(dev_priv,
> > pipe);
> > - else if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> > - hsw_trans_edp_pipe_A_crc_wa(dev_priv,
> > false);
> > -
> > - hsw_enable_ips(crtc);
> > + else if ((IS_HASWELL(dev_priv) ||
> > + IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
>
> Moving the gen+pipe checks to inside the function would deduplicate
> them and, IMHO, make the code slightly easier to read.
Hmm. Not sure. The scrambler reset stuff is here anyway. Or perhaps
we could move that stuff into some function as well.
>
> Now here's a question that maybe I never asked myself before: if we
> have two pipes enabled (A and B), and our system is properly configured
> for power management so we're reaching the deep PC states (depending on
> your machine, all you need to do is to run powertop --auto-tune), does
> pipe B also change its CRCs when IPS is enabled on pipe A? The reason I
> ask this is because IPS enables deeper PC states, but maybe it's the
> deeper PC states that causes different CRC calculation, not IPS, and
> then this would also affect the CRCs for non-IPS pipes (AKA pipe B). It
> would be great if you could check this, because I don't really remember
> if I checked for this when I wrote this code. Of course, the fix for
> this would be a separate patch since it's not the problem you're
> touching here. But then, maybe we could do something else to prevent
> deep PC states instead of disabling IPS.
I would assume it's IPS itself causing the problem. The package C
state having some undocumented effect on the pipe would sound quite bad
to me. But I must admit that I haven't tried it either. I guess I need
to fire up my HSW again and see how it behaves.
>
> Most of (all?) my suggestions above are bikesheds, so with or without
> them:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>
> > + hsw_pipe_A_crc_wa(dev_priv, false);
> > }
> >
> > ret = 0;
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list