| Message ID | 20190808014423.20377-2-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Display uncore | expand |
Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> writes: > Multiple uncore structures will share the debug infrastructure, so > move it to a common place and add extra locking around it. > Also, since we now have a separate object, it is cleaner to have > dedicated functions working on the object to stop and restart the > mmio debug. Apart from the cosmetic changes, this patch introduces > 2 functional updates: > > - All calls to check_for_unclaimed_mmio will now return false when > the debug is suspended, not just the ones that are active only when > i915_modparams.mmio_debug is set. If we don't trust the result of the > check while a user is doing mmio access then we shouldn't attempt the > check anywhere. > > - i915_modparams.mmio_debug is not save/restored anymore around user > access. The value is now never touched by the kernel while debug is > disabled so no need for save/restore. > > v2: squash mmio_debug patches, restrict mmio_debug lock usage (Chris) > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 2 +- > drivers/gpu/drm/i915/i915_drv.c | 3 +- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_uncore.c | 91 ++++++++++++++++++++--------- > drivers/gpu/drm/i915/intel_uncore.h | 18 +++--- > 5 files changed, 79 insertions(+), 36 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 3b15266c54fd..2310512111ab 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1129,7 +1129,7 @@ static int i915_forcewake_domains(struct seq_file *m, void *data) > unsigned int tmp; > > seq_printf(m, "user.bypass_count = %u\n", > - uncore->user_forcewake.count); > + uncore->user_forcewake_count); > > for_each_fw_domain(fw_domain, uncore, tmp) > seq_printf(m, "%s.wake_count = %u\n", > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 3480db36b63f..fbbff4a133ba 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -744,6 +744,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv) > > intel_device_info_subplatform_init(dev_priv); > > + intel_uncore_mmio_debug_init_early(&dev_priv->mmio_debug); > intel_uncore_init_early(&dev_priv->uncore, dev_priv); > > spin_lock_init(&dev_priv->irq_lock); > @@ -2044,7 +2045,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) > > out: > enable_rpm_wakeref_asserts(rpm); > - if (!dev_priv->uncore.user_forcewake.count) > + if (!dev_priv->uncore.user_forcewake_count) > intel_runtime_pm_driver_release(rpm); > > return ret; > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c9476f24f5c1..13c27a75dca8 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1346,6 +1346,7 @@ struct drm_i915_private { > resource_size_t stolen_usable_size; /* Total size minus reserved ranges */ > > struct intel_uncore uncore; > + struct intel_uncore_mmio_debug mmio_debug; > > struct i915_virtual_gpu vgpu; > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 39e8710afdd9..9e583f13a9e4 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -34,6 +34,32 @@ > > #define __raw_posting_read(...) ((void)__raw_uncore_read32(__VA_ARGS__)) > > +void > +intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug) > +{ > + spin_lock_init(&mmio_debug->lock); > + mmio_debug->unclaimed_mmio_check = 1; > +} > + > +static void mmio_debug_suspend(struct intel_uncore_mmio_debug *mmio_debug) > +{ > + lockdep_assert_held(&mmio_debug->lock); > + > + /* Save and disable mmio debugging for the user bypass */ > + if (!mmio_debug->suspend_count++) { > + mmio_debug->saved_mmio_check = mmio_debug->unclaimed_mmio_check; > + mmio_debug->unclaimed_mmio_check = 0; > + } > +} > + > +static void mmio_debug_resume(struct intel_uncore_mmio_debug *mmio_debug) > +{ > + lockdep_assert_held(&mmio_debug->lock); > + > + if (!--mmio_debug->suspend_count) > + mmio_debug->unclaimed_mmio_check = mmio_debug->saved_mmio_check; > +} > + > static const char * const forcewake_domain_names[] = { > "render", > "blitter", > @@ -476,6 +502,11 @@ check_for_unclaimed_mmio(struct intel_uncore *uncore) > { > bool ret = false; > > + lockdep_assert_held(&uncore->debug->lock); > + > + if (uncore->debug->suspend_count) > + return false; > + > if (intel_uncore_has_fpga_dbg_unclaimed(uncore)) > ret |= fpga_check_for_unclaimed_mmio(uncore); > > @@ -608,17 +639,11 @@ void intel_uncore_forcewake_get(struct intel_uncore *uncore, > void intel_uncore_forcewake_user_get(struct intel_uncore *uncore) > { > spin_lock_irq(&uncore->lock); > - if (!uncore->user_forcewake.count++) { > + if (!uncore->user_forcewake_count++) { > intel_uncore_forcewake_get__locked(uncore, FORCEWAKE_ALL); > - > - /* Save and disable mmio debugging for the user bypass */ > - uncore->user_forcewake.saved_mmio_check = > - uncore->unclaimed_mmio_check; > - uncore->user_forcewake.saved_mmio_debug = > - i915_modparams.mmio_debug; > - > - uncore->unclaimed_mmio_check = 0; > - i915_modparams.mmio_debug = 0; > + spin_lock(&uncore->debug->lock); For me it looks like you need spin_lock_irq here also like with the parent lock above. -Mika > + mmio_debug_suspend(uncore->debug); > + spin_unlock(&uncore->debug->lock); > } > spin_unlock_irq(&uncore->lock); > } > @@ -633,15 +658,14 @@ void intel_uncore_forcewake_user_get(struct intel_uncore *uncore) > void intel_uncore_forcewake_user_put(struct intel_uncore *uncore) > { > spin_lock_irq(&uncore->lock); > - if (!--uncore->user_forcewake.count) { > - if (intel_uncore_unclaimed_mmio(uncore)) > + if (!--uncore->user_forcewake_count) { > + spin_lock(&uncore->debug->lock); > + mmio_debug_resume(uncore->debug); > + > + if (check_for_unclaimed_mmio(uncore)) > dev_info(uncore->i915->drm.dev, > "Invalid mmio detected during user access\n"); > - > - uncore->unclaimed_mmio_check = > - uncore->user_forcewake.saved_mmio_check; > - i915_modparams.mmio_debug = > - uncore->user_forcewake.saved_mmio_debug; > + spin_unlock(&uncore->debug->lock); > > intel_uncore_forcewake_put__locked(uncore, FORCEWAKE_ALL); > } > @@ -1088,7 +1112,16 @@ unclaimed_reg_debug(struct intel_uncore *uncore, > if (likely(!i915_modparams.mmio_debug)) > return; > > + /* interrupts are disabled and re-enabled around uncore->lock usage */ > + lockdep_assert_held(&uncore->lock); > + > + if (before) > + spin_lock(&uncore->debug->lock); > + > __unclaimed_reg_debug(uncore, reg, read, before); > + > + if (!before) > + spin_unlock(&uncore->debug->lock); > } > > #define GEN2_READ_HEADER(x) \ > @@ -1607,6 +1640,7 @@ void intel_uncore_init_early(struct intel_uncore *uncore, > spin_lock_init(&uncore->lock); > uncore->i915 = i915; > uncore->rpm = &i915->runtime_pm; > + uncore->debug = &i915->mmio_debug; > } > > static void uncore_raw_init(struct intel_uncore *uncore) > @@ -1632,7 +1666,6 @@ static int uncore_forcewake_init(struct intel_uncore *uncore) > ret = intel_uncore_fw_domains_init(uncore); > if (ret) > return ret; > - > forcewake_early_sanitize(uncore, 0); > > if (IS_GEN_RANGE(i915, 6, 7)) { > @@ -1681,8 +1714,6 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) > if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915)) > uncore->flags |= UNCORE_HAS_FORCEWAKE; > > - uncore->unclaimed_mmio_check = 1; > - > if (!intel_uncore_has_forcewake(uncore)) { > uncore_raw_init(uncore); > } else { > @@ -1707,7 +1738,7 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) > uncore->flags |= UNCORE_HAS_FIFO; > > /* clear out unclaimed reg detection bit */ > - if (check_for_unclaimed_mmio(uncore)) > + if (intel_uncore_unclaimed_mmio(uncore)) > DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n"); > > return 0; > @@ -1952,7 +1983,13 @@ int __intel_wait_for_register(struct intel_uncore *uncore, > > bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore) > { > - return check_for_unclaimed_mmio(uncore); > + bool ret; > + > + spin_lock_irq(&uncore->debug->lock); > + ret = check_for_unclaimed_mmio(uncore); > + spin_unlock_irq(&uncore->debug->lock); > + > + return ret; > } > > bool > @@ -1960,24 +1997,24 @@ intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore) > { > bool ret = false; > > - spin_lock_irq(&uncore->lock); > + spin_lock_irq(&uncore->debug->lock); > > - if (unlikely(uncore->unclaimed_mmio_check <= 0)) > + if (unlikely(uncore->debug->unclaimed_mmio_check <= 0)) > goto out; > > - if (unlikely(intel_uncore_unclaimed_mmio(uncore))) { > + if (unlikely(check_for_unclaimed_mmio(uncore))) { > if (!i915_modparams.mmio_debug) { > DRM_DEBUG("Unclaimed register detected, " > "enabling oneshot unclaimed register reporting. " > "Please use i915.mmio_debug=N for more information.\n"); > i915_modparams.mmio_debug++; > } > - uncore->unclaimed_mmio_check--; > + uncore->debug->unclaimed_mmio_check--; > ret = true; > } > > out: > - spin_unlock_irq(&uncore->lock); > + spin_unlock_irq(&uncore->debug->lock); > > return ret; > } > diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h > index e603d210a34d..414fc2cb0459 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.h > +++ b/drivers/gpu/drm/i915/intel_uncore.h > @@ -36,6 +36,13 @@ struct drm_i915_private; > struct intel_runtime_pm; > struct intel_uncore; > > +struct intel_uncore_mmio_debug { > + spinlock_t lock; /** lock is also taken in irq contexts. */ > + int unclaimed_mmio_check; > + int saved_mmio_check; > + u32 suspend_count; > +}; > + > enum forcewake_domain_id { > FW_DOMAIN_ID_RENDER = 0, > FW_DOMAIN_ID_BLITTER, > @@ -137,14 +144,9 @@ struct intel_uncore { > u32 __iomem *reg_ack; > } *fw_domain[FW_DOMAIN_ID_COUNT]; > > - struct { > - unsigned int count; > - > - int saved_mmio_check; > - int saved_mmio_debug; > - } user_forcewake; > + unsigned int user_forcewake_count; > > - int unclaimed_mmio_check; > + struct intel_uncore_mmio_debug *debug; > }; > > /* Iterate over initialised fw domains */ > @@ -179,6 +181,8 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore) > return uncore->flags & UNCORE_HAS_FIFO; > } > > +void > +intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug); > void intel_uncore_init_early(struct intel_uncore *uncore, > struct drm_i915_private *i915); > int intel_uncore_init_mmio(struct intel_uncore *uncore); > -- > 2.22.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 8/8/19 6:08 AM, Mika Kuoppala wrote: > Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> writes: > >> Multiple uncore structures will share the debug infrastructure, so >> move it to a common place and add extra locking around it. >> Also, since we now have a separate object, it is cleaner to have >> dedicated functions working on the object to stop and restart the >> mmio debug. Apart from the cosmetic changes, this patch introduces >> 2 functional updates: >> >> - All calls to check_for_unclaimed_mmio will now return false when >> the debug is suspended, not just the ones that are active only when >> i915_modparams.mmio_debug is set. If we don't trust the result of the >> check while a user is doing mmio access then we shouldn't attempt the >> check anywhere. >> >> - i915_modparams.mmio_debug is not save/restored anymore around user >> access. The value is now never touched by the kernel while debug is >> disabled so no need for save/restore. >> >> v2: squash mmio_debug patches, restrict mmio_debug lock usage (Chris) >> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 2 +- >> drivers/gpu/drm/i915/i915_drv.c | 3 +- >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/intel_uncore.c | 91 ++++++++++++++++++++--------- >> drivers/gpu/drm/i915/intel_uncore.h | 18 +++--- >> 5 files changed, 79 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >> index 3b15266c54fd..2310512111ab 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -1129,7 +1129,7 @@ static int i915_forcewake_domains(struct seq_file *m, void *data) >> unsigned int tmp; >> >> seq_printf(m, "user.bypass_count = %u\n", >> - uncore->user_forcewake.count); >> + uncore->user_forcewake_count); >> >> for_each_fw_domain(fw_domain, uncore, tmp) >> seq_printf(m, "%s.wake_count = %u\n", >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index 3480db36b63f..fbbff4a133ba 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -744,6 +744,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv) >> >> intel_device_info_subplatform_init(dev_priv); >> >> + intel_uncore_mmio_debug_init_early(&dev_priv->mmio_debug); >> intel_uncore_init_early(&dev_priv->uncore, dev_priv); >> >> spin_lock_init(&dev_priv->irq_lock); >> @@ -2044,7 +2045,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) >> >> out: >> enable_rpm_wakeref_asserts(rpm); >> - if (!dev_priv->uncore.user_forcewake.count) >> + if (!dev_priv->uncore.user_forcewake_count) >> intel_runtime_pm_driver_release(rpm); >> >> return ret; >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index c9476f24f5c1..13c27a75dca8 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1346,6 +1346,7 @@ struct drm_i915_private { >> resource_size_t stolen_usable_size; /* Total size minus reserved ranges */ >> >> struct intel_uncore uncore; >> + struct intel_uncore_mmio_debug mmio_debug; >> >> struct i915_virtual_gpu vgpu; >> >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c >> index 39e8710afdd9..9e583f13a9e4 100644 >> --- a/drivers/gpu/drm/i915/intel_uncore.c >> +++ b/drivers/gpu/drm/i915/intel_uncore.c >> @@ -34,6 +34,32 @@ >> >> #define __raw_posting_read(...) ((void)__raw_uncore_read32(__VA_ARGS__)) >> >> +void >> +intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug) >> +{ >> + spin_lock_init(&mmio_debug->lock); >> + mmio_debug->unclaimed_mmio_check = 1; >> +} >> + >> +static void mmio_debug_suspend(struct intel_uncore_mmio_debug *mmio_debug) >> +{ >> + lockdep_assert_held(&mmio_debug->lock); >> + >> + /* Save and disable mmio debugging for the user bypass */ >> + if (!mmio_debug->suspend_count++) { >> + mmio_debug->saved_mmio_check = mmio_debug->unclaimed_mmio_check; >> + mmio_debug->unclaimed_mmio_check = 0; >> + } >> +} >> + >> +static void mmio_debug_resume(struct intel_uncore_mmio_debug *mmio_debug) >> +{ >> + lockdep_assert_held(&mmio_debug->lock); >> + >> + if (!--mmio_debug->suspend_count) >> + mmio_debug->unclaimed_mmio_check = mmio_debug->saved_mmio_check; >> +} >> + >> static const char * const forcewake_domain_names[] = { >> "render", >> "blitter", >> @@ -476,6 +502,11 @@ check_for_unclaimed_mmio(struct intel_uncore *uncore) >> { >> bool ret = false; >> >> + lockdep_assert_held(&uncore->debug->lock); >> + >> + if (uncore->debug->suspend_count) >> + return false; >> + >> if (intel_uncore_has_fpga_dbg_unclaimed(uncore)) >> ret |= fpga_check_for_unclaimed_mmio(uncore); >> >> @@ -608,17 +639,11 @@ void intel_uncore_forcewake_get(struct intel_uncore *uncore, >> void intel_uncore_forcewake_user_get(struct intel_uncore *uncore) >> { >> spin_lock_irq(&uncore->lock); >> - if (!uncore->user_forcewake.count++) { >> + if (!uncore->user_forcewake_count++) { >> intel_uncore_forcewake_get__locked(uncore, FORCEWAKE_ALL); >> - >> - /* Save and disable mmio debugging for the user bypass */ >> - uncore->user_forcewake.saved_mmio_check = >> - uncore->unclaimed_mmio_check; >> - uncore->user_forcewake.saved_mmio_debug = >> - i915_modparams.mmio_debug; >> - >> - uncore->unclaimed_mmio_check = 0; >> - i915_modparams.mmio_debug = 0; >> + spin_lock(&uncore->debug->lock); > > For me it looks like you need spin_lock_irq here also > like with the parent lock above. The parent lock should ensure that the irqs are already off at this point, so no need to disable them twice. Also, spin_unlock_irq can't be called twice because the first of the 2 calls would re-enable interrupts while the second lock is still taken. Irqflags would solve that, but still wouldn't give any benefit compared to a straight lock IMO. Daniele > -Mika > >> + mmio_debug_suspend(uncore->debug); >> + spin_unlock(&uncore->debug->lock); >> } >> spin_unlock_irq(&uncore->lock); >> } >> @@ -633,15 +658,14 @@ void intel_uncore_forcewake_user_get(struct intel_uncore *uncore) >> void intel_uncore_forcewake_user_put(struct intel_uncore *uncore) >> { >> spin_lock_irq(&uncore->lock); >> - if (!--uncore->user_forcewake.count) { >> - if (intel_uncore_unclaimed_mmio(uncore)) >> + if (!--uncore->user_forcewake_count) { >> + spin_lock(&uncore->debug->lock); >> + mmio_debug_resume(uncore->debug); >> + >> + if (check_for_unclaimed_mmio(uncore)) >> dev_info(uncore->i915->drm.dev, >> "Invalid mmio detected during user access\n"); >> - >> - uncore->unclaimed_mmio_check = >> - uncore->user_forcewake.saved_mmio_check; >> - i915_modparams.mmio_debug = >> - uncore->user_forcewake.saved_mmio_debug; >> + spin_unlock(&uncore->debug->lock); >> >> intel_uncore_forcewake_put__locked(uncore, FORCEWAKE_ALL); >> } >> @@ -1088,7 +1112,16 @@ unclaimed_reg_debug(struct intel_uncore *uncore, >> if (likely(!i915_modparams.mmio_debug)) >> return; >> >> + /* interrupts are disabled and re-enabled around uncore->lock usage */ >> + lockdep_assert_held(&uncore->lock); >> + >> + if (before) >> + spin_lock(&uncore->debug->lock); >> + >> __unclaimed_reg_debug(uncore, reg, read, before); >> + >> + if (!before) >> + spin_unlock(&uncore->debug->lock); >> } >> >> #define GEN2_READ_HEADER(x) \ >> @@ -1607,6 +1640,7 @@ void intel_uncore_init_early(struct intel_uncore *uncore, >> spin_lock_init(&uncore->lock); >> uncore->i915 = i915; >> uncore->rpm = &i915->runtime_pm; >> + uncore->debug = &i915->mmio_debug; >> } >> >> static void uncore_raw_init(struct intel_uncore *uncore) >> @@ -1632,7 +1666,6 @@ static int uncore_forcewake_init(struct intel_uncore *uncore) >> ret = intel_uncore_fw_domains_init(uncore); >> if (ret) >> return ret; >> - >> forcewake_early_sanitize(uncore, 0); >> >> if (IS_GEN_RANGE(i915, 6, 7)) { >> @@ -1681,8 +1714,6 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) >> if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915)) >> uncore->flags |= UNCORE_HAS_FORCEWAKE; >> >> - uncore->unclaimed_mmio_check = 1; >> - >> if (!intel_uncore_has_forcewake(uncore)) { >> uncore_raw_init(uncore); >> } else { >> @@ -1707,7 +1738,7 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) >> uncore->flags |= UNCORE_HAS_FIFO; >> >> /* clear out unclaimed reg detection bit */ >> - if (check_for_unclaimed_mmio(uncore)) >> + if (intel_uncore_unclaimed_mmio(uncore)) >> DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n"); >> >> return 0; >> @@ -1952,7 +1983,13 @@ int __intel_wait_for_register(struct intel_uncore *uncore, >> >> bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore) >> { >> - return check_for_unclaimed_mmio(uncore); >> + bool ret; >> + >> + spin_lock_irq(&uncore->debug->lock); >> + ret = check_for_unclaimed_mmio(uncore); >> + spin_unlock_irq(&uncore->debug->lock); >> + >> + return ret; >> } >> >> bool >> @@ -1960,24 +1997,24 @@ intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore) >> { >> bool ret = false; >> >> - spin_lock_irq(&uncore->lock); >> + spin_lock_irq(&uncore->debug->lock); >> >> - if (unlikely(uncore->unclaimed_mmio_check <= 0)) >> + if (unlikely(uncore->debug->unclaimed_mmio_check <= 0)) >> goto out; >> >> - if (unlikely(intel_uncore_unclaimed_mmio(uncore))) { >> + if (unlikely(check_for_unclaimed_mmio(uncore))) { >> if (!i915_modparams.mmio_debug) { >> DRM_DEBUG("Unclaimed register detected, " >> "enabling oneshot unclaimed register reporting. " >> "Please use i915.mmio_debug=N for more information.\n"); >> i915_modparams.mmio_debug++; >> } >> - uncore->unclaimed_mmio_check--; >> + uncore->debug->unclaimed_mmio_check--; >> ret = true; >> } >> >> out: >> - spin_unlock_irq(&uncore->lock); >> + spin_unlock_irq(&uncore->debug->lock); >> >> return ret; >> } >> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h >> index e603d210a34d..414fc2cb0459 100644 >> --- a/drivers/gpu/drm/i915/intel_uncore.h >> +++ b/drivers/gpu/drm/i915/intel_uncore.h >> @@ -36,6 +36,13 @@ struct drm_i915_private; >> struct intel_runtime_pm; >> struct intel_uncore; >> >> +struct intel_uncore_mmio_debug { >> + spinlock_t lock; /** lock is also taken in irq contexts. */ >> + int unclaimed_mmio_check; >> + int saved_mmio_check; >> + u32 suspend_count; >> +}; >> + >> enum forcewake_domain_id { >> FW_DOMAIN_ID_RENDER = 0, >> FW_DOMAIN_ID_BLITTER, >> @@ -137,14 +144,9 @@ struct intel_uncore { >> u32 __iomem *reg_ack; >> } *fw_domain[FW_DOMAIN_ID_COUNT]; >> >> - struct { >> - unsigned int count; >> - >> - int saved_mmio_check; >> - int saved_mmio_debug; >> - } user_forcewake; >> + unsigned int user_forcewake_count; >> >> - int unclaimed_mmio_check; >> + struct intel_uncore_mmio_debug *debug; >> }; >> >> /* Iterate over initialised fw domains */ >> @@ -179,6 +181,8 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore) >> return uncore->flags & UNCORE_HAS_FIFO; >> } >> >> +void >> +intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug); >> void intel_uncore_init_early(struct intel_uncore *uncore, >> struct drm_i915_private *i915); >> int intel_uncore_init_mmio(struct intel_uncore *uncore); >> -- >> 2.22.0 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Daniele Ceraolo Spurio (2019-08-08 02:44:21) > Multiple uncore structures will share the debug infrastructure, so > move it to a common place and add extra locking around it. > Also, since we now have a separate object, it is cleaner to have > dedicated functions working on the object to stop and restart the > mmio debug. Apart from the cosmetic changes, this patch introduces > 2 functional updates: > > - All calls to check_for_unclaimed_mmio will now return false when > the debug is suspended, not just the ones that are active only when > i915_modparams.mmio_debug is set. If we don't trust the result of the > check while a user is doing mmio access then we shouldn't attempt the > check anywhere. > > - i915_modparams.mmio_debug is not save/restored anymore around user > access. The value is now never touched by the kernel while debug is > disabled so no need for save/restore. Yup. Early return from check_for_unclaimed_mmio() if suspended, with user_forcewake calling mmio_suspend. > v2: squash mmio_debug patches, restrict mmio_debug lock usage (Chris) > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 3b15266c54fd..2310512111ab 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1129,7 +1129,7 @@ static int i915_forcewake_domains(struct seq_file *m, void *data) unsigned int tmp; seq_printf(m, "user.bypass_count = %u\n", - uncore->user_forcewake.count); + uncore->user_forcewake_count); for_each_fw_domain(fw_domain, uncore, tmp) seq_printf(m, "%s.wake_count = %u\n", diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 3480db36b63f..fbbff4a133ba 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -744,6 +744,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv) intel_device_info_subplatform_init(dev_priv); + intel_uncore_mmio_debug_init_early(&dev_priv->mmio_debug); intel_uncore_init_early(&dev_priv->uncore, dev_priv); spin_lock_init(&dev_priv->irq_lock); @@ -2044,7 +2045,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) out: enable_rpm_wakeref_asserts(rpm); - if (!dev_priv->uncore.user_forcewake.count) + if (!dev_priv->uncore.user_forcewake_count) intel_runtime_pm_driver_release(rpm); return ret; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c9476f24f5c1..13c27a75dca8 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1346,6 +1346,7 @@ struct drm_i915_private { resource_size_t stolen_usable_size; /* Total size minus reserved ranges */ struct intel_uncore uncore; + struct intel_uncore_mmio_debug mmio_debug; struct i915_virtual_gpu vgpu; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 39e8710afdd9..9e583f13a9e4 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -34,6 +34,32 @@ #define __raw_posting_read(...) ((void)__raw_uncore_read32(__VA_ARGS__)) +void +intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug) +{ + spin_lock_init(&mmio_debug->lock); + mmio_debug->unclaimed_mmio_check = 1; +} + +static void mmio_debug_suspend(struct intel_uncore_mmio_debug *mmio_debug) +{ + lockdep_assert_held(&mmio_debug->lock); + + /* Save and disable mmio debugging for the user bypass */ + if (!mmio_debug->suspend_count++) { + mmio_debug->saved_mmio_check = mmio_debug->unclaimed_mmio_check; + mmio_debug->unclaimed_mmio_check = 0; + } +} + +static void mmio_debug_resume(struct intel_uncore_mmio_debug *mmio_debug) +{ + lockdep_assert_held(&mmio_debug->lock); + + if (!--mmio_debug->suspend_count) + mmio_debug->unclaimed_mmio_check = mmio_debug->saved_mmio_check; +} + static const char * const forcewake_domain_names[] = { "render", "blitter", @@ -476,6 +502,11 @@ check_for_unclaimed_mmio(struct intel_uncore *uncore) { bool ret = false; + lockdep_assert_held(&uncore->debug->lock); + + if (uncore->debug->suspend_count) + return false; + if (intel_uncore_has_fpga_dbg_unclaimed(uncore)) ret |= fpga_check_for_unclaimed_mmio(uncore); @@ -608,17 +639,11 @@ void intel_uncore_forcewake_get(struct intel_uncore *uncore, void intel_uncore_forcewake_user_get(struct intel_uncore *uncore) { spin_lock_irq(&uncore->lock); - if (!uncore->user_forcewake.count++) { + if (!uncore->user_forcewake_count++) { intel_uncore_forcewake_get__locked(uncore, FORCEWAKE_ALL); - - /* Save and disable mmio debugging for the user bypass */ - uncore->user_forcewake.saved_mmio_check = - uncore->unclaimed_mmio_check; - uncore->user_forcewake.saved_mmio_debug = - i915_modparams.mmio_debug; - - uncore->unclaimed_mmio_check = 0; - i915_modparams.mmio_debug = 0; + spin_lock(&uncore->debug->lock); + mmio_debug_suspend(uncore->debug); + spin_unlock(&uncore->debug->lock); } spin_unlock_irq(&uncore->lock); } @@ -633,15 +658,14 @@ void intel_uncore_forcewake_user_get(struct intel_uncore *uncore) void intel_uncore_forcewake_user_put(struct intel_uncore *uncore) { spin_lock_irq(&uncore->lock); - if (!--uncore->user_forcewake.count) { - if (intel_uncore_unclaimed_mmio(uncore)) + if (!--uncore->user_forcewake_count) { + spin_lock(&uncore->debug->lock); + mmio_debug_resume(uncore->debug); + + if (check_for_unclaimed_mmio(uncore)) dev_info(uncore->i915->drm.dev, "Invalid mmio detected during user access\n"); - - uncore->unclaimed_mmio_check = - uncore->user_forcewake.saved_mmio_check; - i915_modparams.mmio_debug = - uncore->user_forcewake.saved_mmio_debug; + spin_unlock(&uncore->debug->lock); intel_uncore_forcewake_put__locked(uncore, FORCEWAKE_ALL); } @@ -1088,7 +1112,16 @@ unclaimed_reg_debug(struct intel_uncore *uncore, if (likely(!i915_modparams.mmio_debug)) return; + /* interrupts are disabled and re-enabled around uncore->lock usage */ + lockdep_assert_held(&uncore->lock); + + if (before) + spin_lock(&uncore->debug->lock); + __unclaimed_reg_debug(uncore, reg, read, before); + + if (!before) + spin_unlock(&uncore->debug->lock); } #define GEN2_READ_HEADER(x) \ @@ -1607,6 +1640,7 @@ void intel_uncore_init_early(struct intel_uncore *uncore, spin_lock_init(&uncore->lock); uncore->i915 = i915; uncore->rpm = &i915->runtime_pm; + uncore->debug = &i915->mmio_debug; } static void uncore_raw_init(struct intel_uncore *uncore) @@ -1632,7 +1666,6 @@ static int uncore_forcewake_init(struct intel_uncore *uncore) ret = intel_uncore_fw_domains_init(uncore); if (ret) return ret; - forcewake_early_sanitize(uncore, 0); if (IS_GEN_RANGE(i915, 6, 7)) { @@ -1681,8 +1714,6 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915)) uncore->flags |= UNCORE_HAS_FORCEWAKE; - uncore->unclaimed_mmio_check = 1; - if (!intel_uncore_has_forcewake(uncore)) { uncore_raw_init(uncore); } else { @@ -1707,7 +1738,7 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) uncore->flags |= UNCORE_HAS_FIFO; /* clear out unclaimed reg detection bit */ - if (check_for_unclaimed_mmio(uncore)) + if (intel_uncore_unclaimed_mmio(uncore)) DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n"); return 0; @@ -1952,7 +1983,13 @@ int __intel_wait_for_register(struct intel_uncore *uncore, bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore) { - return check_for_unclaimed_mmio(uncore); + bool ret; + + spin_lock_irq(&uncore->debug->lock); + ret = check_for_unclaimed_mmio(uncore); + spin_unlock_irq(&uncore->debug->lock); + + return ret; } bool @@ -1960,24 +1997,24 @@ intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore) { bool ret = false; - spin_lock_irq(&uncore->lock); + spin_lock_irq(&uncore->debug->lock); - if (unlikely(uncore->unclaimed_mmio_check <= 0)) + if (unlikely(uncore->debug->unclaimed_mmio_check <= 0)) goto out; - if (unlikely(intel_uncore_unclaimed_mmio(uncore))) { + if (unlikely(check_for_unclaimed_mmio(uncore))) { if (!i915_modparams.mmio_debug) { DRM_DEBUG("Unclaimed register detected, " "enabling oneshot unclaimed register reporting. " "Please use i915.mmio_debug=N for more information.\n"); i915_modparams.mmio_debug++; } - uncore->unclaimed_mmio_check--; + uncore->debug->unclaimed_mmio_check--; ret = true; } out: - spin_unlock_irq(&uncore->lock); + spin_unlock_irq(&uncore->debug->lock); return ret; } diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h index e603d210a34d..414fc2cb0459 100644 --- a/drivers/gpu/drm/i915/intel_uncore.h +++ b/drivers/gpu/drm/i915/intel_uncore.h @@ -36,6 +36,13 @@ struct drm_i915_private; struct intel_runtime_pm; struct intel_uncore; +struct intel_uncore_mmio_debug { + spinlock_t lock; /** lock is also taken in irq contexts. */ + int unclaimed_mmio_check; + int saved_mmio_check; + u32 suspend_count; +}; + enum forcewake_domain_id { FW_DOMAIN_ID_RENDER = 0, FW_DOMAIN_ID_BLITTER, @@ -137,14 +144,9 @@ struct intel_uncore { u32 __iomem *reg_ack; } *fw_domain[FW_DOMAIN_ID_COUNT]; - struct { - unsigned int count; - - int saved_mmio_check; - int saved_mmio_debug; - } user_forcewake; + unsigned int user_forcewake_count; - int unclaimed_mmio_check; + struct intel_uncore_mmio_debug *debug; }; /* Iterate over initialised fw domains */ @@ -179,6 +181,8 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore) return uncore->flags & UNCORE_HAS_FIFO; } +void +intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug); void intel_uncore_init_early(struct intel_uncore *uncore, struct drm_i915_private *i915); int intel_uncore_init_mmio(struct intel_uncore *uncore);
Multiple uncore structures will share the debug infrastructure, so move it to a common place and add extra locking around it. Also, since we now have a separate object, it is cleaner to have dedicated functions working on the object to stop and restart the mmio debug. Apart from the cosmetic changes, this patch introduces 2 functional updates: - All calls to check_for_unclaimed_mmio will now return false when the debug is suspended, not just the ones that are active only when i915_modparams.mmio_debug is set. If we don't trust the result of the check while a user is doing mmio access then we shouldn't attempt the check anywhere. - i915_modparams.mmio_debug is not save/restored anymore around user access. The value is now never touched by the kernel while debug is disabled so no need for save/restore. v2: squash mmio_debug patches, restrict mmio_debug lock usage (Chris) Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_debugfs.c | 2 +- drivers/gpu/drm/i915/i915_drv.c | 3 +- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_uncore.c | 91 ++++++++++++++++++++--------- drivers/gpu/drm/i915/intel_uncore.h | 18 +++--- 5 files changed, 79 insertions(+), 36 deletions(-)