Re: [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 10-01-18 14:18, Hans de Goede wrote:
Hi,

On 10-01-18 13:55, Chris Wilson wrote:
While we talk to the punit over its sideband, we need to prevent the cpu
from sleeping in order to prevent a potential machine hang.

Note that by itself, it appears that pm_qos_update_request (via
intel_idle) doesn't provide a sufficient barrier to ensure that all core
are indeed awake (out of Cstate) and that the package is awake. To do so,
we need to supplement the pm_qos with a manual ping on_each_cpu.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109051
References: https://bugs.freedesktop.org/show_bug.cgi?id=102657
References: https://bugzilla.kernel.org/show_bug.cgi?id=195255
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
Cc: Hans de Goede <hdegoede@xxxxxxxxxx>

Interesting, I've added similar pm_qos code in
drivers/i2c/busses/i2c-designware-baytrail.c quite a while ago because
the CPU transitioning to higher C-state while accessing the i2c bus to
the pmic (if it is shared) also causes the SoC to hang.

I could reproduce this quite easily by doing "i2cdump" on the pmic,
usually the system would hang in one or 2 i2cdump calls.

Note IIRC this was on CHT.

I see that you also block any pmic-i2c bus accesses while doing
punit access by calling iosf_mbi_punit_acquire();

Maybe we need to move the pm_qos stuff out of
drivers/i2c/busses/i2c-designware-baytrail.c

And into iosf_mbi_punit_acquire? The i2c-designware-baytrail.c
does its own pm_qos dance directly after calling
iosf_mbi_punit_acquire / before calling iosf_mbi_punit_release();

I just remembered that the i915 code also call iosf_mbi_punit_acquire()
from drivers/gpu/drm/i915/intel_uncore.c and does it for non VLV/CHT
boards there too. Currently that is not an issue because
iosf_mbi_punit_acquire() currently is a nop for non VLV/CHT, but if
we move the pm_qos stuff there then it no longer is a nop, so it is
probably best to keep separate pm_qos code in intel_sideband.c and
i2c-designware-baytrail.c, iow move ahead with this patch as as,
except for maybe moving the ping stuff to the intel_idle.c code.

Regards,

Hans



---
  drivers/gpu/drm/i915/i915_drv.c       |  6 ++++
  drivers/gpu/drm/i915/i915_drv.h       |  1 +
  drivers/gpu/drm/i915/intel_sideband.c | 61 ++++++++++++++++++++++++-----------
  3 files changed, 50 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6c8da9d20c33..d4b90cc0130b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -902,6 +902,9 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
      spin_lock_init(&dev_priv->uncore.lock);
      mutex_init(&dev_priv->sb_lock);
+    pm_qos_add_request(&dev_priv->sb_qos,
+               PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
+
      mutex_init(&dev_priv->modeset_restore_lock);
      mutex_init(&dev_priv->av_mutex);
      mutex_init(&dev_priv->wm.wm_mutex);
@@ -953,6 +956,9 @@ static void i915_driver_cleanup_early(struct drm_i915_private *dev_priv)
      intel_irq_fini(dev_priv);
      i915_workqueues_cleanup(dev_priv);
      i915_engines_cleanup(dev_priv);
+
+    pm_qos_remove_request(&dev_priv->sb_qos);
+    mutex_destroy(&dev_priv->sb_lock);
  }
  static int i915_mmio_setup(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a689396d0ff6..ff3f9effc0bb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1887,6 +1887,7 @@ struct drm_i915_private {
      /* Sideband mailbox protection */
      struct mutex sb_lock;
+    struct pm_qos_request sb_qos;
      /** Cached value of IMR to avoid reads in updating the bitfield */
      union {
diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
index 75c872bb8cc9..02bdd2e2cef6 100644
--- a/drivers/gpu/drm/i915/intel_sideband.c
+++ b/drivers/gpu/drm/i915/intel_sideband.c
@@ -22,6 +22,8 @@
   *
   */
+#include <asm/iosf_mbi.h>
+
  #include "i915_drv.h"
  #include "intel_drv.h"
@@ -39,18 +41,20 @@
  /* Private register write, double-word addressing, non-posted */
  #define SB_CRWRDA_NP    0x07
-static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
-               u32 port, u32 opcode, u32 addr, u32 *val)
+static void ping(void *info)
  {
-    u32 cmd, be = 0xf, bar = 0;
-    bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
+}
-    cmd = (devfn << IOSF_DEVFN_SHIFT) | (opcode << IOSF_OPCODE_SHIFT) |
-        (port << IOSF_PORT_SHIFT) | (be << IOSF_BYTE_ENABLES_SHIFT) |
-        (bar << IOSF_BAR_SHIFT);
+static int vlv_sideband_rw(struct drm_i915_private *dev_priv,
+               u32 devfn, u32 port, u32 opcode,
+               u32 addr, u32 *val)
+{
+    const bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
+    int err;
-    WARN_ON(!mutex_is_locked(&dev_priv->sb_lock));
+    lockdep_assert_held(&dev_priv->sb_lock);
+    /* Flush the previous comms, just in case it failed last time. */
      if (intel_wait_for_register(dev_priv,
                      VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
                      5)) {
@@ -59,22 +63,43 @@ static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
          return -EAGAIN;
      }
-    I915_WRITE(VLV_IOSF_ADDR, addr);
-    I915_WRITE(VLV_IOSF_DATA, is_read ? 0 : *val);
-    I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
+    iosf_mbi_punit_acquire();
-    if (intel_wait_for_register(dev_priv,
-                    VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
-                    5)) {
+    /*
+     * Prevent the cpu from sleeping while we use this sideband, otherwise
+     * the punit may cause a machine hang.
+     */
+    pm_qos_update_request(&dev_priv->sb_qos, 0);
+    on_each_cpu(ping, NULL, 1);
+    preempt_disable();
+
+    I915_WRITE_FW(VLV_IOSF_ADDR, addr);
+    I915_WRITE_FW(VLV_IOSF_DATA, is_read ? 0 : *val);
+    I915_WRITE_FW(VLV_IOSF_DOORBELL_REQ,
+              (devfn << IOSF_DEVFN_SHIFT) |
+              (opcode << IOSF_OPCODE_SHIFT) |
+              (port << IOSF_PORT_SHIFT) |
+              (0xf << IOSF_BYTE_ENABLES_SHIFT) |
+              (0 << IOSF_BAR_SHIFT) |
+              IOSF_SB_BUSY);
+
+    if (__intel_wait_for_register_fw(dev_priv,
+                     VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
+                     10000, 0, NULL) == 0) {
+        if (is_read)
+            *val = I915_READ_FW(VLV_IOSF_DATA);
+        err = 0;
+    } else {
          DRM_DEBUG_DRIVER("IOSF sideband finish wait (%s) timed out\n",
                   is_read ? "read" : "write");
-        return -ETIMEDOUT;
+        err = -ETIMEDOUT;
      }
-    if (is_read)
-        *val = I915_READ(VLV_IOSF_DATA);
+    preempt_enable();
+    pm_qos_update_request(&dev_priv->sb_qos, PM_QOS_DEFAULT_VALUE);
+    iosf_mbi_punit_release();
-    return 0;
+    return err;
  }
  u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux