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

[PATCH v2 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             | 75 +++++++++++++++++++
 4 files changed, 114 insertions(+), 12 deletions(-)

-- 
2.33.1

[PATCH v2 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 v2 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 v2 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>
---
v2: Drop unneeded msm_kms_show_priv [Dmitry B]

 drivers/gpu/drm/msm/msm_debugfs.c | 75 +++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
index 956b1efc3721..0804c31e8962 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,73 @@ static const struct file_operations msm_gpu_fops = {
 	.release = msm_gpu_release,
 };
 
+/*
+ * Display Snapshot:
+ */
+
+static int msm_kms_show(struct seq_file *m, void *arg)
+{
+	struct drm_printer p = drm_seq_file_printer(m);
+	struct msm_disp_state *state = m->private;
+
+	msm_disp_state_print(state, &p);
+
+	return 0;
+}
+
+static int msm_kms_release(struct inode *inode, struct file *file)
+{
+	struct seq_file *m = file->private_data;
+	struct msm_disp_state *state = m->private;
+
+	msm_disp_state_free(state);
+
+	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_disp_state *state;
+	int ret;
+
+	if (!priv->kms)
+		return -ENODEV;
+
+	ret = mutex_lock_interruptible(&priv->kms->dump_mutex);
+	if (ret)
+		return ret;
+
+	state = msm_disp_snapshot_state_sync(priv->kms);
+
+	mutex_unlock(&priv->kms->dump_mutex);
+
+	if (IS_ERR(state)) {
+		return PTR_ERR(state);
+	}
+
+	ret = single_open(file, msm_kms_show, state);
+	if (ret) {
+		msm_disp_state_free(state);
+		return ret;
+	}
+
+	return 0;
+}
+
+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 +311,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 v2 1/3] drm/msm/disp: Tweak display snapshot to match gpu snapshot

[Dmitry Baryshkov]

On Wed, 15 Dec 2021 at 20:49, Rob Clark <robdclark@gmail.com> wrote:
>
> 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>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.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(-)

We should pull this out of disp/, it's no longer disp-specific.


-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Wed, 15 Dec 2021 at 20:49, Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> We'll re-use this for debugfs.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.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(-)
>


-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 15/12/2021 20:45, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
> v2: Drop unneeded msm_kms_show_priv [Dmitry B]
> 
>   drivers/gpu/drm/msm/msm_debugfs.c | 75 +++++++++++++++++++++++++++++++
>   1 file changed, 75 insertions(+)-- 
With best wishes
Dmitry

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

[Rob Clark]

On Wed, Dec 15, 2021 at 11:17 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Wed, 15 Dec 2021 at 20:49, Rob Clark <robdclark@gmail.com> wrote:
> >
> > 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>
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.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(-)
>
> We should pull this out of disp/, it's no longer disp-specific.

Or possibly just move dsi/etc into disp?  It is disp specific in the
sense that dumping GPU state works quite differently..

BR,
-R

>
> --
> With best wishes
> Dmitry

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

[Dmitry Baryshkov]

On Wed, 15 Dec 2021 at 23:09, Rob Clark <robdclark@gmail.com> wrote:
>
> On Wed, Dec 15, 2021 at 11:17 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Wed, 15 Dec 2021 at 20:49, Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > 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>
> >
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.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(-)
> >
> > We should pull this out of disp/, it's no longer disp-specific.
>
> Or possibly just move dsi/etc into disp?  It is disp specific in the
> sense that dumping GPU state works quite differently..

Not sure about this.  dsi/hdmi/dp seem to be perfect top-level
entities. I see your point here, however I'd rather prefer to move
mdp4/mdp5/dpu1 out of disp subdir rather than pushing all non-GPU code
into disp/

When I tried to move dsi/phy to a separate phy driver, I reworked
msm_disp_snapshot by splitting some parts into lib/ code. But I can
not say that I completely liked what I did. Partially it was one of
the reasons for me not pushing the dsi/phy patchset past RFC.

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 15/12/2021 20:45, Rob Clark wrote:
> 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;

drivers/gpu/drm/msm/disp/msm_disp_snapshot.c:56:28: warning: unused 
variable ‘drm_dev’ [-Wunused-variable]
    56 |         struct drm_device *drm_dev = kms->dev;

I will apply the fixup locally.

> +	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
> 


-- 
With best wishes
Dmitry