On Fri, Nov 09, 2018 at 04:44:54PM +0200, Mika Kuoppala wrote: > Skip the hardware dbuf slice update if we don't have active > pipes. With no active pipes, we don't have powerwell and thus > programming the dbuf slice counts leads to accessing > hardware without runtime pm ref. > > v2: bugzilla tag (Imre) > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108654 > Cc: Imre Deak <imre.deak@xxxxxxxxx> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/i915/intel_display.c | 32 +++++++++++++++---------- > drivers/gpu/drm/i915/intel_drv.h | 3 +-- > drivers/gpu/drm/i915/intel_runtime_pm.c | 12 ++++++++-- > 3 files changed, 30 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 05125c7c2aa1..0514b89611ac 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12644,23 +12644,23 @@ static void skl_update_crtcs(struct drm_atomic_state *state) > struct intel_crtc *intel_crtc; > struct drm_crtc_state *old_crtc_state, *new_crtc_state; > struct intel_crtc_state *cstate; > - unsigned int updated = 0; > + unsigned int updated = 0, active_count = 0; > + u8 required_slices; > bool progress; > enum pipe pipe; > int i; > - u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices; > - u8 required_slices = intel_state->wm_results.ddb.enabled_slices; > - > const struct skl_ddb_entry *entries[I915_MAX_PIPES] = {}; > > - for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) > + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { > /* ignore allocations for crtc's that have been turned off. */ > - if (new_crtc_state->active) > + if (new_crtc_state->active) { > + active_count++; > entries[i] = &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb; > + } > + } > > - /* If 2nd DBuf slice required, enable it here */ > - if (INTEL_GEN(dev_priv) >= 11 && required_slices > hw_enabled_slices) > - icl_dbuf_slices_update(dev_priv, required_slices); Looks like this is the update that triggers the problem in the bugzilla ticket. Wondering how this can happen since we have all outputs and runtime suspended. Then by now icl_dbuf_disable() should have zeroed dev_priv->wm.skl_hw.ddb.enabled_slices and skl_compute_ddb() copied this 0 to intel_state->wm_results.ddb.enabled_slices during the compute phase of the problematic commit (since no crtcs are active this 0 should've been kept as-is). Then here both hw_enabled_slices and required_slices should be 0, but it's obviously not the case. What could happen I think is that the store in icl_dbuf_disable() / icl_dbuf_enable() can race with the load in skl_compute_ddb(). So perhaps skl_compute_ddb() copied from dev_priv enabled_slices as 1 or 2 (the only values we compute during a commit), then icl_dbuf_disable() zeroed in a racy way enabled_slices in dev_priv and we get here required_slices being 1 or 2 and hw_enabled_slices being 0. So while the change in this patch gets rid of the symptom, I think we should fix the root cause: 1. Do not update enabled_slices in icl_dbuf_enable() / icl_dbuf_disable() We instead depend on the DDB HW readout to set the correct initial value after module loading/resume and any following commit to update it if needed (based on new resolution etc.). After this we could still end up with stale values in wm.skl_hw.ddb.enabled_slices, for instance if it's 2 b/c of two pipes being enabled we wouldn't set it to 1 atm when disabling both pipes in the same atomic commit. So we'd also need the following 2 changes. 2. Force-enable only a single slice in icl_dbuf_enable() (as the spec requires) keeping the 2nd slice enabled only if BIOS has enabled it (DDB HW readout will set the correct enabled_slices afterwards). 3. Make sure we always compute the proper enabled_slices in the atomic state, by moving the calculation from intel_get_ddb_size() to earlier, performing it even if all crtcs are disabled. > + required_slices = active_count ? intel_state->wm_results.ddb.enabled_slices : 0; > + intel_dbuf_slices_update(dev_priv, required_slices); > > /* > * Whenever the number of active pipes changes, we need to make sure we > @@ -12670,6 +12670,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state) > */ > do { > progress = false; > + active_count = 0; > > for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { > bool vbl_wait = false; > @@ -12679,7 +12680,12 @@ static void skl_update_crtcs(struct drm_atomic_state *state) > cstate = to_intel_crtc_state(new_crtc_state); > pipe = intel_crtc->pipe; > > - if (updated & cmask || !cstate->base.active) > + if (!cstate->base.active) > + continue; > + > + active_count++; > + > + if (updated & cmask) > continue; > > if (skl_ddb_allocation_overlaps(dev_priv, > @@ -12713,9 +12719,9 @@ static void skl_update_crtcs(struct drm_atomic_state *state) > } > } while (progress); > > - /* If 2nd DBuf slice is no more required disable it */ > - if (INTEL_GEN(dev_priv) >= 11 && required_slices < hw_enabled_slices) > - icl_dbuf_slices_update(dev_priv, required_slices); > + > + required_slices = active_count ? intel_state->wm_results.ddb.enabled_slices : 0; > + intel_dbuf_slices_update(dev_priv, required_slices); > } > > static void intel_atomic_helper_free_state(struct drm_i915_private *dev_priv) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 21819a9bdcae..d643f8877097 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -2086,8 +2086,7 @@ bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv, > enum intel_display_power_domain domain); > void intel_display_power_put(struct drm_i915_private *dev_priv, > enum intel_display_power_domain domain); > -void icl_dbuf_slices_update(struct drm_i915_private *dev_priv, > - u8 req_slices); > +void intel_dbuf_slices_update(struct drm_i915_private *dev_priv, u8 req_slices); > > static inline void > assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv) > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 770de2632530..3a271ac22fec 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -3233,8 +3233,8 @@ static u8 intel_dbuf_max_slices(struct drm_i915_private *dev_priv) > return 2; > } > > -void icl_dbuf_slices_update(struct drm_i915_private *dev_priv, > - u8 req_slices) > +static void icl_dbuf_slices_update(struct drm_i915_private *dev_priv, > + const u8 req_slices) > { > const u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices; > bool ret; > @@ -3256,6 +3256,14 @@ void icl_dbuf_slices_update(struct drm_i915_private *dev_priv, > dev_priv->wm.skl_hw.ddb.enabled_slices = req_slices; > } > > +void intel_dbuf_slices_update(struct drm_i915_private *dev_priv, u8 slices) > +{ > + if (INTEL_GEN(dev_priv) < 11) > + return; > + > + icl_dbuf_slices_update(dev_priv, slices); > +} > + > static void icl_dbuf_enable(struct drm_i915_private *dev_priv) > { > I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) | DBUF_POWER_REQUEST); > -- > 2.17.1 > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
