On 8/8/19 6:08 AM, Mika Kuoppala wrote:
Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> 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@xxxxxxxxx> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> --- 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.cindex 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
