[PATCH v4 0/3] Add devcoredump support for DPU

[PATCH v4 0/3] Add devcoredump support for DPU

[Abhinav Kumar]

This series adds support to use devcoredump for DPU driver. It introduces
the msm_disp_snapshot module which assists in the capturing of register dumps during
error scenarios. When a display related error happens, the msm_disp_snapshot module
captures all the relevant register dumps along with the snapshot of the drm
atomic state and triggers a devcoredump.

changes in v4:
 - rename dpu_dbg to msm_disp_snapshot and move it to msm/disp
 - start using a list of blocks to store the hardware block information
 - cleanup block allocation and freeing logic to simplify it

Abhinav Kumar (3):
  drm: allow drm_atomic_print_state() to accept any drm_printer
  drm/msm: add support to take dpu snapshot
  drm/msm: add disp snapshot points across dpu driver

 drivers/gpu/drm/drm_atomic.c                       |  28 +++-
 drivers/gpu/drm/drm_atomic_uapi.c                  |   4 +-
 drivers/gpu/drm/drm_crtc_internal.h                |   4 +-
 drivers/gpu/drm/msm/Makefile                       |   2 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        |  18 +-
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   |  14 +-
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   |   8 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h     |   2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c            |  61 +++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h            |   5 +
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c       | 161 ++++++++++++++++++
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h       | 167 +++++++++++++++++++
 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c  | 181 +++++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_catalog.c                |  12 ++
 drivers/gpu/drm/msm/dp/dp_catalog.h                |   4 +
 drivers/gpu/drm/msm/dp/dp_display.c                |  29 ++++
 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                 |  19 +++
 drivers/gpu/drm/msm/msm_drv.c                      |  29 +++-
 drivers/gpu/drm/msm/msm_drv.h                      |   2 +
 drivers/gpu/drm/selftests/test-drm_framebuffer.c   |   1 +
 23 files changed, 743 insertions(+), 18 deletions(-)
 create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
 create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
 create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 1/3] drm: allow drm_atomic_print_state() to accept any drm_printer

[Abhinav Kumar]

Currently drm_atomic_print_state() internally allocates and uses a
drm_info printer. Allow it to accept any drm_printer type so that
the API can be leveraged even for taking drm snapshot.

Rename the drm_atomic_print_state() to drm_atomic_print_new_state()
so that it reflects its functionality better.

changes in v4:
- none

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_atomic.c                     | 28 +++++++++++++++++++-----
 drivers/gpu/drm/drm_atomic_uapi.c                |  4 +++-
 drivers/gpu/drm/drm_crtc_internal.h              |  4 +++-
 drivers/gpu/drm/selftests/test-drm_framebuffer.c |  1 +
 4 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index dda6005..7041a26 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1,6 +1,7 @@
 /*
  * Copyright (C) 2014 Red Hat
  * Copyright (C) 2014 Intel Corp.
+ * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the "Software"),
@@ -1573,9 +1574,20 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set,
 }
 EXPORT_SYMBOL(__drm_atomic_helper_set_config);
 
-void drm_atomic_print_state(const struct drm_atomic_state *state)
+/**
+ * drm_atomic_print_new_state - prints drm atomic state
+ * @state: atomic configuration to check
+ * @p: drm printer
+ *
+ * This functions prints the drm atomic state snapshot using the drm printer
+ * which is passed to it. This snapshot can be used for debugging purposes.
+ *
+ * Note that this function looks into the new state objects and hence its not
+ * safe to be used after the call to drm_atomic_helper_commit_hw_done().
+ */
+void drm_atomic_print_new_state(const struct drm_atomic_state *state,
+		struct drm_printer *p)
 {
-	struct drm_printer p = drm_info_printer(state->dev->dev);
 	struct drm_plane *plane;
 	struct drm_plane_state *plane_state;
 	struct drm_crtc *crtc;
@@ -1584,17 +1596,23 @@ void drm_atomic_print_state(const struct drm_atomic_state *state)
 	struct drm_connector_state *connector_state;
 	int i;
 
+	if (!p) {
+		DRM_ERROR("invalid drm printer\n");
+		return;
+	}
+
 	DRM_DEBUG_ATOMIC("checking %p\n", state);
 
 	for_each_new_plane_in_state(state, plane, plane_state, i)
-		drm_atomic_plane_print_state(&p, plane_state);
+		drm_atomic_plane_print_state(p, plane_state);
 
 	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
-		drm_atomic_crtc_print_state(&p, crtc_state);
+		drm_atomic_crtc_print_state(p, crtc_state);
 
 	for_each_new_connector_in_state(state, connector, connector_state, i)
-		drm_atomic_connector_print_state(&p, connector_state);
+		drm_atomic_connector_print_state(p, connector_state);
 }
+EXPORT_SYMBOL(drm_atomic_print_new_state);
 
 static void __drm_state_dump(struct drm_device *dev, struct drm_printer *p,
 			     bool take_locks)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 268bb69..c340a67 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -2,6 +2,7 @@
  * Copyright (C) 2014 Red Hat
  * Copyright (C) 2014 Intel Corp.
  * Copyright (C) 2018 Intel Corp.
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the "Software"),
@@ -1321,6 +1322,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	struct drm_out_fence_state *fence_state;
 	int ret = 0;
 	unsigned int i, j, num_fences;
+	struct drm_printer p = drm_info_printer(dev->dev);
 
 	/* disallow for drivers not supporting atomic: */
 	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1453,7 +1455,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 		ret = drm_atomic_nonblocking_commit(state);
 	} else {
 		if (drm_debug_enabled(DRM_UT_STATE))
-			drm_atomic_print_state(state);
+			drm_atomic_print_new_state(state, &p);
 
 		ret = drm_atomic_commit(state);
 	}
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 54d4cf1..1ca51ad 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -5,6 +5,7 @@
  *   Jesse Barnes <jesse.barnes@intel.com>
  * Copyright © 2014 Intel Corporation
  *   Daniel Vetter <daniel.vetter@ffwll.ch>
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the "Software"),
@@ -236,7 +237,8 @@ int __drm_atomic_helper_disable_plane(struct drm_plane *plane,
 int __drm_atomic_helper_set_config(struct drm_mode_set *set,
 				   struct drm_atomic_state *state);
 
-void drm_atomic_print_state(const struct drm_atomic_state *state);
+void drm_atomic_print_new_state(const struct drm_atomic_state *state,
+		struct drm_printer *p);
 
 /* drm_atomic_uapi.c */
 int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
diff --git a/drivers/gpu/drm/selftests/test-drm_framebuffer.c b/drivers/gpu/drm/selftests/test-drm_framebuffer.c
index 789f227..61b44d3 100644
--- a/drivers/gpu/drm/selftests/test-drm_framebuffer.c
+++ b/drivers/gpu/drm/selftests/test-drm_framebuffer.c
@@ -8,6 +8,7 @@
 #include <drm/drm_device.h>
 #include <drm/drm_mode.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_print.h>
 
 #include "../drm_crtc_internal.h"
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 2/3] drm/msm: add support to take dpu snapshot

[Abhinav Kumar]

Add the msm_disp_snapshot module which adds supports to dump dpu
registers and capture the drm atomic state which can be used in
case of error conditions.

changes in v4:
 - rename dpu_dbg to msm_disp_snapshot and move it to msm/disp
 - start using a list of blocks to store the hardware block information
 - cleanup block allocation and freeing logic to simplify it

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
---
 drivers/gpu/drm/msm/Makefile                      |   2 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    |   2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c           |  61 ++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h           |   5 +
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c      | 161 +++++++++++++++++++
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h      | 167 ++++++++++++++++++++
 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 181 ++++++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_catalog.c               |  12 ++
 drivers/gpu/drm/msm/dp/dp_catalog.h               |   4 +
 drivers/gpu/drm/msm/dp/dp_display.c               |  29 ++++
 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                |  19 +++
 drivers/gpu/drm/msm/msm_drv.c                     |  29 +++-
 drivers/gpu/drm/msm/msm_drv.h                     |   2 +
 16 files changed, 682 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
 create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
 create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c

diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 610d630..65d86ce 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -77,6 +77,8 @@ msm-y := \
 	disp/dpu1/dpu_plane.o \
 	disp/dpu1/dpu_rm.o \
 	disp/dpu1/dpu_vbif.o \
+	disp/msm_disp_snapshot.o \
+	disp/msm_disp_snapshot_util.o \
 	msm_atomic.o \
 	msm_atomic_tracepoints.o \
 	msm_debugfs.o \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index 4dfd8a2..0f9f0a5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -1,5 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/* Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights reserved.
  */
 
 #ifndef _DPU_HW_CATALOG_H
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 88e9cc3..7529566 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -798,6 +798,67 @@ static void dpu_irq_uninstall(struct msm_kms *kms)
 	dpu_core_irq_uninstall(dpu_kms);
 }
 
+void dpu_kms_mdp_snapshot(struct drm_device *dev)
+{
+	int i;
+	struct msm_drm_private *priv;
+	struct dpu_kms *dpu_kms;
+	struct dpu_mdss_cfg *cat;
+	struct dpu_hw_mdp *top;
+	struct msm_disp_state *disp_state;
+	char name[SZ_128];
+
+	priv = dev->dev_private;
+	dpu_kms = to_dpu_kms(priv->kms);
+	disp_state = dpu_kms->disp_state;
+
+	cat = dpu_kms->catalog;
+	top = dpu_kms->hw_mdp;
+
+	pm_runtime_get_sync(&dpu_kms->pdev->dev);
+
+	/* dump CTL sub-blocks HW regs info */
+	for (i = 0; i < cat->ctl_count; i++) {
+		snprintf(name, SZ_128, "ctl_%d", i);
+		msm_disp_snapshot_add_block(disp_state, name, cat->ctl[i].len,
+				dpu_kms->mmio + cat->ctl[i].base);
+	}
+
+	/* dump DSPP sub-blocks HW regs info */
+	for (i = 0; i < cat->dspp_count; i++) {
+		snprintf(name, SZ_128, "dspp_%d", i);
+		msm_disp_snapshot_add_block(disp_state, name, cat->dspp[i].len,
+				dpu_kms->mmio + cat->dspp[i].base);
+	}
+
+	/* dump INTF sub-blocks HW regs info */
+	for (i = 0; i < cat->intf_count; i++) {
+		snprintf(name, SZ_128, "intf_%d", i);
+		msm_disp_snapshot_add_block(disp_state, name, cat->intf[i].len,
+				dpu_kms->mmio + cat->intf[i].base);
+	}
+
+	/* dump PP sub-blocks HW regs info */
+	for (i = 0; i < cat->pingpong_count; i++) {
+		snprintf(name, SZ_128, "ping-pong_%d", i);
+		msm_disp_snapshot_add_block(disp_state, name, cat->pingpong[i].len,
+				dpu_kms->mmio + cat->pingpong[i].base);
+	}
+
+	/* dump SSPP sub-blocks HW regs info */
+	for (i = 0; i < cat->sspp_count; i++) {
+		snprintf(name, SZ_128, "sspp_%d", i);
+		msm_disp_snapshot_add_block(disp_state, name, cat->sspp[i].len,
+				dpu_kms->mmio + cat->sspp[i].base);
+	}
+
+	snprintf(name, SZ_128, "top");
+	msm_disp_snapshot_add_block(disp_state, name, top->hw.length,
+			dpu_kms->mmio + top->hw.blk_off);
+
+	pm_runtime_put_sync(&dpu_kms->pdev->dev);
+}
+
 static const struct msm_kms_funcs kms_funcs = {
 	.hw_init         = dpu_kms_hw_init,
 	.irq_preinstall  = dpu_irq_preinstall,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index d6717d6..74b6ce6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -13,6 +13,7 @@
 #include <drm/drm_drv.h>
 
 #include "msm_drv.h"
+#include "disp/msm_disp_snapshot.h"
 #include "msm_kms.h"
 #include "msm_mmu.h"
 #include "msm_gem.h"
@@ -132,6 +133,8 @@ struct dpu_kms {
 
 	struct opp_table *opp_table;
 
+	struct msm_disp_state *disp_state;
+
 	struct dss_module_power mp;
 
 	/* reference count bandwidth requests, so we know when we can
@@ -265,4 +268,6 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder);
  */
 u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name);
 
+void dpu_kms_mdp_snapshot(struct drm_device *dev);
+
 #endif /* __dpu_kms_H__ */
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
new file mode 100644
index 0000000..1bdad16
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
+ */
+
+#define pr_fmt(fmt)	"[drm:%s:%d] " fmt, __func__, __LINE__
+
+#include "msm_disp_snapshot.h"
+
+#ifdef CONFIG_DEV_COREDUMP
+static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
+		size_t count, void *data, size_t datalen)
+{
+	struct drm_print_iterator iter;
+	struct drm_printer p;
+	struct msm_disp_state *disp_state;
+
+	disp_state = data;
+
+	iter.data = buffer;
+	iter.offset = 0;
+	iter.start = offset;
+	iter.remain = count;
+
+	p = drm_coredump_printer(&iter);
+
+	msm_disp_state_print(disp_state, &p, MSM_DISP_SNAPSHOT_IN_COREDUMP);
+
+	return count - iter.remain;
+}
+
+static void disp_devcoredump_free(void *data)
+{
+	struct msm_disp_state *disp_state;
+
+	disp_state = data;
+
+	msm_disp_state_free(disp_state);
+
+	disp_state->coredump_pending = false;
+}
+#endif /* CONFIG_DEV_COREDUMP */
+
+static void _msm_disp_snapshot_work(struct kthread_work *work)
+{
+	struct msm_disp_state *disp_state = container_of(work, struct msm_disp_state, dump_work);
+	struct drm_printer p;
+
+	mutex_lock(&disp_state->mutex);
+
+	msm_disp_snapshot_capture_state(disp_state);
+
+	if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
+		p = drm_info_printer(disp_state->drm_dev->dev);
+		msm_disp_state_print(disp_state, &p, MSM_DISP_SNAPSHOT_IN_LOG);
+	}
+
+	/*
+	 * if devcoredump is not defined free the state immediately
+	 * otherwise it will be freed in the free handler.
+	 */
+#ifdef CONFIG_DEV_COREDUMP
+	dev_coredumpm(disp_state->dev, THIS_MODULE, disp_state, 0, GFP_KERNEL,
+			disp_devcoredump_read, disp_devcoredump_free);
+	disp_state->coredump_pending = true;
+#else
+	msm_disp_state_free(disp_state);
+#endif
+
+	mutex_unlock(&disp_state->mutex);
+}
+
+void msm_disp_snapshot_state(struct drm_device *drm_dev)
+{
+	struct msm_drm_private *priv;
+	struct dpu_kms *dpu_kms;
+	struct msm_disp_state *disp_state;
+
+	if (!drm_dev) {
+		DRM_ERROR("invalid params\n");
+		return;
+	}
+
+	priv = drm_dev->dev_private;
+	dpu_kms = to_dpu_kms(priv->kms);
+	disp_state = dpu_kms->disp_state;
+
+	if (!disp_state) {
+		DRM_ERROR("invalid params\n");
+		return;
+	}
+
+	/*
+	 * if there is a coredump pending return immediately till dump
+	 * if read by userspace or timeout happens
+	 */
+	if (disp_state->coredump_pending) {
+		DRM_DEBUG("coredump is pending read\n");
+		return;
+	}
+
+	kthread_queue_work(disp_state->dump_worker,
+			&disp_state->dump_work);
+}
+
+int msm_disp_snapshot_init(struct drm_device *drm_dev)
+{
+	struct dpu_kms *dpu_kms;
+	struct msm_drm_private *priv;
+	struct msm_disp_state *disp_state;
+
+	if (!drm_dev) {
+		DRM_ERROR("invalid params\n");
+		return -EINVAL;
+	}
+
+	priv = drm_dev->dev_private;
+	dpu_kms = to_dpu_kms(priv->kms);
+
+	disp_state = devm_kzalloc(drm_dev->dev, sizeof(struct msm_disp_state), GFP_KERNEL);
+
+	mutex_init(&disp_state->mutex);
+
+	disp_state->dev = drm_dev->dev;
+	disp_state->drm_dev = drm_dev;
+
+	INIT_LIST_HEAD(&disp_state->blocks);
+
+	disp_state->dump_worker = kthread_create_worker(0, "%s", "disp_snapshot");
+	if (IS_ERR(disp_state->dump_worker))
+		DRM_ERROR("failed to create disp state task\n");
+
+	kthread_init_work(&disp_state->dump_work, _msm_disp_snapshot_work);
+
+	dpu_kms->disp_state = disp_state;
+
+	return 0;
+}
+
+void msm_disp_snapshot_destroy(struct drm_device *drm_dev)
+{
+	struct dpu_kms *dpu_kms;
+	struct msm_drm_private *priv;
+	struct msm_disp_state *disp_state;
+
+	if (!drm_dev) {
+		DRM_ERROR("invalid params\n");
+		return;
+	}
+
+	priv = drm_dev->dev_private;
+	dpu_kms = to_dpu_kms(priv->kms);
+	disp_state = dpu_kms->disp_state;
+
+	if (disp_state->dump_worker)
+		kthread_destroy_worker(disp_state->dump_worker);
+
+	list_del(&disp_state->blocks);
+
+	mutex_destroy(&disp_state->mutex);
+}
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
new file mode 100644
index 0000000..0ccb7e7
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
@@ -0,0 +1,167 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef MSM_DISP_SNAPSHOT_H_
+#define MSM_DISP_SNAPSHOT_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"
+
+enum msm_disp_snapshot_dump_flag {
+	MSM_DISP_SNAPSHOT_IN_LOG = BIT(0),
+	MSM_DISP_SNAPSHOT_IN_MEM = BIT(1),
+	MSM_DISP_SNAPSHOT_IN_COREDUMP = BIT(2),
+};
+
+#define MSM_DISP_SNAPSHOT_MAX_BLKS		10
+
+/* debug option to print the registers in logs */
+#define MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE 0
+
+/* print debug ranges in groups of 4 u32s */
+#define REG_DUMP_ALIGN		16
+
+/**
+ * struct msm_disp_state - structure to store current dpu state
+ * @dev: device pointer
+ * @drm_dev: drm device pointer
+ * @mutex: mutex to serialize access to serialze dumps, debugfs access
+ * @coredump_pending: coredump is pending read from userspace
+ * @atomic_state: atomic state duplicated at the time of the error
+ * @dump_worker: kworker thread which runs the dump work
+ * @dump_work: kwork which dumps the registers and drm state
+ * @timestamp: timestamp at which the coredump was captured
+ */
+struct msm_disp_state {
+	struct device *dev;
+	struct drm_device *drm_dev;
+	struct mutex mutex;
+
+	bool coredump_pending;
+
+	struct list_head blocks;
+
+	struct drm_atomic_state *atomic_state;
+
+	struct kthread_worker *dump_worker;
+	struct kthread_work dump_work;
+	ktime_t timestamp;
+};
+
+/**
+ * struct msm_disp_state_block - structure to store each hardware block state
+ * @name: name of the block
+ * @drm_dev: handle to the linked list head
+ * @size: size of the register space of this hardware block
+ * @state: array holding the register dump of this hardware block
+ * @base_addr: starting address of this hardware block's register space
+ */
+struct msm_disp_state_block {
+	char name[SZ_128];
+	struct list_head node;
+	unsigned int size;
+	u32 *state;
+	void __iomem *base_addr;
+};
+
+/**
+ * MSM_DISP_SNAPSHOT - trigger to dump the display snapshot
+ * @drm_dev:	handle to drm device
+ */
+#define MSM_DISP_SNAPSHOT(drm_dev) msm_disp_snapshot_state(drm_dev)
+
+/**
+ * msm_disp_snapshot_init - initialize global sde debug facilities: evtlog, regdump
+ * @drm_dev:	drm device handle
+ *
+ * Returns:		0 or -ERROR
+ */
+int msm_disp_snapshot_init(struct drm_device *drm_dev);
+
+/**
+ * msm_disp_snapshot_destroy - destroy the global sde debug facilities
+ * @drm_dev:    drm device handle
+ *
+ * Returns:	none
+ */
+void msm_disp_snapshot_destroy(struct drm_device *drm_dev);
+
+/**
+ * msm_disp_snapshot_state - trigger to dump the display snapshot
+ * @drm_dev:	handle to drm device
+
+ * Returns:	none
+ */
+void msm_disp_snapshot_state(struct drm_device *drm_dev);
+
+/**
+ * msm_disp_state_get - get the handle to msm_disp_state struct from the drm device
+ * @drm:	    handle to drm device
+
+ * Returns:	handle to the msm_disp_state struct
+ */
+struct msm_disp_state *msm_disp_state_get(struct drm_device *drm);
+
+/**
+ * msm_disp_state_print - print out the current dpu state
+ * @disp_state:	    handle to drm device
+ * @p:	    handle to drm printer
+ * @reg_dump_method: tells whether the state has to be dumped in log or coredump
+ *
+ * Returns:	none
+ */
+void msm_disp_state_print(struct msm_disp_state *disp_state, struct drm_printer *p,
+		u8 reg_dump_method);
+
+/**
+ * msm_disp_snapshot_capture_state - utility to capture atomic state and hw registers
+ * @disp_state:	    handle to msm_disp_state struct
+
+ * Returns:	none
+ */
+void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state);
+
+/**
+ * msm_disp_state_free - free the memory after the coredump has been read
+ * @disp_state:	    handle to struct msm_disp_state
+
+ * Returns: none
+ */
+void msm_disp_state_free(struct msm_disp_state *disp_state);
+
+/**
+ * msm_disp_snapshot_add_block - add a hardware block with its register dump
+ * @disp_state:	    handle to struct msm_disp_state
+ * @name:           name of the hardware block
+ * @len:            size of the register space of the hardware block
+ * @base_addr:      starting address of the register space of the hardware block
+ *
+ * Returns: none
+ */
+void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, char *name, u32 len,
+		void __iomem *base_addr);
+
+#endif /* MSM_DISP_SNAPSHOT_H_ */
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
new file mode 100644
index 0000000..383f225
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
+ */
+
+#define pr_fmt(fmt)	"[drm:%s:%d] " fmt, __func__, __LINE__
+
+#include "msm_disp_snapshot.h"
+
+static void msm_disp_state_dump_regs(u32 **reg, u32 len, void __iomem *base_addr,
+		u32 dump_op, struct drm_printer *p)
+{
+	u32 len_aligned, len_padded;
+	u32 x0, x4, x8, xc;
+	void __iomem *addr;
+	u32 *dump_addr = NULL;
+	void __iomem *end_addr;
+	int i;
+	bool in_drm_printer = false;
+
+	len_aligned = (len + REG_DUMP_ALIGN - 1) / REG_DUMP_ALIGN;
+	len_padded = len_aligned * REG_DUMP_ALIGN;
+
+	addr = base_addr;
+	end_addr = base_addr + len;
+
+	if ((dump_op == MSM_DISP_SNAPSHOT_IN_COREDUMP) ||
+		(dump_op == MSM_DISP_SNAPSHOT_IN_LOG))
+		in_drm_printer = true;
+
+	if (in_drm_printer && !p) {
+		DRM_ERROR("invalid drm printer\n");
+		return;
+	}
+
+	if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM && !(*reg))
+		*reg = kzalloc(len_padded, GFP_KERNEL);
+
+	if (*reg)
+		dump_addr = *reg;
+
+	for (i = 0; i < len_aligned; i++) {
+		if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM) {
+			x0 = (addr < end_addr) ? readl_relaxed(addr + 0x0) : 0;
+			x4 = (addr + 0x4 < end_addr) ? readl_relaxed(addr + 0x4) : 0;
+			x8 = (addr + 0x8 < end_addr) ? readl_relaxed(addr + 0x8) : 0;
+			xc = (addr + 0xc < end_addr) ? readl_relaxed(addr + 0xc) : 0;
+		}
+
+		if (dump_addr) {
+			if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM) {
+				dump_addr[i * 4] = x0;
+				dump_addr[i * 4 + 1] = x4;
+				dump_addr[i * 4 + 2] = x8;
+				dump_addr[i * 4 + 3] = xc;
+			} else if (in_drm_printer) {
+				drm_printf(p, "0x%lx : %08x %08x %08x %08x\n",
+						(unsigned long)(addr - base_addr),
+						dump_addr[i * 4], dump_addr[i * 4 + 1],
+						dump_addr[i * 4 + 2], dump_addr[i * 4 + 3]);
+			}
+		}
+
+		addr += REG_DUMP_ALIGN;
+	}
+}
+
+struct msm_disp_state *msm_disp_state_get(struct drm_device *drm)
+{
+	struct msm_drm_private *priv;
+	struct dpu_kms *dpu_kms;
+
+	priv = drm->dev_private;
+	dpu_kms = to_dpu_kms(priv->kms);
+
+	return dpu_kms->disp_state;
+}
+
+void msm_disp_state_print(struct msm_disp_state *state, struct drm_printer *p,
+		u8 reg_dump_method)
+{
+	struct msm_disp_state_block *block, *tmp;
+
+	if (!p) {
+		DRM_ERROR("invalid drm printer\n");
+		return;
+	}
+
+	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(state->timestamp));
+
+	list_for_each_entry_safe(block, tmp, &state->blocks, node) {
+		drm_printf(p, "====================%s================\n", block->name);
+		msm_disp_state_dump_regs(&block->state, block->size, block->base_addr,
+				reg_dump_method, p);
+	}
+
+	drm_printf(p, "===================dpu drm state================\n");
+
+	if (state->atomic_state)
+		drm_atomic_print_new_state(state->atomic_state, p);
+}
+
+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();
+
+	ddev = disp_state->drm_dev;
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+	while (drm_modeset_lock_all_ctx(ddev, &ctx) != 0)
+		drm_modeset_backoff(&ctx);
+
+	disp_state->atomic_state = drm_atomic_helper_duplicate_state(ddev,
+			&ctx);
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+}
+
+void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
+{
+	struct msm_drm_private *priv;
+	int i;
+	struct drm_device *drm_dev;
+
+	drm_dev = disp_state->drm_dev;
+	priv = drm_dev->dev_private;
+
+	msm_disp_capture_atomic_state(disp_state);
+
+	if (priv->dp)
+		msm_dp_snapshot(priv->dp);
+
+	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
+		if (!priv->dsi[i])
+			continue;
+
+		msm_dsi_snapshot(priv->dsi[i]);
+	}
+
+	dpu_kms_mdp_snapshot(drm_dev);
+}
+
+void msm_disp_state_free(struct msm_disp_state *disp_state)
+{
+	struct msm_disp_state_block *block, *tmp;
+
+	if (disp_state->atomic_state) {
+		drm_atomic_state_put(disp_state->atomic_state);
+		disp_state->atomic_state = NULL;
+	}
+
+	list_for_each_entry_safe(block, tmp, &disp_state->blocks, node) {
+		kfree(block->state);
+		list_del(&block->node);
+		kfree(block);
+	}
+}
+
+void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, char *name, u32 len,
+		void __iomem *base_addr)
+{
+	struct msm_disp_state_block *new_blk;
+
+	new_blk = kzalloc(sizeof(struct msm_disp_state_block), GFP_KERNEL);
+
+	strscpy(new_blk->name, name, sizeof(new_blk->name));
+	INIT_LIST_HEAD(&new_blk->node);
+	new_blk->size = len;
+	new_blk->base_addr = base_addr;
+	msm_disp_state_dump_regs(&new_blk->state, len, base_addr,
+			MSM_DISP_SNAPSHOT_IN_MEM, NULL);
+	list_add(&new_blk->node, &disp_state->blocks);
+}
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index b1a9b1b..ce8d19a 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -62,6 +62,18 @@ struct dp_catalog_private {
 	u8 aux_lut_cfg_index[PHY_AUX_CFG_MAX];
 };
 
+void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct msm_disp_state *disp_state)
+{
+	struct dp_catalog_private *catalog = container_of(dp_catalog,
+			struct dp_catalog_private, dp_catalog);
+	char name[SZ_128];
+
+	snprintf(name, SZ_128, "dp_ctrl");
+
+	msm_disp_snapshot_add_block(disp_state, name, catalog->io->dp_controller.len,
+			catalog->io->dp_controller.base);
+}
+
 static inline u32 dp_read_aux(struct dp_catalog_private *catalog, u32 offset)
 {
 	offset += MSM_DP_CONTROLLER_AUX_OFFSET;
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 176a902..e7e8b13 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -9,6 +9,7 @@
 #include <drm/drm_modes.h>
 
 #include "dp_parser.h"
+#include "disp/msm_disp_snapshot.h"
 
 /* interrupts */
 #define DP_INTR_HPD		BIT(0)
@@ -71,6 +72,9 @@ struct dp_catalog {
 	u32 audio_data;
 };
 
+/* Debug module */
+void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct msm_disp_state *disp_state);
+
 /* AUX APIs */
 u32 dp_catalog_aux_read_data(struct dp_catalog *dp_catalog);
 int dp_catalog_aux_write_data(struct dp_catalog *dp_catalog);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 5a39da6..6670558 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1009,6 +1009,35 @@ int dp_display_get_test_bpp(struct msm_dp *dp)
 		dp_display->link->test_video.test_bit_depth);
 }
 
+void msm_dp_snapshot(struct msm_dp *dp)
+{
+	struct dp_display_private *dp_display;
+	struct drm_device *drm;
+	struct msm_disp_state *disp_state;
+
+	dp_display = container_of(dp, struct dp_display_private, dp_display);
+	drm = dp->drm_dev;
+	disp_state = msm_disp_state_get(drm);
+
+	/*
+	 * if we are reading registers we need the link clocks to be on
+	 * however till DP cable is connected this will not happen as we
+	 * do not know the resolution to power up with. Hence check the
+	 * power_on status before dumping DP registers to avoid crash due
+	 * to unclocked access
+	 */
+	mutex_lock(&dp_display->event_mutex);
+
+	if (!dp->power_on) {
+		mutex_unlock(&dp_display->event_mutex);
+		return;
+	}
+
+	dp_catalog_snapshot(dp_display->catalog, disp_state);
+
+	mutex_unlock(&dp_display->event_mutex);
+}
+
 static void dp_display_config_hpd(struct dp_display_private *dp)
 {
 
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 6092ba1..4d39373 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -8,6 +8,7 @@
 
 #include "dp_panel.h"
 #include <sound/hdmi-codec.h>
+#include "disp/msm_disp_snapshot.h"
 
 struct msm_dp {
 	struct drm_device *drm_dev;
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 62704885..bccc006 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -266,3 +266,8 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 	return ret;
 }
 
+void msm_dsi_snapshot(struct msm_dsi *msm_dsi)
+{
+	msm_dsi_host_snapshot(msm_dsi->host);
+}
+
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 7abfeab..bb39403 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -15,6 +15,7 @@
 #include <drm/drm_panel.h>
 
 #include "msm_drv.h"
+#include "disp/msm_disp_snapshot.h"
 
 #define DSI_0	0
 #define DSI_1	1
@@ -90,6 +91,8 @@ static inline bool msm_dsi_device_connected(struct msm_dsi *msm_dsi)
 	return msm_dsi->panel || msm_dsi->external_bridge;
 }
 
+void msm_dsi_snapshot(struct msm_dsi *msm_dsi);
+
 struct drm_encoder *msm_dsi_get_encoder(struct msm_dsi *msm_dsi);
 
 /* dsi host */
@@ -146,6 +149,7 @@ int dsi_clk_init_v2(struct msm_dsi_host *msm_host);
 int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host);
 int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_dual_dsi);
 int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_dual_dsi);
+void msm_dsi_host_snapshot(struct mipi_dsi_host *host);
 
 /* dsi phy */
 struct msm_dsi_phy;
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 8a10e43..d9fdc07 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -2487,3 +2487,22 @@ 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_snapshot(struct mipi_dsi_host *host)
+{
+	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
+	struct drm_device *dev = msm_host->dev;
+	struct msm_disp_state *disp_state;
+	char name[SZ_128];
+
+	disp_state = msm_disp_state_get(dev);
+
+	pm_runtime_get_sync(&msm_host->pdev->dev);
+
+	snprintf(name, SZ_128, "dsi%d_ctrl", msm_host->id);
+
+	msm_disp_snapshot_add_block(disp_state, name, msm_iomap_size(msm_host->pdev, "dsi_ctrl"),
+			msm_host->ctrl_base);
+
+	pm_runtime_put_sync(&msm_host->pdev->dev);
+}
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index e1104d2..ebf9283 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2016-2018, 2020-2021 The Linux Foundation. All rights reserved.
  * Copyright (C) 2013 Red Hat
  * Author: Rob Clark <robdclark@gmail.com>
  */
@@ -19,6 +19,7 @@
 #include <drm/drm_of.h>
 #include <drm/drm_vblank.h>
 
+#include "disp/msm_disp_snapshot.h"
 #include "msm_drv.h"
 #include "msm_debugfs.h"
 #include "msm_fence.h"
@@ -167,6 +168,24 @@ void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name,
 	return _msm_ioremap(pdev, name, dbgname, true);
 }
 
+unsigned long msm_iomap_size(struct platform_device *pdev, const char *name)
+{
+	struct resource *res;
+
+	if (name)
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
+	else
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	if (!res) {
+		dev_dbg(&pdev->dev, "failed to get memory resource: %s\n",
+				name);
+		return 0;
+	}
+
+	return resource_size(res);
+}
+
 void msm_writel(u32 data, void __iomem *addr)
 {
 	if (reglog)
@@ -278,6 +297,8 @@ static int msm_drm_uninit(struct device *dev)
 		msm_fbdev_free(ddev);
 #endif
 
+	msm_disp_snapshot_destroy(ddev);
+
 	drm_mode_config_cleanup(ddev);
 
 	pm_runtime_get_sync(dev);
@@ -550,6 +571,12 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 	if (ret)
 		goto err_msm_uninit;
 
+	if (get_mdp_ver(pdev) == KMS_DPU) {
+		ret = msm_disp_snapshot_init(ddev);
+		if (ret)
+			DRM_DEV_ERROR(dev, "msm_disp_snapshot_init failed ret = %d\n", ret);
+	}
+
 	drm_mode_config_reset(ddev);
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 2668941..9c40bac 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -367,6 +367,7 @@ void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,
 				struct drm_display_mode *mode,
 				struct drm_display_mode *adjusted_mode);
 void msm_dp_irq_postinstall(struct msm_dp *dp_display);
+void msm_dp_snapshot(struct msm_dp *dp_display);
 
 void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
 
@@ -450,6 +451,7 @@ void __iomem *msm_ioremap(struct platform_device *pdev, const char *name,
 		const char *dbgname);
 void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name,
 		const char *dbgname);
+unsigned long msm_iomap_size(struct platform_device *pdev, const char *name);
 void msm_writel(u32 data, void __iomem *addr);
 u32 msm_readl(const void __iomem *addr);
 void msm_rmw(void __iomem *addr, u32 mask, u32 or);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 2/3] drm/msm: add support to take dpu snapshot

[Dmitry Baryshkov]

On 15/04/2021 02:11, Abhinav Kumar wrote:
> Add the msm_disp_snapshot module which adds supports to dump dpu
> registers and capture the drm atomic state which can be used in
> case of error conditions.
> 
> changes in v4:
>   - rename dpu_dbg to msm_disp_snapshot and move it to msm/disp
>   - start using a list of blocks to store the hardware block information
>   - cleanup block allocation and freeing logic to simplify it
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>   drivers/gpu/drm/msm/Makefile                      |   2 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    |   2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c           |  61 ++++++++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h           |   5 +
>   drivers/gpu/drm/msm/disp/msm_disp_snapshot.c      | 161 +++++++++++++++++++
>   drivers/gpu/drm/msm/disp/msm_disp_snapshot.h      | 167 ++++++++++++++++++++
>   drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 181 ++++++++++++++++++++++
>   drivers/gpu/drm/msm/dp/dp_catalog.c               |  12 ++
>   drivers/gpu/drm/msm/dp/dp_catalog.h               |   4 +
>   drivers/gpu/drm/msm/dp/dp_display.c               |  29 ++++
>   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                |  19 +++
>   drivers/gpu/drm/msm/msm_drv.c                     |  29 +++-
>   drivers/gpu/drm/msm/msm_drv.h                     |   2 +

Generic suggestion: Could you please split this patch, separating core 
snapshotting functionality, dpu1, dsi and dp changes? This would ease 
reviewing.

>   16 files changed, 682 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
>   create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
>   create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> 
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index 610d630..65d86ce 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -77,6 +77,8 @@ msm-y := \
>   	disp/dpu1/dpu_plane.o \
>   	disp/dpu1/dpu_rm.o \
>   	disp/dpu1/dpu_vbif.o \
> +	disp/msm_disp_snapshot.o \
> +	disp/msm_disp_snapshot_util.o \

If snapshotting is going to be used by dsi/dp, what about landing it 
into the top drivers/gpu/drm/msm dir? This way we'd point that it is a 
generic piece of code shared between all drm/msm drivers.

>   	msm_atomic.o \
>   	msm_atomic_tracepoints.o \
>   	msm_debugfs.o \
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> index 4dfd8a2..0f9f0a5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -1,5 +1,5 @@
>   /* SPDX-License-Identifier: GPL-2.0-only */
> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> +/* Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights reserved.
>    */
>   
>   #ifndef _DPU_HW_CATALOG_H
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 88e9cc3..7529566 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -798,6 +798,67 @@ static void dpu_irq_uninstall(struct msm_kms *kms)
>   	dpu_core_irq_uninstall(dpu_kms);
>   }
>   
> +void dpu_kms_mdp_snapshot(struct drm_device *dev)
> +{
> +	int i;
> +	struct msm_drm_private *priv;
> +	struct dpu_kms *dpu_kms;
> +	struct dpu_mdss_cfg *cat;
> +	struct dpu_hw_mdp *top;
> +	struct msm_disp_state *disp_state;
> +	char name[SZ_128];
> +
> +	priv = dev->dev_private;
> +	dpu_kms = to_dpu_kms(priv->kms);
> +	disp_state = dpu_kms->disp_state;
> +
> +	cat = dpu_kms->catalog;
> +	top = dpu_kms->hw_mdp;
> +
> +	pm_runtime_get_sync(&dpu_kms->pdev->dev);
> +
> +	/* dump CTL sub-blocks HW regs info */
> +	for (i = 0; i < cat->ctl_count; i++) {
> +		snprintf(name, SZ_128, "ctl_%d", i);

snprintf(name, sizeof(name), "ctl_%d", i);
(the same bellow)

> +		msm_disp_snapshot_add_block(disp_state, name, cat->ctl[i].len,
> +				dpu_kms->mmio + cat->ctl[i].base);
> +	}
> +
> +	/* dump DSPP sub-blocks HW regs info */
> +	for (i = 0; i < cat->dspp_count; i++) {
> +		snprintf(name, SZ_128, "dspp_%d", i);
> +		msm_disp_snapshot_add_block(disp_state, name, cat->dspp[i].len,
> +				dpu_kms->mmio + cat->dspp[i].base);
> +	}
> +
> +	/* dump INTF sub-blocks HW regs info */
> +	for (i = 0; i < cat->intf_count; i++) {
> +		snprintf(name, SZ_128, "intf_%d", i);
> +		msm_disp_snapshot_add_block(disp_state, name, cat->intf[i].len,
> +				dpu_kms->mmio + cat->intf[i].base);
> +	}
> +
> +	/* dump PP sub-blocks HW regs info */
> +	for (i = 0; i < cat->pingpong_count; i++) {
> +		snprintf(name, SZ_128, "ping-pong_%d", i);

Just a nitpick: we usually call it pp or pingpong, without dash.

> +		msm_disp_snapshot_add_block(disp_state, name, cat->pingpong[i].len,
> +				dpu_kms->mmio + cat->pingpong[i].base);
> +	}
> +
> +	/* dump SSPP sub-blocks HW regs info */
> +	for (i = 0; i < cat->sspp_count; i++) {
> +		snprintf(name, SZ_128, "sspp_%d", i);
> +		msm_disp_snapshot_add_block(disp_state, name, cat->sspp[i].len,
> +				dpu_kms->mmio + cat->sspp[i].base);
> +	}
> +
> +	snprintf(name, SZ_128, "top");
> +	msm_disp_snapshot_add_block(disp_state, name, top->hw.length,
> +			dpu_kms->mmio + top->hw.blk_off);
> +
> +	pm_runtime_put_sync(&dpu_kms->pdev->dev);
> +}
> +
>   static const struct msm_kms_funcs kms_funcs = {
>   	.hw_init         = dpu_kms_hw_init,
>   	.irq_preinstall  = dpu_irq_preinstall,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index d6717d6..74b6ce6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -13,6 +13,7 @@
>   #include <drm/drm_drv.h>
>   
>   #include "msm_drv.h"
> +#include "disp/msm_disp_snapshot.h"
>   #include "msm_kms.h"
>   #include "msm_mmu.h"
>   #include "msm_gem.h"
> @@ -132,6 +133,8 @@ struct dpu_kms {
>   
>   	struct opp_table *opp_table;
>   
> +	struct msm_disp_state *disp_state;
> +
>   	struct dss_module_power mp;
>   
>   	/* reference count bandwidth requests, so we know when we can
> @@ -265,4 +268,6 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder);
>    */
>   u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name);
>   
> +void dpu_kms_mdp_snapshot(struct drm_device *dev);
> +
>   #endif /* __dpu_kms_H__ */
> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> new file mode 100644
> index 0000000..1bdad16
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> @@ -0,0 +1,161 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
> + */
> +
> +#define pr_fmt(fmt)	"[drm:%s:%d] " fmt, __func__, __LINE__
> +
> +#include "msm_disp_snapshot.h"
> +
> +#ifdef CONFIG_DEV_COREDUMP
> +static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
> +		size_t count, void *data, size_t datalen)
> +{
> +	struct drm_print_iterator iter;
> +	struct drm_printer p;
> +	struct msm_disp_state *disp_state;
> +
> +	disp_state = data;
> +
> +	iter.data = buffer;
> +	iter.offset = 0;
> +	iter.start = offset;
> +	iter.remain = count;
> +
> +	p = drm_coredump_printer(&iter);
> +
> +	msm_disp_state_print(disp_state, &p, MSM_DISP_SNAPSHOT_IN_COREDUMP);
> +
> +	return count - iter.remain;
> +}
> +
> +static void disp_devcoredump_free(void *data)
> +{
> +	struct msm_disp_state *disp_state;
> +
> +	disp_state = data;
> +
> +	msm_disp_state_free(disp_state);
> +
> +	disp_state->coredump_pending = false;
> +}
> +#endif /* CONFIG_DEV_COREDUMP */
> +
> +static void _msm_disp_snapshot_work(struct kthread_work *work)
> +{
> +	struct msm_disp_state *disp_state = container_of(work, struct msm_disp_state, dump_work);
> +	struct drm_printer p;
> +
> +	mutex_lock(&disp_state->mutex);
> +
> +	msm_disp_snapshot_capture_state(disp_state);
> +
> +	if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
> +		p = drm_info_printer(disp_state->drm_dev->dev);
> +		msm_disp_state_print(disp_state, &p, MSM_DISP_SNAPSHOT_IN_LOG);
> +	}
> +
> +	/*
> +	 * if devcoredump is not defined free the state immediately
> +	 * otherwise it will be freed in the free handler.
> +	 */
> +#ifdef CONFIG_DEV_COREDUMP
> +	dev_coredumpm(disp_state->dev, THIS_MODULE, disp_state, 0, GFP_KERNEL,
> +			disp_devcoredump_read, disp_devcoredump_free);
> +	disp_state->coredump_pending = true;
> +#else
> +	msm_disp_state_free(disp_state);
> +#endif
> +
> +	mutex_unlock(&disp_state->mutex);
> +}
> +
> +void msm_disp_snapshot_state(struct drm_device *drm_dev)
> +{
> +	struct msm_drm_private *priv;
> +	struct dpu_kms *dpu_kms;
> +	struct msm_disp_state *disp_state;
> +
> +	if (!drm_dev) {
> +		DRM_ERROR("invalid params\n");
> +		return;
> +	}
> +
> +	priv = drm_dev->dev_private;
> +	dpu_kms = to_dpu_kms(priv->kms);

If we are outside of disp/dpu1, we shouldn't reference dpu_kms 
internals. Probably this can be changed to use msm_drm_private, msm_kms 
or msm_mdss?

> +	disp_state = dpu_kms->disp_state;
> +
> +	if (!disp_state) {
> +		DRM_ERROR("invalid params\n");
> +		return;
> +	}
> +
> +	/*
> +	 * if there is a coredump pending return immediately till dump
> +	 * if read by userspace or timeout happens

Where do we handle the timeout? Stale comment or did I miss something?

> +	 */
> +	if (disp_state->coredump_pending) {
> +		DRM_DEBUG("coredump is pending read\n");
> +		return;
> +	}
> +
> +	kthread_queue_work(disp_state->dump_worker,
> +			&disp_state->dump_work);
> +}
> +
> +int msm_disp_snapshot_init(struct drm_device *drm_dev)
> +{
> +	struct dpu_kms *dpu_kms;
> +	struct msm_drm_private *priv;
> +	struct msm_disp_state *disp_state;
> +
> +	if (!drm_dev) {
> +		DRM_ERROR("invalid params\n");
> +		return -EINVAL;
> +	}
> +
> +	priv = drm_dev->dev_private;
> +	dpu_kms = to_dpu_kms(priv->kms);
> +
> +	disp_state = devm_kzalloc(drm_dev->dev, sizeof(struct msm_disp_state), GFP_KERNEL);
> +
> +	mutex_init(&disp_state->mutex);
> +
> +	disp_state->dev = drm_dev->dev;
> +	disp_state->drm_dev = drm_dev;
> +
> +	INIT_LIST_HEAD(&disp_state->blocks);
> +
> +	disp_state->dump_worker = kthread_create_worker(0, "%s", "disp_snapshot");
> +	if (IS_ERR(disp_state->dump_worker))
> +		DRM_ERROR("failed to create disp state task\n");
> +
> +	kthread_init_work(&disp_state->dump_work, _msm_disp_snapshot_work);
> +
> +	dpu_kms->disp_state = disp_state;

Judging from this code, I think it should be possible to push disp_state 
directly into struct msm_drm_private.

> +
> +	return 0;
> +}
> +
> +void msm_disp_snapshot_destroy(struct drm_device *drm_dev)
> +{
> +	struct dpu_kms *dpu_kms;
> +	struct msm_drm_private *priv;
> +	struct msm_disp_state *disp_state;
> +
> +	if (!drm_dev) {
> +		DRM_ERROR("invalid params\n");
> +		return;
> +	}
> +
> +	priv = drm_dev->dev_private;
> +	dpu_kms = to_dpu_kms(priv->kms);
> +	disp_state = dpu_kms->disp_state;
> +
> +	if (disp_state->dump_worker)
> +		kthread_destroy_worker(disp_state->dump_worker);
> +
> +	list_del(&disp_state->blocks);
> +
> +	mutex_destroy(&disp_state->mutex);
> +}
> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
> new file mode 100644
> index 0000000..0ccb7e7
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
> @@ -0,0 +1,167 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef MSM_DISP_SNAPSHOT_H_
> +#define MSM_DISP_SNAPSHOT_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"
> +
> +enum msm_disp_snapshot_dump_flag {
> +	MSM_DISP_SNAPSHOT_IN_LOG = BIT(0),
> +	MSM_DISP_SNAPSHOT_IN_MEM = BIT(1),
> +	MSM_DISP_SNAPSHOT_IN_COREDUMP = BIT(2),

Are they mutually exclusive or can they be specified in parallel?	

> +};
> +
> +#define MSM_DISP_SNAPSHOT_MAX_BLKS		10
> +
> +/* debug option to print the registers in logs */
> +#define MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE 0
> +
> +/* print debug ranges in groups of 4 u32s */
> +#define REG_DUMP_ALIGN		16
> +
> +/**
> + * struct msm_disp_state - structure to store current dpu state
> + * @dev: device pointer
> + * @drm_dev: drm device pointer
> + * @mutex: mutex to serialize access to serialze dumps, debugfs access
> + * @coredump_pending: coredump is pending read from userspace
> + * @atomic_state: atomic state duplicated at the time of the error
> + * @dump_worker: kworker thread which runs the dump work
> + * @dump_work: kwork which dumps the registers and drm state
> + * @timestamp: timestamp at which the coredump was captured
> + */
> +struct msm_disp_state {
> +	struct device *dev;
> +	struct drm_device *drm_dev;
> +	struct mutex mutex;
> +
> +	bool coredump_pending;
> +
> +	struct list_head blocks;
> +
> +	struct drm_atomic_state *atomic_state;
> +
> +	struct kthread_worker *dump_worker;
> +	struct kthread_work dump_work;
> +	ktime_t timestamp;
> +};
> +
> +/**
> + * struct msm_disp_state_block - structure to store each hardware block state
> + * @name: name of the block
> + * @drm_dev: handle to the linked list head
> + * @size: size of the register space of this hardware block
> + * @state: array holding the register dump of this hardware block
> + * @base_addr: starting address of this hardware block's register space
> + */
> +struct msm_disp_state_block {
> +	char name[SZ_128];
> +	struct list_head node;
> +	unsigned int size;
> +	u32 *state;
> +	void __iomem *base_addr;
> +};
> +
> +/**
> + * MSM_DISP_SNAPSHOT - trigger to dump the display snapshot
> + * @drm_dev:	handle to drm device
> + */
> +#define MSM_DISP_SNAPSHOT(drm_dev) msm_disp_snapshot_state(drm_dev)

Why do you need it?

> +
> +/**
> + * msm_disp_snapshot_init - initialize global sde debug facilities: evtlog, regdump

Just a nitpick here and below: there is no sde in the upstream. And no 
evtlog.

> + * @drm_dev:	drm device handle
> + *
> + * Returns:		0 or -ERROR
> + */
> +int msm_disp_snapshot_init(struct drm_device *drm_dev);
> +
> +/**
> + * msm_disp_snapshot_destroy - destroy the global sde debug facilities
> + * @drm_dev:    drm device handle
> + *
> + * Returns:	none
> + */
> +void msm_disp_snapshot_destroy(struct drm_device *drm_dev);
> +
> +/**
> + * msm_disp_snapshot_state - trigger to dump the display snapshot
> + * @drm_dev:	handle to drm device
> +
> + * Returns:	none
> + */
> +void msm_disp_snapshot_state(struct drm_device *drm_dev);
> +
> +/**
> + * msm_disp_state_get - get the handle to msm_disp_state struct from the drm device
> + * @drm:	    handle to drm device
> +
> + * Returns:	handle to the msm_disp_state struct
> + */
> +struct msm_disp_state *msm_disp_state_get(struct drm_device *drm);
> +
> +/**
> + * msm_disp_state_print - print out the current dpu state
> + * @disp_state:	    handle to drm device
> + * @p:	    handle to drm printer
> + * @reg_dump_method: tells whether the state has to be dumped in log or coredump
> + *
> + * Returns:	none
> + */
> +void msm_disp_state_print(struct msm_disp_state *disp_state, struct drm_printer *p,
> +		u8 reg_dump_method);
> +
> +/**
> + * msm_disp_snapshot_capture_state - utility to capture atomic state and hw registers
> + * @disp_state:	    handle to msm_disp_state struct
> +
> + * Returns:	none
> + */
> +void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state);
> +
> +/**
> + * msm_disp_state_free - free the memory after the coredump has been read
> + * @disp_state:	    handle to struct msm_disp_state
> +
> + * Returns: none
> + */
> +void msm_disp_state_free(struct msm_disp_state *disp_state);
> +
> +/**
> + * msm_disp_snapshot_add_block - add a hardware block with its register dump
> + * @disp_state:	    handle to struct msm_disp_state
> + * @name:           name of the hardware block
> + * @len:            size of the register space of the hardware block
> + * @base_addr:      starting address of the register space of the hardware block
> + *
> + * Returns: none
> + */
> +void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, char *name, u32 len,
> +		void __iomem *base_addr);
> +
> +#endif /* MSM_DISP_SNAPSHOT_H_ */
> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> new file mode 100644
> index 0000000..383f225
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
> + */
> +
> +#define pr_fmt(fmt)	"[drm:%s:%d] " fmt, __func__, __LINE__
> +
> +#include "msm_disp_snapshot.h"
> +
> +static void msm_disp_state_dump_regs(u32 **reg, u32 len, void __iomem *base_addr,
> +		u32 dump_op, struct drm_printer *p)
> +{
> +	u32 len_aligned, len_padded;
> +	u32 x0, x4, x8, xc;
> +	void __iomem *addr;
> +	u32 *dump_addr = NULL;
> +	void __iomem *end_addr;
> +	int i;
> +	bool in_drm_printer = false;
> +
> +	len_aligned = (len + REG_DUMP_ALIGN - 1) / REG_DUMP_ALIGN;
> +	len_padded = len_aligned * REG_DUMP_ALIGN;
> +
> +	addr = base_addr;
> +	end_addr = base_addr + len;
> +
> +	if ((dump_op == MSM_DISP_SNAPSHOT_IN_COREDUMP) ||
> +		(dump_op == MSM_DISP_SNAPSHOT_IN_LOG))
> +		in_drm_printer = true;
> +
> +	if (in_drm_printer && !p) {
> +		DRM_ERROR("invalid drm printer\n");
> +		return;
> +	}
> +
> +	if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM && !(*reg))
> +		*reg = kzalloc(len_padded, GFP_KERNEL);
> +
> +	if (*reg)
> +		dump_addr = *reg;
> +
> +	for (i = 0; i < len_aligned; i++) {
> +		if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM) {
> +			x0 = (addr < end_addr) ? readl_relaxed(addr + 0x0) : 0;
> +			x4 = (addr + 0x4 < end_addr) ? readl_relaxed(addr + 0x4) : 0;
> +			x8 = (addr + 0x8 < end_addr) ? readl_relaxed(addr + 0x8) : 0;
> +			xc = (addr + 0xc < end_addr) ? readl_relaxed(addr + 0xc) : 0;
> +		}
> +
> +		if (dump_addr) {
> +			if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM) {
> +				dump_addr[i * 4] = x0;
> +				dump_addr[i * 4 + 1] = x4;
> +				dump_addr[i * 4 + 2] = x8;
> +				dump_addr[i * 4 + 3] = xc;
> +			} else if (in_drm_printer) {
> +				drm_printf(p, "0x%lx : %08x %08x %08x %08x\n",
> +						(unsigned long)(addr - base_addr),
> +						dump_addr[i * 4], dump_addr[i * 4 + 1],
> +						dump_addr[i * 4 + 2], dump_addr[i * 4 + 3]);
> +			}
> +		}
> +
> +		addr += REG_DUMP_ALIGN;
> +	}
> +}

I'd just split this function into two different cases, IN_MEM and 
DRM_PRINTER.

> +
> +struct msm_disp_state *msm_disp_state_get(struct drm_device *drm)
> +{
> +	struct msm_drm_private *priv;
> +	struct dpu_kms *dpu_kms;
> +
> +	priv = drm->dev_private;
> +	dpu_kms = to_dpu_kms(priv->kms);
> +
> +	return dpu_kms->disp_state;
> +}
> +
> +void msm_disp_state_print(struct msm_disp_state *state, struct drm_printer *p,
> +		u8 reg_dump_method)
> +{
> +	struct msm_disp_state_block *block, *tmp;
> +
> +	if (!p) {
> +		DRM_ERROR("invalid drm printer\n");
> +		return;
> +	}
> +
> +	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(state->timestamp));
> +
> +	list_for_each_entry_safe(block, tmp, &state->blocks, node) {
> +		drm_printf(p, "====================%s================\n", block->name);
> +		msm_disp_state_dump_regs(&block->state, block->size, block->base_addr,
> +				reg_dump_method, p);
> +	}
> +
> +	drm_printf(p, "===================dpu drm state================\n");
> +
> +	if (state->atomic_state)
> +		drm_atomic_print_new_state(state->atomic_state, p);
> +}
> +
> +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();
> +
> +	ddev = disp_state->drm_dev;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	while (drm_modeset_lock_all_ctx(ddev, &ctx) != 0)
> +		drm_modeset_backoff(&ctx);
> +
> +	disp_state->atomic_state = drm_atomic_helper_duplicate_state(ddev,
> +			&ctx);
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}
> +
> +void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
> +{
> +	struct msm_drm_private *priv;
> +	int i;
> +	struct drm_device *drm_dev;
> +
> +	drm_dev = disp_state->drm_dev;
> +	priv = drm_dev->dev_private;
> +
> +	msm_disp_capture_atomic_state(disp_state);
> +
> +	if (priv->dp)
> +		msm_dp_snapshot(priv->dp);
> +
> +	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
> +		if (!priv->dsi[i])
> +			continue;
> +
> +		msm_dsi_snapshot(priv->dsi[i]);
> +	}
> +
> +	dpu_kms_mdp_snapshot(drm_dev);

Hmm, I now see why do you need the dpu_kms. Can we have a check for the 
mdp version here? The rest of the code seems generic enough, so we can 
at least dump the dsi/dp (and hopefully eDP/HDMI) data for MDP4/5.

Ideally subdrivers should be able to report back to the core, which 
regions are available, so that we won't have this static code. But this 
might look like an overkill. Not to mention that it might lead to the 
not-so-static sequence of dumps. (As you can judge, I quite liked the 
previous approach, when the drivers registered the dumping regions and 
then the dumper would go through them.)

Let's have the static config in place with the version check.

> +}
> +
> +void msm_disp_state_free(struct msm_disp_state *disp_state)
> +{
> +	struct msm_disp_state_block *block, *tmp;
> +
> +	if (disp_state->atomic_state) {
> +		drm_atomic_state_put(disp_state->atomic_state);
> +		disp_state->atomic_state = NULL;
> +	}
> +
> +	list_for_each_entry_safe(block, tmp, &disp_state->blocks, node) {
> +		kfree(block->state);
> +		list_del(&block->node);

I'd swap these two lines, so while the block is in the list, its state 
is coherent.

> +		kfree(block);
> +	}
> +}
> +
> +void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, char *name, u32 len,
> +		void __iomem *base_addr)
> +{
> +	struct msm_disp_state_block *new_blk;
> +
> +	new_blk = kzalloc(sizeof(struct msm_disp_state_block), GFP_KERNEL);
> +
> +	strscpy(new_blk->name, name, sizeof(new_blk->name));
> +	INIT_LIST_HEAD(&new_blk->node);
> +	new_blk->size = len;
> +	new_blk->base_addr = base_addr;
> +	msm_disp_state_dump_regs(&new_blk->state, len, base_addr,
> +			MSM_DISP_SNAPSHOT_IN_MEM, NULL);
> +	list_add(&new_blk->node, &disp_state->blocks);
> +}
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index b1a9b1b..ce8d19a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -62,6 +62,18 @@ struct dp_catalog_private {
>   	u8 aux_lut_cfg_index[PHY_AUX_CFG_MAX];
>   };
>   
> +void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct msm_disp_state *disp_state)
> +{
> +	struct dp_catalog_private *catalog = container_of(dp_catalog,
> +			struct dp_catalog_private, dp_catalog);
> +	char name[SZ_128];
> +
> +	snprintf(name, SZ_128, "dp_ctrl");
> +
> +	msm_disp_snapshot_add_block(disp_state, name, catalog->io->dp_controller.len,
> +			catalog->io->dp_controller.base);
> +}
> +
>   static inline u32 dp_read_aux(struct dp_catalog_private *catalog, u32 offset)
>   {
>   	offset += MSM_DP_CONTROLLER_AUX_OFFSET;
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
> index 176a902..e7e8b13 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -9,6 +9,7 @@
>   #include <drm/drm_modes.h>
>   
>   #include "dp_parser.h"
> +#include "disp/msm_disp_snapshot.h"
>   
>   /* interrupts */
>   #define DP_INTR_HPD		BIT(0)
> @@ -71,6 +72,9 @@ struct dp_catalog {
>   	u32 audio_data;
>   };
>   
> +/* Debug module */
> +void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct msm_disp_state *disp_state);
> +
>   /* AUX APIs */
>   u32 dp_catalog_aux_read_data(struct dp_catalog *dp_catalog);
>   int dp_catalog_aux_write_data(struct dp_catalog *dp_catalog);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 5a39da6..6670558 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1009,6 +1009,35 @@ int dp_display_get_test_bpp(struct msm_dp *dp)
>   		dp_display->link->test_video.test_bit_depth);
>   }
>   
> +void msm_dp_snapshot(struct msm_dp *dp)
> +{
> +	struct dp_display_private *dp_display;
> +	struct drm_device *drm;
> +	struct msm_disp_state *disp_state;
> +
> +	dp_display = container_of(dp, struct dp_display_private, dp_display);
> +	drm = dp->drm_dev;
> +	disp_state = msm_disp_state_get(drm);
> +
> +	/*
> +	 * if we are reading registers we need the link clocks to be on
> +	 * however till DP cable is connected this will not happen as we
> +	 * do not know the resolution to power up with. Hence check the
> +	 * power_on status before dumping DP registers to avoid crash due
> +	 * to unclocked access
> +	 */
> +	mutex_lock(&dp_display->event_mutex);
> +
> +	if (!dp->power_on) {
> +		mutex_unlock(&dp_display->event_mutex);
> +		return;
> +	}
> +
> +	dp_catalog_snapshot(dp_display->catalog, disp_state);
> +
> +	mutex_unlock(&dp_display->event_mutex);
> +}
> +
>   static void dp_display_config_hpd(struct dp_display_private *dp)
>   {
>   
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> index 6092ba1..4d39373 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> @@ -8,6 +8,7 @@
>   
>   #include "dp_panel.h"
>   #include <sound/hdmi-codec.h>
> +#include "disp/msm_disp_snapshot.h"
>   
>   struct msm_dp {
>   	struct drm_device *drm_dev;
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
> index 62704885..bccc006 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> @@ -266,3 +266,8 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
>   	return ret;
>   }
>   
> +void msm_dsi_snapshot(struct msm_dsi *msm_dsi)
> +{
> +	msm_dsi_host_snapshot(msm_dsi->host);
> +}
> +
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index 7abfeab..bb39403 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -15,6 +15,7 @@
>   #include <drm/drm_panel.h>
>   
>   #include "msm_drv.h"
> +#include "disp/msm_disp_snapshot.h"
>   
>   #define DSI_0	0
>   #define DSI_1	1
> @@ -90,6 +91,8 @@ static inline bool msm_dsi_device_connected(struct msm_dsi *msm_dsi)
>   	return msm_dsi->panel || msm_dsi->external_bridge;
>   }
>   
> +void msm_dsi_snapshot(struct msm_dsi *msm_dsi);
> +
>   struct drm_encoder *msm_dsi_get_encoder(struct msm_dsi *msm_dsi);
>   
>   /* dsi host */
> @@ -146,6 +149,7 @@ int dsi_clk_init_v2(struct msm_dsi_host *msm_host);
>   int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host);
>   int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_dual_dsi);
>   int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_dual_dsi);
> +void msm_dsi_host_snapshot(struct mipi_dsi_host *host);
>   
>   /* dsi phy */
>   struct msm_dsi_phy;
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 8a10e43..d9fdc07 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -2487,3 +2487,22 @@ 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_snapshot(struct mipi_dsi_host *host)
> +{
> +	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
> +	struct drm_device *dev = msm_host->dev;
> +	struct msm_disp_state *disp_state;
> +	char name[SZ_128];
> +
> +	disp_state = msm_disp_state_get(dev);
> +
> +	pm_runtime_get_sync(&msm_host->pdev->dev);
> +
> +	snprintf(name, SZ_128, "dsi%d_ctrl", msm_host->id);
> +
> +	msm_disp_snapshot_add_block(disp_state, name, msm_iomap_size(msm_host->pdev, "dsi_ctrl"),

You can store the size into the msm_host, rather than querrying it on 
each dump.

> +			msm_host->ctrl_base);
> +
> +	pm_runtime_put_sync(&msm_host->pdev->dev);
> +}
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index e1104d2..ebf9283 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1,6 +1,6 @@
>   // SPDX-License-Identifier: GPL-2.0-only
>   /*
> - * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2016-2018, 2020-2021 The Linux Foundation. All rights reserved.
>    * Copyright (C) 2013 Red Hat
>    * Author: Rob Clark <robdclark@gmail.com>
>    */
> @@ -19,6 +19,7 @@
>   #include <drm/drm_of.h>
>   #include <drm/drm_vblank.h>
>   
> +#include "disp/msm_disp_snapshot.h"
>   #include "msm_drv.h"
>   #include "msm_debugfs.h"
>   #include "msm_fence.h"
> @@ -167,6 +168,24 @@ void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name,
>   	return _msm_ioremap(pdev, name, dbgname, true);
>   }
>   
> +unsigned long msm_iomap_size(struct platform_device *pdev, const char *name)
> +{
> +	struct resource *res;
> +
> +	if (name)
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> +	else
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	if (!res) {
> +		dev_dbg(&pdev->dev, "failed to get memory resource: %s\n",
> +				name);
> +		return 0;
> +	}
> +
> +	return resource_size(res);
> +}
> +
>   void msm_writel(u32 data, void __iomem *addr)
>   {
>   	if (reglog)
> @@ -278,6 +297,8 @@ static int msm_drm_uninit(struct device *dev)
>   		msm_fbdev_free(ddev);
>   #endif
>   
> +	msm_disp_snapshot_destroy(ddev);
> +
>   	drm_mode_config_cleanup(ddev);
>   
>   	pm_runtime_get_sync(dev);
> @@ -550,6 +571,12 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
>   	if (ret)
>   		goto err_msm_uninit;
>   
> +	if (get_mdp_ver(pdev) == KMS_DPU) {
> +		ret = msm_disp_snapshot_init(ddev);
> +		if (ret)
> +			DRM_DEV_ERROR(dev, "msm_disp_snapshot_init failed ret = %d\n", ret);
> +	}
> +

Let's get it out of if (KMS_DPU), even if we do not dump mdp4/mdp5 
registers for now.

>   	drm_mode_config_reset(ddev);
>   
>   #ifdef CONFIG_DRM_FBDEV_EMULATION
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 2668941..9c40bac 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -367,6 +367,7 @@ void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,
>   				struct drm_display_mode *mode,
>   				struct drm_display_mode *adjusted_mode);
>   void msm_dp_irq_postinstall(struct msm_dp *dp_display);
> +void msm_dp_snapshot(struct msm_dp *dp_display);
>   
>   void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
>   
> @@ -450,6 +451,7 @@ void __iomem *msm_ioremap(struct platform_device *pdev, const char *name,
>   		const char *dbgname);
>   void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name,
>   		const char *dbgname);
> +unsigned long msm_iomap_size(struct platform_device *pdev, const char *name);
>   void msm_writel(u32 data, void __iomem *addr);
>   u32 msm_readl(const void __iomem *addr);
>   void msm_rmw(void __iomem *addr, u32 mask, u32 or);
> 


-- 
With best wishes
Dmitry

Re: [PATCH v4 2/3] drm/msm: add support to take dpu snapshot

[abhinavk]

Hi Dmitry

Thanks for the review.

On 2021-04-14 17:16, Dmitry Baryshkov wrote:
> On 15/04/2021 02:11, Abhinav Kumar wrote:
>> Add the msm_disp_snapshot module which adds supports to dump dpu
>> registers and capture the drm atomic state which can be used in
>> case of error conditions.
>> 
>> changes in v4:
>>   - rename dpu_dbg to msm_disp_snapshot and move it to msm/disp
>>   - start using a list of blocks to store the hardware block 
>> information
>>   - cleanup block allocation and freeing logic to simplify it
>> 
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
>> ---
>>   drivers/gpu/drm/msm/Makefile                      |   2 +
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    |   2 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c           |  61 ++++++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h           |   5 +
>>   drivers/gpu/drm/msm/disp/msm_disp_snapshot.c      | 161 
>> +++++++++++++++++++
>>   drivers/gpu/drm/msm/disp/msm_disp_snapshot.h      | 167 
>> ++++++++++++++++++++
>>   drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 181 
>> ++++++++++++++++++++++
>>   drivers/gpu/drm/msm/dp/dp_catalog.c               |  12 ++
>>   drivers/gpu/drm/msm/dp/dp_catalog.h               |   4 +
>>   drivers/gpu/drm/msm/dp/dp_display.c               |  29 ++++
>>   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                |  19 +++
>>   drivers/gpu/drm/msm/msm_drv.c                     |  29 +++-
>>   drivers/gpu/drm/msm/msm_drv.h                     |   2 +
> 
> Generic suggestion: Could you please split this patch, separating core
> snapshotting functionality, dpu1, dsi and dp changes? This would ease
> reviewing.

Core snapshotting functionality does include calling into the individual 
driver's snapshot function.
So the diffstat which you are seeing in the dpu1/dsi and dp are just 
their snapshotting functions and nothing else.

> 
>>   16 files changed, 682 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
>>   create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
>>   create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
>> 
>> diff --git a/drivers/gpu/drm/msm/Makefile 
>> b/drivers/gpu/drm/msm/Makefile
>> index 610d630..65d86ce 100644
>> --- a/drivers/gpu/drm/msm/Makefile
>> +++ b/drivers/gpu/drm/msm/Makefile
>> @@ -77,6 +77,8 @@ msm-y := \
>>   	disp/dpu1/dpu_plane.o \
>>   	disp/dpu1/dpu_rm.o \
>>   	disp/dpu1/dpu_vbif.o \
>> +	disp/msm_disp_snapshot.o \
>> +	disp/msm_disp_snapshot_util.o \
> 
> If snapshotting is going to be used by dsi/dp, what about landing it
> into the top drivers/gpu/drm/msm dir? This way we'd point that it is a
> generic piece of code shared between all drm/msm drivers.

I dont have a strong preference of drm/msm Vs drm/msm/disp. I just put 
them here
based on Rob's comment on the prev patchset 
https://patchwork.freedesktop.org/patch/427690/?series=82942&rev=3

> 
>>   	msm_atomic.o \
>>   	msm_atomic_tracepoints.o \
>>   	msm_debugfs.o \
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> index 4dfd8a2..0f9f0a5 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -1,5 +1,5 @@
>>   /* SPDX-License-Identifier: GPL-2.0-only */
>> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights 
>> reserved.
>> +/* Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights 
>> reserved.
>>    */
>>     #ifndef _DPU_HW_CATALOG_H
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 88e9cc3..7529566 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -798,6 +798,67 @@ static void dpu_irq_uninstall(struct msm_kms 
>> *kms)
>>   	dpu_core_irq_uninstall(dpu_kms);
>>   }
>>   +void dpu_kms_mdp_snapshot(struct drm_device *dev)
>> +{
>> +	int i;
>> +	struct msm_drm_private *priv;
>> +	struct dpu_kms *dpu_kms;
>> +	struct dpu_mdss_cfg *cat;
>> +	struct dpu_hw_mdp *top;
>> +	struct msm_disp_state *disp_state;
>> +	char name[SZ_128];
>> +
>> +	priv = dev->dev_private;
>> +	dpu_kms = to_dpu_kms(priv->kms);
>> +	disp_state = dpu_kms->disp_state;
>> +
>> +	cat = dpu_kms->catalog;
>> +	top = dpu_kms->hw_mdp;
>> +
>> +	pm_runtime_get_sync(&dpu_kms->pdev->dev);
>> +
>> +	/* dump CTL sub-blocks HW regs info */
>> +	for (i = 0; i < cat->ctl_count; i++) {
>> +		snprintf(name, SZ_128, "ctl_%d", i);
> 
> snprintf(name, sizeof(name), "ctl_%d", i);
> (the same bellow)
Rob just mentioned to me on IRC to move this snprintf inside 
msm_disp_snapshot_add_block(). I am planning to do that in the next 
patchset.
With that will fix this too.
> 
>> +		msm_disp_snapshot_add_block(disp_state, name, cat->ctl[i].len,
>> +				dpu_kms->mmio + cat->ctl[i].base);
>> +	}
>> +
>> +	/* dump DSPP sub-blocks HW regs info */
>> +	for (i = 0; i < cat->dspp_count; i++) {
>> +		snprintf(name, SZ_128, "dspp_%d", i);
>> +		msm_disp_snapshot_add_block(disp_state, name, cat->dspp[i].len,
>> +				dpu_kms->mmio + cat->dspp[i].base);
>> +	}
>> +
>> +	/* dump INTF sub-blocks HW regs info */
>> +	for (i = 0; i < cat->intf_count; i++) {
>> +		snprintf(name, SZ_128, "intf_%d", i);
>> +		msm_disp_snapshot_add_block(disp_state, name, cat->intf[i].len,
>> +				dpu_kms->mmio + cat->intf[i].base);
>> +	}
>> +
>> +	/* dump PP sub-blocks HW regs info */
>> +	for (i = 0; i < cat->pingpong_count; i++) {
>> +		snprintf(name, SZ_128, "ping-pong_%d", i);
> 
> Just a nitpick: we usually call it pp or pingpong, without dash.

Sure I will fix this.

> 
>> +		msm_disp_snapshot_add_block(disp_state, name, cat->pingpong[i].len,
>> +				dpu_kms->mmio + cat->pingpong[i].base);
>> +	}
>> +
>> +	/* dump SSPP sub-blocks HW regs info */
>> +	for (i = 0; i < cat->sspp_count; i++) {
>> +		snprintf(name, SZ_128, "sspp_%d", i);
>> +		msm_disp_snapshot_add_block(disp_state, name, cat->sspp[i].len,
>> +				dpu_kms->mmio + cat->sspp[i].base);
>> +	}
>> +
>> +	snprintf(name, SZ_128, "top");
>> +	msm_disp_snapshot_add_block(disp_state, name, top->hw.length,
>> +			dpu_kms->mmio + top->hw.blk_off);
>> +
>> +	pm_runtime_put_sync(&dpu_kms->pdev->dev);
>> +}
>> +
>>   static const struct msm_kms_funcs kms_funcs = {
>>   	.hw_init         = dpu_kms_hw_init,
>>   	.irq_preinstall  = dpu_irq_preinstall,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>> index d6717d6..74b6ce6 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>> @@ -13,6 +13,7 @@
>>   #include <drm/drm_drv.h>
>>     #include "msm_drv.h"
>> +#include "disp/msm_disp_snapshot.h"
>>   #include "msm_kms.h"
>>   #include "msm_mmu.h"
>>   #include "msm_gem.h"
>> @@ -132,6 +133,8 @@ struct dpu_kms {
>>     	struct opp_table *opp_table;
>>   +	struct msm_disp_state *disp_state;
>> +
>>   	struct dss_module_power mp;
>>     	/* reference count bandwidth requests, so we know when we can
>> @@ -265,4 +268,6 @@ void dpu_kms_encoder_enable(struct drm_encoder 
>> *encoder);
>>    */
>>   u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name);
>>   +void dpu_kms_mdp_snapshot(struct drm_device *dev);
>> +
>>   #endif /* __dpu_kms_H__ */
>> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c 
>> b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
>> new file mode 100644
>> index 0000000..1bdad16
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
>> @@ -0,0 +1,161 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2020-2021, The Linux Foundation. All rights 
>> reserved.
>> + */
>> +
>> +#define pr_fmt(fmt)	"[drm:%s:%d] " fmt, __func__, __LINE__
>> +
>> +#include "msm_disp_snapshot.h"
>> +
>> +#ifdef CONFIG_DEV_COREDUMP
>> +static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
>> +		size_t count, void *data, size_t datalen)
>> +{
>> +	struct drm_print_iterator iter;
>> +	struct drm_printer p;
>> +	struct msm_disp_state *disp_state;
>> +
>> +	disp_state = data;
>> +
>> +	iter.data = buffer;
>> +	iter.offset = 0;
>> +	iter.start = offset;
>> +	iter.remain = count;
>> +
>> +	p = drm_coredump_printer(&iter);
>> +
>> +	msm_disp_state_print(disp_state, &p, MSM_DISP_SNAPSHOT_IN_COREDUMP);
>> +
>> +	return count - iter.remain;
>> +}
>> +
>> +static void disp_devcoredump_free(void *data)
>> +{
>> +	struct msm_disp_state *disp_state;
>> +
>> +	disp_state = data;
>> +
>> +	msm_disp_state_free(disp_state);
>> +
>> +	disp_state->coredump_pending = false;
>> +}
>> +#endif /* CONFIG_DEV_COREDUMP */
>> +
>> +static void _msm_disp_snapshot_work(struct kthread_work *work)
>> +{
>> +	struct msm_disp_state *disp_state = container_of(work, struct 
>> msm_disp_state, dump_work);
>> +	struct drm_printer p;
>> +
>> +	mutex_lock(&disp_state->mutex);
>> +
>> +	msm_disp_snapshot_capture_state(disp_state);
>> +
>> +	if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
>> +		p = drm_info_printer(disp_state->drm_dev->dev);
>> +		msm_disp_state_print(disp_state, &p, MSM_DISP_SNAPSHOT_IN_LOG);
>> +	}
>> +
>> +	/*
>> +	 * if devcoredump is not defined free the state immediately
>> +	 * otherwise it will be freed in the free handler.
>> +	 */
>> +#ifdef CONFIG_DEV_COREDUMP
>> +	dev_coredumpm(disp_state->dev, THIS_MODULE, disp_state, 0, 
>> GFP_KERNEL,
>> +			disp_devcoredump_read, disp_devcoredump_free);
>> +	disp_state->coredump_pending = true;
>> +#else
>> +	msm_disp_state_free(disp_state);
>> +#endif
>> +
>> +	mutex_unlock(&disp_state->mutex);
>> +}
>> +
>> +void msm_disp_snapshot_state(struct drm_device *drm_dev)
>> +{
>> +	struct msm_drm_private *priv;
>> +	struct dpu_kms *dpu_kms;
>> +	struct msm_disp_state *disp_state;
>> +
>> +	if (!drm_dev) {
>> +		DRM_ERROR("invalid params\n");
>> +		return;
>> +	}
>> +
>> +	priv = drm_dev->dev_private;
>> +	dpu_kms = to_dpu_kms(priv->kms);
> 
> If we are outside of disp/dpu1, we shouldn't reference dpu_kms
> internals. Probably this can be changed to use msm_drm_private,
> msm_kms or msm_mdss?
This was originally within dpu1 but based on Rob's comment I moved it to 
disp.
Since this is a debug module I thought its okay to reference it here. 
Its not
modifying anything which will impact the current dpu state.
If there is a strong preference for this, sure I can move it to 
msm_drm_private.
> 
>> +	disp_state = dpu_kms->disp_state;
>> +
>> +	if (!disp_state) {
>> +		DRM_ERROR("invalid params\n");
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * if there is a coredump pending return immediately till dump
>> +	 * if read by userspace or timeout happens
> 
> Where do we handle the timeout? Stale comment or did I miss something?
The timeout happens in the devcoredump driver and it calls the free() 
from there.
https://github.com/torvalds/linux/blob/master/drivers/base/devcoredump.c#L304

> 
>> +	 */
>> +	if (disp_state->coredump_pending) {
>> +		DRM_DEBUG("coredump is pending read\n");
>> +		return;
>> +	}
>> +
>> +	kthread_queue_work(disp_state->dump_worker,
>> +			&disp_state->dump_work);
>> +}
>> +
>> +int msm_disp_snapshot_init(struct drm_device *drm_dev)
>> +{
>> +	struct dpu_kms *dpu_kms;
>> +	struct msm_drm_private *priv;
>> +	struct msm_disp_state *disp_state;
>> +
>> +	if (!drm_dev) {
>> +		DRM_ERROR("invalid params\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	priv = drm_dev->dev_private;
>> +	dpu_kms = to_dpu_kms(priv->kms);
>> +
>> +	disp_state = devm_kzalloc(drm_dev->dev, sizeof(struct 
>> msm_disp_state), GFP_KERNEL);
>> +
>> +	mutex_init(&disp_state->mutex);
>> +
>> +	disp_state->dev = drm_dev->dev;
>> +	disp_state->drm_dev = drm_dev;
>> +
>> +	INIT_LIST_HEAD(&disp_state->blocks);
>> +
>> +	disp_state->dump_worker = kthread_create_worker(0, "%s", 
>> "disp_snapshot");
>> +	if (IS_ERR(disp_state->dump_worker))
>> +		DRM_ERROR("failed to create disp state task\n");
>> +
>> +	kthread_init_work(&disp_state->dump_work, _msm_disp_snapshot_work);
>> +
>> +	dpu_kms->disp_state = disp_state;
> 
> Judging from this code, I think it should be possible to push
> disp_state directly into struct msm_drm_private.
Again no strong preference here. I just kept it in dpu_kms based on a 
discussion with Rob.
We can move it to msm_drm_private too if there is a strong preference to 
do so.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +void msm_disp_snapshot_destroy(struct drm_device *drm_dev)
>> +{
>> +	struct dpu_kms *dpu_kms;
>> +	struct msm_drm_private *priv;
>> +	struct msm_disp_state *disp_state;
>> +
>> +	if (!drm_dev) {
>> +		DRM_ERROR("invalid params\n");
>> +		return;
>> +	}
>> +
>> +	priv = drm_dev->dev_private;
>> +	dpu_kms = to_dpu_kms(priv->kms);
>> +	disp_state = dpu_kms->disp_state;
>> +
>> +	if (disp_state->dump_worker)
>> +		kthread_destroy_worker(disp_state->dump_worker);
>> +
>> +	list_del(&disp_state->blocks);
>> +
>> +	mutex_destroy(&disp_state->mutex);
>> +}
>> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h 
>> b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
>> new file mode 100644
>> index 0000000..0ccb7e7
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
>> @@ -0,0 +1,167 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2020-2021, The Linux Foundation. All rights 
>> reserved.
>> + */
>> +
>> +#ifndef MSM_DISP_SNAPSHOT_H_
>> +#define MSM_DISP_SNAPSHOT_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"
>> +
>> +enum msm_disp_snapshot_dump_flag {
>> +	MSM_DISP_SNAPSHOT_IN_LOG = BIT(0),
>> +	MSM_DISP_SNAPSHOT_IN_MEM = BIT(1),
>> +	MSM_DISP_SNAPSHOT_IN_COREDUMP = BIT(2),
> 
> Are they mutually exclusive or can they be specified in parallel?
They are mutually exclusive
> 
>> +};
>> +
>> +#define MSM_DISP_SNAPSHOT_MAX_BLKS		10
>> +
>> +/* debug option to print the registers in logs */
>> +#define MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE 0
>> +
>> +/* print debug ranges in groups of 4 u32s */
>> +#define REG_DUMP_ALIGN		16
>> +
>> +/**
>> + * struct msm_disp_state - structure to store current dpu state
>> + * @dev: device pointer
>> + * @drm_dev: drm device pointer
>> + * @mutex: mutex to serialize access to serialze dumps, debugfs 
>> access
>> + * @coredump_pending: coredump is pending read from userspace
>> + * @atomic_state: atomic state duplicated at the time of the error
>> + * @dump_worker: kworker thread which runs the dump work
>> + * @dump_work: kwork which dumps the registers and drm state
>> + * @timestamp: timestamp at which the coredump was captured
>> + */
>> +struct msm_disp_state {
>> +	struct device *dev;
>> +	struct drm_device *drm_dev;
>> +	struct mutex mutex;
>> +
>> +	bool coredump_pending;
>> +
>> +	struct list_head blocks;
>> +
>> +	struct drm_atomic_state *atomic_state;
>> +
>> +	struct kthread_worker *dump_worker;
>> +	struct kthread_work dump_work;
>> +	ktime_t timestamp;
>> +};
>> +
>> +/**
>> + * struct msm_disp_state_block - structure to store each hardware 
>> block state
>> + * @name: name of the block
>> + * @drm_dev: handle to the linked list head
>> + * @size: size of the register space of this hardware block
>> + * @state: array holding the register dump of this hardware block
>> + * @base_addr: starting address of this hardware block's register 
>> space
>> + */
>> +struct msm_disp_state_block {
>> +	char name[SZ_128];
>> +	struct list_head node;
>> +	unsigned int size;
>> +	u32 *state;
>> +	void __iomem *base_addr;
>> +};
>> +
>> +/**
>> + * MSM_DISP_SNAPSHOT - trigger to dump the display snapshot
>> + * @drm_dev:	handle to drm device
>> + */
>> +#define MSM_DISP_SNAPSHOT(drm_dev) msm_disp_snapshot_state(drm_dev)
> 
> Why do you need it?
Not really needed now, I can just call into msm_disp_snapshot_state() 
instead
> 
>> +
>> +/**
>> + * msm_disp_snapshot_init - initialize global sde debug facilities: 
>> evtlog, regdump
> 
> Just a nitpick here and below: there is no sde in the upstream. And no 
> evtlog.
Sorry this is a stale comment, I thought I got rid of this, will fix 
this.
> 
>> + * @drm_dev:	drm device handle
>> + *
>> + * Returns:		0 or -ERROR
>> + */
>> +int msm_disp_snapshot_init(struct drm_device *drm_dev);
>> +
>> +/**
>> + * msm_disp_snapshot_destroy - destroy the global sde debug 
>> facilities
>> + * @drm_dev:    drm device handle
>> + *
>> + * Returns:	none
>> + */
>> +void msm_disp_snapshot_destroy(struct drm_device *drm_dev);
>> +
>> +/**
>> + * msm_disp_snapshot_state - trigger to dump the display snapshot
>> + * @drm_dev:	handle to drm device
>> +
>> + * Returns:	none
>> + */
>> +void msm_disp_snapshot_state(struct drm_device *drm_dev);
>> +
>> +/**
>> + * msm_disp_state_get - get the handle to msm_disp_state struct from 
>> the drm device
>> + * @drm:	    handle to drm device
>> +
>> + * Returns:	handle to the msm_disp_state struct
>> + */
>> +struct msm_disp_state *msm_disp_state_get(struct drm_device *drm);
>> +
>> +/**
>> + * msm_disp_state_print - print out the current dpu state
>> + * @disp_state:	    handle to drm device
>> + * @p:	    handle to drm printer
>> + * @reg_dump_method: tells whether the state has to be dumped in log 
>> or coredump
>> + *
>> + * Returns:	none
>> + */
>> +void msm_disp_state_print(struct msm_disp_state *disp_state, struct 
>> drm_printer *p,
>> +		u8 reg_dump_method);
>> +
>> +/**
>> + * msm_disp_snapshot_capture_state - utility to capture atomic state 
>> and hw registers
>> + * @disp_state:	    handle to msm_disp_state struct
>> +
>> + * Returns:	none
>> + */
>> +void msm_disp_snapshot_capture_state(struct msm_disp_state 
>> *disp_state);
>> +
>> +/**
>> + * msm_disp_state_free - free the memory after the coredump has been 
>> read
>> + * @disp_state:	    handle to struct msm_disp_state
>> +
>> + * Returns: none
>> + */
>> +void msm_disp_state_free(struct msm_disp_state *disp_state);
>> +
>> +/**
>> + * msm_disp_snapshot_add_block - add a hardware block with its 
>> register dump
>> + * @disp_state:	    handle to struct msm_disp_state
>> + * @name:           name of the hardware block
>> + * @len:            size of the register space of the hardware block
>> + * @base_addr:      starting address of the register space of the 
>> hardware block
>> + *
>> + * Returns: none
>> + */
>> +void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, 
>> char *name, u32 len,
>> +		void __iomem *base_addr);
>> +
>> +#endif /* MSM_DISP_SNAPSHOT_H_ */
>> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c 
>> b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
>> new file mode 100644
>> index 0000000..383f225
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
>> @@ -0,0 +1,181 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2020-2021, The Linux Foundation. All rights 
>> reserved.
>> + */
>> +
>> +#define pr_fmt(fmt)	"[drm:%s:%d] " fmt, __func__, __LINE__
>> +
>> +#include "msm_disp_snapshot.h"
>> +
>> +static void msm_disp_state_dump_regs(u32 **reg, u32 len, void __iomem 
>> *base_addr,
>> +		u32 dump_op, struct drm_printer *p)
>> +{
>> +	u32 len_aligned, len_padded;
>> +	u32 x0, x4, x8, xc;
>> +	void __iomem *addr;
>> +	u32 *dump_addr = NULL;
>> +	void __iomem *end_addr;
>> +	int i;
>> +	bool in_drm_printer = false;
>> +
>> +	len_aligned = (len + REG_DUMP_ALIGN - 1) / REG_DUMP_ALIGN;
>> +	len_padded = len_aligned * REG_DUMP_ALIGN;
>> +
>> +	addr = base_addr;
>> +	end_addr = base_addr + len;
>> +
>> +	if ((dump_op == MSM_DISP_SNAPSHOT_IN_COREDUMP) ||
>> +		(dump_op == MSM_DISP_SNAPSHOT_IN_LOG))
>> +		in_drm_printer = true;
>> +
>> +	if (in_drm_printer && !p) {
>> +		DRM_ERROR("invalid drm printer\n");
>> +		return;
>> +	}
>> +
>> +	if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM && !(*reg))
>> +		*reg = kzalloc(len_padded, GFP_KERNEL);
>> +
>> +	if (*reg)
>> +		dump_addr = *reg;
>> +
>> +	for (i = 0; i < len_aligned; i++) {
>> +		if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM) {
>> +			x0 = (addr < end_addr) ? readl_relaxed(addr + 0x0) : 0;
>> +			x4 = (addr + 0x4 < end_addr) ? readl_relaxed(addr + 0x4) : 0;
>> +			x8 = (addr + 0x8 < end_addr) ? readl_relaxed(addr + 0x8) : 0;
>> +			xc = (addr + 0xc < end_addr) ? readl_relaxed(addr + 0xc) : 0;
>> +		}
>> +
>> +		if (dump_addr) {
>> +			if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM) {
>> +				dump_addr[i * 4] = x0;
>> +				dump_addr[i * 4 + 1] = x4;
>> +				dump_addr[i * 4 + 2] = x8;
>> +				dump_addr[i * 4 + 3] = xc;
>> +			} else if (in_drm_printer) {
>> +				drm_printf(p, "0x%lx : %08x %08x %08x %08x\n",
>> +						(unsigned long)(addr - base_addr),
>> +						dump_addr[i * 4], dump_addr[i * 4 + 1],
>> +						dump_addr[i * 4 + 2], dump_addr[i * 4 + 3]);
>> +			}
>> +		}
>> +
>> +		addr += REG_DUMP_ALIGN;
>> +	}
>> +}
> 
> I'd just split this function into two different cases, IN_MEM and 
> DRM_PRINTER.
I wanted to utilize the common loop here, if it will make it more clear 
I can just have two functions.
But it will duplicate a lot of code.
> 
>> +
>> +struct msm_disp_state *msm_disp_state_get(struct drm_device *drm)
>> +{
>> +	struct msm_drm_private *priv;
>> +	struct dpu_kms *dpu_kms;
>> +
>> +	priv = drm->dev_private;
>> +	dpu_kms = to_dpu_kms(priv->kms);
>> +
>> +	return dpu_kms->disp_state;
>> +}
>> +
>> +void msm_disp_state_print(struct msm_disp_state *state, struct 
>> drm_printer *p,
>> +		u8 reg_dump_method)
>> +{
>> +	struct msm_disp_state_block *block, *tmp;
>> +
>> +	if (!p) {
>> +		DRM_ERROR("invalid drm printer\n");
>> +		return;
>> +	}
>> +
>> +	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(state->timestamp));
>> +
>> +	list_for_each_entry_safe(block, tmp, &state->blocks, node) {
>> +		drm_printf(p, "====================%s================\n", 
>> block->name);
>> +		msm_disp_state_dump_regs(&block->state, block->size, 
>> block->base_addr,
>> +				reg_dump_method, p);
>> +	}
>> +
>> +	drm_printf(p, "===================dpu drm state================\n");
>> +
>> +	if (state->atomic_state)
>> +		drm_atomic_print_new_state(state->atomic_state, p);
>> +}
>> +
>> +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();
>> +
>> +	ddev = disp_state->drm_dev;
>> +
>> +	drm_modeset_acquire_init(&ctx, 0);
>> +
>> +	while (drm_modeset_lock_all_ctx(ddev, &ctx) != 0)
>> +		drm_modeset_backoff(&ctx);
>> +
>> +	disp_state->atomic_state = drm_atomic_helper_duplicate_state(ddev,
>> +			&ctx);
>> +	drm_modeset_drop_locks(&ctx);
>> +	drm_modeset_acquire_fini(&ctx);
>> +}
>> +
>> +void msm_disp_snapshot_capture_state(struct msm_disp_state 
>> *disp_state)
>> +{
>> +	struct msm_drm_private *priv;
>> +	int i;
>> +	struct drm_device *drm_dev;
>> +
>> +	drm_dev = disp_state->drm_dev;
>> +	priv = drm_dev->dev_private;
>> +
>> +	msm_disp_capture_atomic_state(disp_state);
>> +
>> +	if (priv->dp)
>> +		msm_dp_snapshot(priv->dp);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
>> +		if (!priv->dsi[i])
>> +			continue;
>> +
>> +		msm_dsi_snapshot(priv->dsi[i]);
>> +	}
>> +
>> +	dpu_kms_mdp_snapshot(drm_dev);
> 
> Hmm, I now see why do you need the dpu_kms. Can we have a check for
> the mdp version here? The rest of the code seems generic enough, so we
> can at least dump the dsi/dp (and hopefully eDP/HDMI) data for MDP4/5.
> 
> Ideally subdrivers should be able to report back to the core, which
> regions are available, so that we won't have this static code. But
> this might look like an overkill. Not to mention that it might lead to
> the not-so-static sequence of dumps. (As you can judge, I quite liked
> the previous approach, when the drivers registered the dumping regions
> and then the dumper would go through them.)
> 
> Let's have the static config in place with the version check.
Yes, I can have a DPU check here.
> 
>> +}
>> +
>> +void msm_disp_state_free(struct msm_disp_state *disp_state)
>> +{
>> +	struct msm_disp_state_block *block, *tmp;
>> +
>> +	if (disp_state->atomic_state) {
>> +		drm_atomic_state_put(disp_state->atomic_state);
>> +		disp_state->atomic_state = NULL;
>> +	}
>> +
>> +	list_for_each_entry_safe(block, tmp, &disp_state->blocks, node) {
>> +		kfree(block->state);
>> +		list_del(&block->node);
> 
> I'd swap these two lines, so while the block is in the list, its state
> is coherent.
Ok, will fix this.
> 
>> +		kfree(block);
>> +	}
>> +}
>> +
>> +void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, 
>> char *name, u32 len,
>> +		void __iomem *base_addr)
>> +{
>> +	struct msm_disp_state_block *new_blk;
>> +
>> +	new_blk = kzalloc(sizeof(struct msm_disp_state_block), GFP_KERNEL);
>> +
>> +	strscpy(new_blk->name, name, sizeof(new_blk->name));
>> +	INIT_LIST_HEAD(&new_blk->node);
>> +	new_blk->size = len;
>> +	new_blk->base_addr = base_addr;
>> +	msm_disp_state_dump_regs(&new_blk->state, len, base_addr,
>> +			MSM_DISP_SNAPSHOT_IN_MEM, NULL);
>> +	list_add(&new_blk->node, &disp_state->blocks);
>> +}
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c 
>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index b1a9b1b..ce8d19a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -62,6 +62,18 @@ struct dp_catalog_private {
>>   	u8 aux_lut_cfg_index[PHY_AUX_CFG_MAX];
>>   };
>>   +void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct 
>> msm_disp_state *disp_state)
>> +{
>> +	struct dp_catalog_private *catalog = container_of(dp_catalog,
>> +			struct dp_catalog_private, dp_catalog);
>> +	char name[SZ_128];
>> +
>> +	snprintf(name, SZ_128, "dp_ctrl");
>> +
>> +	msm_disp_snapshot_add_block(disp_state, name, 
>> catalog->io->dp_controller.len,
>> +			catalog->io->dp_controller.base);
>> +}
>> +
>>   static inline u32 dp_read_aux(struct dp_catalog_private *catalog, 
>> u32 offset)
>>   {
>>   	offset += MSM_DP_CONTROLLER_AUX_OFFSET;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h 
>> b/drivers/gpu/drm/msm/dp/dp_catalog.h
>> index 176a902..e7e8b13 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
>> @@ -9,6 +9,7 @@
>>   #include <drm/drm_modes.h>
>>     #include "dp_parser.h"
>> +#include "disp/msm_disp_snapshot.h"
>>     /* interrupts */
>>   #define DP_INTR_HPD		BIT(0)
>> @@ -71,6 +72,9 @@ struct dp_catalog {
>>   	u32 audio_data;
>>   };
>>   +/* Debug module */
>> +void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct 
>> msm_disp_state *disp_state);
>> +
>>   /* AUX APIs */
>>   u32 dp_catalog_aux_read_data(struct dp_catalog *dp_catalog);
>>   int dp_catalog_aux_write_data(struct dp_catalog *dp_catalog);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 5a39da6..6670558 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1009,6 +1009,35 @@ int dp_display_get_test_bpp(struct msm_dp *dp)
>>   		dp_display->link->test_video.test_bit_depth);
>>   }
>>   +void msm_dp_snapshot(struct msm_dp *dp)
>> +{
>> +	struct dp_display_private *dp_display;
>> +	struct drm_device *drm;
>> +	struct msm_disp_state *disp_state;
>> +
>> +	dp_display = container_of(dp, struct dp_display_private, 
>> dp_display);
>> +	drm = dp->drm_dev;
>> +	disp_state = msm_disp_state_get(drm);
>> +
>> +	/*
>> +	 * if we are reading registers we need the link clocks to be on
>> +	 * however till DP cable is connected this will not happen as we
>> +	 * do not know the resolution to power up with. Hence check the
>> +	 * power_on status before dumping DP registers to avoid crash due
>> +	 * to unclocked access
>> +	 */
>> +	mutex_lock(&dp_display->event_mutex);
>> +
>> +	if (!dp->power_on) {
>> +		mutex_unlock(&dp_display->event_mutex);
>> +		return;
>> +	}
>> +
>> +	dp_catalog_snapshot(dp_display->catalog, disp_state);
>> +
>> +	mutex_unlock(&dp_display->event_mutex);
>> +}
>> +
>>   static void dp_display_config_hpd(struct dp_display_private *dp)
>>   {
>>   diff --git a/drivers/gpu/drm/msm/dp/dp_display.h 
>> b/drivers/gpu/drm/msm/dp/dp_display.h
>> index 6092ba1..4d39373 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>> @@ -8,6 +8,7 @@
>>     #include "dp_panel.h"
>>   #include <sound/hdmi-codec.h>
>> +#include "disp/msm_disp_snapshot.h"
>>     struct msm_dp {
>>   	struct drm_device *drm_dev;
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c 
>> b/drivers/gpu/drm/msm/dsi/dsi.c
>> index 62704885..bccc006 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
>> @@ -266,3 +266,8 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, 
>> struct drm_device *dev,
>>   	return ret;
>>   }
>>   +void msm_dsi_snapshot(struct msm_dsi *msm_dsi)
>> +{
>> +	msm_dsi_host_snapshot(msm_dsi->host);
>> +}
>> +
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
>> b/drivers/gpu/drm/msm/dsi/dsi.h
>> index 7abfeab..bb39403 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>> @@ -15,6 +15,7 @@
>>   #include <drm/drm_panel.h>
>>     #include "msm_drv.h"
>> +#include "disp/msm_disp_snapshot.h"
>>     #define DSI_0	0
>>   #define DSI_1	1
>> @@ -90,6 +91,8 @@ static inline bool msm_dsi_device_connected(struct 
>> msm_dsi *msm_dsi)
>>   	return msm_dsi->panel || msm_dsi->external_bridge;
>>   }
>>   +void msm_dsi_snapshot(struct msm_dsi *msm_dsi);
>> +
>>   struct drm_encoder *msm_dsi_get_encoder(struct msm_dsi *msm_dsi);
>>     /* dsi host */
>> @@ -146,6 +149,7 @@ int dsi_clk_init_v2(struct msm_dsi_host 
>> *msm_host);
>>   int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host);
>>   int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool 
>> is_dual_dsi);
>>   int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool 
>> is_dual_dsi);
>> +void msm_dsi_host_snapshot(struct mipi_dsi_host *host);
>>     /* dsi phy */
>>   struct msm_dsi_phy;
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 8a10e43..d9fdc07 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -2487,3 +2487,22 @@ 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_snapshot(struct mipi_dsi_host *host)
>> +{
>> +	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>> +	struct drm_device *dev = msm_host->dev;
>> +	struct msm_disp_state *disp_state;
>> +	char name[SZ_128];
>> +
>> +	disp_state = msm_disp_state_get(dev);
>> +
>> +	pm_runtime_get_sync(&msm_host->pdev->dev);
>> +
>> +	snprintf(name, SZ_128, "dsi%d_ctrl", msm_host->id);
>> +
>> +	msm_disp_snapshot_add_block(disp_state, name, 
>> msm_iomap_size(msm_host->pdev, "dsi_ctrl"),
> 
> You can store the size into the msm_host, rather than querrying it on 
> each dump.

We need to query the base address anyway so it wont lead to much saving.

> 
>> +			msm_host->ctrl_base);
>> +
>> +	pm_runtime_put_sync(&msm_host->pdev->dev);
>> +}
>> diff --git a/drivers/gpu/drm/msm/msm_drv.c 
>> b/drivers/gpu/drm/msm/msm_drv.c
>> index e1104d2..ebf9283 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.c
>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>> @@ -1,6 +1,6 @@
>>   // SPDX-License-Identifier: GPL-2.0-only
>>   /*
>> - * Copyright (c) 2016-2018, The Linux Foundation. All rights 
>> reserved.
>> + * Copyright (c) 2016-2018, 2020-2021 The Linux Foundation. All 
>> rights reserved.
>>    * Copyright (C) 2013 Red Hat
>>    * Author: Rob Clark <robdclark@gmail.com>
>>    */
>> @@ -19,6 +19,7 @@
>>   #include <drm/drm_of.h>
>>   #include <drm/drm_vblank.h>
>>   +#include "disp/msm_disp_snapshot.h"
>>   #include "msm_drv.h"
>>   #include "msm_debugfs.h"
>>   #include "msm_fence.h"
>> @@ -167,6 +168,24 @@ void __iomem *msm_ioremap_quiet(struct 
>> platform_device *pdev, const char *name,
>>   	return _msm_ioremap(pdev, name, dbgname, true);
>>   }
>>   +unsigned long msm_iomap_size(struct platform_device *pdev, const 
>> char *name)
>> +{
>> +	struct resource *res;
>> +
>> +	if (name)
>> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
>> +	else
>> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> +	if (!res) {
>> +		dev_dbg(&pdev->dev, "failed to get memory resource: %s\n",
>> +				name);
>> +		return 0;
>> +	}
>> +
>> +	return resource_size(res);
>> +}
>> +
>>   void msm_writel(u32 data, void __iomem *addr)
>>   {
>>   	if (reglog)
>> @@ -278,6 +297,8 @@ static int msm_drm_uninit(struct device *dev)
>>   		msm_fbdev_free(ddev);
>>   #endif
>>   +	msm_disp_snapshot_destroy(ddev);
>> +
>>   	drm_mode_config_cleanup(ddev);
>>     	pm_runtime_get_sync(dev);
>> @@ -550,6 +571,12 @@ static int msm_drm_init(struct device *dev, const 
>> struct drm_driver *drv)
>>   	if (ret)
>>   		goto err_msm_uninit;
>>   +	if (get_mdp_ver(pdev) == KMS_DPU) {
>> +		ret = msm_disp_snapshot_init(ddev);
>> +		if (ret)
>> +			DRM_DEV_ERROR(dev, "msm_disp_snapshot_init failed ret = %d\n", 
>> ret);
>> +	}
>> +
> 
> Let's get it out of if (KMS_DPU), even if we do not dump mdp4/mdp5
> registers for now.
since its stored inside dpu_kms right now, we need to have this DPU 
check.
If we agree on moving this out of dpu_kms then this check can be 
relaxed. But another issue
is I dont have a non-DPU_KMS board to verify this to see if it will 
break.
So at the moment, I am restricting this to it.

> 
>>   	drm_mode_config_reset(ddev);
>>     #ifdef CONFIG_DRM_FBDEV_EMULATION
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h 
>> b/drivers/gpu/drm/msm/msm_drv.h
>> index 2668941..9c40bac 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -367,6 +367,7 @@ void msm_dp_display_mode_set(struct msm_dp *dp, 
>> struct drm_encoder *encoder,
>>   				struct drm_display_mode *mode,
>>   				struct drm_display_mode *adjusted_mode);
>>   void msm_dp_irq_postinstall(struct msm_dp *dp_display);
>> +void msm_dp_snapshot(struct msm_dp *dp_display);
>>     void msm_dp_debugfs_init(struct msm_dp *dp_display, struct 
>> drm_minor *minor);
>>   @@ -450,6 +451,7 @@ void __iomem *msm_ioremap(struct platform_device 
>> *pdev, const char *name,
>>   		const char *dbgname);
>>   void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const 
>> char *name,
>>   		const char *dbgname);
>> +unsigned long msm_iomap_size(struct platform_device *pdev, const char 
>> *name);
>>   void msm_writel(u32 data, void __iomem *addr);
>>   u32 msm_readl(const void __iomem *addr);
>>   void msm_rmw(void __iomem *addr, u32 mask, u32 or);
>> 

Thanks

Abhinav

Re: [PATCH v4 2/3] drm/msm: add support to take dpu snapshot

[Rob Clark]

On Wed, Apr 14, 2021 at 5:16 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 15/04/2021 02:11, Abhinav Kumar wrote:
> > Add the msm_disp_snapshot module which adds supports to dump dpu
> > registers and capture the drm atomic state which can be used in
> > case of error conditions.
> >
> > changes in v4:
> >   - rename dpu_dbg to msm_disp_snapshot and move it to msm/disp
> >   - start using a list of blocks to store the hardware block information
> >   - cleanup block allocation and freeing logic to simplify it
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> > ---
> >   drivers/gpu/drm/msm/Makefile                      |   2 +
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    |   2 +-
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c           |  61 ++++++++
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h           |   5 +
> >   drivers/gpu/drm/msm/disp/msm_disp_snapshot.c      | 161 +++++++++++++++++++
> >   drivers/gpu/drm/msm/disp/msm_disp_snapshot.h      | 167 ++++++++++++++++++++
> >   drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 181 ++++++++++++++++++++++
> >   drivers/gpu/drm/msm/dp/dp_catalog.c               |  12 ++
> >   drivers/gpu/drm/msm/dp/dp_catalog.h               |   4 +
> >   drivers/gpu/drm/msm/dp/dp_display.c               |  29 ++++
> >   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                |  19 +++
> >   drivers/gpu/drm/msm/msm_drv.c                     |  29 +++-
> >   drivers/gpu/drm/msm/msm_drv.h                     |   2 +
>
> Generic suggestion: Could you please split this patch, separating core
> snapshotting functionality, dpu1, dsi and dp changes? This would ease
> reviewing.
>
> >   16 files changed, 682 insertions(+), 2 deletions(-)
> >   create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> >   create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
> >   create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> >
> > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> > index 610d630..65d86ce 100644
> > --- a/drivers/gpu/drm/msm/Makefile
> > +++ b/drivers/gpu/drm/msm/Makefile
> > @@ -77,6 +77,8 @@ msm-y := \
> >       disp/dpu1/dpu_plane.o \
> >       disp/dpu1/dpu_rm.o \
> >       disp/dpu1/dpu_vbif.o \
> > +     disp/msm_disp_snapshot.o \
> > +     disp/msm_disp_snapshot_util.o \
>
> If snapshotting is going to be used by dsi/dp, what about landing it
> into the top drivers/gpu/drm/msm dir? This way we'd point that it is a
> generic piece of code shared between all drm/msm drivers.

I'm ok with either drm/msm or drm/msm/disp (the latter because it is
really about the display related components of drm/msm)..  but we
should not introduce dependencies in (for ex) drm/msm/dsi on dpu

>
> >       msm_atomic.o \
> >       msm_atomic_tracepoints.o \
> >       msm_debugfs.o \
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > index 4dfd8a2..0f9f0a5 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > @@ -1,5 +1,5 @@
> >   /* SPDX-License-Identifier: GPL-2.0-only */
> > -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> > +/* Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights reserved.
> >    */
> >
> >   #ifndef _DPU_HW_CATALOG_H
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 88e9cc3..7529566 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -798,6 +798,67 @@ static void dpu_irq_uninstall(struct msm_kms *kms)
> >       dpu_core_irq_uninstall(dpu_kms);
> >   }
> >
> > +void dpu_kms_mdp_snapshot(struct drm_device *dev)
> > +{
> > +     int i;
> > +     struct msm_drm_private *priv;
> > +     struct dpu_kms *dpu_kms;
> > +     struct dpu_mdss_cfg *cat;
> > +     struct dpu_hw_mdp *top;
> > +     struct msm_disp_state *disp_state;
> > +     char name[SZ_128];
> > +
> > +     priv = dev->dev_private;
> > +     dpu_kms = to_dpu_kms(priv->kms);
> > +     disp_state = dpu_kms->disp_state;
> > +
> > +     cat = dpu_kms->catalog;
> > +     top = dpu_kms->hw_mdp;
> > +
> > +     pm_runtime_get_sync(&dpu_kms->pdev->dev);
> > +
> > +     /* dump CTL sub-blocks HW regs info */
> > +     for (i = 0; i < cat->ctl_count; i++) {
> > +             snprintf(name, SZ_128, "ctl_%d", i);
>
> snprintf(name, sizeof(name), "ctl_%d", i);
> (the same bellow)

let's just get rid of all the snprintf and make
msm_disp_snapshot_add_block() printf-like

(also, I'm not 100% sure off the top of my head if kernel's sprintf()
differs from libc as far as losing terminating null if you overflow,
but it would be better to deal with this in one place than N places)

>
> > +             msm_disp_snapshot_add_block(disp_state, name, cat->ctl[i].len,
> > +                             dpu_kms->mmio + cat->ctl[i].base);
> > +     }
> > +
> > +     /* dump DSPP sub-blocks HW regs info */
> > +     for (i = 0; i < cat->dspp_count; i++) {
> > +             snprintf(name, SZ_128, "dspp_%d", i);
> > +             msm_disp_snapshot_add_block(disp_state, name, cat->dspp[i].len,
> > +                             dpu_kms->mmio + cat->dspp[i].base);
> > +     }
> > +
> > +     /* dump INTF sub-blocks HW regs info */
> > +     for (i = 0; i < cat->intf_count; i++) {
> > +             snprintf(name, SZ_128, "intf_%d", i);
> > +             msm_disp_snapshot_add_block(disp_state, name, cat->intf[i].len,
> > +                             dpu_kms->mmio + cat->intf[i].base);
> > +     }
> > +
> > +     /* dump PP sub-blocks HW regs info */
> > +     for (i = 0; i < cat->pingpong_count; i++) {
> > +             snprintf(name, SZ_128, "ping-pong_%d", i);
>
> Just a nitpick: we usually call it pp or pingpong, without dash.
>
> > +             msm_disp_snapshot_add_block(disp_state, name, cat->pingpong[i].len,
> > +                             dpu_kms->mmio + cat->pingpong[i].base);
> > +     }
> > +
> > +     /* dump SSPP sub-blocks HW regs info */
> > +     for (i = 0; i < cat->sspp_count; i++) {
> > +             snprintf(name, SZ_128, "sspp_%d", i);
> > +             msm_disp_snapshot_add_block(disp_state, name, cat->sspp[i].len,
> > +                             dpu_kms->mmio + cat->sspp[i].base);
> > +     }
> > +
> > +     snprintf(name, SZ_128, "top");
> > +     msm_disp_snapshot_add_block(disp_state, name, top->hw.length,
> > +                     dpu_kms->mmio + top->hw.blk_off);
> > +
> > +     pm_runtime_put_sync(&dpu_kms->pdev->dev);
> > +}
> > +
> >   static const struct msm_kms_funcs kms_funcs = {
> >       .hw_init         = dpu_kms_hw_init,
> >       .irq_preinstall  = dpu_irq_preinstall,
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > index d6717d6..74b6ce6 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > @@ -13,6 +13,7 @@
> >   #include <drm/drm_drv.h>
> >
> >   #include "msm_drv.h"
> > +#include "disp/msm_disp_snapshot.h"
> >   #include "msm_kms.h"
> >   #include "msm_mmu.h"
> >   #include "msm_gem.h"
> > @@ -132,6 +133,8 @@ struct dpu_kms {
> >
> >       struct opp_table *opp_table;
> >
> > +     struct msm_disp_state *disp_state;
> > +
> >       struct dss_module_power mp;
> >
> >       /* reference count bandwidth requests, so we know when we can
> > @@ -265,4 +268,6 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder);
> >    */
> >   u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name);
> >
> > +void dpu_kms_mdp_snapshot(struct drm_device *dev);
> > +
> >   #endif /* __dpu_kms_H__ */
> > diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> > new file mode 100644
> > index 0000000..1bdad16
> > --- /dev/null
> > +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> > @@ -0,0 +1,161 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#define pr_fmt(fmt)  "[drm:%s:%d] " fmt, __func__, __LINE__
> > +
> > +#include "msm_disp_snapshot.h"
> > +
> > +#ifdef CONFIG_DEV_COREDUMP
> > +static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
> > +             size_t count, void *data, size_t datalen)
> > +{
> > +     struct drm_print_iterator iter;
> > +     struct drm_printer p;
> > +     struct msm_disp_state *disp_state;
> > +
> > +     disp_state = data;
> > +
> > +     iter.data = buffer;
> > +     iter.offset = 0;
> > +     iter.start = offset;
> > +     iter.remain = count;
> > +
> > +     p = drm_coredump_printer(&iter);
> > +
> > +     msm_disp_state_print(disp_state, &p, MSM_DISP_SNAPSHOT_IN_COREDUMP);
> > +
> > +     return count - iter.remain;
> > +}
> > +
> > +static void disp_devcoredump_free(void *data)
> > +{
> > +     struct msm_disp_state *disp_state;
> > +
> > +     disp_state = data;
> > +
> > +     msm_disp_state_free(disp_state);
> > +
> > +     disp_state->coredump_pending = false;
> > +}
> > +#endif /* CONFIG_DEV_COREDUMP */
> > +
> > +static void _msm_disp_snapshot_work(struct kthread_work *work)
> > +{
> > +     struct msm_disp_state *disp_state = container_of(work, struct msm_disp_state, dump_work);
> > +     struct drm_printer p;
> > +
> > +     mutex_lock(&disp_state->mutex);
> > +
> > +     msm_disp_snapshot_capture_state(disp_state);
> > +
> > +     if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
> > +             p = drm_info_printer(disp_state->drm_dev->dev);
> > +             msm_disp_state_print(disp_state, &p, MSM_DISP_SNAPSHOT_IN_LOG);
> > +     }
> > +
> > +     /*
> > +      * if devcoredump is not defined free the state immediately
> > +      * otherwise it will be freed in the free handler.
> > +      */
> > +#ifdef CONFIG_DEV_COREDUMP
> > +     dev_coredumpm(disp_state->dev, THIS_MODULE, disp_state, 0, GFP_KERNEL,
> > +                     disp_devcoredump_read, disp_devcoredump_free);
> > +     disp_state->coredump_pending = true;
> > +#else
> > +     msm_disp_state_free(disp_state);
> > +#endif
> > +
> > +     mutex_unlock(&disp_state->mutex);
> > +}
> > +
> > +void msm_disp_snapshot_state(struct drm_device *drm_dev)
> > +{
> > +     struct msm_drm_private *priv;
> > +     struct dpu_kms *dpu_kms;
> > +     struct msm_disp_state *disp_state;
> > +
> > +     if (!drm_dev) {
> > +             DRM_ERROR("invalid params\n");
> > +             return;
> > +     }
> > +
> > +     priv = drm_dev->dev_private;
> > +     dpu_kms = to_dpu_kms(priv->kms);
>
> If we are outside of disp/dpu1, we shouldn't reference dpu_kms
> internals. Probably this can be changed to use msm_drm_private, msm_kms
> or msm_mdss?

agreed

>
> > +     disp_state = dpu_kms->disp_state;
> > +
> > +     if (!disp_state) {
> > +             DRM_ERROR("invalid params\n");
> > +             return;
> > +     }
> > +
> > +     /*
> > +      * if there is a coredump pending return immediately till dump
> > +      * if read by userspace or timeout happens
>
> Where do we handle the timeout? Stale comment or did I miss something?
>
> > +      */
> > +     if (disp_state->coredump_pending) {
> > +             DRM_DEBUG("coredump is pending read\n");
> > +             return;
> > +     }
> > +
> > +     kthread_queue_work(disp_state->dump_worker,
> > +                     &disp_state->dump_work);
> > +}
> > +
> > +int msm_disp_snapshot_init(struct drm_device *drm_dev)
> > +{
> > +     struct dpu_kms *dpu_kms;
> > +     struct msm_drm_private *priv;
> > +     struct msm_disp_state *disp_state;
> > +
> > +     if (!drm_dev) {
> > +             DRM_ERROR("invalid params\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     priv = drm_dev->dev_private;
> > +     dpu_kms = to_dpu_kms(priv->kms);
> > +
> > +     disp_state = devm_kzalloc(drm_dev->dev, sizeof(struct msm_disp_state), GFP_KERNEL);
> > +
> > +     mutex_init(&disp_state->mutex);
> > +
> > +     disp_state->dev = drm_dev->dev;
> > +     disp_state->drm_dev = drm_dev;
> > +
> > +     INIT_LIST_HEAD(&disp_state->blocks);
> > +
> > +     disp_state->dump_worker = kthread_create_worker(0, "%s", "disp_snapshot");
> > +     if (IS_ERR(disp_state->dump_worker))
> > +             DRM_ERROR("failed to create disp state task\n");
> > +
> > +     kthread_init_work(&disp_state->dump_work, _msm_disp_snapshot_work);
> > +
> > +     dpu_kms->disp_state = disp_state;
>
> Judging from this code, I think it should be possible to push disp_state
> directly into struct msm_drm_private.

I'd be ok w/ msm_kms since it is display related, but msm_drm_private
is ok.. but defn should be higher level than dpu_kms

>
> > +
> > +     return 0;
> > +}
> > +
> > +void msm_disp_snapshot_destroy(struct drm_device *drm_dev)
> > +{
> > +     struct dpu_kms *dpu_kms;
> > +     struct msm_drm_private *priv;
> > +     struct msm_disp_state *disp_state;
> > +
> > +     if (!drm_dev) {
> > +             DRM_ERROR("invalid params\n");
> > +             return;
> > +     }
> > +
> > +     priv = drm_dev->dev_private;
> > +     dpu_kms = to_dpu_kms(priv->kms);
> > +     disp_state = dpu_kms->disp_state;
> > +
> > +     if (disp_state->dump_worker)
> > +             kthread_destroy_worker(disp_state->dump_worker);
> > +
> > +     list_del(&disp_state->blocks);
> > +
> > +     mutex_destroy(&disp_state->mutex);
> > +}
> > diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
> > new file mode 100644
> > index 0000000..0ccb7e7
> > --- /dev/null
> > +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
> > @@ -0,0 +1,167 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#ifndef MSM_DISP_SNAPSHOT_H_
> > +#define MSM_DISP_SNAPSHOT_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"
> > +
> > +enum msm_disp_snapshot_dump_flag {
> > +     MSM_DISP_SNAPSHOT_IN_LOG = BIT(0),
> > +     MSM_DISP_SNAPSHOT_IN_MEM = BIT(1),
> > +     MSM_DISP_SNAPSHOT_IN_COREDUMP = BIT(2),
>
> Are they mutually exclusive or can they be specified in parallel?

I'd kinda like to see less MSM_DISP_SNAPSHOT_IN_*.. and no
MSM_DISP_SNAPSHOT_IN_MEM

Like it is ok for caller requesting snapshot to ask for IN_LOG vs
IN_COREDUMP, which is really more of a "which drm_printer do you
pick", but it shouldn't be more than that.. both cases should call all
the sub-blocks to snapshot to memory, and after that it should be just
calling a msm_disp_snapshot_print() helper with an appropriate
drm_printer.. with msm_disp_snapshot_print() being mostly an
over-glorified hexdump.

So MSM_DISP_SNAPSHOT_IN_MEM should be dropped.. beyond that, if you
want a boolean vs bitmask or enum for what should be done with the
snapshot after it is collected, I'm not picky..


> > +};
> > +
> > +#define MSM_DISP_SNAPSHOT_MAX_BLKS           10
> > +
> > +/* debug option to print the registers in logs */
> > +#define MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE 0
> > +
> > +/* print debug ranges in groups of 4 u32s */
> > +#define REG_DUMP_ALIGN               16
> > +
> > +/**
> > + * struct msm_disp_state - structure to store current dpu state
> > + * @dev: device pointer
> > + * @drm_dev: drm device pointer
> > + * @mutex: mutex to serialize access to serialze dumps, debugfs access
> > + * @coredump_pending: coredump is pending read from userspace
> > + * @atomic_state: atomic state duplicated at the time of the error
> > + * @dump_worker: kworker thread which runs the dump work
> > + * @dump_work: kwork which dumps the registers and drm state
> > + * @timestamp: timestamp at which the coredump was captured
> > + */
> > +struct msm_disp_state {
> > +     struct device *dev;
> > +     struct drm_device *drm_dev;
> > +     struct mutex mutex;
> > +
> > +     bool coredump_pending;
> > +
> > +     struct list_head blocks;
> > +
> > +     struct drm_atomic_state *atomic_state;
> > +
> > +     struct kthread_worker *dump_worker;
> > +     struct kthread_work dump_work;
> > +     ktime_t timestamp;
> > +};
> > +
> > +/**
> > + * struct msm_disp_state_block - structure to store each hardware block state
> > + * @name: name of the block
> > + * @drm_dev: handle to the linked list head
> > + * @size: size of the register space of this hardware block
> > + * @state: array holding the register dump of this hardware block
> > + * @base_addr: starting address of this hardware block's register space
> > + */
> > +struct msm_disp_state_block {
> > +     char name[SZ_128];
> > +     struct list_head node;
> > +     unsigned int size;
> > +     u32 *state;
> > +     void __iomem *base_addr;
> > +};
> > +
> > +/**
> > + * MSM_DISP_SNAPSHOT - trigger to dump the display snapshot
> > + * @drm_dev: handle to drm device
> > + */
> > +#define MSM_DISP_SNAPSHOT(drm_dev) msm_disp_snapshot_state(drm_dev)
>
> Why do you need it?
>
> > +
> > +/**
> > + * msm_disp_snapshot_init - initialize global sde debug facilities: evtlog, regdump
>
> Just a nitpick here and below: there is no sde in the upstream. And no
> evtlog.
>
> > + * @drm_dev: drm device handle
> > + *
> > + * Returns:          0 or -ERROR
> > + */
> > +int msm_disp_snapshot_init(struct drm_device *drm_dev);
> > +
> > +/**
> > + * msm_disp_snapshot_destroy - destroy the global sde debug facilities
> > + * @drm_dev:    drm device handle
> > + *
> > + * Returns:  none
> > + */
> > +void msm_disp_snapshot_destroy(struct drm_device *drm_dev);
> > +
> > +/**
> > + * msm_disp_snapshot_state - trigger to dump the display snapshot
> > + * @drm_dev: handle to drm device
> > +
> > + * Returns:  none
> > + */
> > +void msm_disp_snapshot_state(struct drm_device *drm_dev);
> > +
> > +/**
> > + * msm_disp_state_get - get the handle to msm_disp_state struct from the drm device
> > + * @drm:         handle to drm device
> > +
> > + * Returns:  handle to the msm_disp_state struct
> > + */
> > +struct msm_disp_state *msm_disp_state_get(struct drm_device *drm);
> > +
> > +/**
> > + * msm_disp_state_print - print out the current dpu state
> > + * @disp_state:          handle to drm device
> > + * @p:           handle to drm printer
> > + * @reg_dump_method: tells whether the state has to be dumped in log or coredump
> > + *
> > + * Returns:  none
> > + */
> > +void msm_disp_state_print(struct msm_disp_state *disp_state, struct drm_printer *p,
> > +             u8 reg_dump_method);
> > +
> > +/**
> > + * msm_disp_snapshot_capture_state - utility to capture atomic state and hw registers
> > + * @disp_state:          handle to msm_disp_state struct
> > +
> > + * Returns:  none
> > + */
> > +void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state);
> > +
> > +/**
> > + * msm_disp_state_free - free the memory after the coredump has been read
> > + * @disp_state:          handle to struct msm_disp_state
> > +
> > + * Returns: none
> > + */
> > +void msm_disp_state_free(struct msm_disp_state *disp_state);
> > +
> > +/**
> > + * msm_disp_snapshot_add_block - add a hardware block with its register dump
> > + * @disp_state:          handle to struct msm_disp_state
> > + * @name:           name of the hardware block
> > + * @len:            size of the register space of the hardware block
> > + * @base_addr:      starting address of the register space of the hardware block
> > + *
> > + * Returns: none
> > + */
> > +void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, char *name, u32 len,
> > +             void __iomem *base_addr);
> > +
> > +#endif /* MSM_DISP_SNAPSHOT_H_ */
> > diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> > new file mode 100644
> > index 0000000..383f225
> > --- /dev/null
> > +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> > @@ -0,0 +1,181 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#define pr_fmt(fmt)  "[drm:%s:%d] " fmt, __func__, __LINE__
> > +
> > +#include "msm_disp_snapshot.h"
> > +
> > +static void msm_disp_state_dump_regs(u32 **reg, u32 len, void __iomem *base_addr,
> > +             u32 dump_op, struct drm_printer *p)
> > +{
> > +     u32 len_aligned, len_padded;
> > +     u32 x0, x4, x8, xc;
> > +     void __iomem *addr;
> > +     u32 *dump_addr = NULL;
> > +     void __iomem *end_addr;
> > +     int i;
> > +     bool in_drm_printer = false;
> > +
> > +     len_aligned = (len + REG_DUMP_ALIGN - 1) / REG_DUMP_ALIGN;
> > +     len_padded = len_aligned * REG_DUMP_ALIGN;
> > +
> > +     addr = base_addr;
> > +     end_addr = base_addr + len;
> > +
> > +     if ((dump_op == MSM_DISP_SNAPSHOT_IN_COREDUMP) ||
> > +             (dump_op == MSM_DISP_SNAPSHOT_IN_LOG))
> > +             in_drm_printer = true;
> > +
> > +     if (in_drm_printer && !p) {
> > +             DRM_ERROR("invalid drm printer\n");
> > +             return;
> > +     }
> > +
> > +     if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM && !(*reg))
> > +             *reg = kzalloc(len_padded, GFP_KERNEL);
> > +
> > +     if (*reg)
> > +             dump_addr = *reg;
> > +
> > +     for (i = 0; i < len_aligned; i++) {
> > +             if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM) {
> > +                     x0 = (addr < end_addr) ? readl_relaxed(addr + 0x0) : 0;
> > +                     x4 = (addr + 0x4 < end_addr) ? readl_relaxed(addr + 0x4) : 0;
> > +                     x8 = (addr + 0x8 < end_addr) ? readl_relaxed(addr + 0x8) : 0;
> > +                     xc = (addr + 0xc < end_addr) ? readl_relaxed(addr + 0xc) : 0;
> > +             }
> > +
> > +             if (dump_addr) {
> > +                     if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM) {
> > +                             dump_addr[i * 4] = x0;
> > +                             dump_addr[i * 4 + 1] = x4;
> > +                             dump_addr[i * 4 + 2] = x8;
> > +                             dump_addr[i * 4 + 3] = xc;
> > +                     } else if (in_drm_printer) {
> > +                             drm_printf(p, "0x%lx : %08x %08x %08x %08x\n",
> > +                                             (unsigned long)(addr - base_addr),
> > +                                             dump_addr[i * 4], dump_addr[i * 4 + 1],
> > +                                             dump_addr[i * 4 + 2], dump_addr[i * 4 + 3]);
> > +                     }
> > +             }
> > +
> > +             addr += REG_DUMP_ALIGN;
> > +     }
> > +}
>
> I'd just split this function into two different cases, IN_MEM and
> DRM_PRINTER.
>
> > +
> > +struct msm_disp_state *msm_disp_state_get(struct drm_device *drm)
> > +{
> > +     struct msm_drm_private *priv;
> > +     struct dpu_kms *dpu_kms;
> > +
> > +     priv = drm->dev_private;
> > +     dpu_kms = to_dpu_kms(priv->kms);
> > +
> > +     return dpu_kms->disp_state;
> > +}
> > +
> > +void msm_disp_state_print(struct msm_disp_state *state, struct drm_printer *p,
> > +             u8 reg_dump_method)
> > +{
> > +     struct msm_disp_state_block *block, *tmp;
> > +
> > +     if (!p) {
> > +             DRM_ERROR("invalid drm printer\n");
> > +             return;
> > +     }
> > +
> > +     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(state->timestamp));
> > +
> > +     list_for_each_entry_safe(block, tmp, &state->blocks, node) {
> > +             drm_printf(p, "====================%s================\n", block->name);
> > +             msm_disp_state_dump_regs(&block->state, block->size, block->base_addr,
> > +                             reg_dump_method, p);
> > +     }
> > +
> > +     drm_printf(p, "===================dpu drm state================\n");
> > +
> > +     if (state->atomic_state)
> > +             drm_atomic_print_new_state(state->atomic_state, p);
> > +}
> > +
> > +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();
> > +
> > +     ddev = disp_state->drm_dev;
> > +
> > +     drm_modeset_acquire_init(&ctx, 0);
> > +
> > +     while (drm_modeset_lock_all_ctx(ddev, &ctx) != 0)
> > +             drm_modeset_backoff(&ctx);
> > +
> > +     disp_state->atomic_state = drm_atomic_helper_duplicate_state(ddev,
> > +                     &ctx);
> > +     drm_modeset_drop_locks(&ctx);
> > +     drm_modeset_acquire_fini(&ctx);
> > +}
> > +
> > +void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
> > +{
> > +     struct msm_drm_private *priv;
> > +     int i;
> > +     struct drm_device *drm_dev;
> > +
> > +     drm_dev = disp_state->drm_dev;
> > +     priv = drm_dev->dev_private;
> > +
> > +     msm_disp_capture_atomic_state(disp_state);
> > +
> > +     if (priv->dp)
> > +             msm_dp_snapshot(priv->dp);
> > +
> > +     for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
> > +             if (!priv->dsi[i])
> > +                     continue;
> > +
> > +             msm_dsi_snapshot(priv->dsi[i]);
> > +     }
> > +
> > +     dpu_kms_mdp_snapshot(drm_dev);
>
> Hmm, I now see why do you need the dpu_kms. Can we have a check for the
> mdp version here? The rest of the code seems generic enough, so we can
> at least dump the dsi/dp (and hopefully eDP/HDMI) data for MDP4/5.

or instead:

   if (kms->funcs->snapshot)
      kms->funcs->snapshot(kms)

At this level we shouldn't depend on dpu.. just add a kms vfunc.


>
> Ideally subdrivers should be able to report back to the core, which
> regions are available, so that we won't have this static code. But this
> might look like an overkill. Not to mention that it might lead to the
> not-so-static sequence of dumps. (As you can judge, I quite liked the
> previous approach, when the drivers registered the dumping regions and
> then the dumper would go through them.)

To be fair, I argued against registration.. at the top level, there
are just a few things: hdmi, edp, dp, and dsi's.. so one small
function to call the sub-blocks is a lot less code than inventing a
registration infrastructure..  the "kms" object is the one place (so
far) where we need a virtual method..  let's just add it to kms->funcs

BR,
-R

> Let's have the static config in place with the version check.
>
> > +}
> > +
> > +void msm_disp_state_free(struct msm_disp_state *disp_state)
> > +{
> > +     struct msm_disp_state_block *block, *tmp;
> > +
> > +     if (disp_state->atomic_state) {
> > +             drm_atomic_state_put(disp_state->atomic_state);
> > +             disp_state->atomic_state = NULL;
> > +     }
> > +
> > +     list_for_each_entry_safe(block, tmp, &disp_state->blocks, node) {
> > +             kfree(block->state);
> > +             list_del(&block->node);
>
> I'd swap these two lines, so while the block is in the list, its state
> is coherent.
>
> > +             kfree(block);
> > +     }
> > +}
> > +
> > +void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, char *name, u32 len,
> > +             void __iomem *base_addr)
> > +{
> > +     struct msm_disp_state_block *new_blk;
> > +
> > +     new_blk = kzalloc(sizeof(struct msm_disp_state_block), GFP_KERNEL);
> > +
> > +     strscpy(new_blk->name, name, sizeof(new_blk->name));
> > +     INIT_LIST_HEAD(&new_blk->node);
> > +     new_blk->size = len;
> > +     new_blk->base_addr = base_addr;
> > +     msm_disp_state_dump_regs(&new_blk->state, len, base_addr,
> > +                     MSM_DISP_SNAPSHOT_IN_MEM, NULL);
> > +     list_add(&new_blk->node, &disp_state->blocks);
> > +}
> > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > index b1a9b1b..ce8d19a 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > @@ -62,6 +62,18 @@ struct dp_catalog_private {
> >       u8 aux_lut_cfg_index[PHY_AUX_CFG_MAX];
> >   };
> >
> > +void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct msm_disp_state *disp_state)
> > +{
> > +     struct dp_catalog_private *catalog = container_of(dp_catalog,
> > +                     struct dp_catalog_private, dp_catalog);
> > +     char name[SZ_128];
> > +
> > +     snprintf(name, SZ_128, "dp_ctrl");
> > +
> > +     msm_disp_snapshot_add_block(disp_state, name, catalog->io->dp_controller.len,
> > +                     catalog->io->dp_controller.base);
> > +}
> > +
> >   static inline u32 dp_read_aux(struct dp_catalog_private *catalog, u32 offset)
> >   {
> >       offset += MSM_DP_CONTROLLER_AUX_OFFSET;
> > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
> > index 176a902..e7e8b13 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> > @@ -9,6 +9,7 @@
> >   #include <drm/drm_modes.h>
> >
> >   #include "dp_parser.h"
> > +#include "disp/msm_disp_snapshot.h"
> >
> >   /* interrupts */
> >   #define DP_INTR_HPD         BIT(0)
> > @@ -71,6 +72,9 @@ struct dp_catalog {
> >       u32 audio_data;
> >   };
> >
> > +/* Debug module */
> > +void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct msm_disp_state *disp_state);
> > +
> >   /* AUX APIs */
> >   u32 dp_catalog_aux_read_data(struct dp_catalog *dp_catalog);
> >   int dp_catalog_aux_write_data(struct dp_catalog *dp_catalog);
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 5a39da6..6670558 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -1009,6 +1009,35 @@ int dp_display_get_test_bpp(struct msm_dp *dp)
> >               dp_display->link->test_video.test_bit_depth);
> >   }
> >
> > +void msm_dp_snapshot(struct msm_dp *dp)
> > +{
> > +     struct dp_display_private *dp_display;
> > +     struct drm_device *drm;
> > +     struct msm_disp_state *disp_state;
> > +
> > +     dp_display = container_of(dp, struct dp_display_private, dp_display);
> > +     drm = dp->drm_dev;
> > +     disp_state = msm_disp_state_get(drm);
> > +
> > +     /*
> > +      * if we are reading registers we need the link clocks to be on
> > +      * however till DP cable is connected this will not happen as we
> > +      * do not know the resolution to power up with. Hence check the
> > +      * power_on status before dumping DP registers to avoid crash due
> > +      * to unclocked access
> > +      */
> > +     mutex_lock(&dp_display->event_mutex);
> > +
> > +     if (!dp->power_on) {
> > +             mutex_unlock(&dp_display->event_mutex);
> > +             return;
> > +     }
> > +
> > +     dp_catalog_snapshot(dp_display->catalog, disp_state);
> > +
> > +     mutex_unlock(&dp_display->event_mutex);
> > +}
> > +
> >   static void dp_display_config_hpd(struct dp_display_private *dp)
> >   {
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> > index 6092ba1..4d39373 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> > @@ -8,6 +8,7 @@
> >
> >   #include "dp_panel.h"
> >   #include <sound/hdmi-codec.h>
> > +#include "disp/msm_disp_snapshot.h"
> >
> >   struct msm_dp {
> >       struct drm_device *drm_dev;
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
> > index 62704885..bccc006 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> > @@ -266,3 +266,8 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
> >       return ret;
> >   }
> >
> > +void msm_dsi_snapshot(struct msm_dsi *msm_dsi)
> > +{
> > +     msm_dsi_host_snapshot(msm_dsi->host);
> > +}
> > +
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> > index 7abfeab..bb39403 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi.h
> > +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> > @@ -15,6 +15,7 @@
> >   #include <drm/drm_panel.h>
> >
> >   #include "msm_drv.h"
> > +#include "disp/msm_disp_snapshot.h"
> >
> >   #define DSI_0       0
> >   #define DSI_1       1
> > @@ -90,6 +91,8 @@ static inline bool msm_dsi_device_connected(struct msm_dsi *msm_dsi)
> >       return msm_dsi->panel || msm_dsi->external_bridge;
> >   }
> >
> > +void msm_dsi_snapshot(struct msm_dsi *msm_dsi);
> > +
> >   struct drm_encoder *msm_dsi_get_encoder(struct msm_dsi *msm_dsi);
> >
> >   /* dsi host */
> > @@ -146,6 +149,7 @@ int dsi_clk_init_v2(struct msm_dsi_host *msm_host);
> >   int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host);
> >   int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_dual_dsi);
> >   int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_dual_dsi);
> > +void msm_dsi_host_snapshot(struct mipi_dsi_host *host);
> >
> >   /* dsi phy */
> >   struct msm_dsi_phy;
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index 8a10e43..d9fdc07 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -2487,3 +2487,22 @@ 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_snapshot(struct mipi_dsi_host *host)
> > +{
> > +     struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
> > +     struct drm_device *dev = msm_host->dev;
> > +     struct msm_disp_state *disp_state;
> > +     char name[SZ_128];
> > +
> > +     disp_state = msm_disp_state_get(dev);
> > +
> > +     pm_runtime_get_sync(&msm_host->pdev->dev);
> > +
> > +     snprintf(name, SZ_128, "dsi%d_ctrl", msm_host->id);
> > +
> > +     msm_disp_snapshot_add_block(disp_state, name, msm_iomap_size(msm_host->pdev, "dsi_ctrl"),
>
> You can store the size into the msm_host, rather than querrying it on
> each dump.
>
> > +                     msm_host->ctrl_base);
> > +
> > +     pm_runtime_put_sync(&msm_host->pdev->dev);
> > +}
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index e1104d2..ebf9283 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -1,6 +1,6 @@
> >   // SPDX-License-Identifier: GPL-2.0-only
> >   /*
> > - * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> > + * Copyright (c) 2016-2018, 2020-2021 The Linux Foundation. All rights reserved.
> >    * Copyright (C) 2013 Red Hat
> >    * Author: Rob Clark <robdclark@gmail.com>
> >    */
> > @@ -19,6 +19,7 @@
> >   #include <drm/drm_of.h>
> >   #include <drm/drm_vblank.h>
> >
> > +#include "disp/msm_disp_snapshot.h"
> >   #include "msm_drv.h"
> >   #include "msm_debugfs.h"
> >   #include "msm_fence.h"
> > @@ -167,6 +168,24 @@ void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name,
> >       return _msm_ioremap(pdev, name, dbgname, true);
> >   }
> >
> > +unsigned long msm_iomap_size(struct platform_device *pdev, const char *name)
> > +{
> > +     struct resource *res;
> > +
> > +     if (name)
> > +             res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> > +     else
> > +             res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +
> > +     if (!res) {
> > +             dev_dbg(&pdev->dev, "failed to get memory resource: %s\n",
> > +                             name);
> > +             return 0;
> > +     }
> > +
> > +     return resource_size(res);
> > +}
> > +
> >   void msm_writel(u32 data, void __iomem *addr)
> >   {
> >       if (reglog)
> > @@ -278,6 +297,8 @@ static int msm_drm_uninit(struct device *dev)
> >               msm_fbdev_free(ddev);
> >   #endif
> >
> > +     msm_disp_snapshot_destroy(ddev);
> > +
> >       drm_mode_config_cleanup(ddev);
> >
> >       pm_runtime_get_sync(dev);
> > @@ -550,6 +571,12 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> >       if (ret)
> >               goto err_msm_uninit;
> >
> > +     if (get_mdp_ver(pdev) == KMS_DPU) {
> > +             ret = msm_disp_snapshot_init(ddev);
> > +             if (ret)
> > +                     DRM_DEV_ERROR(dev, "msm_disp_snapshot_init failed ret = %d\n", ret);
> > +     }
> > +
>
> Let's get it out of if (KMS_DPU), even if we do not dump mdp4/mdp5
> registers for now.
>
> >       drm_mode_config_reset(ddev);
> >
> >   #ifdef CONFIG_DRM_FBDEV_EMULATION
> > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> > index 2668941..9c40bac 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.h
> > +++ b/drivers/gpu/drm/msm/msm_drv.h
> > @@ -367,6 +367,7 @@ void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,
> >                               struct drm_display_mode *mode,
> >                               struct drm_display_mode *adjusted_mode);
> >   void msm_dp_irq_postinstall(struct msm_dp *dp_display);
> > +void msm_dp_snapshot(struct msm_dp *dp_display);
> >
> >   void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
> >
> > @@ -450,6 +451,7 @@ void __iomem *msm_ioremap(struct platform_device *pdev, const char *name,
> >               const char *dbgname);
> >   void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name,
> >               const char *dbgname);
> > +unsigned long msm_iomap_size(struct platform_device *pdev, const char *name);
> >   void msm_writel(u32 data, void __iomem *addr);
> >   u32 msm_readl(const void __iomem *addr);
> >   void msm_rmw(void __iomem *addr, u32 mask, u32 or);
> >
>
>
> --
> With best wishes
> Dmitry

[PATCH v4 3/3] drm/msm: add disp snapshot points across dpu driver

[Abhinav Kumar]

Add snapshot points across dpu driver to trigger dumps when critical
errors are hit.

changes in v4:
 - change the callers to use the new snapshot macro

Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          | 18 +++++++++++++++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 14 +++++++++-----
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c |  8 +++++++-
 3 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 8d94205..39b2df9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Copyright (c) 2014-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2014-2018, 2020-2021 The Linux Foundation. All rights reserved.
  * Copyright (C) 2013 Red Hat
  * Author: Rob Clark <robdclark@gmail.com>
  */
@@ -26,6 +26,7 @@
 #include "dpu_crtc.h"
 #include "dpu_trace.h"
 #include "dpu_core_irq.h"
+#include "disp/msm_disp_snapshot.h"
 
 #define DPU_DEBUG_ENC(e, fmt, ...) DPU_DEBUG("enc%d " fmt,\
 		(e) ? (e)->base.base.id : -1, ##__VA_ARGS__)
@@ -1336,6 +1337,13 @@ static void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc,
 
 	DPU_ATRACE_BEGIN("encoder_underrun_callback");
 	atomic_inc(&phy_enc->underrun_cnt);
+
+	/* trigger dump only on the first underrun */
+	if (atomic_read(&phy_enc->underrun_cnt) == 1) {
+		pr_err("triggering dump\n");
+		MSM_DISP_SNAPSHOT(drm_enc->dev);
+	}
+
 	trace_dpu_enc_underrun_cb(DRMID(drm_enc),
 				  atomic_read(&phy_enc->underrun_cnt));
 	DPU_ATRACE_END("encoder_underrun_callback");
@@ -1565,19 +1573,23 @@ static void dpu_encoder_helper_hw_reset(struct dpu_encoder_phys *phys_enc)
 	struct dpu_encoder_virt *dpu_enc;
 	struct dpu_hw_ctl *ctl;
 	int rc;
+	struct drm_encoder *drm_enc;
 
 	dpu_enc = to_dpu_encoder_virt(phys_enc->parent);
 	ctl = phys_enc->hw_ctl;
+	drm_enc = phys_enc->parent;
 
 	if (!ctl->ops.reset)
 		return;
 
-	DRM_DEBUG_KMS("id:%u ctl %d reset\n", DRMID(phys_enc->parent),
+	DRM_DEBUG_KMS("id:%u ctl %d reset\n", DRMID(drm_enc),
 		      ctl->idx);
 
 	rc = ctl->ops.reset(ctl);
-	if (rc)
+	if (rc) {
 		DPU_ERROR_ENC(dpu_enc, "ctl %d reset failure\n",  ctl->idx);
+		MSM_DISP_SNAPSHOT(drm_enc->dev);
+	}
 
 	phys_enc->enable_state = DPU_ENC_ENABLED;
 }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index b2be39b..f587985 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Copyright (c) 2015-2018 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All rights reserved.
  */
 
 #define pr_fmt(fmt)	"[drm:%s:%d] " fmt, __func__, __LINE__
@@ -11,6 +11,7 @@
 #include "dpu_core_irq.h"
 #include "dpu_formats.h"
 #include "dpu_trace.h"
+#include "disp/msm_disp_snapshot.h"
 
 #define DPU_DEBUG_CMDENC(e, fmt, ...) DPU_DEBUG("enc%d intf%d " fmt, \
 		(e) && (e)->base.parent ? \
@@ -191,10 +192,13 @@ static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
 			to_dpu_encoder_phys_cmd(phys_enc);
 	u32 frame_event = DPU_ENCODER_FRAME_EVENT_ERROR;
 	bool do_log = false;
+	struct drm_encoder *drm_enc;
 
 	if (!phys_enc->hw_pp)
 		return -EINVAL;
 
+	drm_enc = phys_enc->parent;
+
 	cmd_enc->pp_timeout_report_cnt++;
 	if (cmd_enc->pp_timeout_report_cnt == PP_TIMEOUT_MAX_TRIALS) {
 		frame_event |= DPU_ENCODER_FRAME_EVENT_PANEL_DEAD;
@@ -203,7 +207,7 @@ static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
 		do_log = true;
 	}
 
-	trace_dpu_enc_phys_cmd_pdone_timeout(DRMID(phys_enc->parent),
+	trace_dpu_enc_phys_cmd_pdone_timeout(DRMID(drm_enc),
 		     phys_enc->hw_pp->idx - PINGPONG_0,
 		     cmd_enc->pp_timeout_report_cnt,
 		     atomic_read(&phys_enc->pending_kickoff_cnt),
@@ -212,12 +216,12 @@ static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
 	/* to avoid flooding, only log first time, and "dead" time */
 	if (do_log) {
 		DRM_ERROR("id:%d pp:%d kickoff timeout %d cnt %d koff_cnt %d\n",
-			  DRMID(phys_enc->parent),
+			  DRMID(drm_enc),
 			  phys_enc->hw_pp->idx - PINGPONG_0,
 			  phys_enc->hw_ctl->idx - CTL_0,
 			  cmd_enc->pp_timeout_report_cnt,
 			  atomic_read(&phys_enc->pending_kickoff_cnt));
-
+		MSM_DISP_SNAPSHOT(drm_enc->dev);
 		dpu_encoder_helper_unregister_irq(phys_enc, INTR_IDX_RDPTR);
 	}
 
@@ -228,7 +232,7 @@ static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
 
 	if (phys_enc->parent_ops->handle_frame_done)
 		phys_enc->parent_ops->handle_frame_done(
-				phys_enc->parent, phys_enc, frame_event);
+				drm_enc, phys_enc, frame_event);
 
 	return -ETIMEDOUT;
 }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 0e06b7e..9fdf888 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/* Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All rights reserved.
  */
 
 #define pr_fmt(fmt)	"[drm:%s:%d] " fmt, __func__, __LINE__
@@ -9,6 +9,7 @@
 #include "dpu_core_irq.h"
 #include "dpu_formats.h"
 #include "dpu_trace.h"
+#include "disp/msm_disp_snapshot.h"
 
 #define DPU_DEBUG_VIDENC(e, fmt, ...) DPU_DEBUG("enc%d intf%d " fmt, \
 		(e) && (e)->parent ? \
@@ -468,6 +469,7 @@ static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc)
 		"update pending flush ctl %d intf %d\n",
 		ctl->idx - CTL_0, phys_enc->hw_intf->idx);
 
+	atomic_set(&phys_enc->underrun_cnt, 0);
 
 	/* ctl_flush & timing engine enable will be triggered by framework */
 	if (phys_enc->enable_state == DPU_ENC_DISABLED)
@@ -537,6 +539,9 @@ static void dpu_encoder_phys_vid_prepare_for_kickoff(
 {
 	struct dpu_hw_ctl *ctl;
 	int rc;
+	struct drm_encoder *drm_enc;
+
+	drm_enc = phys_enc->parent;
 
 	ctl = phys_enc->hw_ctl;
 	if (!ctl->ops.wait_reset_status)
@@ -550,6 +555,7 @@ static void dpu_encoder_phys_vid_prepare_for_kickoff(
 	if (rc) {
 		DPU_ERROR_VIDENC(phys_enc, "ctl %d reset failure: %d\n",
 				ctl->idx, rc);
+		MSM_DISP_SNAPSHOT(drm_enc->dev);
 		dpu_encoder_helper_unregister_irq(phys_enc, INTR_IDX_VSYNC);
 	}
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project