[PATCH 0/3] drm/msm: Add display snapshot debugfs

[PATCH 0/3] drm/msm: Add display snapshot debugfs

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

This series adds a "kms" debugfs file to dump display register + atomic
state, which is useful for debugging issues that don't trigger a display
error irq (such as dsi phy misconfiguration).

Rob Clark (3):
  drm/msm/disp: Tweak display snapshot to match gpu snapshot
  drm/msm/disp: Export helper for capturing snapshot
  drm/msm/debugfs: Add display/kms state snapshot

 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c  | 28 ++++--
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h  | 14 ++-
 .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |  9 +-
 drivers/gpu/drm/msm/msm_debugfs.c             | 90 +++++++++++++++++++
 4 files changed, 129 insertions(+), 12 deletions(-)

-- 
2.33.1

[PATCH 1/3] drm/msm/disp: Tweak display snapshot to match gpu snapshot

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Add UTS_RELEASE and show timestamp the same way for consistency.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h      | 4 ++--
 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 9 ++++++---
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
index 4c619307612c..31ad68be3391 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
@@ -39,7 +39,7 @@
  * @dev: device pointer
  * @drm_dev: drm device pointer
  * @atomic_state: atomic state duplicated at the time of the error
- * @timestamp: timestamp at which the coredump was captured
+ * @time: timestamp at which the coredump was captured
  */
 struct msm_disp_state {
 	struct device *dev;
@@ -49,7 +49,7 @@ struct msm_disp_state {
 
 	struct drm_atomic_state *atomic_state;
 
-	ktime_t timestamp;
+	struct timespec64 time;
 };
 
 /**
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
index 2e1acb1bc390..5d2ff6791058 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -5,6 +5,8 @@
 
 #define pr_fmt(fmt)	"[drm:%s:%d] " fmt, __func__, __LINE__
 
+#include <generated/utsrelease.h>
+
 #include "msm_disp_snapshot.h"
 
 static void msm_disp_state_dump_regs(u32 **reg, u32 aligned_len, void __iomem *base_addr)
@@ -79,10 +81,11 @@ void msm_disp_state_print(struct msm_disp_state *state, struct drm_printer *p)
 	}
 
 	drm_printf(p, "---\n");
-
+	drm_printf(p, "kernel: " UTS_RELEASE "\n");
 	drm_printf(p, "module: " KBUILD_MODNAME "\n");
 	drm_printf(p, "dpu devcoredump\n");
-	drm_printf(p, "timestamp %lld\n", ktime_to_ns(state->timestamp));
+	drm_printf(p, "time: %lld.%09ld\n",
+		state->time.tv_sec, state->time.tv_nsec);
 
 	list_for_each_entry_safe(block, tmp, &state->blocks, node) {
 		drm_printf(p, "====================%s================\n", block->name);
@@ -100,7 +103,7 @@ static void msm_disp_capture_atomic_state(struct msm_disp_state *disp_state)
 	struct drm_device *ddev;
 	struct drm_modeset_acquire_ctx ctx;
 
-	disp_state->timestamp = ktime_get();
+	ktime_get_real_ts64(&disp_state->time);
 
 	ddev = disp_state->drm_dev;
 
-- 
2.33.1

[PATCH 2/3] drm/msm/disp: Export helper for capturing snapshot

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

We'll re-use this for debugfs.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c | 28 +++++++++++++++-----
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h | 10 +++++++
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
index a4a7cb06bc87..580ea01b13ab 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
@@ -28,29 +28,43 @@ static ssize_t __maybe_unused disp_devcoredump_read(char *buffer, loff_t offset,
 	return count - iter.remain;
 }
 
-static void _msm_disp_snapshot_work(struct kthread_work *work)
+struct msm_disp_state *
+msm_disp_snapshot_state_sync(struct msm_kms *kms)
 {
-	struct msm_kms *kms = container_of(work, struct msm_kms, dump_work);
 	struct drm_device *drm_dev = kms->dev;
 	struct msm_disp_state *disp_state;
-	struct drm_printer p;
+
+	WARN_ON(!mutex_is_locked(&kms->dump_mutex));
 
 	disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
 	if (!disp_state)
-		return;
+		return ERR_PTR(-ENOMEM);
 
 	disp_state->dev = drm_dev->dev;
 	disp_state->drm_dev = drm_dev;
 
 	INIT_LIST_HEAD(&disp_state->blocks);
 
-	/* Serialize dumping here */
-	mutex_lock(&kms->dump_mutex);
-
 	msm_disp_snapshot_capture_state(disp_state);
 
+	return disp_state;
+}
+
+static void _msm_disp_snapshot_work(struct kthread_work *work)
+{
+	struct msm_kms *kms = container_of(work, struct msm_kms, dump_work);
+	struct drm_device *drm_dev = kms->dev;
+	struct msm_disp_state *disp_state;
+	struct drm_printer p;
+
+	/* Serialize dumping here */
+	mutex_lock(&kms->dump_mutex);
+	disp_state = msm_disp_snapshot_state_sync(kms);
 	mutex_unlock(&kms->dump_mutex);
 
+	if (IS_ERR(disp_state))
+		return;
+
 	if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
 		p = drm_info_printer(disp_state->drm_dev->dev);
 		msm_disp_state_print(disp_state, &p);
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
index 31ad68be3391..b5f452bd7ada 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
@@ -84,6 +84,16 @@ int msm_disp_snapshot_init(struct drm_device *drm_dev);
  */
 void msm_disp_snapshot_destroy(struct drm_device *drm_dev);
 
+/**
+ * msm_disp_snapshot_state_sync - synchronously snapshot display state
+ * @kms:  the kms object
+ *
+ * Returns state or error
+ *
+ * Must be called with &kms->dump_mutex held
+ */
+struct msm_disp_state *msm_disp_snapshot_state_sync(struct msm_kms *kms);
+
 /**
  * msm_disp_snapshot_state - trigger to dump the display snapshot
  * @drm_dev:	handle to drm device
-- 
2.33.1

[PATCH 3/3] drm/msm/debugfs: Add display/kms state snapshot

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/msm_debugfs.c | 90 +++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
index 956b1efc3721..088f1160c892 100644
--- a/drivers/gpu/drm/msm/msm_debugfs.c
+++ b/drivers/gpu/drm/msm/msm_debugfs.c
@@ -15,6 +15,11 @@
 #include "msm_gpu.h"
 #include "msm_kms.h"
 #include "msm_debugfs.h"
+#include "disp/msm_disp_snapshot.h"
+
+/*
+ * GPU Snapshot:
+ */
 
 struct msm_gpu_show_priv {
 	struct msm_gpu_state *state;
@@ -109,6 +114,88 @@ static const struct file_operations msm_gpu_fops = {
 	.release = msm_gpu_release,
 };
 
+/*
+ * Display Snapshot:
+ */
+
+struct msm_kms_show_priv {
+	struct msm_disp_state *state;
+	struct drm_device *dev;
+};
+
+static int msm_kms_show(struct seq_file *m, void *arg)
+{
+	struct drm_printer p = drm_seq_file_printer(m);
+	struct msm_kms_show_priv *show_priv = m->private;
+
+	msm_disp_state_print(show_priv->state, &p);
+
+	return 0;
+}
+
+static int msm_kms_release(struct inode *inode, struct file *file)
+{
+	struct seq_file *m = file->private_data;
+	struct msm_kms_show_priv *show_priv = m->private;
+
+	msm_disp_state_free(show_priv->state);
+	kfree(show_priv);
+
+	return single_release(inode, file);
+}
+
+static int msm_kms_open(struct inode *inode, struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+	struct msm_drm_private *priv = dev->dev_private;
+	struct msm_kms_show_priv *show_priv;
+	int ret;
+
+	if (!priv->kms)
+		return -ENODEV;
+
+	show_priv = kmalloc(sizeof(*show_priv), GFP_KERNEL);
+	if (!show_priv)
+		return -ENOMEM;
+
+	ret = mutex_lock_interruptible(&priv->kms->dump_mutex);
+	if (ret)
+		goto free_priv;
+
+	show_priv->state = msm_disp_snapshot_state_sync(priv->kms);
+
+	mutex_unlock(&priv->kms->dump_mutex);
+
+	if (IS_ERR(show_priv->state)) {
+		ret = PTR_ERR(show_priv->state);
+		goto free_priv;
+	}
+
+	show_priv->dev = dev;
+
+	ret = single_open(file, msm_kms_show, show_priv);
+	if (ret)
+		goto free_priv;
+
+	return 0;
+
+free_priv:
+	kfree(show_priv);
+	return ret;
+}
+
+static const struct file_operations msm_kms_fops = {
+	.owner = THIS_MODULE,
+	.open = msm_kms_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = msm_kms_release,
+};
+
+/*
+ * Other debugfs:
+ */
+
 static unsigned long last_shrink_freed;
 
 static int
@@ -239,6 +326,9 @@ void msm_debugfs_init(struct drm_minor *minor)
 	debugfs_create_file("gpu", S_IRUSR, minor->debugfs_root,
 		dev, &msm_gpu_fops);
 
+	debugfs_create_file("kms", S_IRUSR, minor->debugfs_root,
+		dev, &msm_kms_fops);
+
 	debugfs_create_u32("hangcheck_period_ms", 0600, minor->debugfs_root,
 		&priv->hangcheck_period);
 
-- 
2.33.1

Re: [PATCH 3/3] drm/msm/debugfs: Add display/kms state snapshot

[Dmitry Baryshkov]

On Tue, 14 Dec 2021 at 20:34, Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/msm/msm_debugfs.c | 90 +++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> index 956b1efc3721..088f1160c892 100644
> --- a/drivers/gpu/drm/msm/msm_debugfs.c
> +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> @@ -15,6 +15,11 @@
>  #include "msm_gpu.h"
>  #include "msm_kms.h"
>  #include "msm_debugfs.h"
> +#include "disp/msm_disp_snapshot.h"
> +
> +/*
> + * GPU Snapshot:
> + */
>
>  struct msm_gpu_show_priv {
>         struct msm_gpu_state *state;
> @@ -109,6 +114,88 @@ static const struct file_operations msm_gpu_fops = {
>         .release = msm_gpu_release,
>  };
>
> +/*
> + * Display Snapshot:
> + */
> +
> +struct msm_kms_show_priv {
> +       struct msm_disp_state *state;
> +       struct drm_device *dev;

I don't see ->dev being used outside of _open() callback. So, I'd
suggest removing it and using struct msm_disp_state instead of struct
msm_kms_show_priv.

> +};
> +
> +static int msm_kms_show(struct seq_file *m, void *arg)
> +{
> +       struct drm_printer p = drm_seq_file_printer(m);
> +       struct msm_kms_show_priv *show_priv = m->private;
> +
> +       msm_disp_state_print(show_priv->state, &p);
> +
> +       return 0;
> +}
> +
> +static int msm_kms_release(struct inode *inode, struct file *file)
> +{
> +       struct seq_file *m = file->private_data;
> +       struct msm_kms_show_priv *show_priv = m->private;
> +
> +       msm_disp_state_free(show_priv->state);
> +       kfree(show_priv);
> +
> +       return single_release(inode, file);
> +}
> +
> +static int msm_kms_open(struct inode *inode, struct file *file)
> +{
> +       struct drm_device *dev = inode->i_private;
> +       struct msm_drm_private *priv = dev->dev_private;
> +       struct msm_kms_show_priv *show_priv;
> +       int ret;
> +
> +       if (!priv->kms)
> +               return -ENODEV;
> +
> +       show_priv = kmalloc(sizeof(*show_priv), GFP_KERNEL);
> +       if (!show_priv)
> +               return -ENOMEM;
> +
> +       ret = mutex_lock_interruptible(&priv->kms->dump_mutex);
> +       if (ret)
> +               goto free_priv;
> +
> +       show_priv->state = msm_disp_snapshot_state_sync(priv->kms);
> +
> +       mutex_unlock(&priv->kms->dump_mutex);
> +
> +       if (IS_ERR(show_priv->state)) {
> +               ret = PTR_ERR(show_priv->state);
> +               goto free_priv;
> +       }
> +
> +       show_priv->dev = dev;
> +
> +       ret = single_open(file, msm_kms_show, show_priv);
> +       if (ret)
> +               goto free_priv;
> +
> +       return 0;
> +
> +free_priv:
> +       kfree(show_priv);
> +       return ret;
> +}
> +
> +static const struct file_operations msm_kms_fops = {
> +       .owner = THIS_MODULE,
> +       .open = msm_kms_open,
> +       .read = seq_read,
> +       .llseek = seq_lseek,
> +       .release = msm_kms_release,
> +};
> +
> +/*
> + * Other debugfs:
> + */
> +
>  static unsigned long last_shrink_freed;
>
>  static int
> @@ -239,6 +326,9 @@ void msm_debugfs_init(struct drm_minor *minor)
>         debugfs_create_file("gpu", S_IRUSR, minor->debugfs_root,
>                 dev, &msm_gpu_fops);
>
> +       debugfs_create_file("kms", S_IRUSR, minor->debugfs_root,
> +               dev, &msm_kms_fops);
> +
>         debugfs_create_u32("hangcheck_period_ms", 0600, minor->debugfs_root,
>                 &priv->hangcheck_period);
>
> --
> 2.33.1
>


-- 
With best wishes
Dmitry