drm/msm: what's in store for 3.15

msm.hang_debug=y will dump out current register values if the gpu locks up, for easier debugging.

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/a3xx_gpu.c   | 34 ++++++++++++++++++++++++++++++++-
  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 18 +++++++++++++++++
  drivers/gpu/drm/msm/adreno/adreno_gpu.h |  1 +
  3 files changed, 52 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/adreno/a3xx_gpu.c +6 -0

index 461df93..8b6fb84 100644
@@ -35,6 +35,12 @@
  	 A3XX_INT0_CP_AHB_ERROR_HALT |     \
  	 A3XX_INT0_UCHE_OOB_ACCESS)
 
+
+static bool hang_debug = false;
+MODULE_PARM_DESC(hang_debug, "Dump registers when hang is detected (can be slow!)");
+module_param_named(hang_debug, hang_debug, bool, 0600);
+static void a3xx_dump(struct msm_gpu *gpu);
+
  static struct platform_device *a3xx_pdev;
 
  static void a3xx_me_init(struct msm_gpu *gpu)

drivers/gpu/drm/msm/adreno/a3xx_gpu.c +24 -1

  	gpu_write(gpu, REG_A3XX_RBBM_SW_RESET_CMD, 1);
  	gpu_read(gpu, REG_A3XX_RBBM_SW_RESET_CMD);
  	gpu_write(gpu, REG_A3XX_RBBM_SW_RESET_CMD, 0);

@@ -352,7 +361,6 @@ static irqreturn_t a3xx_irq(struct msm_gpu *gpu)
  	return IRQ_HANDLED;
  }
 
-#ifdef CONFIG_DEBUG_FS
  static const unsigned int a3xx_registers[] = {
  	0x0000, 0x0002, 0x0010, 0x0012, 0x0018, 0x0018, 0x0020, 0x0027,
  	0x0029, 0x002b, 0x002e, 0x0033, 0x0040, 0x0042, 0x0050, 0x005c,

@@ -392,6 +400,7 @@ static const unsigned int a3xx_registers[] = {
  	0x303c, 0x303c, 0x305e, 0x305f,
  };
 
+#ifdef CONFIG_DEBUG_FS
  static void a3xx_show(struct msm_gpu *gpu, struct seq_file *m)
  {
  	int i;

@@ -415,6 +424,29 @@ static void a3xx_show(struct msm_gpu *gpu, struct seq_file *m)
  }
  #endif
 
+/* would be nice to not have to duplicate the _show() stuff with printk(): */
+static void a3xx_dump(struct msm_gpu *gpu)
+{
+	int i;
+
+	adreno_dump(gpu);
+	printk("status:   %08x\n",
+			gpu_read(gpu, REG_A3XX_RBBM_STATUS));
+
+	/* dump these out in a form that can be parsed by demsm: */
+	printk("IO:region %s 00000000 00020000\n", gpu->name);
+	for (i = 0; i < ARRAY_SIZE(a3xx_registers); i += 2) {
+		uint32_t start = a3xx_registers[i];
+		uint32_t end   = a3xx_registers[i+1];
+		uint32_t addr;
+
+		for (addr = start; addr <= end; addr++) {
+			uint32_t val = gpu_read(gpu, addr);
+			printk("IO:R %08x %08x\n", addr<<2, val);
+		}
+	}
+}
+
  static const struct adreno_gpu_funcs funcs = {
  	.base = {
  		.get_param = adreno_get_param,

drivers/gpu/drm/msm/adreno/adreno_gpu.c +8 -0

index d321099..cf6eb97 100644
@@ -260,6 +260,24 @@ void adreno_show(struct msm_gpu *gpu, struct seq_file *m)
  }
  #endif
 
+/* would be nice to not have to duplicate the _show() stuff with printk(): */
+void adreno_dump(struct msm_gpu *gpu)
+{
+	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+
+	printk("revision: %d (%d.%d.%d.%d)\n",
+			adreno_gpu->info->revn, adreno_gpu->rev.core,
+			adreno_gpu->rev.major, adreno_gpu->rev.minor,

Quoted 32 lines · Rob Clark

msm.hang_debug=y will dump out current register values if the gpu locks up, for easier debugging.

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/a3xx_gpu.c   | 34
++++++++++++++++++++++++++++++++-
  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 18 +++++++++++++++++
  drivers/gpu/drm/msm/adreno/adreno_gpu.h |  1 +
  3 files changed, 52 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/adreno/a3xx_gpu.c +6 -0

index 461df93..8b6fb84 100644
@@ -35,6 +35,12 @@
         A3XX_INT0_CP_AHB_ERROR_HALT |     \
         A3XX_INT0_UCHE_OOB_ACCESS)
 
+
+static bool hang_debug = false;
+MODULE_PARM_DESC(hang_debug, "Dump registers when hang is detected (can
 be slow!)");
+module_param_named(hang_debug, hang_debug, bool, 0600);
+static void a3xx_dump(struct msm_gpu *gpu);
+
  static struct platform_device *a3xx_pdev;
 

static void a3xx_me_init(struct msm_gpu *gpu)

drivers/gpu/drm/msm/adreno/a3xx_gpu.c static int a3xx_hw_init(struct msm_gpu *gpu) · line 302

Quoted 6 lines

drivers/gpu/drm/msm/adreno/a3xx_gpu.c +3 -0

@@ -291,6 +297,9 @@ static int a3xx_hw_init(struct msm_gpu *gpu)
 
  static void a3xx_recover(struct msm_gpu *gpu)
  {
+       /* dump registers before resetting gpu, if enabled: */
+       if (hang_debug)
+               a3xx_dump(gpu);

You will need to be careful here - not all the registers are suitable for reading back.
On some flavors of A330 in particular some HLSQ registers reads will bring the system
down. You can see the gory details in the KGSL driver. If this works for you for now
great but fair warning that some cores might have different experiences.

From what I've seen, certain types of GPU lockup (for example shader

Quoted 78 lines

drivers/gpu/drm/msm/adreno/a3xx_gpu.c +2 -1

        gpu_write(gpu, REG_A3XX_RBBM_SW_RESET_CMD, 1);
        gpu_read(gpu, REG_A3XX_RBBM_SW_RESET_CMD);
        gpu_write(gpu, REG_A3XX_RBBM_SW_RESET_CMD, 0);

@@ -352,7 +361,6 @@ static irqreturn_t a3xx_irq(struct msm_gpu *gpu)
        return IRQ_HANDLED;
  }
 
-#ifdef CONFIG_DEBUG_FS
  static const unsigned int a3xx_registers[] = {
        0x0000, 0x0002, 0x0010, 0x0012, 0x0018, 0x0018, 0x0020, 0x0027,
        0x0029, 0x002b, 0x002e, 0x0033, 0x0040, 0x0042, 0x0050, 0x005c,

@@ -392,6 +400,7 @@ static const unsigned int a3xx_registers[] = {
        0x303c, 0x303c, 0x305e, 0x305f,
  };
 
+#ifdef CONFIG_DEBUG_FS
  static void a3xx_show(struct msm_gpu *gpu, struct seq_file *m)
  {
        int i;

@@ -415,6 +424,29 @@ static void a3xx_show(struct msm_gpu *gpu, struct
 seq_file *m)
  }
  #endif
 
+/* would be nice to not have to duplicate the _show() stuff with

printk(): */

drivers/gpu/drm/msm/adreno/a3xx_gpu.c +22 -0

+static void a3xx_dump(struct msm_gpu *gpu)
+{
+       int i;
+
+       adreno_dump(gpu);
+       printk("status:   %08x\n",
+                       gpu_read(gpu, REG_A3XX_RBBM_STATUS));
+
+       /* dump these out in a form that can be parsed by demsm: */
+       printk("IO:region %s 00000000 00020000\n", gpu->name);
+       for (i = 0; i < ARRAY_SIZE(a3xx_registers); i += 2) {
+               uint32_t start = a3xx_registers[i];
+               uint32_t end   = a3xx_registers[i+1];
+               uint32_t addr;
+
+               for (addr = start; addr <= end; addr++) {
+                       uint32_t val = gpu_read(gpu, addr);
+                       printk("IO:R %08x %08x\n", addr<<2, val);
+               }
+       }
+}
+
  static const struct adreno_gpu_funcs funcs = {
        .base = {
                .get_param = adreno_get_param,

drivers/gpu/drm/msm/adreno/adreno_gpu.c +1 -0

index d321099..cf6eb97 100644
@@ -260,6 +260,24 @@ void adreno_show(struct msm_gpu *gpu, struct seq_file
 *m)
  }
  #endif
 
+/* would be nice to not have to duplicate the _show() stuff with

printk(): */

drivers/gpu/drm/msm/adreno/adreno_gpu.c +14 -0

+void adreno_dump(struct msm_gpu *gpu)
+{
+       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+
+       printk("revision: %d (%d.%d.%d.%d)\n",
+                       adreno_gpu->info->revn, adreno_gpu->rev.core,
+                       adreno_gpu->rev.major, adreno_gpu->rev.minor,
+                       adreno_gpu->rev.patchid);
+
+       printk("fence:    %d/%d\n", adreno_gpu->memptrs->fence,
+                       gpu->submitted_fence);
+       printk("rptr:     %d\n", adreno_gpu->memptrs->rptr);
+       printk("wptr:     %d\n", adreno_gpu->memptrs->wptr);
+       printk("rb wptr:  %d\n", get_wptr(gpu->rb));

drivers/gpu/drm/msm/adreno/adreno_gpu.c

Quoted 3 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.c +3 -0

+
+}
+

We should introduce you to the gooey wonderfulness of the snapshot :) All in
due time.

Jordan
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Helper macro to simplify places where we need to poll with timeout waiting for gpu.

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/a3xx_gpu.c   | 14 +++--------
  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 41 ++++++++++++---------------------
  drivers/gpu/drm/msm/adreno/adreno_gpu.h | 15 +++++++++++-
  3 files changed, 32 insertions(+), 38 deletions(-)

drivers/gpu/drm/msm/adreno/a3xx_gpu.c +3 -11

index 8b6fb84..59ed762 100644
@@ -326,21 +326,13 @@ static void a3xx_destroy(struct msm_gpu *gpu)
 
  static void a3xx_idle(struct msm_gpu *gpu)
  {
-	unsigned long t;
-
  	/* wait for ringbuffer to drain: */
  	adreno_idle(gpu);
 
-	t = jiffies + ADRENO_IDLE_TIMEOUT;
-
  	/* then wait for GPU to finish: */
-	do {
-		uint32_t rbbm_status = gpu_read(gpu, REG_A3XX_RBBM_STATUS);
-		if (!(rbbm_status & A3XX_RBBM_STATUS_GPU_BUSY))
-			return;
-	} while(time_before(jiffies, t));
-
-	DRM_ERROR("timeout waiting for %s to idle!\n", gpu->name);
+	if (spin_until(!(gpu_read(gpu, REG_A3XX_RBBM_STATUS) &
+			A3XX_RBBM_STATUS_GPU_BUSY)))
+		DRM_ERROR("%s: timeout waiting for GPU to idle!\n", gpu->name);
 
  	/* TODO maybe we need to reset GPU here to recover from hang? */
  }

drivers/gpu/drm/msm/adreno/adreno_gpu.c +15 -26

index cf6eb97..7a11563 100644
@@ -225,19 +225,11 @@ void adreno_flush(struct msm_gpu *gpu)
  void adreno_idle(struct msm_gpu *gpu)
  {
  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
-	uint32_t rptr, wptr = get_wptr(gpu->rb);
-	unsigned long t;
-
-	t = jiffies + ADRENO_IDLE_TIMEOUT;
-
-	/* then wait for CP to drain ringbuffer: */
-	do {
-		rptr = adreno_gpu->memptrs->rptr;
-		if (rptr == wptr)
-			return;
-	} while(time_before(jiffies, t));
+	uint32_t wptr = get_wptr(gpu->rb);
 
-	DRM_ERROR("%s: timeout waiting to drain ringbuffer!\n", gpu->name);
+	/* wait for CP to drain ringbuffer: */
+	if (spin_until(adreno_gpu->memptrs->rptr == wptr))
+		DRM_ERROR("%s: timeout waiting to drain ringbuffer!\n", gpu->name);
 
  	/* TODO maybe we need to reset GPU here to recover from hang? */
  }

@@ -278,22 +270,19 @@ void adreno_dump(struct msm_gpu *gpu)
 
  }
 
-void adreno_wait_ring(struct msm_gpu *gpu, uint32_t ndwords)
+static uint32_t ring_freewords(struct msm_gpu *gpu)
  {
  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
-	uint32_t freedwords;
-	unsigned long t = jiffies + ADRENO_IDLE_TIMEOUT;
-	do {
-		uint32_t size = gpu->rb->size / 4;
-		uint32_t wptr = get_wptr(gpu->rb);
-		uint32_t rptr = adreno_gpu->memptrs->rptr;
-		freedwords = (rptr + (size - 1) - wptr) % size;
-
-		if (time_after(jiffies, t)) {
-			DRM_ERROR("%s: timeout waiting for ringbuffer space\n", gpu->name);
-			break;
-		}
-	} while(freedwords < ndwords);
+	uint32_t size = gpu->rb->size / 4;
+	uint32_t wptr = get_wptr(gpu->rb);
+	uint32_t rptr = adreno_gpu->memptrs->rptr;
+	return (rptr + (size - 1) - wptr) % size;
+}
+
+void adreno_wait_ring(struct msm_gpu *gpu, uint32_t ndwords)
+{
+	if (spin_until(ring_freewords(gpu) >= ndwords))
+		DRM_ERROR("%s: timeout waiting for ringbuffer space\n", gpu->name);
  }
 
  static const char *iommu_ports[] = {

drivers/gpu/drm/msm/adreno/adreno_gpu.h +14 -1

index e16200d..63c36ce 100644
@@ -76,7 +76,20 @@ struct adreno_platform_config {
  #endif
  };
 
-#define ADRENO_IDLE_TIMEOUT (20 * 1000)
+#define ADRENO_IDLE_TIMEOUT msecs_to_jiffies(1000)
+
+#define spin_until(X) ({                                   \
+	int __ret = -ETIMEDOUT;                            \
+	unsigned long __t = jiffies + ADRENO_IDLE_TIMEOUT; \
+	do {                                               \
+		if (X) {                                   \
+			__ret = 0;                         \
+			break;                             \
+		}                                          \
+	} while (time_before(jiffies, __t));               \
+	__ret;                                             \
+})
+
 
  static inline bool adreno_is_a3xx(struct adreno_gpu *gpu)
  {

Shut down the clks when the gpu has nothing to do. A short inactivity timer is used to provide a low pass filter for power transitions.

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 10 +++++
  drivers/gpu/drm/msm/msm_drv.c         |  7 ++-
  drivers/gpu/drm/msm/msm_gpu.c         | 85 +++++++++++++++++++++++++++++++++--
  drivers/gpu/drm/msm/msm_gpu.h         | 16 ++++++-
  4 files changed, 113 insertions(+), 5 deletions(-)

drivers/gpu/drm/msm/adreno/a3xx_gpu.c +10 -0

index 59ed762..e6cb2bc 100644
@@ -395,9 +395,15 @@ static const unsigned int a3xx_registers[] = {
  #ifdef CONFIG_DEBUG_FS
  static void a3xx_show(struct msm_gpu *gpu, struct seq_file *m)
  {
+	struct drm_device *dev = gpu->dev;
  	int i;
 
  	adreno_show(gpu, m);
+
+	mutex_lock(&dev->struct_mutex);
+
+	gpu->funcs->pm_resume(gpu);
+
  	seq_printf(m, "status:   %08x\n",
  			gpu_read(gpu, REG_A3XX_RBBM_STATUS));
 

@@ -413,6 +419,10 @@ static void a3xx_show(struct msm_gpu *gpu, struct seq_file *m)
  			seq_printf(m, "IO:R %08x %08x\n", addr<<2, val);
  		}
  	}
+
+	gpu->funcs->pm_suspend(gpu);
+
+	mutex_unlock(&dev->struct_mutex);
  }
  #endif
 

drivers/gpu/drm/msm/msm_drv.c +6 -1

index e6adafc..e913efa 100644
@@ -311,7 +311,6 @@ static void load_gpu(struct drm_device *dev)
  		gpu = NULL;
  		/* not fatal */
  	}
-	mutex_unlock(&dev->struct_mutex);
 
  	if (gpu) {
  		int ret;

@@ -321,10 +320,16 @@ static void load_gpu(struct drm_device *dev)
  			dev_err(dev->dev, "gpu hw init failed: %d\n", ret);
  			gpu->funcs->destroy(gpu);
  			gpu = NULL;
+		} else {
+			/* give inactive pm a chance to kick in: */
+			msm_gpu_retire(gpu);
  		}
+
  	}
 
  	priv->gpu = gpu;
+
+	mutex_unlock(&dev->struct_mutex);
  }
 
  static int msm_open(struct drm_device *dev, struct drm_file *file)

drivers/gpu/drm/msm/msm_gpu.c +82 -3

index 0cfe3f4..3e667ca 100644
@@ -154,9 +154,18 @@ static int disable_axi(struct msm_gpu *gpu)
 
  int msm_gpu_pm_resume(struct msm_gpu *gpu)
  {
+	struct drm_device *dev = gpu->dev;
  	int ret;
 
-	DBG("%s", gpu->name);
+	DBG("%s: active_cnt=%d", gpu->name, gpu->active_cnt);
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	if (gpu->active_cnt++ > 0)
+		return 0;
+
+	if (WARN_ON(gpu->active_cnt <= 0))
+		return -EINVAL;
 
  	ret = enable_pwrrail(gpu);
  	if (ret)

@@ -175,9 +184,18 @@ int msm_gpu_pm_resume(struct msm_gpu *gpu)
 
  int msm_gpu_pm_suspend(struct msm_gpu *gpu)
  {
+	struct drm_device *dev = gpu->dev;
  	int ret;
 
-	DBG("%s", gpu->name);
+	DBG("%s: active_cnt=%d", gpu->name, gpu->active_cnt);
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	if (--gpu->active_cnt > 0)
+		return 0;
+
+	if (WARN_ON(gpu->active_cnt < 0))
+		return -EINVAL;
 
  	ret = disable_axi(gpu);
  	if (ret)

@@ -195,6 +213,55 @@ int msm_gpu_pm_suspend(struct msm_gpu *gpu)
  }
 
  /*
+ * Inactivity detection (for suspend):
+ */
+
+static void inactive_worker(struct work_struct *work)
+{
+	struct msm_gpu *gpu = container_of(work, struct msm_gpu, inactive_work);
+	struct drm_device *dev = gpu->dev;
+
+	if (gpu->inactive)
+		return;
+
+	DBG("%s: inactive!\n", gpu->name);
+	mutex_lock(&dev->struct_mutex);
+	if (!(msm_gpu_active(gpu) || gpu->inactive)) {
+		disable_axi(gpu);
+		disable_clk(gpu);
+		gpu->inactive = true;
+	}
+	mutex_unlock(&dev->struct_mutex);
+}
+
+static void inactive_handler(unsigned long data)
+{
+	struct msm_gpu *gpu = (struct msm_gpu *)data;
+	struct msm_drm_private *priv = gpu->dev->dev_private;
+
+	queue_work(priv->wq, &gpu->inactive_work);
+}
+
+/* cancel inactive timer and make sure we are awake: */
+static void inactive_cancel(struct msm_gpu *gpu)
+{
+	DBG("%s", gpu->name);
+	del_timer(&gpu->inactive_timer);
+	if (gpu->inactive) {
+		enable_clk(gpu);
+		enable_axi(gpu);
+		gpu->inactive = false;
+	}
+}
+
+static void inactive_start(struct msm_gpu *gpu)
+{
+	DBG("%s", gpu->name);
+	mod_timer(&gpu->inactive_timer,
+			round_jiffies_up(jiffies + DRM_MSM_INACTIVE_JIFFIES));
+}
+
+/*
   * Hangcheck detection for locked gpu:
   */
 

@@ -206,7 +273,10 @@ static void recover_worker(struct work_struct *work)
  	dev_err(dev->dev, "%s: hangcheck recover!\n", gpu->name);
 
  	mutex_lock(&dev->struct_mutex);
-	gpu->funcs->recover(gpu);
+	if (msm_gpu_active(gpu)) {
+		inactive_cancel(gpu);
+		gpu->funcs->recover(gpu);
+	}
  	mutex_unlock(&dev->struct_mutex);
 
  	msm_gpu_retire(gpu);

@@ -281,6 +351,9 @@ static void retire_worker(struct work_struct *work)
  	}
 
  	mutex_unlock(&dev->struct_mutex);
+
+	if (!msm_gpu_active(gpu))
+		inactive_start(gpu);
  }
 
  /* call from irq handler to schedule work to retire bo's */

@@ -302,6 +375,8 @@ int msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit,
 
  	gpu->submitted_fence = submit->fence;
 
+	inactive_cancel(gpu);
+
  	ret = gpu->funcs->submit(gpu, submit, ctx);
  	priv->lastctx = ctx;
 

@@ -357,11 +432,15 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
  	gpu->dev = drm;
  	gpu->funcs = funcs;
  	gpu->name = name;
+	gpu->inactive = true;
 
  	INIT_LIST_HEAD(&gpu->active_list);
  	INIT_WORK(&gpu->retire_work, retire_worker);
+	INIT_WORK(&gpu->inactive_work, inactive_worker);
  	INIT_WORK(&gpu->recover_work, recover_worker);
 
+	setup_timer(&gpu->inactive_timer, inactive_handler,
+			(unsigned long)gpu);
  	setup_timer(&gpu->hangcheck_timer, hangcheck_handler,
  			(unsigned long)gpu);
 

drivers/gpu/drm/msm/msm_gpu.h +15 -1

index 458db8c..fad2700 100644
@@ -72,6 +72,10 @@ struct msm_gpu {
 
  	uint32_t submitted_fence;
 
+	/* is gpu powered/active? */
+	int active_cnt;
+	bool inactive;
+
  	/* worker for handling active-list retiring: */
  	struct work_struct retire_work;
 

@@ -91,7 +95,12 @@ struct msm_gpu {
  	uint32_t bsc;
  #endif
 
-	/* Hang Detction: */
+	/* Hang and Inactivity Detection:
+	 */
+#define DRM_MSM_INACTIVE_PERIOD   66 /* in ms (roughly four frames) */
+#define DRM_MSM_INACTIVE_JIFFIES  msecs_to_jiffies(DRM_MSM_INACTIVE_PERIOD)
+	struct timer_list inactive_timer;
+	struct work_struct inactive_work;
  #define DRM_MSM_HANGCHECK_PERIOD 500 /* in ms */
  #define DRM_MSM_HANGCHECK_JIFFIES msecs_to_jiffies(DRM_MSM_HANGCHECK_PERIOD)
  	struct timer_list hangcheck_timer;

@@ -99,6 +108,11 @@ struct msm_gpu {
  	struct work_struct recover_work;
  };
 
+static inline bool msm_gpu_active(struct msm_gpu *gpu)
+{
+	return gpu->submitted_fence > gpu->funcs->last_fence(gpu);
+}
+
  static inline void gpu_write(struct msm_gpu *gpu, u32 reg, u32 data)
  {
  	msm_writel(data, gpu->mmio + (reg << 2));

Quoted 3 lines · Rob Clark

Shut down the clks when the gpu has nothing to do. A short inactivity timer is used to provide a low pass filter for power transitions.

Good luck. Power management will take years off your life.

Quoted 2 lines

Signed-off-by Rob Clark <[email protected]>

Acked-by Jordan Crouse <[email protected]>

Quoted 256 lines

---
  drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 10 +++++
  drivers/gpu/drm/msm/msm_drv.c         |  7 ++-
  drivers/gpu/drm/msm/msm_gpu.c         | 85
+++++++++++++++++++++++++++++++++--
  drivers/gpu/drm/msm/msm_gpu.h         | 16 ++++++-
  4 files changed, 113 insertions(+), 5 deletions(-)

drivers/gpu/drm/msm/adreno/a3xx_gpu.c +10 -0

index 59ed762..e6cb2bc 100644
@@ -395,9 +395,15 @@ static const unsigned int a3xx_registers[] = {
  #ifdef CONFIG_DEBUG_FS
  static void a3xx_show(struct msm_gpu *gpu, struct seq_file *m)
  {
+       struct drm_device *dev = gpu->dev;
        int i;
 
        adreno_show(gpu, m);
+
+       mutex_lock(&dev->struct_mutex);
+
+       gpu->funcs->pm_resume(gpu);
+
        seq_printf(m, "status:   %08x\n",
                        gpu_read(gpu, REG_A3XX_RBBM_STATUS));
 

@@ -413,6 +419,10 @@ static void a3xx_show(struct msm_gpu *gpu, struct
 seq_file *m)
                        seq_printf(m, "IO:R %08x %08x\n", addr<<2, val);
                }
        }
+
+       gpu->funcs->pm_suspend(gpu);
+
+       mutex_unlock(&dev->struct_mutex);
  }
  #endif

drivers/gpu/drm/msm/msm_drv.c +6 -1

index e6adafc..e913efa 100644
@@ -311,7 +311,6 @@ static void load_gpu(struct drm_device *dev)
                gpu = NULL;
                /* not fatal */
        }
-       mutex_unlock(&dev->struct_mutex);
 
        if (gpu) {
                int ret;

@@ -321,10 +320,16 @@ static void load_gpu(struct drm_device *dev)
                        dev_err(dev->dev, "gpu hw init failed: %d\n",
 ret);
                        gpu->funcs->destroy(gpu);
                        gpu = NULL;
+               } else {
+                       /* give inactive pm a chance to kick in: */
+                       msm_gpu_retire(gpu);
                }
+
        }
 
        priv->gpu = gpu;
+
+       mutex_unlock(&dev->struct_mutex);
  }
 

static int msm_open(struct drm_device *dev, struct drm_file *file)

drivers/gpu/drm/msm/msm_gpu.c +69 -2

index 0cfe3f4..3e667ca 100644
@@ -154,9 +154,18 @@ static int disable_axi(struct msm_gpu *gpu)
 
  int msm_gpu_pm_resume(struct msm_gpu *gpu)
  {
+       struct drm_device *dev = gpu->dev;
        int ret;
 
-       DBG("%s", gpu->name);
+       DBG("%s: active_cnt=%d", gpu->name, gpu->active_cnt);
+
+       WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+       if (gpu->active_cnt++ > 0)
+               return 0;
+
+       if (WARN_ON(gpu->active_cnt <= 0))
+               return -EINVAL;
 
        ret = enable_pwrrail(gpu);
        if (ret)

@@ -175,9 +184,18 @@ int msm_gpu_pm_resume(struct msm_gpu *gpu)
 
  int msm_gpu_pm_suspend(struct msm_gpu *gpu)
  {
+       struct drm_device *dev = gpu->dev;
        int ret;
 
-       DBG("%s", gpu->name);
+       DBG("%s: active_cnt=%d", gpu->name, gpu->active_cnt);
+
+       WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+       if (--gpu->active_cnt > 0)
+               return 0;
+
+       if (WARN_ON(gpu->active_cnt < 0))
+               return -EINVAL;
 
        ret = disable_axi(gpu);
        if (ret)

@@ -195,6 +213,55 @@ int msm_gpu_pm_suspend(struct msm_gpu *gpu)
  }
 
  /*
+ * Inactivity detection (for suspend):
+ */
+
+static void inactive_worker(struct work_struct *work)
+{
+       struct msm_gpu *gpu = container_of(work, struct msm_gpu,
 inactive_work);
+       struct drm_device *dev = gpu->dev;
+
+       if (gpu->inactive)
+               return;
+
+       DBG("%s: inactive!\n", gpu->name);
+       mutex_lock(&dev->struct_mutex);
+       if (!(msm_gpu_active(gpu) || gpu->inactive)) {
+               disable_axi(gpu);
+               disable_clk(gpu);
+               gpu->inactive = true;
+       }
+       mutex_unlock(&dev->struct_mutex);
+}
+
+static void inactive_handler(unsigned long data)
+{
+       struct msm_gpu *gpu = (struct msm_gpu *)data;
+       struct msm_drm_private *priv = gpu->dev->dev_private;
+
+       queue_work(priv->wq, &gpu->inactive_work);
+}
+
+/* cancel inactive timer and make sure we are awake: */
+static void inactive_cancel(struct msm_gpu *gpu)
+{
+       DBG("%s", gpu->name);
+       del_timer(&gpu->inactive_timer);
+       if (gpu->inactive) {
+               enable_clk(gpu);
+               enable_axi(gpu);
+               gpu->inactive = false;
+       }
+}
+
+static void inactive_start(struct msm_gpu *gpu)
+{
+       DBG("%s", gpu->name);
+       mod_timer(&gpu->inactive_timer,
+                       round_jiffies_up(jiffies +
 DRM_MSM_INACTIVE_JIFFIES));
+}
+
+/*
   * Hangcheck detection for locked gpu:

   */

drivers/gpu/drm/msm/msm_gpu.c +13 -1

@@ -206,7 +273,10 @@ static void recover_worker(struct work_struct *work)
        dev_err(dev->dev, "%s: hangcheck recover!\n", gpu->name);
 
        mutex_lock(&dev->struct_mutex);
-       gpu->funcs->recover(gpu);
+       if (msm_gpu_active(gpu)) {
+               inactive_cancel(gpu);
+               gpu->funcs->recover(gpu);
+       }
        mutex_unlock(&dev->struct_mutex);
 
        msm_gpu_retire(gpu);

@@ -281,6 +351,9 @@ static void retire_worker(struct work_struct *work)
        }
 
        mutex_unlock(&dev->struct_mutex);
+
+       if (!msm_gpu_active(gpu))
+               inactive_start(gpu);
  }
 
  /* call from irq handler to schedule work to retire bo's */

@@ -302,6 +375,8 @@ int msm_gpu_submit(struct msm_gpu *gpu, struct
 msm_gem_submit *submit,
 
        gpu->submitted_fence = submit->fence;
 
+       inactive_cancel(gpu);
+
        ret = gpu->funcs->submit(gpu, submit, ctx);
        priv->lastctx = ctx;

@@ -357,11 +432,15 @@ int msm_gpu_init(struct drm_device *drm, struct
 platform_device *pdev,
        gpu->dev = drm;
        gpu->funcs = funcs;
        gpu->name = name;
+       gpu->inactive = true;
 
        INIT_LIST_HEAD(&gpu->active_list);
        INIT_WORK(&gpu->retire_work, retire_worker);
+       INIT_WORK(&gpu->inactive_work, inactive_worker);
        INIT_WORK(&gpu->recover_work, recover_worker);
 
+       setup_timer(&gpu->inactive_timer, inactive_handler,
+                       (unsigned long)gpu);
        setup_timer(&gpu->hangcheck_timer, hangcheck_handler,
                        (unsigned long)gpu);

drivers/gpu/drm/msm/msm_gpu.h +10 -1

index 458db8c..fad2700 100644
@@ -72,6 +72,10 @@ struct msm_gpu {
 
        uint32_t submitted_fence;
 
+       /* is gpu powered/active? */
+       int active_cnt;
+       bool inactive;
+
        /* worker for handling active-list retiring: */
        struct work_struct retire_work;
 

@@ -91,7 +95,12 @@ struct msm_gpu {
        uint32_t bsc;
  #endif
 
-       /* Hang Detction: */
+       /* Hang and Inactivity Detection:
+        */
+#define DRM_MSM_INACTIVE_PERIOD   66 /* in ms (roughly four frames) */
+#define DRM_MSM_INACTIVE_JIFFIES
 msecs_to_jiffies(DRM_MSM_INACTIVE_PERIOD)
+       struct timer_list inactive_timer;
+       struct work_struct inactive_work;
  #define DRM_MSM_HANGCHECK_PERIOD 500 /* in ms */
  #define DRM_MSM_HANGCHECK_JIFFIES

msecs_to_jiffies(DRM_MSM_HANGCHECK_PERIOD)

        struct timer_list hangcheck_timer;

drivers/gpu/drm/msm/msm_gpu.h +0 -0

@@ -99,6 +108,11 @@ struct msm_gpu {
        struct work_struct recover_work;

drivers/gpu/drm/msm/msm_gpu.h struct msm_gpu { · line 118

Quoted 10 lines

drivers/gpu/drm/msm/msm_gpu.h +5 -0

  };
 
+static inline bool msm_gpu_active(struct msm_gpu *gpu)
+{
+       return gpu->submitted_fence > gpu->funcs->last_fence(gpu);
+}
+
  static inline void gpu_write(struct msm_gpu *gpu, u32 reg, u32 data)
  {
        msm_writel(data, gpu->mmio + (reg << 2));

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Some of the w/a or different behavior of userspace blob driver seem to be keyed to gpu patch revision, rather than gpu-id. So expose the full chip-id to userspace so it can DTRT.

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 6 ++++++
  include/uapi/drm/msm_drm.h              | 1 +
  2 files changed, 7 insertions(+)

drivers/gpu/drm/msm/adreno/adreno_gpu.c +6 -0

index 7a11563..28ca8cd 100644
@@ -73,6 +73,12 @@ int adreno_get_param(struct msm_gpu *gpu, uint32_t param, uint64_t *value)
  	case MSM_PARAM_GMEM_SIZE:
  		*value = adreno_gpu->gmem;
  		return 0;
+	case MSM_PARAM_CHIP_ID:
+		*value = adreno_gpu->rev.patchid |
+				(adreno_gpu->rev.minor << 8) |
+				(adreno_gpu->rev.major << 16) |
+				(adreno_gpu->rev.core << 24);
+		return 0;
  	default:
  		DBG("%s: invalid param: %u", gpu->name, param);
  		return -EINVAL;

include/uapi/drm/msm_drm.h +1 -0

index d3c6207..bf91a78 100644
@@ -50,6 +50,7 @@ struct drm_msm_timespec {
 
  #define MSM_PARAM_GPU_ID     0x01
  #define MSM_PARAM_GMEM_SIZE  0x02
+#define MSM_PARAM_CHIP_ID    0x03
 
  struct drm_msm_param {
  	uint32_t pipe;           /* in, MSM_PIPE_x */

After reading a nice article on LWN[1], I went back and double checked my handling of invalid-input checking. Turns out there were a couple places I had missed.

Since the driver is fairly young, and the devices it supports are really only just barely usable for basic stuff (serial console) with an upstream kernel, I think we should fix this now and revert specific parts of this patch later in the unlikely event that a regression is reported.

[1] https://lwn.net/Articles/588444/

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/msm_drv.c        | 20 +++++++++++++++++++-
  drivers/gpu/drm/msm/msm_gem_submit.c | 15 +++++++++++++--
  include/uapi/drm/msm_drm.h           | 11 +++++++++++
  3 files changed, 43 insertions(+), 3 deletions(-)

drivers/gpu/drm/msm/msm_drv.c +19 -1

index 9ffc275..eee8d37 100644
@@ -664,6 +664,12 @@ static int msm_ioctl_gem_new(struct drm_device *dev, void *data,
  		struct drm_file *file)
  {
  	struct drm_msm_gem_new *args = data;
+
+	if (args->flags & ~MSM_BO_FLAGS) {
+		DRM_ERROR("invalid flags: %08x\n", args->flags);
+		return -EINVAL;
+	}
+
  	return msm_gem_new_handle(dev, file, args->size,
  			args->flags, &args->handle);
  }

@@ -677,6 +683,11 @@ static int msm_ioctl_gem_cpu_prep(struct drm_device *dev, void *data,
  	struct drm_gem_object *obj;
  	int ret;
 
+	if (args->op & ~MSM_PREP_FLAGS) {
+		DRM_ERROR("invalid op: %08x\n", args->op);
+		return -EINVAL;
+	}
+
  	obj = drm_gem_object_lookup(dev, file, args->handle);
  	if (!obj)
  		return -ENOENT;

@@ -731,7 +742,14 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data,
  		struct drm_file *file)
  {
  	struct drm_msm_wait_fence *args = data;
-	return msm_wait_fence_interruptable(dev, args->fence, &TS(args->timeout));
+
+	if (args->pad) {
+		DRM_ERROR("invalid pad: %08x\n", args->pad);
+		return -EINVAL;
+	}
+
+	return msm_wait_fence_interruptable(dev, args->fence,
+			&TS(args->timeout));
  }
 
  static const struct drm_ioctl_desc msm_ioctls[] = {

drivers/gpu/drm/msm/msm_gem_submit.c +13 -2

index 5423e91..1f1f4cf 100644
@@ -23,7 +23,6 @@
   * Cmdstream submission:
   */
 
-#define BO_INVALID_FLAGS ~(MSM_SUBMIT_BO_READ | MSM_SUBMIT_BO_WRITE)
  /* make sure these don't conflict w/ MSM_SUBMIT_BO_x */
  #define BO_VALID    0x8000
  #define BO_LOCKED   0x4000

@@ -77,7 +76,7 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,
  			goto out_unlock;
  		}
 
-		if (submit_bo.flags & BO_INVALID_FLAGS) {
+		if (submit_bo.flags & ~MSM_SUBMIT_BO_FLAGS) {
  			DRM_ERROR("invalid flags: %x\n", submit_bo.flags);
  			ret = -EINVAL;
  			goto out_unlock;

@@ -369,6 +368,18 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
  			goto out;
  		}
 
+		/* validate input from userspace: */
+		switch (submit_cmd.type) {
+		case MSM_SUBMIT_CMD_BUF:
+		case MSM_SUBMIT_CMD_IB_TARGET_BUF:
+		case MSM_SUBMIT_CMD_CTX_RESTORE_BUF:
+			break;
+		default:
+			DRM_ERROR("invalid type: %08x\n", submit_cmd.type);
+			ret = -EINVAL;
+			goto out;
+		}
+
  		ret = submit_bo(submit, submit_cmd.submit_idx,
  				&msm_obj, &iova, NULL);
  		if (ret)

include/uapi/drm/msm_drm.h +11 -0

index bf91a78..0664c31 100644
@@ -70,6 +70,12 @@ struct drm_msm_param {
  #define MSM_BO_WC            0x00020000
  #define MSM_BO_UNCACHED      0x00040000
 
+#define MSM_BO_FLAGS         (MSM_BO_SCANOUT | \
+                              MSM_BO_GPU_READONLY | \
+                              MSM_BO_CACHED | \
+                              MSM_BO_WC | \
+                              MSM_BO_UNCACHED)
+
  struct drm_msm_gem_new {
  	uint64_t size;           /* in */
  	uint32_t flags;          /* in, mask of MSM_BO_x */

@@ -86,6 +92,8 @@ struct drm_msm_gem_info {
  #define MSM_PREP_WRITE       0x02
  #define MSM_PREP_NOSYNC      0x04
 
+#define MSM_PREP_FLAGS       (MSM_PREP_READ | MSM_PREP_WRITE | MSM_PREP_NOSYNC)
+
  struct drm_msm_gem_cpu_prep {
  	uint32_t handle;         /* in */
  	uint32_t op;             /* in, mask of MSM_PREP_x */

@@ -153,6 +161,9 @@ struct drm_msm_gem_submit_cmd {
   */
  #define MSM_SUBMIT_BO_READ             0x0001
  #define MSM_SUBMIT_BO_WRITE            0x0002
+
+#define MSM_SUBMIT_BO_FLAGS            (MSM_SUBMIT_BO_READ | MSM_SUBMIT_BO_WRITE)
+
  struct drm_msm_gem_submit_bo {
  	uint32_t flags;          /* in, mask of MSM_SUBMIT_BO_x */
  	uint32_t handle;         /* in, GEM handle */