| Message ID | 20161221093105.19120-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 21, 2016 at 11:31:05AM +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@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. > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Ville Syrjälä <ville.syrjala@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) > { > 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) And naturally I typoed that. What I really wanted was: if (pipe_config->ips_enabled == enable) > 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); > > *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) > + hsw_pipe_A_crc_wa(dev_priv, false); > } > > ret = 0; > -- > 2.10.2
Em Qua, 2016-12-21 às 11:31 +0200, ville.syrjala@linux.intel.com escreveu: > From: Ville Syrjälä <ville.syrjala@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. Maybe also "Bugzilla:" if there's any. > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Ville Syrjälä <ville.syrjala@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. > > *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. 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. Most of (all?) my suggestions above are bikesheds, so with or without them: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > + hsw_pipe_A_crc_wa(dev_priv, false); > } > > ret = 0;
On Wed, Dec 21, 2016 at 03:04:43PM -0200, Paulo Zanoni wrote: > Em Qua, 2016-12-21 às 11:31 +0200, ville.syrjala@linux.intel.com > escreveu: > > From: Ville Syrjälä <ville.syrjala@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@intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Ville Syrjälä <ville.syrjala@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@intel.com> > > > + hsw_pipe_A_crc_wa(dev_priv, false); > > } > > > > ret = 0;
On Thu, Aug 17, 2017 at 03:58:19PM -0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: Beef up the IPS vs. CRC workaround (rev3) > URL : https://patchwork.freedesktop.org/series/17084/ > State : warning > > == Summary == > > Series 17084v3 drm/i915: Beef up the IPS vs. CRC workaround > https://patchwork.freedesktop.org/api/1.0/series/17084/revisions/3/mbox/ > > Test gem_exec_flush: > Subgroup basic-batch-kernel-default-uc: > pass -> FAIL (fi-snb-2600) fdo#100007 > Test kms_frontbuffer_tracking: > Subgroup basic: > pass -> DMESG-WARN (fi-bdw-5557u) [ 362.598196] [drm:intel_fbc_work_fn [i915]] *ERROR* vblank not available for FBC on pipe A I guess there is some kind of race with crtc enabling/disabling in the FBC code . I've seen this a few times on my IVB laptop when running with FBC enabled. > Test kms_pipe_crc_basic: > Subgroup suspend-read-crc-pipe-b: > pass -> DMESG-WARN (fi-byt-n2820) fdo#101705 > > fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007 > fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705 > > fi-bdw-5557u total:279 pass:267 dwarn:1 dfail:0 fail:0 skip:11 time:455s > fi-bdw-gvtdvm total:279 pass:265 dwarn:0 dfail:0 fail:0 skip:14 time:443s > fi-blb-e6850 total:279 pass:224 dwarn:1 dfail:0 fail:0 skip:54 time:359s > fi-bsw-n3050 total:279 pass:243 dwarn:0 dfail:0 fail:0 skip:36 time:535s > fi-bxt-j4205 total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:531s > fi-byt-j1900 total:279 pass:254 dwarn:1 dfail:0 fail:0 skip:24 time:525s > fi-byt-n2820 total:279 pass:250 dwarn:1 dfail:0 fail:0 skip:28 time:511s > fi-glk-2a total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:607s > fi-hsw-4770 total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:443s > fi-hsw-4770r total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:421s > fi-ilk-650 total:279 pass:229 dwarn:0 dfail:0 fail:0 skip:50 time:428s > fi-ivb-3520m total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:506s > fi-ivb-3770 total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:477s > fi-kbl-7500u total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:474s > fi-kbl-7560u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:595s > fi-kbl-r total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:596s > fi-pnv-d510 total:279 pass:223 dwarn:1 dfail:0 fail:0 skip:55 time:530s > fi-skl-6260u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:468s > fi-skl-6770hq total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:488s > fi-skl-gvtdvm total:279 pass:266 dwarn:0 dfail:0 fail:0 skip:13 time:438s > fi-skl-x1585l total:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:485s > fi-snb-2520m total:279 pass:251 dwarn:0 dfail:0 fail:0 skip:28 time:550s > fi-snb-2600 total:279 pass:249 dwarn:0 dfail:0 fail:1 skip:29 time:403s > > e1b18fbd4daecb1c1cf31ca101f64e29a8933bcf drm-tip: 2017y-08m-17d-14h-58m-23s UTC integration manifest > 568266b967eb drm/i915: Beef up the IPS vs. CRC workaround > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5430/
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) { 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) 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); *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) + hsw_pipe_A_crc_wa(dev_priv, false); } ret = 0;