[RFC 0/8] drm/msm: split DSI PHY to generic PHY subsystem

[RFC 0/8] drm/msm: split DSI PHY to generic PHY subsystem

[Dmitry Baryshkov]

This patchseries is an RFC for splitting Qualcomm DSI PHY from drm/msm
driver to generic PHY subsystem.

Dependencies:
 - msm_disp_snapshot fix at https://lore.kernel.org/linux-arm-msm/20210527220330.3364716-1-dmitry.baryshkov@linaro.org/
 - Patches 1-7 from https://lore.kernel.org/linux-arm-msm/20210515131217.1540412-1-dmitry.baryshkov@linaro.org/

PATCH 8 from this series is quite big, it might be split further if
required.

[RFC 1/8] drm/msm/dsi: make msm_dsi_phy_pll_restore_state static function

[Dmitry Baryshkov]

msm_dsi_phy_pll_restore_state() is only called from msm_dsi_phy_enable(),
so there is no need to export it. Mark it static.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi.h         |  1 -
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 30 +++++++++++++--------------
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 876053ba615b..c467ad609453 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -170,7 +170,6 @@ void msm_dsi_phy_disable(struct msm_dsi_phy *phy);
 void msm_dsi_phy_set_usecase(struct msm_dsi_phy *phy,
 			     enum msm_dsi_phy_usecase uc);
 void msm_dsi_phy_pll_save_state(struct msm_dsi_phy *phy);
-int msm_dsi_phy_pll_restore_state(struct msm_dsi_phy *phy);
 void msm_dsi_phy_snapshot(struct msm_disp_state *disp_state, struct msm_dsi_phy *phy);
 
 #endif /* __DSI_CONNECTOR_H__ */
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index f479e37d6428..36878504bbb8 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -559,6 +559,21 @@ static void dsi_phy_disable_resource(struct msm_dsi_phy *phy)
 	pm_runtime_put_autosuspend(&phy->pdev->dev);
 }
 
+static int msm_dsi_phy_pll_restore_state(struct msm_dsi_phy *phy)
+{
+	int ret;
+
+	if (phy->cfg->ops.restore_pll_state && phy->state_saved) {
+		ret = phy->cfg->ops.restore_pll_state(phy);
+		if (ret)
+			return ret;
+
+		phy->state_saved = false;
+	}
+
+	return 0;
+}
+
 static const struct of_device_id dsi_phy_dt_match[] = {
 #ifdef CONFIG_DRM_MSM_DSI_28NM_PHY
 	{ .compatible = "qcom,dsi-phy-28nm-hpm",
@@ -838,21 +853,6 @@ void msm_dsi_phy_pll_save_state(struct msm_dsi_phy *phy)
 	}
 }
 
-int msm_dsi_phy_pll_restore_state(struct msm_dsi_phy *phy)
-{
-	int ret;
-
-	if (phy->cfg->ops.restore_pll_state && phy->state_saved) {
-		ret = phy->cfg->ops.restore_pll_state(phy);
-		if (ret)
-			return ret;
-
-		phy->state_saved = false;
-	}
-
-	return 0;
-}
-
 void msm_dsi_phy_snapshot(struct msm_disp_state *disp_state, struct msm_dsi_phy *phy)
 {
 	msm_disp_snapshot_add_block(disp_state,
-- 
2.30.2

[RFC 2/8] drm/msm/dsi: save PLL registers across first PHY reset

[Dmitry Baryshkov]

From: Benjamin Li <benl@squareup.com>

Take advantage of previously-added support for persisting PLL
registers across DSI PHY disable/enable cycles (see 328e1a6
'drm/msm/dsi: Save/Restore PLL status across PHY reset') to
support persisting across the very first DSI PHY enable at
boot.

The bootloader may have left the PLL registers in a non-default
state. For example, for dsi_pll_28nm.c on 8x16/8x39, the byte
clock mux's power-on reset configuration is to bypass DIV1, but
depending on bandwidth requirements[1] the bootloader may have
set the DIV1 path.

When the byte clock mux is registered with the generic clock
framework at probe time, the framework reads & caches the value
of the mux bit field (the initial clock parent). After PHY enable,
when clk_set_rate is called on the byte clock, the framework
assumes there is no need to reparent, and doesn't re-write the
mux bit field. But PHY enable resets PLL registers, so the mux
bit field actually silently reverted to the DIV1 bypass path.
This causes the byte clock to be off by a factor of e.g. 2 for
our tested WXGA panel.

The above issue manifests as the display not working and a
constant stream of FIFO/LP0 contention errors.

[1] The specific requirement for triggering the DIV1 path (and
thus this issue) on 28nm is a panel with pixel clock <116.7MHz
(one-third the minimum VCO setting). FHD/1080p (~145MHz) is fine,
WXGA/1280x800 (~75MHz) is not.

Signed-off-by: Benjamin Li <benl@squareup.com>
---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 36878504bbb8..e5d25b44f8cb 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -738,6 +738,22 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
 		goto fail;
 	}
 
+	/*
+	 * As explained in msm_dsi_phy_enable, resetting the DSI PHY (as done
+	 * in dsi_mgr_phy_enable) silently changes its PLL registers to power-on
+	 * defaults, but the generic clock framework manages and caches several
+	 * of the PLL registers. It initializes these caches at registration
+	 * time via register read.
+	 *
+	 * As a result, we need to save DSI PLL registers once at probe in order
+	 * for the first call to msm_dsi_phy_enable to successfully bring PLL
+	 * registers back in line with what the generic clock framework expects.
+	 *
+	 * Subsequent PLL restores during msm_dsi_phy_enable will always be
+	 * paired with PLL saves in msm_dsi_phy_disable.
+	 */
+	msm_dsi_phy_pll_save_state(phy);
+
 	dsi_phy_disable_resource(phy);
 
 	platform_set_drvdata(pdev, phy);
-- 
2.30.2

[RFC 3/8] drm/msm/dsi: drop msm_dsi_phy_pll_save_state from 7nm and 10nm drivers

[Dmitry Baryshkov]

Stop calling msm_dsi_phy_pll_save_state() from dsi_pll_7nm_init() and
dsi_pll_10nm_init(), as this is handled now by the generic code.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c | 3 ---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c  | 3 ---
 2 files changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
index 34bc93548fcf..229e98a496f7 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
@@ -706,9 +706,6 @@ static int dsi_pll_10nm_init(struct msm_dsi_phy *phy)
 
 	phy->vco_hw = &pll_10nm->clk_hw;
 
-	/* TODO: Remove this when we have proper display handover support */
-	msm_dsi_phy_pll_save_state(phy);
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
index e76ce40a12ab..752a78c9dfcc 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
@@ -734,9 +734,6 @@ static int dsi_pll_7nm_init(struct msm_dsi_phy *phy)
 
 	phy->vco_hw = &pll_7nm->clk_hw;
 
-	/* TODO: Remove this when we have proper display handover support */
-	msm_dsi_phy_pll_save_state(phy);
-
 	return 0;
 }
 
-- 
2.30.2

[RFC 4/8] drm/msm/dsi: move msm_dsi_phy_pll_save_state call to msm_dsi_phy_disable

[Dmitry Baryshkov]

Move DSI PHY state saving from dsi manager to dsi_phy driver. This way
the manager does not need to know that the DSI state is not preserved
acrosss resets. Everything is handled by the DSI PHY driver.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi.h         |  1 -
 drivers/gpu/drm/msm/dsi/dsi_manager.c |  3 ---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 21 ++++++++++++---------
 3 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index c467ad609453..d64db1badd4b 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -169,7 +169,6 @@ int msm_dsi_phy_enable(struct msm_dsi_phy *phy,
 void msm_dsi_phy_disable(struct msm_dsi_phy *phy);
 void msm_dsi_phy_set_usecase(struct msm_dsi_phy *phy,
 			     enum msm_dsi_phy_usecase uc);
-void msm_dsi_phy_pll_save_state(struct msm_dsi_phy *phy);
 void msm_dsi_phy_snapshot(struct msm_disp_state *disp_state, struct msm_dsi_phy *phy);
 
 #endif /* __DSI_CONNECTOR_H__ */
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 88d56a2bc8ab..de1c41dc5c15 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -524,9 +524,6 @@ static void dsi_mgr_bridge_post_disable(struct drm_bridge *bridge)
 								id, ret);
 	}
 
-	/* Save PHY status if it is a clock source */
-	msm_dsi_phy_pll_save_state(msm_dsi->phy);
-
 	ret = msm_dsi_host_power_off(host);
 	if (ret)
 		pr_err("%s: host %d power off failed,%d\n", __func__, id, ret);
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index e5d25b44f8cb..b7b178cc318b 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -559,6 +559,14 @@ static void dsi_phy_disable_resource(struct msm_dsi_phy *phy)
 	pm_runtime_put_autosuspend(&phy->pdev->dev);
 }
 
+static void msm_dsi_phy_pll_save_state(struct msm_dsi_phy *phy)
+{
+	if (phy->cfg->ops.save_pll_state) {
+		phy->cfg->ops.save_pll_state(phy);
+		phy->state_saved = true;
+	}
+}
+
 static int msm_dsi_phy_pll_restore_state(struct msm_dsi_phy *phy)
 {
 	int ret;
@@ -845,6 +853,10 @@ int msm_dsi_phy_enable(struct msm_dsi_phy *phy,
 
 void msm_dsi_phy_disable(struct msm_dsi_phy *phy)
 {
+	/* Save PHY status if it is a clock source */
+	if (phy->usecase != MSM_DSI_PHY_SLAVE)
+		msm_dsi_phy_pll_save_state(phy);
+
 	if (!phy || !phy->cfg->ops.disable)
 		return;
 
@@ -860,15 +872,6 @@ void msm_dsi_phy_set_usecase(struct msm_dsi_phy *phy,
 	if (phy)
 		phy->usecase = uc;
 }
-
-void msm_dsi_phy_pll_save_state(struct msm_dsi_phy *phy)
-{
-	if (phy->cfg->ops.save_pll_state) {
-		phy->cfg->ops.save_pll_state(phy);
-		phy->state_saved = true;
-	}
-}
-
 void msm_dsi_phy_snapshot(struct msm_disp_state *disp_state, struct msm_dsi_phy *phy)
 {
 	msm_disp_snapshot_add_block(disp_state,
-- 
2.30.2

[RFC 5/8] lib: add small API for handling register snapshots

[Dmitry Baryshkov]

Add small API covering lists of register dumps. Currently this is a part
of MSM DRM driver, but is extracted as it might be usefull to other
drivers too.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 include/linux/dump_state.h | 78 ++++++++++++++++++++++++++++++++++++++
 lib/Kconfig                |  3 ++
 lib/Makefile               |  1 +
 lib/dump_state.c           | 51 +++++++++++++++++++++++++
 4 files changed, 133 insertions(+)
 create mode 100644 include/linux/dump_state.h
 create mode 100644 lib/dump_state.c

diff --git a/include/linux/dump_state.h b/include/linux/dump_state.h
new file mode 100644
index 000000000000..9cf2cd2e99a6
--- /dev/null
+++ b/include/linux/dump_state.h
@@ -0,0 +1,78 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2021, Linaro Ltd
+ */
+
+#ifndef LINUX_DUMP_STATE_H
+#define LINUX_DUMP_STATE_H
+
+#include <linux/list.h>
+#include <linux/sizes.h>
+
+/**
+ * struct dump_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 dump_state_block {
+	char name[SZ_128];
+	struct list_head node;
+	void __iomem *base_addr;
+	size_t size;
+	u8 state[0] __aligned(8);
+};
+
+/**
+ * struct dump_state - structure to store reg dump state
+ * @blocks: hardware blocks info related to this dump state
+ */
+struct dump_state {
+	struct list_head blocks;
+};
+
+static inline void dump_state_init(struct dump_state *state)
+{
+	INIT_LIST_HEAD(&state->blocks);
+}
+
+#define dump_state_for_each_block(state, block) \
+	list_for_each_entry(block, &(state)->blocks, node)
+
+/**
+ * dump_state_free_blocks - free allocated blocks
+ * @state:	    handle to struct dump_state
+ */
+void dump_state_free_blocks(struct dump_state *state);
+
+/**
+ * dump_state_allocate_block - allocate data block for register dumps
+ * @len:            size of the register space of the hardware block
+ * @base_addr:      starting address of the register space of the hardware block
+ * @gfp:            type of memory allocation
+ * @fmt:            format in which the block names need to be printed
+ *
+ * Returns: new block
+ */
+extern __printf(4, 5)
+struct dump_state_block *dump_state_allocate_block(void __iomem *base_addr, size_t len, gfp_t gfp, const char *fmt, ...);
+
+/**
+ * dump_state_allocate_block_va - allocate data block for register dumps
+ * @len:            size of the register space of the hardware block
+ * @base_addr:      starting address of the register space of the hardware block
+ * @gfp:            type of memory allocation
+ * @fmt:            format in which the block names need to be printed
+ *
+ * Returns: new block
+ */
+extern __printf(4, 0)
+struct dump_state_block *dump_state_allocate_block_va(void __iomem *base_addr, size_t len, gfp_t gfp, const char *fmt, va_list args);
+
+#define dump_state_add_block(state, block) \
+	list_add_tail(&(state)->blocks, &(block)->node)
+
+#endif
diff --git a/lib/Kconfig b/lib/Kconfig
index ac3b30697b2b..ab654232ecb6 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -704,3 +704,6 @@ config PLDMFW
 
 config ASN1_ENCODER
        tristate
+
+config DUMP_STATE
+	tristate
diff --git a/lib/Makefile b/lib/Makefile
index 2cc359ec1fdd..4836a0e3aef2 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -356,3 +356,4 @@ obj-$(CONFIG_BITS_TEST) += test_bits.o
 obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
 
 obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
+obj-$(CONFIG_DUMP_STATE) += dump_state.o
diff --git a/lib/dump_state.c b/lib/dump_state.c
new file mode 100644
index 000000000000..58d88be65c0a
--- /dev/null
+++ b/lib/dump_state.c
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2021, Linaro Ltd
+ */
+
+#include <linux/dump_state.h>
+#include <linux/slab.h>
+
+void dump_state_free_blocks(struct dump_state *state)
+{
+	struct dump_state_block *block, *tmp;
+
+	list_for_each_entry_safe(block, tmp, &state->blocks, node) {
+		list_del(&block->node);
+		kfree(block);
+	}
+}
+EXPORT_SYMBOL(dump_state_free_blocks);
+
+struct dump_state_block *dump_state_allocate_block_va(void __iomem *base_addr, size_t len, gfp_t gfp, const char *fmt, va_list args)
+{
+	struct dump_state_block *block = kzalloc(sizeof(*block) + len, gfp);
+
+	if (!block)
+		return ERR_PTR(-ENOMEM);
+
+	vsnprintf(block->name, sizeof(block->name), fmt, args);
+
+	INIT_LIST_HEAD(&block->node);
+	block->size = len;
+	block->base_addr = base_addr;
+
+	return block;
+}
+EXPORT_SYMBOL(dump_state_allocate_block);
+
+struct dump_state_block *dump_state_allocate_block(void __iomem *base_addr, size_t len, gfp_t gfp, const char *fmt, ...)
+{
+	struct dump_state_block *block;
+	va_list va;
+
+	va_start(va, fmt);
+
+	block = dump_state_allocate_block_va(base_addr, len, gfp, fmt, va);
+
+	va_end(va);
+
+	return block;
+}
+EXPORT_SYMBOL(dump_state_allocate_block_va);
-- 
2.30.2

[RFC 6/8] drm/msm: port msm_disp_snapshot to dump_state

[Dmitry Baryshkov]

Change msm_disp_snapshot to use dump_state API.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/Kconfig                   |  1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  2 +-
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c  |  2 +-
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h  | 21 +------
 .../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 56 ++++++-------------
 drivers/gpu/drm/msm/dp/dp_catalog.c           |  3 +-
 drivers/gpu/drm/msm/dp/dp_catalog.h           |  4 +-
 drivers/gpu/drm/msm/dp/dp_display.c           |  2 +-
 drivers/gpu/drm/msm/dsi/dsi.c                 |  2 +-
 drivers/gpu/drm/msm/dsi/dsi.h                 |  4 +-
 drivers/gpu/drm/msm/dsi/dsi_host.c            |  2 +-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c         |  2 +-
 drivers/gpu/drm/msm/msm_drv.h                 |  9 +--
 drivers/gpu/drm/msm/msm_kms.h                 |  2 +-
 14 files changed, 38 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index 10f693ea89d3..d3e151ffa1f7 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -21,6 +21,7 @@ config DRM_MSM
 	select SYNC_FILE
 	select PM_OPP
 	select NVMEM
+	select DUMP_STATE
 	help
 	  DRM/KMS driver for MSM/snapdragon.
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index e500a9294528..d85f425db087 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -799,7 +799,7 @@ static void dpu_irq_uninstall(struct msm_kms *kms)
 	dpu_core_irq_uninstall(dpu_kms);
 }
 
-static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_kms *kms)
+static void dpu_kms_mdp_snapshot(struct dump_state *disp_state, struct msm_kms *kms)
 {
 	int i;
 	struct dpu_kms *dpu_kms;
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
index a4a7cb06bc87..c1f725c0cf60 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
@@ -42,7 +42,7 @@ static void _msm_disp_snapshot_work(struct kthread_work *work)
 	disp_state->dev = drm_dev->dev;
 	disp_state->drm_dev = drm_dev;
 
-	INIT_LIST_HEAD(&disp_state->blocks);
+	dump_state_init(&disp_state->dump);
 
 	/* Serialize dumping here */
 	mutex_lock(&kms->dump_mutex);
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
index c92a9508c8d3..327df4892a9c 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
@@ -25,6 +25,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/kthread.h>
 #include <linux/devcoredump.h>
+#include <linux/dump_state.h>
 #include <stdarg.h>
 #include "msm_kms.h"
 
@@ -47,27 +48,11 @@ struct msm_disp_state {
 	struct device *dev;
 	struct drm_device *drm_dev;
 
-	struct list_head blocks;
-
 	struct drm_atomic_state *atomic_state;
 
 	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;
+	struct dump_state dump;
 };
 
 /**
@@ -130,7 +115,7 @@ void msm_disp_state_free(void *data);
  * Returns: none
  */
 __printf(4, 5)
-void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, u32 len,
+void msm_disp_snapshot_add_block(struct dump_state *disp_state, u32 len,
 		void __iomem *base_addr, const char *fmt, ...);
 
 #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
index cabe15190ec1..9f61e376bec2 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -7,13 +7,12 @@
 
 #include "msm_disp_snapshot.h"
 
-static void msm_disp_state_dump_regs(u32 **reg, u32 aligned_len, void __iomem *base_addr)
+static void msm_disp_state_dump_regs(u32 *dump_addr, size_t aligned_len, void __iomem *base_addr)
 {
 	u32 len_padded;
 	u32 num_rows;
 	u32 x0, x4, x8, xc;
 	void __iomem *addr;
-	u32 *dump_addr = NULL;
 	void __iomem *end_addr;
 	int i;
 
@@ -23,12 +22,6 @@ static void msm_disp_state_dump_regs(u32 **reg, u32 aligned_len, void __iomem *b
 	addr = base_addr;
 	end_addr = base_addr + aligned_len;
 
-	if (!(*reg))
-		*reg = kzalloc(len_padded, GFP_KERNEL);
-
-	if (*reg)
-		dump_addr = *reg;
-
 	for (i = 0; i < num_rows; i++) {
 		x0 = (addr < end_addr) ? readl_relaxed(addr + 0x0) : 0;
 		x4 = (addr + 0x4 < end_addr) ? readl_relaxed(addr + 0x4) : 0;
@@ -46,20 +39,16 @@ static void msm_disp_state_dump_regs(u32 **reg, u32 aligned_len, void __iomem *b
 	}
 }
 
-static void msm_disp_state_print_regs(u32 **reg, u32 len, void __iomem *base_addr,
+static void msm_disp_state_print_regs(u32 *dump_addr, size_t len, void __iomem *base_addr,
 		struct drm_printer *p)
 {
 	int i;
-	u32 *dump_addr = NULL;
 	void __iomem *addr;
 	u32 num_rows;
 
 	addr = base_addr;
 	num_rows = len / REG_DUMP_ALIGN;
 
-	if (*reg)
-		dump_addr = *reg;
-
 	for (i = 0; i < num_rows; i++) {
 		drm_printf(p, "0x%lx : %08x %08x %08x %08x\n",
 				(unsigned long)(addr - base_addr),
@@ -71,7 +60,7 @@ static void msm_disp_state_print_regs(u32 **reg, u32 len, void __iomem *base_add
 
 void msm_disp_state_print(struct msm_disp_state *state, struct drm_printer *p)
 {
-	struct msm_disp_state_block *block, *tmp;
+	struct dump_state_block *block;
 
 	if (!p) {
 		DRM_ERROR("invalid drm printer\n");
@@ -84,9 +73,9 @@ void msm_disp_state_print(struct msm_disp_state *state, struct drm_printer *p)
 	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) {
+	dump_state_for_each_block(&state->dump, block) {
 		drm_printf(p, "====================%s================\n", block->name);
-		msm_disp_state_print_regs(&block->state, block->size, block->base_addr, p);
+		msm_disp_state_print_regs((u32 *)block->state, block->size, block->base_addr, p);
 	}
 
 	drm_printf(p, "===================dpu drm state================\n");
@@ -127,17 +116,17 @@ void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
 	kms = priv->kms;
 
 	if (priv->dp)
-		msm_dp_snapshot(disp_state, priv->dp);
+		msm_dp_snapshot(&disp_state->dump, priv->dp);
 
 	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
 		if (!priv->dsi[i])
 			continue;
 
-		msm_dsi_snapshot(disp_state, priv->dsi[i]);
+		msm_dsi_snapshot(&disp_state->dump, priv->dsi[i]);
 	}
 
 	if (kms->funcs->snapshot)
-		kms->funcs->snapshot(disp_state, kms);
+		kms->funcs->snapshot(&disp_state->dump, kms);
 
 	msm_disp_capture_atomic_state(disp_state);
 }
@@ -145,43 +134,30 @@ void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
 void msm_disp_state_free(void *data)
 {
 	struct msm_disp_state *disp_state = data;
-	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) {
-		list_del(&block->node);
-		kfree(block->state);
-		kfree(block);
-	}
+	dump_state_free_blocks(&disp_state->dump);
 
 	kfree(disp_state);
 }
 
-void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, u32 len,
+void msm_disp_snapshot_add_block(struct dump_state *disp_state, u32 len,
 		void __iomem *base_addr, const char *fmt, ...)
 {
-	struct msm_disp_state_block *new_blk;
-	struct va_format vaf;
+	struct dump_state_block *new_blk;
 	va_list va;
 
-	new_blk = kzalloc(sizeof(struct msm_disp_state_block), GFP_KERNEL);
-
 	va_start(va, fmt);
-
-	vaf.fmt = fmt;
-	vaf.va = &va;
-	snprintf(new_blk->name, sizeof(new_blk->name), "%pV", &vaf);
-
+	new_blk = dump_state_allocate_block_va(base_addr, ALIGN(len, REG_DUMP_ALIGN), GFP_KERNEL, fmt, va);
 	va_end(va);
 
-	INIT_LIST_HEAD(&new_blk->node);
-	new_blk->size = ALIGN(len, REG_DUMP_ALIGN);
-	new_blk->base_addr = base_addr;
+	if (IS_ERR(new_blk))
+		return;
 
-	msm_disp_state_dump_regs(&new_blk->state, new_blk->size, base_addr);
-	list_add(&new_blk->node, &disp_state->blocks);
+	msm_disp_state_dump_regs((u32 *)new_blk->state, new_blk->size, base_addr);
+	dump_state_add_block(disp_state, new_blk);
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 854c41d05b19..1155571beef4 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -15,6 +15,7 @@
 
 #include "dp_catalog.h"
 #include "dp_reg.h"
+#include "disp/msm_disp_snapshot.h"
 
 #define POLLING_SLEEP_US			1000
 #define POLLING_TIMEOUT_US			10000
@@ -62,7 +63,7 @@ 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)
+void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct dump_state *disp_state)
 {
 	struct dp_catalog_private *catalog = container_of(dp_catalog,
 			struct dp_catalog_private, dp_catalog);
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
index e7e8b13d1753..9fc9019fac35 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -7,9 +7,9 @@
 #define _DP_CATALOG_H_
 
 #include <drm/drm_modes.h>
+#include <linux/dump_state.h>
 
 #include "dp_parser.h"
-#include "disp/msm_disp_snapshot.h"
 
 /* interrupts */
 #define DP_INTR_HPD		BIT(0)
@@ -73,7 +73,7 @@ struct dp_catalog {
 };
 
 /* Debug module */
-void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct msm_disp_state *disp_state);
+void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct dump_state *disp_state);
 
 /* AUX APIs */
 u32 dp_catalog_aux_read_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 cf9c64534624..0ef4080e1e79 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1039,7 +1039,7 @@ int dp_display_get_test_bpp(struct msm_dp *dp)
 		dp_display->link->test_video.test_bit_depth);
 }
 
-void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
+void msm_dp_snapshot(struct dump_state *disp_state, struct msm_dp *dp)
 {
 	struct dp_display_private *dp_display;
 	struct drm_device *drm;
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 75afc12a7b25..ce76edfa681a 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -266,7 +266,7 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 	return ret;
 }
 
-void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi)
+void msm_dsi_snapshot(struct dump_state *disp_state, struct msm_dsi *msm_dsi)
 {
 	msm_dsi_host_snapshot(disp_state, msm_dsi->host);
 	msm_dsi_phy_snapshot(disp_state, msm_dsi->phy);
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index d64db1badd4b..9ccdf1563cf2 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -147,7 +147,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 msm_disp_state *disp_state, struct mipi_dsi_host *host);
+void msm_dsi_host_snapshot(struct dump_state *disp_state, struct mipi_dsi_host *host);
 /* dsi phy */
 struct msm_dsi_phy;
 struct msm_dsi_phy_shared_timings {
@@ -169,7 +169,7 @@ int msm_dsi_phy_enable(struct msm_dsi_phy *phy,
 void msm_dsi_phy_disable(struct msm_dsi_phy *phy);
 void msm_dsi_phy_set_usecase(struct msm_dsi_phy *phy,
 			     enum msm_dsi_phy_usecase uc);
-void msm_dsi_phy_snapshot(struct msm_disp_state *disp_state, struct msm_dsi_phy *phy);
+void msm_dsi_phy_snapshot(struct dump_state *disp_state, struct msm_dsi_phy *phy);
 
 #endif /* __DSI_CONNECTOR_H__ */
 
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 4d5b73c031ee..72e48d83189d 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -2438,7 +2438,7 @@ 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 msm_disp_state *disp_state, struct mipi_dsi_host *host)
+void msm_dsi_host_snapshot(struct dump_state *disp_state, struct mipi_dsi_host *host)
 {
 	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
 
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index b7b178cc318b..10ac875d89ac 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -872,7 +872,7 @@ void msm_dsi_phy_set_usecase(struct msm_dsi_phy *phy,
 	if (phy)
 		phy->usecase = uc;
 }
-void msm_dsi_phy_snapshot(struct msm_disp_state *disp_state, struct msm_dsi_phy *phy)
+void msm_dsi_phy_snapshot(struct dump_state *disp_state, struct msm_dsi_phy *phy)
 {
 	msm_disp_snapshot_add_block(disp_state,
 			phy->base_size, phy->base,
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index ba60bf6f124c..32601212f02a 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/clk.h>
 #include <linux/cpufreq.h>
+#include <linux/dump_state.h>
 #include <linux/module.h>
 #include <linux/component.h>
 #include <linux/platform_device.h>
@@ -43,7 +44,6 @@ struct msm_gem_submit;
 struct msm_fence_context;
 struct msm_gem_address_space;
 struct msm_gem_vma;
-struct msm_disp_state;
 
 #define MAX_CRTCS      8
 #define MAX_PLANES     20
@@ -341,7 +341,7 @@ void __init msm_dsi_register(void);
 void __exit msm_dsi_unregister(void);
 int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 			 struct drm_encoder *encoder);
-void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi);
+void msm_dsi_snapshot(struct dump_state *disp_state, struct msm_dsi *msm_dsi);
 
 #else
 static inline void __init msm_dsi_register(void)
@@ -356,7 +356,7 @@ static inline int msm_dsi_modeset_init(struct msm_dsi *msm_dsi,
 {
 	return -EINVAL;
 }
-static inline void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi)
+static inline void msm_dsi_snapshot(struct dump_state *disp_state, struct msm_dsi *msm_dsi)
 {
 }
 
@@ -374,7 +374,8 @@ 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_disp_state *disp_state, struct msm_dp *dp_display);
+struct dump_state;
+void msm_dp_snapshot(struct dump_state *disp_state, struct msm_dp *dp_display);
 
 void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
 
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 086a2d59b8c8..da27b94b11bf 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -124,7 +124,7 @@ struct msm_kms_funcs {
 	void (*destroy)(struct msm_kms *kms);
 
 	/* snapshot: */
-	void (*snapshot)(struct msm_disp_state *disp_state, struct msm_kms *kms);
+	void (*snapshot)(struct dump_state *disp_state, struct msm_kms *kms);
 
 #ifdef CONFIG_DEBUG_FS
 	/* debugfs: */
-- 
2.30.2

[RFC 7/8] drm/msm: do include unused headers in msm_disp_snapshot.h

[Dmitry Baryshkov]

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c  |  3 +++
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h  | 19 -------------------
 .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |  8 ++++++++
 3 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
index c1f725c0cf60..802e860cd045 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
@@ -5,6 +5,9 @@
 
 #define pr_fmt(fmt)	"[drm:%s:%d] " fmt, __func__, __LINE__
 
+#include <linux/devcoredump.h>
+
+#include "msm_kms.h"
 #include "msm_disp_snapshot.h"
 
 static ssize_t __maybe_unused disp_devcoredump_read(char *buffer, loff_t offset,
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
index 327df4892a9c..402bde48a2a7 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
@@ -6,28 +6,9 @@
 #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 <linux/dump_state.h>
-#include <stdarg.h>
-#include "msm_kms.h"
 
 #define MSM_DISP_SNAPSHOT_MAX_BLKS		10
 
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
index 9f61e376bec2..5b278329a9ee 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -5,6 +5,14 @@
 
 #define pr_fmt(fmt)	"[drm:%s:%d] " fmt, __func__, __LINE__
 
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_print.h>
+#include <linux/io.h>
+
+#include "../../../drm_crtc_internal.h"
+
+#include "msm_kms.h"
+#include "dsi.h"
 #include "msm_disp_snapshot.h"
 
 static void msm_disp_state_dump_regs(u32 *dump_addr, size_t aligned_len, void __iomem *base_addr)
-- 
2.30.2

Re: [RFC 5/8] lib: add small API for handling register snapshots

[Rob Clark]

On Fri, May 28, 2021 at 5:25 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> Add small API covering lists of register dumps. Currently this is a part
> of MSM DRM driver, but is extracted as it might be usefull to other
> drivers too.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  include/linux/dump_state.h | 78 ++++++++++++++++++++++++++++++++++++++
>  lib/Kconfig                |  3 ++
>  lib/Makefile               |  1 +
>  lib/dump_state.c           | 51 +++++++++++++++++++++++++
>  4 files changed, 133 insertions(+)
>  create mode 100644 include/linux/dump_state.h
>  create mode 100644 lib/dump_state.c
>
[snip]
> diff --git a/lib/dump_state.c b/lib/dump_state.c
> new file mode 100644
> index 000000000000..58d88be65c0a
> --- /dev/null
> +++ b/lib/dump_state.c
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2021, Linaro Ltd
> + */
> +
> +#include <linux/dump_state.h>
> +#include <linux/slab.h>
> +
> +void dump_state_free_blocks(struct dump_state *state)
> +{
> +       struct dump_state_block *block, *tmp;
> +
> +       list_for_each_entry_safe(block, tmp, &state->blocks, node) {
> +               list_del(&block->node);
> +               kfree(block);
> +       }
> +}
> +EXPORT_SYMBOL(dump_state_free_blocks);

nit, perhaps EXPORT_SYMBOL_GPL()?

BR,
-R

> +
> +struct dump_state_block *dump_state_allocate_block_va(void __iomem *base_addr, size_t len, gfp_t gfp, const char *fmt, va_list args)
> +{
> +       struct dump_state_block *block = kzalloc(sizeof(*block) + len, gfp);
> +
> +       if (!block)
> +               return ERR_PTR(-ENOMEM);
> +
> +       vsnprintf(block->name, sizeof(block->name), fmt, args);
> +
> +       INIT_LIST_HEAD(&block->node);
> +       block->size = len;
> +       block->base_addr = base_addr;
> +
> +       return block;
> +}
> +EXPORT_SYMBOL(dump_state_allocate_block);
> +
> +struct dump_state_block *dump_state_allocate_block(void __iomem *base_addr, size_t len, gfp_t gfp, const char *fmt, ...)
> +{
> +       struct dump_state_block *block;
> +       va_list va;
> +
> +       va_start(va, fmt);
> +
> +       block = dump_state_allocate_block_va(base_addr, len, gfp, fmt, va);
> +
> +       va_end(va);
> +
> +       return block;
> +}
> +EXPORT_SYMBOL(dump_state_allocate_block_va);
> --
> 2.30.2
>

Re: [RFC 5/8] lib: add small API for handling register snapshots

[Dmitry Baryshkov]

On Thu, 3 Jun 2021 at 17:41, Rob Clark <robdclark@gmail.com> wrote:
>
> On Fri, May 28, 2021 at 5:25 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > Add small API covering lists of register dumps. Currently this is a part
> > of MSM DRM driver, but is extracted as it might be usefull to other
> > drivers too.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  include/linux/dump_state.h | 78 ++++++++++++++++++++++++++++++++++++++
> >  lib/Kconfig                |  3 ++
> >  lib/Makefile               |  1 +
> >  lib/dump_state.c           | 51 +++++++++++++++++++++++++
> >  4 files changed, 133 insertions(+)
> >  create mode 100644 include/linux/dump_state.h
> >  create mode 100644 lib/dump_state.c
> >
> [snip]
> > diff --git a/lib/dump_state.c b/lib/dump_state.c
> > new file mode 100644
> > index 000000000000..58d88be65c0a
> > --- /dev/null
> > +++ b/lib/dump_state.c
> > @@ -0,0 +1,51 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
> > + * Copyright (c) 2021, Linaro Ltd
> > + */
> > +
> > +#include <linux/dump_state.h>
> > +#include <linux/slab.h>
> > +
> > +void dump_state_free_blocks(struct dump_state *state)
> > +{
> > +       struct dump_state_block *block, *tmp;
> > +
> > +       list_for_each_entry_safe(block, tmp, &state->blocks, node) {
> > +               list_del(&block->node);
> > +               kfree(block);
> > +       }
> > +}
> > +EXPORT_SYMBOL(dump_state_free_blocks);
>
> nit, perhaps EXPORT_SYMBOL_GPL()?

I don't really care. What is the current recommendation?

>
> BR,
> -R
>
> > +
> > +struct dump_state_block *dump_state_allocate_block_va(void __iomem *base_addr, size_t len, gfp_t gfp, const char *fmt, va_list args)
> > +{
> > +       struct dump_state_block *block = kzalloc(sizeof(*block) + len, gfp);
> > +
> > +       if (!block)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       vsnprintf(block->name, sizeof(block->name), fmt, args);
> > +
> > +       INIT_LIST_HEAD(&block->node);
> > +       block->size = len;
> > +       block->base_addr = base_addr;
> > +
> > +       return block;
> > +}
> > +EXPORT_SYMBOL(dump_state_allocate_block);
> > +
> > +struct dump_state_block *dump_state_allocate_block(void __iomem *base_addr, size_t len, gfp_t gfp, const char *fmt, ...)
> > +{
> > +       struct dump_state_block *block;
> > +       va_list va;
> > +
> > +       va_start(va, fmt);
> > +
> > +       block = dump_state_allocate_block_va(base_addr, len, gfp, fmt, va);
> > +
> > +       va_end(va);
> > +
> > +       return block;
> > +}
> > +EXPORT_SYMBOL(dump_state_allocate_block_va);
> > --
> > 2.30.2
> >



-- 
With best wishes
Dmitry

Re: [RFC 5/8] lib: add small API for handling register snapshots

[Rob Clark]

On Thu, Jun 3, 2021 at 9:33 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, 3 Jun 2021 at 17:41, Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Fri, May 28, 2021 at 5:25 PM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > Add small API covering lists of register dumps. Currently this is a part
> > > of MSM DRM driver, but is extracted as it might be usefull to other
> > > drivers too.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  include/linux/dump_state.h | 78 ++++++++++++++++++++++++++++++++++++++
> > >  lib/Kconfig                |  3 ++
> > >  lib/Makefile               |  1 +
> > >  lib/dump_state.c           | 51 +++++++++++++++++++++++++
> > >  4 files changed, 133 insertions(+)
> > >  create mode 100644 include/linux/dump_state.h
> > >  create mode 100644 lib/dump_state.c
> > >
> > [snip]
> > > diff --git a/lib/dump_state.c b/lib/dump_state.c
> > > new file mode 100644
> > > index 000000000000..58d88be65c0a
> > > --- /dev/null
> > > +++ b/lib/dump_state.c
> > > @@ -0,0 +1,51 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +/*
> > > + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
> > > + * Copyright (c) 2021, Linaro Ltd
> > > + */
> > > +
> > > +#include <linux/dump_state.h>
> > > +#include <linux/slab.h>
> > > +
> > > +void dump_state_free_blocks(struct dump_state *state)
> > > +{
> > > +       struct dump_state_block *block, *tmp;
> > > +
> > > +       list_for_each_entry_safe(block, tmp, &state->blocks, node) {
> > > +               list_del(&block->node);
> > > +               kfree(block);
> > > +       }
> > > +}
> > > +EXPORT_SYMBOL(dump_state_free_blocks);
> >
> > nit, perhaps EXPORT_SYMBOL_GPL()?
>
> I don't really care. What is the current recommendation?

AFAIU it is to default to EXPORT_SYMBOL_GPL() unless there is a good reason..

BR,
-R

> >
> > BR,
> > -R
> >
> > > +
> > > +struct dump_state_block *dump_state_allocate_block_va(void __iomem *base_addr, size_t len, gfp_t gfp, const char *fmt, va_list args)
> > > +{
> > > +       struct dump_state_block *block = kzalloc(sizeof(*block) + len, gfp);
> > > +
> > > +       if (!block)
> > > +               return ERR_PTR(-ENOMEM);
> > > +
> > > +       vsnprintf(block->name, sizeof(block->name), fmt, args);
> > > +
> > > +       INIT_LIST_HEAD(&block->node);
> > > +       block->size = len;
> > > +       block->base_addr = base_addr;
> > > +
> > > +       return block;
> > > +}
> > > +EXPORT_SYMBOL(dump_state_allocate_block);
> > > +
> > > +struct dump_state_block *dump_state_allocate_block(void __iomem *base_addr, size_t len, gfp_t gfp, const char *fmt, ...)
> > > +{
> > > +       struct dump_state_block *block;
> > > +       va_list va;
> > > +
> > > +       va_start(va, fmt);
> > > +
> > > +       block = dump_state_allocate_block_va(base_addr, len, gfp, fmt, va);
> > > +
> > > +       va_end(va);
> > > +
> > > +       return block;
> > > +}
> > > +EXPORT_SYMBOL(dump_state_allocate_block_va);
> > > --
> > > 2.30.2
> > >
>
>
>
> --
> With best wishes
> Dmitry