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(-)
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;
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.
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
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.
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
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(-)
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
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);
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
dri-devel@lists.freedesktop.org
-
Dmitry Baryshkov -
Rob Clark