drm/i915: split out uncore_mmio_debug

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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Link: https://patchwork.freedesktop.org/patch/msgid/20190809063116.7527-1-chris@chris-wilson.co.uk

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e4f173f2f3c30ba06972aa76ae4bb688c4be4aa1..3d9cd97e152671b5fb1ac08d03e5a87094682c54 100644 (file)
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1149,7 +1149,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 ca917583c41169df126e9e5943b4054c94068471..96bf70b48cf76646c049f790e5020a8fecfc7875 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -485,6 +485,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);
@@ -1785,7 +1786,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 71db63c583a9fd7c5e9133e02f01bf187f62c3e9..4397c6f4bb124eb09b3cb57b51c2813406f86fb2 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1369,6 +1369,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 39e8710afdd9a427b2d2d843572763a3e1a9099e..9e583f13a9e488c9e53df91af60eaf477f9796f9 100644 (file)
--- 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 e603d210a34d6d91b5322266210a8daafdd2b793..414fc2cb04597f09d8b891e381a3523081bc6836 100644 (file)
--- 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);