Hi Rob
Thank you for the review.
On 2021-04-09 13:38, Rob Clark wrote:
On Thu, Apr 8, 2021 at 7:28 PM Abhinav Kumar <abhinavk@xxxxxxxxxxxxxx>
wrote:
Add the dpu_dbg module which adds supports to dump dpu registers
which can be used in case of error conditions.
changes in v3:
- Get rid of registration mechanism for sub-modules and instead get
this information from the dpu catalog itself
- Get rid of global dpu_dbg struct and instead store it in dpu_kms
- delegate the power management of the sub-modules to the resp
drivers
- refactor and remove the linked list logic and simplify it to have
just an array
Change-Id: Ide975ecf5d7952ae44daaa6eb611e27d09630be5
Reported-by: kernel test robot <lkp@xxxxxxxxx>
Signed-off-by: Abhinav Kumar <abhinavk@xxxxxxxxxxxxxx>
---
drivers/gpu/drm/msm/Makefile | 2 +
drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c | 221
+++++++++++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h | 200
+++++++++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_dbg_util.c | 257
+++++++++++++++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 86 +++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 5 +
drivers/gpu/drm/msm/dp/dp_catalog.c | 10 +
drivers/gpu/drm/msm/dp/dp_catalog.h | 5 +
drivers/gpu/drm/msm/dp/dp_display.c | 37 ++++
drivers/gpu/drm/msm/dp/dp_display.h | 1 +
drivers/gpu/drm/msm/dsi/dsi.c | 5 +
drivers/gpu/drm/msm/dsi/dsi.h | 4 +
drivers/gpu/drm/msm/dsi/dsi_host.c | 25 +++
drivers/gpu/drm/msm/msm_drv.c | 29 ++-
drivers/gpu/drm/msm/msm_drv.h | 2 +
16 files changed, 889 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c
create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h
create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dbg_util.c
[snip]
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h
new file mode 100644
index 0000000..302205a
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h
@@ -0,0 +1,200 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020-2021, The Linux Foundation. All rights
reserved.
+ */
+
+#ifndef DPU_DBG_H_
+#define DPU_DBG_H_
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_device.h>
+#include "../../../drm_crtc_internal.h"
+#include <drm/drm_print.h>
+#include <drm/drm_atomic.h>
+#include <linux/debugfs.h>
+#include <linux/list.h>
+#include <linux/delay.h>
+#include <linux/spinlock.h>
+#include <linux/ktime.h>
+#include <linux/debugfs.h>
+#include <linux/uaccess.h>
+#include <linux/dma-buf.h>
+#include <linux/slab.h>
+#include <linux/list_sort.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/kthread.h>
+#include <linux/devcoredump.h>
+#include <stdarg.h>
+#include "dpu_hw_catalog.h"
+#include "dpu_kms.h"
+#include "dsi.h"
+
+#define DPU_DBG_DUMP_DATA_LIMITER (NULL)
+
+enum dpu_dbg_dump_flag {
+ DPU_DBG_DUMP_IN_LOG = BIT(0),
+ DPU_DBG_DUMP_IN_MEM = BIT(1),
+ DPU_DBG_DUMP_IN_COREDUMP = BIT(2),
+};
overall, I like this better, but..
I'm not completely convinced about the need for
DUMP_IN_LOG/DUMP_IN_MEM.. we haven't really needed it on the GPU side
of things, and the only case I can think of where it might be useful
is if you can't boot far enough to get to some minimal userspace..
once you have at least some minimal userspace, you can just pull out
and clear the devcore dump via sysfs. That said, if state snapshot
and printing were better separated it would just take a few lines of
code to use a different drm_printer to print the state snapshot to
dmesg.
[snip]
Yes, dumping the registers to log is just a debug feature and also
useful if
CONFIG_DEVCOREDUMP is not enabled.
Its also useful for developers who can just view the dumps in the logs.
Yes, the way this is written it is already generic enough to just take a
different drm_printer.
For the log i use this:
+ if (DPU_DBG_DUMP_IN_CONSOLE) {
+ p = drm_info_printer(dpu_dbg->drm_dev->dev);
+ dpu_dbg->dpu_dbg_printer = &p;
+ dpu_dbg_print_regs(dpu_dbg->drm_dev, DPU_DBG_DUMP_IN_LOG);
+ }
For the coredump I use this,
p = drm_coredump_printer(&iter);
+
+ drm_printf(&p, "---\n");
+
+ drm_printf(&p, "module: " KBUILD_MODNAME "\n");
+ drm_printf(&p, "dpu devcoredump\n");
+ drm_printf(&p, "timestamp %lld\n", ktime_to_ns(dpu_dbg->timestamp));
+
+ dpu_dbg->dpu_dbg_printer = &p;
+
+ dpu_dbg_print_regs(dpu_dbg->drm_dev, DPU_DBG_DUMP_IN_COREDUMP);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index ab281cb..d1675ee 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -2489,3 +2489,28 @@ struct drm_bridge
*msm_dsi_host_get_bridge(struct mipi_dsi_host *host)
return of_drm_find_bridge(msm_host->device_node);
}
+
+void msm_dsi_host_dump_regs(struct mipi_dsi_host *host)
+{
+ struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
+ struct drm_device *dev = msm_host->dev;
+ struct dpu_dbg_base *dpu_dbg;
+
+ dpu_dbg = dpu_dbg_get(dev);
+
+ if (dpu_dbg_is_drm_printer_needed(dpu_dbg) &&
+ !dpu_dbg->dpu_dbg_printer) {
+ pr_err("invalid drm printer\n");
+ return;
+ }
for example, here ^^^
why should the other blocks even care? All they should know is that
they've been asked to snapshot their state..
You are right, the printer check can be moved to one level up to
dpu_dbg_print_regs().
That way it will avoid this redundant valid printer check in the
dump_regs() function of the sub-modules.
The name of the function is xxx_xxx_dump_regs() but maybe I can just
rename this to capture_state().
The reason for having the same function to capture the state and also
print it out is to avoid code duplication.
+ if (dpu_dbg->reg_dump_method == DPU_DBG_DUMP_IN_MEM)
+ pm_runtime_get_sync(&msm_host->pdev->dev);
+
+ dpu_dbg_dump_regs(&dpu_dbg->dsi_ctrl_regs[msm_host->id],
+ msm_iomap_size(msm_host->pdev, "dsi_ctrl"),
msm_host->ctrl_base,
+ dpu_dbg->reg_dump_method,
dpu_dbg->dpu_dbg_printer);
+
+ if (dpu_dbg->reg_dump_method == DPU_DBG_DUMP_IN_MEM)
+ pm_runtime_put_sync(&msm_host->pdev->dev);
+}
I'm thinking something more along the lines of (in drm/msm/disp so it
is a little less weird for dsi/dp/etc to be #include'ing header with
structs):
Are you suggesting we move the dpu_dbg to drm/msm/disp? I am okay with
that.
struct msm_disp_state {
.. maybe a bit of generic information like kernel version, time,
etc ...
struct list_head blocks; /* list of msm_disp_state_block */
};
struct msm_disp_state_block {
const char *name;
struct list_head node; /* node in msm_disp_state::blocks */
unsigned size;
uint32_t state[];
};
struct msm_disp_state * msm_disp_snapshot_state(struct drm_device *dev)
{
struct msm_disp_state *state = ...;
if (priv->kms)
priv->kms->snapshot(priv->kms, state);
for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
msm_dsi_snapshot(priv->dsi[i], state);
if (priv->dp)
msm_dp_snapshot(priv->dp, state);
return state;
}
This is essentially what the dpu_dbg_dump_regs does right now. It just
delegates the dumping to the respective modules.
+{
+ int i;
+
+ for (i = 0; i < DPU_DBG_BASE_MAX; i++) {
+ if (dpu_dbg->blk_names[i] != NULL) {
+ DRM_DEBUG("blk name is %s\n", dpu_dbg->blk_names[i]);
+ if (!strcmp(dpu_dbg->blk_names[i], "all")) {
+ dpu_dbg_dump_all_regs(dpu_dbg->drm_dev);
+ break;
+ } else if (!strcmp(dpu_dbg->blk_names[i], "mdp")) {
+ dpu_dbg_dump_mdp_regs(dpu_dbg->drm_dev);
+ } else if (!strcmp(dpu_dbg->blk_names[i], "dsi")) {
+ dpu_dbg_dump_dsi_regs(dpu_dbg->drm_dev);
+ } else if (!strcmp(dpu_dbg->blk_names[i], "dp")) {
+ dpu_dbg_dump_dp_regs(dpu_dbg->drm_dev);
+ } else {
+ DRM_ERROR("blk name not found %s\n", dpu_dbg->blk_names[i]);
+ }
+ }
+ }
+}
... maybe add a helper that can be used everywhere to allocate and
append a new state block.. Ie. pass it state/name/len/iomem
void msm_disp_state_free(struct msm_disp_state *state)
{
list_for_each_entry_safe (block, ...)
kfree(block);
kfree(state)
}
void msm_disp_state_print(struct drm_disp_state *state, drm_printer *p)
{
.. print generic state and loop over state->blocks printing them ..
}
... if you do need to support printing to dmesg, you could re-use
msm_disp_state_print() with an drm_info printer in just a few lines of
code, rather than spreading the logic far and wide.
BR,
-R
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
