[PATCH v2 0/4] drm/msm: improve register snapshotting

[PATCH v2 0/4] drm/msm: improve register snapshotting

[Dmitry Baryshkov]

Rework MSM coredump support: add DSI PHY registers, simplify
snapshotting code.

Changes since v1:
 - Readd mutex serializing register snapshot calls

 - Add DSI PHY register dumping support

----------------------------------------------------------------
Dmitry Baryshkov (4):
      drm/msm: pass dump state as a function argument
      drm/msm: make msm_disp_state transient data struct
      drm/msm: get rid of msm_iomap_size
      drm/msm/dsi: add DSI PHY registers to snapshot data

 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c           |  5 +-
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c      | 90 +++++++----------------
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h      | 21 +-----
 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 22 ++----
 drivers/gpu/drm/msm/dp/dp_display.c               |  4 +-
 drivers/gpu/drm/msm/dsi/dsi.c                     |  5 +-
 drivers/gpu/drm/msm/dsi/dsi.h                     |  5 +-
 drivers/gpu/drm/msm/dsi/dsi_host.c                | 11 +--
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c             | 31 +++++++-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h             |  4 +
 drivers/gpu/drm/msm/msm_drv.c                     | 27 +++----
 drivers/gpu/drm/msm/msm_drv.h                     |  6 +-
 drivers/gpu/drm/msm/msm_kms.h                     |  8 +-
 13 files changed, 97 insertions(+), 142 deletions(-)


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/4] drm/msm: pass dump state as a function argument

[Dmitry Baryshkov]

Instead of always getting the disp_state from drm device, pass it as an
argument.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c         |  5 +----
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h    |  8 --------
 .../gpu/drm/msm/disp/msm_disp_snapshot_util.c   | 17 +++--------------
 drivers/gpu/drm/msm/dp/dp_display.c             |  4 +---
 drivers/gpu/drm/msm/dsi/dsi.c                   |  4 ++--
 drivers/gpu/drm/msm/dsi/dsi.h                   |  4 ++--
 drivers/gpu/drm/msm/dsi/dsi_host.c              |  6 +-----
 drivers/gpu/drm/msm/msm_drv.h                   |  3 ++-
 drivers/gpu/drm/msm/msm_kms.h                   |  2 +-
 9 files changed, 13 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index ead247864c1b..e500a9294528 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -799,15 +799,12 @@ static void dpu_irq_uninstall(struct msm_kms *kms)
 	dpu_core_irq_uninstall(dpu_kms);
 }
 
-static void dpu_kms_mdp_snapshot(struct msm_kms *kms)
+static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_kms *kms)
 {
 	int i;
 	struct dpu_kms *dpu_kms;
 	struct dpu_mdss_cfg *cat;
 	struct dpu_hw_mdp *top;
-	struct msm_disp_state *disp_state;
-
-	disp_state = kms->disp_state;
 
 	dpu_kms = to_dpu_kms(kms);
 
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
index 7e075e799f0a..32f52799a1ba 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
@@ -104,14 +104,6 @@ void msm_disp_snapshot_destroy(struct drm_device *drm_dev);
  */
 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
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 44dc68295ddb..ca6632550337 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -69,17 +69,6 @@ static void msm_disp_state_print_regs(u32 **reg, u32 len, void __iomem *base_add
 	}
 }
 
-struct msm_disp_state *msm_disp_state_get(struct drm_device *drm)
-{
-	struct msm_drm_private *priv;
-	struct msm_kms *kms;
-
-	priv = drm->dev_private;
-	kms = priv->kms;
-
-	return kms->disp_state;
-}
-
 void msm_disp_state_print(struct msm_disp_state *state, struct drm_printer *p)
 {
 	struct msm_disp_state_block *block, *tmp;
@@ -138,17 +127,17 @@ void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
 	kms = priv->kms;
 
 	if (priv->dp)
-		msm_dp_snapshot(priv->dp);
+		msm_dp_snapshot(disp_state, priv->dp);
 
 	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
 		if (!priv->dsi[i])
 			continue;
 
-		msm_dsi_snapshot(priv->dsi[i]);
+		msm_dsi_snapshot(disp_state, priv->dsi[i]);
 	}
 
 	if (kms->funcs->snapshot)
-		kms->funcs->snapshot(kms);
+		kms->funcs->snapshot(disp_state, kms);
 
 	msm_disp_capture_atomic_state(disp_state);
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 66705588f751..95d0bba7e172 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1009,15 +1009,13 @@ 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)
+void msm_dp_snapshot(struct msm_disp_state *disp_state, 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
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index bccc00603aa8..322d2e535df0 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -266,8 +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)
+void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi)
 {
-	msm_dsi_host_snapshot(msm_dsi->host);
+	msm_dsi_host_snapshot(disp_state, msm_dsi->host);
 }
 
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index e26223c3b6ec..b5679cf89413 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -91,7 +91,7 @@ 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);
+void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi);
 
 struct drm_encoder *msm_dsi_get_encoder(struct msm_dsi *msm_dsi);
 
@@ -149,7 +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);
+void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, struct mipi_dsi_host *host);
 /* dsi phy */
 struct msm_dsi_phy;
 struct msm_dsi_phy_shared_timings {
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 899b6fc2b634..1a63368c3912 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -2488,13 +2488,9 @@ 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)
+void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, 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;
-
-	disp_state = msm_disp_state_get(dev);
 
 	pm_runtime_get_sync(&msm_host->pdev->dev);
 
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 9c40bac8a050..15cb34451ded 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -367,7 +367,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_dp *dp_display);
+struct msm_disp_state;
+void msm_dp_snapshot(struct msm_disp_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 b31fdad3f055..146dcab123f4 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_kms *kms);
+	void (*snapshot)(struct msm_disp_state *disp_state, struct msm_kms *kms);
 
 #ifdef CONFIG_DEBUG_FS
 	/* debugfs: */
-- 
2.30.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/4] drm/msm: make msm_disp_state transient data struct

[Dmitry Baryshkov]

Instead of allocating snapshotting structure at the driver probe time
and later handling concurrent access, actual state, etc, make
msm_disp_state transient struct. Allocate one when snapshotting happens
and free it after coredump data is read by userspace.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c  | 90 ++++++-------------
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h  | 13 +--
 .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |  5 +-
 drivers/gpu/drm/msm/msm_kms.h                 |  6 +-
 4 files changed, 37 insertions(+), 77 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
index 70fd5a1fe13e..a4a7cb06bc87 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
@@ -7,8 +7,7 @@
 
 #include "msm_disp_snapshot.h"
 
-#ifdef CONFIG_DEV_COREDUMP
-static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
+static ssize_t __maybe_unused disp_devcoredump_read(char *buffer, loff_t offset,
 		size_t count, void *data, size_t datalen)
 {
 	struct drm_print_iterator iter;
@@ -29,52 +28,47 @@ static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
 	return count - iter.remain;
 }
 
-static void disp_devcoredump_free(void *data)
+static void _msm_disp_snapshot_work(struct kthread_work *work)
 {
+	struct msm_kms *kms = container_of(work, struct msm_kms, dump_work);
+	struct drm_device *drm_dev = kms->dev;
 	struct msm_disp_state *disp_state;
+	struct drm_printer p;
 
-	disp_state = data;
-
-	msm_disp_state_free(disp_state);
+	disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
+	if (!disp_state)
+		return;
 
-	disp_state->coredump_pending = false;
-}
-#endif /* CONFIG_DEV_COREDUMP */
+	disp_state->dev = drm_dev->dev;
+	disp_state->drm_dev = drm_dev;
 
-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;
+	INIT_LIST_HEAD(&disp_state->blocks);
 
-	mutex_lock(&disp_state->mutex);
+	/* Serialize dumping here */
+	mutex_lock(&kms->dump_mutex);
 
 	msm_disp_snapshot_capture_state(disp_state);
 
+	mutex_unlock(&kms->dump_mutex);
+
 	if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
 		p = drm_info_printer(disp_state->drm_dev->dev);
 		msm_disp_state_print(disp_state, &p);
 	}
 
 	/*
-	 * if devcoredump is not defined free the state immediately
-	 * otherwise it will be freed in the free handler.
+	 * If COREDUMP is disabled, the stub will call the free function.
+	 * If there is a codedump pending for the device, the dev_coredumpm()
+	 * will also free new coredump state.
 	 */
-#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);
+			disp_devcoredump_read, msm_disp_state_free);
 }
 
 void msm_disp_snapshot_state(struct drm_device *drm_dev)
 {
 	struct msm_drm_private *priv;
 	struct msm_kms *kms;
-	struct msm_disp_state *disp_state;
 
 	if (!drm_dev) {
 		DRM_ERROR("invalid params\n");
@@ -83,30 +77,13 @@ void msm_disp_snapshot_state(struct drm_device *drm_dev)
 
 	priv = drm_dev->dev_private;
 	kms = priv->kms;
-	disp_state = 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);
+	kthread_queue_work(kms->dump_worker, &kms->dump_work);
 }
 
 int msm_disp_snapshot_init(struct drm_device *drm_dev)
 {
 	struct msm_drm_private *priv;
-	struct msm_disp_state *disp_state;
 	struct msm_kms *kms;
 
 	if (!drm_dev) {
@@ -117,22 +94,13 @@ int msm_disp_snapshot_init(struct drm_device *drm_dev)
 	priv = drm_dev->dev_private;
 	kms = priv->kms;
 
-	disp_state = devm_kzalloc(drm_dev->dev, sizeof(struct msm_disp_state), GFP_KERNEL);
-
-	mutex_init(&disp_state->mutex);
+	mutex_init(&kms->dump_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))
+	kms->dump_worker = kthread_create_worker(0, "%s", "disp_snapshot");
+	if (IS_ERR(kms->dump_worker))
 		DRM_ERROR("failed to create disp state task\n");
 
-	kthread_init_work(&disp_state->dump_work, _msm_disp_snapshot_work);
-
-	kms->disp_state = disp_state;
+	kthread_init_work(&kms->dump_work, _msm_disp_snapshot_work);
 
 	return 0;
 }
@@ -141,7 +109,6 @@ void msm_disp_snapshot_destroy(struct drm_device *drm_dev)
 {
 	struct msm_kms *kms;
 	struct msm_drm_private *priv;
-	struct msm_disp_state *disp_state;
 
 	if (!drm_dev) {
 		DRM_ERROR("invalid params\n");
@@ -150,12 +117,9 @@ void msm_disp_snapshot_destroy(struct drm_device *drm_dev)
 
 	priv = drm_dev->dev_private;
 	kms = priv->kms;
-	disp_state = kms->disp_state;
-
-	if (disp_state->dump_worker)
-		kthread_destroy_worker(disp_state->dump_worker);
 
-	list_del(&disp_state->blocks);
+	if (kms->dump_worker)
+		kthread_destroy_worker(kms->dump_worker);
 
-	mutex_destroy(&disp_state->mutex);
+	mutex_destroy(&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 32f52799a1ba..c6174a366095 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
@@ -41,26 +41,17 @@
  * 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;
 };
 
@@ -123,11 +114,11 @@ 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
+ * @data:	    handle to struct msm_disp_state
 
  * Returns: none
  */
-void msm_disp_state_free(struct msm_disp_state *disp_state);
+void msm_disp_state_free(void *data);
 
 /**
  * msm_disp_snapshot_add_block - add a hardware block with its register dump
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 ca6632550337..cabe15190ec1 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -142,8 +142,9 @@ void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
 	msm_disp_capture_atomic_state(disp_state);
 }
 
-void msm_disp_state_free(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) {
@@ -156,6 +157,8 @@ void msm_disp_state_free(struct msm_disp_state *disp_state)
 		kfree(block->state);
 		kfree(block);
 	}
+
+	kfree(disp_state);
 }
 
 void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, u32 len,
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 146dcab123f4..086a2d59b8c8 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -156,8 +156,10 @@ struct msm_kms {
 	/* mapper-id used to request GEM buffer mapped for scanout: */
 	struct msm_gem_address_space *aspace;
 
-	/* handle to disp snapshot state */
-	struct msm_disp_state *disp_state;
+	/* disp snapshot support */
+	struct kthread_worker *dump_worker;
+	struct kthread_work dump_work;
+	struct mutex dump_mutex;
 
 	/*
 	 * For async commit, where ->flush_commit() and later happens
-- 
2.30.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/4] drm/msm: get rid of msm_iomap_size

[Dmitry Baryshkov]

Instead of looping throught the resources each time to get the DSI CTRL
area size, get it at the ioremap time.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c |  5 +++--
 drivers/gpu/drm/msm/msm_drv.c      | 27 +++++++++------------------
 drivers/gpu/drm/msm/msm_drv.h      |  3 ++-
 3 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 1a63368c3912..b3ee5c0bce12 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -102,6 +102,7 @@ struct msm_dsi_host {
 	int id;
 
 	void __iomem *ctrl_base;
+	phys_addr_t ctrl_size;
 	struct regulator_bulk_data supplies[DSI_DEV_REGULATOR_MAX];
 
 	struct clk *bus_clks[DSI_BUS_CLK_MAX];
@@ -1839,7 +1840,7 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
 		goto fail;
 	}
 
-	msm_host->ctrl_base = msm_ioremap(pdev, "dsi_ctrl", "DSI CTRL");
+	msm_host->ctrl_base = msm_ioremap_size(pdev, "dsi_ctrl", "DSI CTRL", &msm_host->ctrl_size);
 	if (IS_ERR(msm_host->ctrl_base)) {
 		pr_err("%s: unable to map Dsi ctrl base\n", __func__);
 		ret = PTR_ERR(msm_host->ctrl_base);
@@ -2494,7 +2495,7 @@ void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, struct mipi_dsi_ho
 
 	pm_runtime_get_sync(&msm_host->pdev->dev);
 
-	msm_disp_snapshot_add_block(disp_state, msm_iomap_size(msm_host->pdev, "dsi_ctrl"),
+	msm_disp_snapshot_add_block(disp_state, msm_host->ctrl_size,
 			msm_host->ctrl_base, "dsi%d_ctrl", msm_host->id);
 
 	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 92fe844b517b..be578fc4e54f 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -124,7 +124,7 @@ struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
 }
 
 static void __iomem *_msm_ioremap(struct platform_device *pdev, const char *name,
-				  const char *dbgname, bool quiet)
+				  const char *dbgname, bool quiet, phys_addr_t *psize)
 {
 	struct resource *res;
 	unsigned long size;
@@ -153,37 +153,28 @@ static void __iomem *_msm_ioremap(struct platform_device *pdev, const char *name
 	if (reglog)
 		printk(KERN_DEBUG "IO:region %s %p %08lx\n", dbgname, ptr, size);
 
+	if (psize)
+		*psize = size;
+
 	return ptr;
 }
 
 void __iomem *msm_ioremap(struct platform_device *pdev, const char *name,
 			  const char *dbgname)
 {
-	return _msm_ioremap(pdev, name, dbgname, false);
+	return _msm_ioremap(pdev, name, dbgname, false, NULL);
 }
 
 void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name,
 				const char *dbgname)
 {
-	return _msm_ioremap(pdev, name, dbgname, true);
+	return _msm_ioremap(pdev, name, dbgname, true, NULL);
 }
 
-unsigned long msm_iomap_size(struct platform_device *pdev, const char *name)
+void __iomem *msm_ioremap_size(struct platform_device *pdev, const char *name,
+			  const char *dbgname, phys_addr_t *psize)
 {
-	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);
+	return _msm_ioremap(pdev, name, dbgname, false, psize);
 }
 
 void msm_writel(u32 data, void __iomem *addr)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 15cb34451ded..c33fc1293789 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -450,9 +450,10 @@ struct clk *msm_clk_bulk_get_clock(struct clk_bulk_data *bulk, int count,
 	const char *name);
 void __iomem *msm_ioremap(struct platform_device *pdev, const char *name,
 		const char *dbgname);
+void __iomem *msm_ioremap_size(struct platform_device *pdev, const char *name,
+		const char *dbgname, phys_addr_t *size);
 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);
-- 
2.30.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/4] drm/msm/dsi: add DSI PHY registers to snapshot data

[Dmitry Baryshkov]

Add DSI PHY registers to the msm state snapshots to be able to check
their contents.

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

diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 322d2e535df0..75afc12a7b25 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -269,5 +269,6 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 void msm_dsi_snapshot(struct msm_disp_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 b5679cf89413..cea73f9c4be9 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -176,6 +176,7 @@ int msm_dsi_phy_get_clk_provider(struct msm_dsi_phy *phy,
 	struct clk **byte_clk_provider, struct clk **pixel_clk_provider);
 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 f0a2ddf96a4b..bf7a4c20c13c 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -658,14 +658,14 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
 	phy->regulator_ldo_mode = of_property_read_bool(dev->of_node,
 				"qcom,dsi-phy-regulator-ldo-mode");
 
-	phy->base = msm_ioremap(pdev, "dsi_phy", "DSI_PHY");
+	phy->base = msm_ioremap_size(pdev, "dsi_phy", "DSI_PHY", &phy->base_size);
 	if (IS_ERR(phy->base)) {
 		DRM_DEV_ERROR(dev, "%s: failed to map phy base\n", __func__);
 		ret = -ENOMEM;
 		goto fail;
 	}
 
-	phy->pll_base = msm_ioremap(pdev, "dsi_pll", "DSI_PLL");
+	phy->pll_base = msm_ioremap_size(pdev, "dsi_pll", "DSI_PLL", &phy->pll_size);
 	if (IS_ERR(phy->pll_base)) {
 		DRM_DEV_ERROR(&pdev->dev, "%s: failed to map pll base\n", __func__);
 		ret = -ENOMEM;
@@ -673,7 +673,7 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
 	}
 
 	if (phy->cfg->has_phy_lane) {
-		phy->lane_base = msm_ioremap(pdev, "dsi_phy_lane", "DSI_PHY_LANE");
+		phy->lane_base = msm_ioremap_size(pdev, "dsi_phy_lane", "DSI_PHY_LANE", &phy->lane_size);
 		if (IS_ERR(phy->lane_base)) {
 			DRM_DEV_ERROR(&pdev->dev, "%s: failed to map phy lane base\n", __func__);
 			ret = -ENOMEM;
@@ -682,7 +682,7 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
 	}
 
 	if (phy->cfg->has_phy_regulator) {
-		phy->reg_base = msm_ioremap(pdev, "dsi_phy_regulator", "DSI_PHY_REG");
+		phy->reg_base = msm_ioremap_size(pdev, "dsi_phy_regulator", "DSI_PHY_REG", &phy->reg_size);
 		if (IS_ERR(phy->reg_base)) {
 			DRM_DEV_ERROR(&pdev->dev, "%s: failed to map phy regulator base\n", __func__);
 			ret = -ENOMEM;
@@ -868,3 +868,26 @@ int msm_dsi_phy_pll_restore_state(struct msm_dsi_phy *phy)
 
 	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,
+			phy->base_size, phy->base,
+			"dsi%d_phy", phy->id);
+
+	/* Do not try accessing PLL registers if it is switched off */
+	if (phy->pll_on)
+		msm_disp_snapshot_add_block(disp_state,
+			phy->pll_size, phy->pll_base,
+			"dsi%d_pll", phy->id);
+
+	if (phy->lane_base)
+		msm_disp_snapshot_add_block(disp_state,
+			phy->lane_size, phy->lane_base,
+			"dsi%d_lane", phy->id);
+
+	if (phy->reg_base)
+		msm_disp_snapshot_add_block(disp_state,
+			phy->reg_size, phy->reg_base,
+			"dsi%d_reg", phy->id);
+}
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
index 94a77ac364d3..5b0feef87127 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
@@ -85,6 +85,10 @@ struct msm_dsi_phy {
 	void __iomem *pll_base;
 	void __iomem *reg_base;
 	void __iomem *lane_base;
+	phys_addr_t base_size;
+	phys_addr_t pll_size;
+	phys_addr_t reg_size;
+	phys_addr_t lane_size;
 	int id;
 
 	struct clk *ahb_clk;
-- 
2.30.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/4] drm/msm: improve register snapshotting

[abhinavk]

On 2021-04-26 17:18, Dmitry Baryshkov wrote:
> Rework MSM coredump support: add DSI PHY registers, simplify
> snapshotting code.
> 
> Changes since v1:
>  - Readd mutex serializing register snapshot calls
> 
>  - Add DSI PHY register dumping support
> 
Need to mention the dependency here , got missed from the prev patchset
> ----------------------------------------------------------------
> Dmitry Baryshkov (4):
>       drm/msm: pass dump state as a function argument
>       drm/msm: make msm_disp_state transient data struct
>       drm/msm: get rid of msm_iomap_size
>       drm/msm/dsi: add DSI PHY registers to snapshot data
> 
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c           |  5 +-
>  drivers/gpu/drm/msm/disp/msm_disp_snapshot.c      | 90 
> +++++++----------------
>  drivers/gpu/drm/msm/disp/msm_disp_snapshot.h      | 21 +-----
>  drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 22 ++----
>  drivers/gpu/drm/msm/dp/dp_display.c               |  4 +-
>  drivers/gpu/drm/msm/dsi/dsi.c                     |  5 +-
>  drivers/gpu/drm/msm/dsi/dsi.h                     |  5 +-
>  drivers/gpu/drm/msm/dsi/dsi_host.c                | 11 +--
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy.c             | 31 +++++++-
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy.h             |  4 +
>  drivers/gpu/drm/msm/msm_drv.c                     | 27 +++----
>  drivers/gpu/drm/msm/msm_drv.h                     |  6 +-
>  drivers/gpu/drm/msm/msm_kms.h                     |  8 +-
>  13 files changed, 97 insertions(+), 142 deletions(-)
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Freedreno] [PATCH v2 2/4] drm/msm: make msm_disp_state transient data struct

[abhinavk]

Hi Dmitry

On 2021-04-26 17:18, Dmitry Baryshkov wrote:
> Instead of allocating snapshotting structure at the driver probe time
> and later handling concurrent access, actual state, etc, make
> msm_disp_state transient struct. Allocate one when snapshotting happens
> and free it after coredump data is read by userspace.
> 
Can you please check my previous comment on coredump_pending?

https://lore.kernel.org/dri-devel/186825e2fb7bea8d45f33b5c1fa3509f@codeaurora.org/T/#u

That helps to serialize read/write of coredump.

Rest of the changes on this one look fine to me.

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/msm/disp/msm_disp_snapshot.c  | 90 ++++++-------------
>  drivers/gpu/drm/msm/disp/msm_disp_snapshot.h  | 13 +--
>  .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |  5 +-
>  drivers/gpu/drm/msm/msm_kms.h                 |  6 +-
>  4 files changed, 37 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> index 70fd5a1fe13e..a4a7cb06bc87 100644
> --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> @@ -7,8 +7,7 @@
> 
>  #include "msm_disp_snapshot.h"
> 
> -#ifdef CONFIG_DEV_COREDUMP
> -static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
> +static ssize_t __maybe_unused disp_devcoredump_read(char *buffer,
> loff_t offset,
>  		size_t count, void *data, size_t datalen)
>  {
>  	struct drm_print_iterator iter;
> @@ -29,52 +28,47 @@ static ssize_t disp_devcoredump_read(char *buffer,
> loff_t offset,
>  	return count - iter.remain;
>  }
> 
> -static void disp_devcoredump_free(void *data)
> +static void _msm_disp_snapshot_work(struct kthread_work *work)
>  {
> +	struct msm_kms *kms = container_of(work, struct msm_kms, dump_work);
> +	struct drm_device *drm_dev = kms->dev;
>  	struct msm_disp_state *disp_state;
> +	struct drm_printer p;
> 
> -	disp_state = data;
> -
> -	msm_disp_state_free(disp_state);
> +	disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
> +	if (!disp_state)
> +		return;
> 
> -	disp_state->coredump_pending = false;
> -}
> -#endif /* CONFIG_DEV_COREDUMP */
> +	disp_state->dev = drm_dev->dev;
> +	disp_state->drm_dev = drm_dev;
> 
> -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;
> +	INIT_LIST_HEAD(&disp_state->blocks);
> 
> -	mutex_lock(&disp_state->mutex);
> +	/* Serialize dumping here */
> +	mutex_lock(&kms->dump_mutex);
> 
>  	msm_disp_snapshot_capture_state(disp_state);
> 
> +	mutex_unlock(&kms->dump_mutex);
> +
>  	if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
>  		p = drm_info_printer(disp_state->drm_dev->dev);
>  		msm_disp_state_print(disp_state, &p);
>  	}
> 
>  	/*
> -	 * if devcoredump is not defined free the state immediately
> -	 * otherwise it will be freed in the free handler.
> +	 * If COREDUMP is disabled, the stub will call the free function.
> +	 * If there is a codedump pending for the device, the dev_coredumpm()
> +	 * will also free new coredump state.
>  	 */
> -#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);
> +			disp_devcoredump_read, msm_disp_state_free);
>  }
> 
>  void msm_disp_snapshot_state(struct drm_device *drm_dev)
>  {
>  	struct msm_drm_private *priv;
>  	struct msm_kms *kms;
> -	struct msm_disp_state *disp_state;
> 
>  	if (!drm_dev) {
>  		DRM_ERROR("invalid params\n");
> @@ -83,30 +77,13 @@ void msm_disp_snapshot_state(struct drm_device 
> *drm_dev)
> 
>  	priv = drm_dev->dev_private;
>  	kms = priv->kms;
> -	disp_state = 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);
> +	kthread_queue_work(kms->dump_worker, &kms->dump_work);
>  }
> 
>  int msm_disp_snapshot_init(struct drm_device *drm_dev)
>  {
>  	struct msm_drm_private *priv;
> -	struct msm_disp_state *disp_state;
>  	struct msm_kms *kms;
> 
>  	if (!drm_dev) {
> @@ -117,22 +94,13 @@ int msm_disp_snapshot_init(struct drm_device 
> *drm_dev)
>  	priv = drm_dev->dev_private;
>  	kms = priv->kms;
> 
> -	disp_state = devm_kzalloc(drm_dev->dev, sizeof(struct
> msm_disp_state), GFP_KERNEL);
> -
> -	mutex_init(&disp_state->mutex);
> +	mutex_init(&kms->dump_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))
> +	kms->dump_worker = kthread_create_worker(0, "%s", "disp_snapshot");
> +	if (IS_ERR(kms->dump_worker))
>  		DRM_ERROR("failed to create disp state task\n");
> 
> -	kthread_init_work(&disp_state->dump_work, _msm_disp_snapshot_work);
> -
> -	kms->disp_state = disp_state;
> +	kthread_init_work(&kms->dump_work, _msm_disp_snapshot_work);
> 
>  	return 0;
>  }
> @@ -141,7 +109,6 @@ void msm_disp_snapshot_destroy(struct drm_device 
> *drm_dev)
>  {
>  	struct msm_kms *kms;
>  	struct msm_drm_private *priv;
> -	struct msm_disp_state *disp_state;
> 
>  	if (!drm_dev) {
>  		DRM_ERROR("invalid params\n");
> @@ -150,12 +117,9 @@ void msm_disp_snapshot_destroy(struct drm_device 
> *drm_dev)
> 
>  	priv = drm_dev->dev_private;
>  	kms = priv->kms;
> -	disp_state = kms->disp_state;
> -
> -	if (disp_state->dump_worker)
> -		kthread_destroy_worker(disp_state->dump_worker);
> 
> -	list_del(&disp_state->blocks);
> +	if (kms->dump_worker)
> +		kthread_destroy_worker(kms->dump_worker);
> 
> -	mutex_destroy(&disp_state->mutex);
> +	mutex_destroy(&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 32f52799a1ba..c6174a366095 100644
> --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
> @@ -41,26 +41,17 @@
>   * 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;
>  };
> 
> @@ -123,11 +114,11 @@ 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
> + * @data:	    handle to struct msm_disp_state
> 
>   * Returns: none
>   */
> -void msm_disp_state_free(struct msm_disp_state *disp_state);
> +void msm_disp_state_free(void *data);
> 
>  /**
>   * msm_disp_snapshot_add_block - add a hardware block with its 
> register dump
> 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 ca6632550337..cabe15190ec1 100644
> --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> @@ -142,8 +142,9 @@ void msm_disp_snapshot_capture_state(struct
> msm_disp_state *disp_state)
>  	msm_disp_capture_atomic_state(disp_state);
>  }
> 
> -void msm_disp_state_free(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) {
> @@ -156,6 +157,8 @@ void msm_disp_state_free(struct msm_disp_state 
> *disp_state)
>  		kfree(block->state);
>  		kfree(block);
>  	}
> +
> +	kfree(disp_state);
>  }
> 
>  void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, 
> u32 len,
> diff --git a/drivers/gpu/drm/msm/msm_kms.h 
> b/drivers/gpu/drm/msm/msm_kms.h
> index 146dcab123f4..086a2d59b8c8 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -156,8 +156,10 @@ struct msm_kms {
>  	/* mapper-id used to request GEM buffer mapped for scanout: */
>  	struct msm_gem_address_space *aspace;
> 
> -	/* handle to disp snapshot state */
> -	struct msm_disp_state *disp_state;
> +	/* disp snapshot support */
> +	struct kthread_worker *dump_worker;
> +	struct kthread_work dump_work;
> +	struct mutex dump_mutex;
> 
>  	/*
>  	 * For async commit, where ->flush_commit() and later happens
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Freedreno] [PATCH v2 3/4] drm/msm: get rid of msm_iomap_size

[abhinavk]

Hi Dmitry

On 2021-04-26 17:18, Dmitry Baryshkov wrote:
> Instead of looping throught the resources each time to get the DSI CTRL
> area size, get it at the ioremap time.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
We will have to call into the individual modules anyway everytime we
take a snapshot as only they have access to the required clocks and the 
base address.

So even though there is nothing wrong with this change, it still adds a 
size member
which can be avoided because we have to call into the module anyway.

Any strong preference to store the size as opposed to just getting it 
when we take
the snapshot?

> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c |  5 +++--
>  drivers/gpu/drm/msm/msm_drv.c      | 27 +++++++++------------------
>  drivers/gpu/drm/msm/msm_drv.h      |  3 ++-
>  3 files changed, 14 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 1a63368c3912..b3ee5c0bce12 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -102,6 +102,7 @@ struct msm_dsi_host {
>  	int id;
> 
>  	void __iomem *ctrl_base;
> +	phys_addr_t ctrl_size;
>  	struct regulator_bulk_data supplies[DSI_DEV_REGULATOR_MAX];
> 
>  	struct clk *bus_clks[DSI_BUS_CLK_MAX];
> @@ -1839,7 +1840,7 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
>  		goto fail;
>  	}
> 
> -	msm_host->ctrl_base = msm_ioremap(pdev, "dsi_ctrl", "DSI CTRL");
> +	msm_host->ctrl_base = msm_ioremap_size(pdev, "dsi_ctrl", "DSI CTRL",
> &msm_host->ctrl_size);
>  	if (IS_ERR(msm_host->ctrl_base)) {
>  		pr_err("%s: unable to map Dsi ctrl base\n", __func__);
>  		ret = PTR_ERR(msm_host->ctrl_base);
> @@ -2494,7 +2495,7 @@ void msm_dsi_host_snapshot(struct msm_disp_state
> *disp_state, struct mipi_dsi_ho
> 
>  	pm_runtime_get_sync(&msm_host->pdev->dev);
> 
> -	msm_disp_snapshot_add_block(disp_state,
> msm_iomap_size(msm_host->pdev, "dsi_ctrl"),
> +	msm_disp_snapshot_add_block(disp_state, msm_host->ctrl_size,
>  			msm_host->ctrl_base, "dsi%d_ctrl", msm_host->id);
> 
>  	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 92fe844b517b..be578fc4e54f 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -124,7 +124,7 @@ struct clk *msm_clk_get(struct platform_device
> *pdev, const char *name)
>  }
> 
>  static void __iomem *_msm_ioremap(struct platform_device *pdev, const
> char *name,
> -				  const char *dbgname, bool quiet)
> +				  const char *dbgname, bool quiet, phys_addr_t *psize)
>  {
>  	struct resource *res;
>  	unsigned long size;
> @@ -153,37 +153,28 @@ static void __iomem *_msm_ioremap(struct
> platform_device *pdev, const char *name
>  	if (reglog)
>  		printk(KERN_DEBUG "IO:region %s %p %08lx\n", dbgname, ptr, size);
> 
> +	if (psize)
> +		*psize = size;
> +
>  	return ptr;
>  }
> 
>  void __iomem *msm_ioremap(struct platform_device *pdev, const char 
> *name,
>  			  const char *dbgname)
>  {
> -	return _msm_ioremap(pdev, name, dbgname, false);
> +	return _msm_ioremap(pdev, name, dbgname, false, NULL);
>  }
> 
>  void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const 
> char *name,
>  				const char *dbgname)
>  {
> -	return _msm_ioremap(pdev, name, dbgname, true);
> +	return _msm_ioremap(pdev, name, dbgname, true, NULL);
>  }
> 
> -unsigned long msm_iomap_size(struct platform_device *pdev, const char 
> *name)
> +void __iomem *msm_ioremap_size(struct platform_device *pdev, const 
> char *name,
> +			  const char *dbgname, phys_addr_t *psize)
>  {
> -	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);
> +	return _msm_ioremap(pdev, name, dbgname, false, psize);
>  }
> 
>  void msm_writel(u32 data, void __iomem *addr)
> diff --git a/drivers/gpu/drm/msm/msm_drv.h 
> b/drivers/gpu/drm/msm/msm_drv.h
> index 15cb34451ded..c33fc1293789 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -450,9 +450,10 @@ struct clk *msm_clk_bulk_get_clock(struct
> clk_bulk_data *bulk, int count,
>  	const char *name);
>  void __iomem *msm_ioremap(struct platform_device *pdev, const char 
> *name,
>  		const char *dbgname);
> +void __iomem *msm_ioremap_size(struct platform_device *pdev, const 
> char *name,
> +		const char *dbgname, phys_addr_t *size);
>  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);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Freedreno] [PATCH v2 2/4] drm/msm: make msm_disp_state transient data struct

[Dmitry Baryshkov]

On Tue, 27 Apr 2021 at 22:19, <abhinavk@codeaurora.org> wrote:
>
> Hi Dmitry
>
> On 2021-04-26 17:18, Dmitry Baryshkov wrote:
> > Instead of allocating snapshotting structure at the driver probe time
> > and later handling concurrent access, actual state, etc, make
> > msm_disp_state transient struct. Allocate one when snapshotting happens
> > and free it after coredump data is read by userspace.
> >
> Can you please check my previous comment on coredump_pending?
>
> https://lore.kernel.org/dri-devel/186825e2fb7bea8d45f33b5c1fa3509f@codeaurora.org/T/#u
>
> That helps to serialize read/write of coredump.

Regarding the serialization of read/write. As the disp_state becomes
the transient object, the need for such serialization vanishes:
 - Before the snapshot is complete, the object is not linked outside
of msm_disp_snapshot functions.
 - When the snapshot is complete, it becomes linked from the codedump
subsystem. After this it is only read by the coredump, nobody will
write to it.
 - Next snapshot will allocate a new disp_state structure.
 - If dev_coredumpm is called when the previous snapshot is still
referenced (the device exists), the new snapshot is silently freed.

Thus there is no need to sync the read/write operations. They are now
naturally serialized.

>
> Rest of the changes on this one look fine to me.
>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/msm/disp/msm_disp_snapshot.c  | 90 ++++++-------------
> >  drivers/gpu/drm/msm/disp/msm_disp_snapshot.h  | 13 +--
> >  .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |  5 +-
> >  drivers/gpu/drm/msm/msm_kms.h                 |  6 +-
> >  4 files changed, 37 insertions(+), 77 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> > b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> > index 70fd5a1fe13e..a4a7cb06bc87 100644
> > --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> > +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> > @@ -7,8 +7,7 @@
> >
> >  #include "msm_disp_snapshot.h"
> >
> > -#ifdef CONFIG_DEV_COREDUMP
> > -static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
> > +static ssize_t __maybe_unused disp_devcoredump_read(char *buffer,
> > loff_t offset,
> >               size_t count, void *data, size_t datalen)
> >  {
> >       struct drm_print_iterator iter;
> > @@ -29,52 +28,47 @@ static ssize_t disp_devcoredump_read(char *buffer,
> > loff_t offset,
> >       return count - iter.remain;
> >  }
> >
> > -static void disp_devcoredump_free(void *data)
> > +static void _msm_disp_snapshot_work(struct kthread_work *work)
> >  {
> > +     struct msm_kms *kms = container_of(work, struct msm_kms, dump_work);
> > +     struct drm_device *drm_dev = kms->dev;
> >       struct msm_disp_state *disp_state;
> > +     struct drm_printer p;
> >
> > -     disp_state = data;
> > -
> > -     msm_disp_state_free(disp_state);
> > +     disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
> > +     if (!disp_state)
> > +             return;
> >
> > -     disp_state->coredump_pending = false;
> > -}
> > -#endif /* CONFIG_DEV_COREDUMP */
> > +     disp_state->dev = drm_dev->dev;
> > +     disp_state->drm_dev = drm_dev;
> >
> > -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;
> > +     INIT_LIST_HEAD(&disp_state->blocks);
> >
> > -     mutex_lock(&disp_state->mutex);
> > +     /* Serialize dumping here */
> > +     mutex_lock(&kms->dump_mutex);
> >
> >       msm_disp_snapshot_capture_state(disp_state);
> >
> > +     mutex_unlock(&kms->dump_mutex);
> > +
> >       if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
> >               p = drm_info_printer(disp_state->drm_dev->dev);
> >               msm_disp_state_print(disp_state, &p);
> >       }
> >
> >       /*
> > -      * if devcoredump is not defined free the state immediately
> > -      * otherwise it will be freed in the free handler.
> > +      * If COREDUMP is disabled, the stub will call the free function.
> > +      * If there is a codedump pending for the device, the dev_coredumpm()
> > +      * will also free new coredump state.
> >        */
> > -#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);
> > +                     disp_devcoredump_read, msm_disp_state_free);
> >  }
> >
> >  void msm_disp_snapshot_state(struct drm_device *drm_dev)
> >  {
> >       struct msm_drm_private *priv;
> >       struct msm_kms *kms;
> > -     struct msm_disp_state *disp_state;
> >
> >       if (!drm_dev) {
> >               DRM_ERROR("invalid params\n");
> > @@ -83,30 +77,13 @@ void msm_disp_snapshot_state(struct drm_device
> > *drm_dev)
> >
> >       priv = drm_dev->dev_private;
> >       kms = priv->kms;
> > -     disp_state = 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);
> > +     kthread_queue_work(kms->dump_worker, &kms->dump_work);
> >  }
> >
> >  int msm_disp_snapshot_init(struct drm_device *drm_dev)
> >  {
> >       struct msm_drm_private *priv;
> > -     struct msm_disp_state *disp_state;
> >       struct msm_kms *kms;
> >
> >       if (!drm_dev) {
> > @@ -117,22 +94,13 @@ int msm_disp_snapshot_init(struct drm_device
> > *drm_dev)
> >       priv = drm_dev->dev_private;
> >       kms = priv->kms;
> >
> > -     disp_state = devm_kzalloc(drm_dev->dev, sizeof(struct
> > msm_disp_state), GFP_KERNEL);
> > -
> > -     mutex_init(&disp_state->mutex);
> > +     mutex_init(&kms->dump_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))
> > +     kms->dump_worker = kthread_create_worker(0, "%s", "disp_snapshot");
> > +     if (IS_ERR(kms->dump_worker))
> >               DRM_ERROR("failed to create disp state task\n");
> >
> > -     kthread_init_work(&disp_state->dump_work, _msm_disp_snapshot_work);
> > -
> > -     kms->disp_state = disp_state;
> > +     kthread_init_work(&kms->dump_work, _msm_disp_snapshot_work);
> >
> >       return 0;
> >  }
> > @@ -141,7 +109,6 @@ void msm_disp_snapshot_destroy(struct drm_device
> > *drm_dev)
> >  {
> >       struct msm_kms *kms;
> >       struct msm_drm_private *priv;
> > -     struct msm_disp_state *disp_state;
> >
> >       if (!drm_dev) {
> >               DRM_ERROR("invalid params\n");
> > @@ -150,12 +117,9 @@ void msm_disp_snapshot_destroy(struct drm_device
> > *drm_dev)
> >
> >       priv = drm_dev->dev_private;
> >       kms = priv->kms;
> > -     disp_state = kms->disp_state;
> > -
> > -     if (disp_state->dump_worker)
> > -             kthread_destroy_worker(disp_state->dump_worker);
> >
> > -     list_del(&disp_state->blocks);
> > +     if (kms->dump_worker)
> > +             kthread_destroy_worker(kms->dump_worker);
> >
> > -     mutex_destroy(&disp_state->mutex);
> > +     mutex_destroy(&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 32f52799a1ba..c6174a366095 100644
> > --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
> > +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
> > @@ -41,26 +41,17 @@
> >   * 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;
> >  };
> >
> > @@ -123,11 +114,11 @@ 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
> > + * @data:        handle to struct msm_disp_state
> >
> >   * Returns: none
> >   */
> > -void msm_disp_state_free(struct msm_disp_state *disp_state);
> > +void msm_disp_state_free(void *data);
> >
> >  /**
> >   * msm_disp_snapshot_add_block - add a hardware block with its
> > register dump
> > 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 ca6632550337..cabe15190ec1 100644
> > --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> > +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> > @@ -142,8 +142,9 @@ void msm_disp_snapshot_capture_state(struct
> > msm_disp_state *disp_state)
> >       msm_disp_capture_atomic_state(disp_state);
> >  }
> >
> > -void msm_disp_state_free(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) {
> > @@ -156,6 +157,8 @@ void msm_disp_state_free(struct msm_disp_state
> > *disp_state)
> >               kfree(block->state);
> >               kfree(block);
> >       }
> > +
> > +     kfree(disp_state);
> >  }
> >
> >  void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state,
> > u32 len,
> > diff --git a/drivers/gpu/drm/msm/msm_kms.h
> > b/drivers/gpu/drm/msm/msm_kms.h
> > index 146dcab123f4..086a2d59b8c8 100644
> > --- a/drivers/gpu/drm/msm/msm_kms.h
> > +++ b/drivers/gpu/drm/msm/msm_kms.h
> > @@ -156,8 +156,10 @@ struct msm_kms {
> >       /* mapper-id used to request GEM buffer mapped for scanout: */
> >       struct msm_gem_address_space *aspace;
> >
> > -     /* handle to disp snapshot state */
> > -     struct msm_disp_state *disp_state;
> > +     /* disp snapshot support */
> > +     struct kthread_worker *dump_worker;
> > +     struct kthread_work dump_work;
> > +     struct mutex dump_mutex;
> >
> >       /*
> >        * For async commit, where ->flush_commit() and later happens



-- 
With best wishes
Dmitry
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Freedreno] [PATCH v2 3/4] drm/msm: get rid of msm_iomap_size

[Dmitry Baryshkov]

On Tue, 27 Apr 2021 at 22:30, <abhinavk@codeaurora.org> wrote:
>
> Hi Dmitry
>
> On 2021-04-26 17:18, Dmitry Baryshkov wrote:
> > Instead of looping throught the resources each time to get the DSI CTRL
> > area size, get it at the ioremap time.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> We will have to call into the individual modules anyway everytime we
> take a snapshot as only they have access to the required clocks and the
> base address.
>
> So even though there is nothing wrong with this change, it still adds a
> size member
> which can be avoided because we have to call into the module anyway.
>
> Any strong preference to store the size as opposed to just getting it
> when we take
> the snapshot?

Locality. We ioremap the resource from one piece of code and then we
get it's length from a completely different piece of code. If we ever
change e.g. the ioremap'ed resource name, we'd have to also update
snapshot users.
With this patch these changes are done in a transparent way. Whichever
we do with the regions in future, there is always a valid base + size
combo.

>
> > ---
> >  drivers/gpu/drm/msm/dsi/dsi_host.c |  5 +++--
> >  drivers/gpu/drm/msm/msm_drv.c      | 27 +++++++++------------------
> >  drivers/gpu/drm/msm/msm_drv.h      |  3 ++-
> >  3 files changed, 14 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index 1a63368c3912..b3ee5c0bce12 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -102,6 +102,7 @@ struct msm_dsi_host {
> >       int id;
> >
> >       void __iomem *ctrl_base;
> > +     phys_addr_t ctrl_size;
> >       struct regulator_bulk_data supplies[DSI_DEV_REGULATOR_MAX];
> >
> >       struct clk *bus_clks[DSI_BUS_CLK_MAX];
> > @@ -1839,7 +1840,7 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
> >               goto fail;
> >       }
> >
> > -     msm_host->ctrl_base = msm_ioremap(pdev, "dsi_ctrl", "DSI CTRL");
> > +     msm_host->ctrl_base = msm_ioremap_size(pdev, "dsi_ctrl", "DSI CTRL",
> > &msm_host->ctrl_size);
> >       if (IS_ERR(msm_host->ctrl_base)) {
> >               pr_err("%s: unable to map Dsi ctrl base\n", __func__);
> >               ret = PTR_ERR(msm_host->ctrl_base);
> > @@ -2494,7 +2495,7 @@ void msm_dsi_host_snapshot(struct msm_disp_state
> > *disp_state, struct mipi_dsi_ho
> >
> >       pm_runtime_get_sync(&msm_host->pdev->dev);
> >
> > -     msm_disp_snapshot_add_block(disp_state,
> > msm_iomap_size(msm_host->pdev, "dsi_ctrl"),
> > +     msm_disp_snapshot_add_block(disp_state, msm_host->ctrl_size,
> >                       msm_host->ctrl_base, "dsi%d_ctrl", msm_host->id);
> >
> >       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 92fe844b517b..be578fc4e54f 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -124,7 +124,7 @@ struct clk *msm_clk_get(struct platform_device
> > *pdev, const char *name)
> >  }
> >
> >  static void __iomem *_msm_ioremap(struct platform_device *pdev, const
> > char *name,
> > -                               const char *dbgname, bool quiet)
> > +                               const char *dbgname, bool quiet, phys_addr_t *psize)
> >  {
> >       struct resource *res;
> >       unsigned long size;
> > @@ -153,37 +153,28 @@ static void __iomem *_msm_ioremap(struct
> > platform_device *pdev, const char *name
> >       if (reglog)
> >               printk(KERN_DEBUG "IO:region %s %p %08lx\n", dbgname, ptr, size);
> >
> > +     if (psize)
> > +             *psize = size;
> > +
> >       return ptr;
> >  }
> >
> >  void __iomem *msm_ioremap(struct platform_device *pdev, const char
> > *name,
> >                         const char *dbgname)
> >  {
> > -     return _msm_ioremap(pdev, name, dbgname, false);
> > +     return _msm_ioremap(pdev, name, dbgname, false, NULL);
> >  }
> >
> >  void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const
> > char *name,
> >                               const char *dbgname)
> >  {
> > -     return _msm_ioremap(pdev, name, dbgname, true);
> > +     return _msm_ioremap(pdev, name, dbgname, true, NULL);
> >  }
> >
> > -unsigned long msm_iomap_size(struct platform_device *pdev, const char
> > *name)
> > +void __iomem *msm_ioremap_size(struct platform_device *pdev, const
> > char *name,
> > +                       const char *dbgname, phys_addr_t *psize)
> >  {
> > -     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);
> > +     return _msm_ioremap(pdev, name, dbgname, false, psize);
> >  }
> >
> >  void msm_writel(u32 data, void __iomem *addr)
> > diff --git a/drivers/gpu/drm/msm/msm_drv.h
> > b/drivers/gpu/drm/msm/msm_drv.h
> > index 15cb34451ded..c33fc1293789 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.h
> > +++ b/drivers/gpu/drm/msm/msm_drv.h
> > @@ -450,9 +450,10 @@ struct clk *msm_clk_bulk_get_clock(struct
> > clk_bulk_data *bulk, int count,
> >       const char *name);
> >  void __iomem *msm_ioremap(struct platform_device *pdev, const char
> > *name,
> >               const char *dbgname);
> > +void __iomem *msm_ioremap_size(struct platform_device *pdev, const
> > char *name,
> > +             const char *dbgname, phys_addr_t *size);
> >  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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Freedreno] [PATCH v2 2/4] drm/msm: make msm_disp_state transient data struct

[abhinavk]

On 2021-04-27 13:29, Dmitry Baryshkov wrote:
> On Tue, 27 Apr 2021 at 22:19, <abhinavk@codeaurora.org> wrote:
>> 
>> Hi Dmitry
>> 
>> On 2021-04-26 17:18, Dmitry Baryshkov wrote:
>> > Instead of allocating snapshotting structure at the driver probe time
>> > and later handling concurrent access, actual state, etc, make
>> > msm_disp_state transient struct. Allocate one when snapshotting happens
>> > and free it after coredump data is read by userspace.
>> >
>> Can you please check my previous comment on coredump_pending?
>> 
>> https://lore.kernel.org/dri-devel/186825e2fb7bea8d45f33b5c1fa3509f@codeaurora.org/T/#u
>> 
>> That helps to serialize read/write of coredump.
> 
> Regarding the serialization of read/write. As the disp_state becomes
> the transient object, the need for such serialization vanishes:
>  - Before the snapshot is complete, the object is not linked outside
> of msm_disp_snapshot functions.
>  - When the snapshot is complete, it becomes linked from the codedump
> subsystem. After this it is only read by the coredump, nobody will
> write to it.
>  - Next snapshot will allocate a new disp_state structure.
>  - If dev_coredumpm is called when the previous snapshot is still
> referenced (the device exists), the new snapshot is silently freed.
> 
> Thus there is no need to sync the read/write operations. They are now
> naturally serialized.
Alright, just make sure to validate the robustness of this using the 
method mentioned earlier.
Apart from that,

Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
> 
>> 
>> Rest of the changes on this one look fine to me.
>> 
>> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> > ---
>> >  drivers/gpu/drm/msm/disp/msm_disp_snapshot.c  | 90 ++++++-------------
>> >  drivers/gpu/drm/msm/disp/msm_disp_snapshot.h  | 13 +--
>> >  .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |  5 +-
>> >  drivers/gpu/drm/msm/msm_kms.h                 |  6 +-
>> >  4 files changed, 37 insertions(+), 77 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
>> > b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
>> > index 70fd5a1fe13e..a4a7cb06bc87 100644
>> > --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
>> > +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
>> > @@ -7,8 +7,7 @@
>> >
>> >  #include "msm_disp_snapshot.h"
>> >
>> > -#ifdef CONFIG_DEV_COREDUMP
>> > -static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
>> > +static ssize_t __maybe_unused disp_devcoredump_read(char *buffer,
>> > loff_t offset,
>> >               size_t count, void *data, size_t datalen)
>> >  {
>> >       struct drm_print_iterator iter;
>> > @@ -29,52 +28,47 @@ static ssize_t disp_devcoredump_read(char *buffer,
>> > loff_t offset,
>> >       return count - iter.remain;
>> >  }
>> >
>> > -static void disp_devcoredump_free(void *data)
>> > +static void _msm_disp_snapshot_work(struct kthread_work *work)
>> >  {
>> > +     struct msm_kms *kms = container_of(work, struct msm_kms, dump_work);
>> > +     struct drm_device *drm_dev = kms->dev;
>> >       struct msm_disp_state *disp_state;
>> > +     struct drm_printer p;
>> >
>> > -     disp_state = data;
>> > -
>> > -     msm_disp_state_free(disp_state);
>> > +     disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
>> > +     if (!disp_state)
>> > +             return;
>> >
>> > -     disp_state->coredump_pending = false;
>> > -}
>> > -#endif /* CONFIG_DEV_COREDUMP */
>> > +     disp_state->dev = drm_dev->dev;
>> > +     disp_state->drm_dev = drm_dev;
>> >
>> > -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;
>> > +     INIT_LIST_HEAD(&disp_state->blocks);
>> >
>> > -     mutex_lock(&disp_state->mutex);
>> > +     /* Serialize dumping here */
>> > +     mutex_lock(&kms->dump_mutex);
>> >
>> >       msm_disp_snapshot_capture_state(disp_state);
>> >
>> > +     mutex_unlock(&kms->dump_mutex);
>> > +
>> >       if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
>> >               p = drm_info_printer(disp_state->drm_dev->dev);
>> >               msm_disp_state_print(disp_state, &p);
>> >       }
>> >
>> >       /*
>> > -      * if devcoredump is not defined free the state immediately
>> > -      * otherwise it will be freed in the free handler.
>> > +      * If COREDUMP is disabled, the stub will call the free function.
>> > +      * If there is a codedump pending for the device, the dev_coredumpm()
>> > +      * will also free new coredump state.
>> >        */
>> > -#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);
>> > +                     disp_devcoredump_read, msm_disp_state_free);
>> >  }
>> >
>> >  void msm_disp_snapshot_state(struct drm_device *drm_dev)
>> >  {
>> >       struct msm_drm_private *priv;
>> >       struct msm_kms *kms;
>> > -     struct msm_disp_state *disp_state;
>> >
>> >       if (!drm_dev) {
>> >               DRM_ERROR("invalid params\n");
>> > @@ -83,30 +77,13 @@ void msm_disp_snapshot_state(struct drm_device
>> > *drm_dev)
>> >
>> >       priv = drm_dev->dev_private;
>> >       kms = priv->kms;
>> > -     disp_state = 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);
>> > +     kthread_queue_work(kms->dump_worker, &kms->dump_work);
>> >  }
>> >
>> >  int msm_disp_snapshot_init(struct drm_device *drm_dev)
>> >  {
>> >       struct msm_drm_private *priv;
>> > -     struct msm_disp_state *disp_state;
>> >       struct msm_kms *kms;
>> >
>> >       if (!drm_dev) {
>> > @@ -117,22 +94,13 @@ int msm_disp_snapshot_init(struct drm_device
>> > *drm_dev)
>> >       priv = drm_dev->dev_private;
>> >       kms = priv->kms;
>> >
>> > -     disp_state = devm_kzalloc(drm_dev->dev, sizeof(struct
>> > msm_disp_state), GFP_KERNEL);
>> > -
>> > -     mutex_init(&disp_state->mutex);
>> > +     mutex_init(&kms->dump_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))
>> > +     kms->dump_worker = kthread_create_worker(0, "%s", "disp_snapshot");
>> > +     if (IS_ERR(kms->dump_worker))
>> >               DRM_ERROR("failed to create disp state task\n");
>> >
>> > -     kthread_init_work(&disp_state->dump_work, _msm_disp_snapshot_work);
>> > -
>> > -     kms->disp_state = disp_state;
>> > +     kthread_init_work(&kms->dump_work, _msm_disp_snapshot_work);
>> >
>> >       return 0;
>> >  }
>> > @@ -141,7 +109,6 @@ void msm_disp_snapshot_destroy(struct drm_device
>> > *drm_dev)
>> >  {
>> >       struct msm_kms *kms;
>> >       struct msm_drm_private *priv;
>> > -     struct msm_disp_state *disp_state;
>> >
>> >       if (!drm_dev) {
>> >               DRM_ERROR("invalid params\n");
>> > @@ -150,12 +117,9 @@ void msm_disp_snapshot_destroy(struct drm_device
>> > *drm_dev)
>> >
>> >       priv = drm_dev->dev_private;
>> >       kms = priv->kms;
>> > -     disp_state = kms->disp_state;
>> > -
>> > -     if (disp_state->dump_worker)
>> > -             kthread_destroy_worker(disp_state->dump_worker);
>> >
>> > -     list_del(&disp_state->blocks);
>> > +     if (kms->dump_worker)
>> > +             kthread_destroy_worker(kms->dump_worker);
>> >
>> > -     mutex_destroy(&disp_state->mutex);
>> > +     mutex_destroy(&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 32f52799a1ba..c6174a366095 100644
>> > --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
>> > +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
>> > @@ -41,26 +41,17 @@
>> >   * 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;
>> >  };
>> >
>> > @@ -123,11 +114,11 @@ 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
>> > + * @data:        handle to struct msm_disp_state
>> >
>> >   * Returns: none
>> >   */
>> > -void msm_disp_state_free(struct msm_disp_state *disp_state);
>> > +void msm_disp_state_free(void *data);
>> >
>> >  /**
>> >   * msm_disp_snapshot_add_block - add a hardware block with its
>> > register dump
>> > 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 ca6632550337..cabe15190ec1 100644
>> > --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
>> > +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
>> > @@ -142,8 +142,9 @@ void msm_disp_snapshot_capture_state(struct
>> > msm_disp_state *disp_state)
>> >       msm_disp_capture_atomic_state(disp_state);
>> >  }
>> >
>> > -void msm_disp_state_free(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) {
>> > @@ -156,6 +157,8 @@ void msm_disp_state_free(struct msm_disp_state
>> > *disp_state)
>> >               kfree(block->state);
>> >               kfree(block);
>> >       }
>> > +
>> > +     kfree(disp_state);
>> >  }
>> >
>> >  void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state,
>> > u32 len,
>> > diff --git a/drivers/gpu/drm/msm/msm_kms.h
>> > b/drivers/gpu/drm/msm/msm_kms.h
>> > index 146dcab123f4..086a2d59b8c8 100644
>> > --- a/drivers/gpu/drm/msm/msm_kms.h
>> > +++ b/drivers/gpu/drm/msm/msm_kms.h
>> > @@ -156,8 +156,10 @@ struct msm_kms {
>> >       /* mapper-id used to request GEM buffer mapped for scanout: */
>> >       struct msm_gem_address_space *aspace;
>> >
>> > -     /* handle to disp snapshot state */
>> > -     struct msm_disp_state *disp_state;
>> > +     /* disp snapshot support */
>> > +     struct kthread_worker *dump_worker;
>> > +     struct kthread_work dump_work;
>> > +     struct mutex dump_mutex;
>> >
>> >       /*
>> >        * For async commit, where ->flush_commit() and later happens
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Freedreno] [PATCH v2 3/4] drm/msm: get rid of msm_iomap_size

[abhinavk]

On 2021-04-27 13:32, Dmitry Baryshkov wrote:
> On Tue, 27 Apr 2021 at 22:30, <abhinavk@codeaurora.org> wrote:
>> 
>> Hi Dmitry
>> 
>> On 2021-04-26 17:18, Dmitry Baryshkov wrote:
>> > Instead of looping throught the resources each time to get the DSI CTRL
>> > area size, get it at the ioremap time.
>> >
>> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> We will have to call into the individual modules anyway everytime we
>> take a snapshot as only they have access to the required clocks and 
>> the
>> base address.
>> 
>> So even though there is nothing wrong with this change, it still adds 
>> a
>> size member
>> which can be avoided because we have to call into the module anyway.
>> 
>> Any strong preference to store the size as opposed to just getting it
>> when we take
>> the snapshot?
> 
> Locality. We ioremap the resource from one piece of code and then we
> get it's length from a completely different piece of code. If we ever
> change e.g. the ioremap'ed resource name, we'd have to also update
> snapshot users.
> With this patch these changes are done in a transparent way. Whichever
> we do with the regions in future, there is always a valid base + size
> combo.
> 
Alright, no further concerns from my side on this:

Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>

>> 
>> > ---
>> >  drivers/gpu/drm/msm/dsi/dsi_host.c |  5 +++--
>> >  drivers/gpu/drm/msm/msm_drv.c      | 27 +++++++++------------------
>> >  drivers/gpu/drm/msm/msm_drv.h      |  3 ++-
>> >  3 files changed, 14 insertions(+), 21 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> > b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> > index 1a63368c3912..b3ee5c0bce12 100644
>> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> > @@ -102,6 +102,7 @@ struct msm_dsi_host {
>> >       int id;
>> >
>> >       void __iomem *ctrl_base;
>> > +     phys_addr_t ctrl_size;
>> >       struct regulator_bulk_data supplies[DSI_DEV_REGULATOR_MAX];
>> >
>> >       struct clk *bus_clks[DSI_BUS_CLK_MAX];
>> > @@ -1839,7 +1840,7 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
>> >               goto fail;
>> >       }
>> >
>> > -     msm_host->ctrl_base = msm_ioremap(pdev, "dsi_ctrl", "DSI CTRL");
>> > +     msm_host->ctrl_base = msm_ioremap_size(pdev, "dsi_ctrl", "DSI CTRL",
>> > &msm_host->ctrl_size);
>> >       if (IS_ERR(msm_host->ctrl_base)) {
>> >               pr_err("%s: unable to map Dsi ctrl base\n", __func__);
>> >               ret = PTR_ERR(msm_host->ctrl_base);
>> > @@ -2494,7 +2495,7 @@ void msm_dsi_host_snapshot(struct msm_disp_state
>> > *disp_state, struct mipi_dsi_ho
>> >
>> >       pm_runtime_get_sync(&msm_host->pdev->dev);
>> >
>> > -     msm_disp_snapshot_add_block(disp_state,
>> > msm_iomap_size(msm_host->pdev, "dsi_ctrl"),
>> > +     msm_disp_snapshot_add_block(disp_state, msm_host->ctrl_size,
>> >                       msm_host->ctrl_base, "dsi%d_ctrl", msm_host->id);
>> >
>> >       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 92fe844b517b..be578fc4e54f 100644
>> > --- a/drivers/gpu/drm/msm/msm_drv.c
>> > +++ b/drivers/gpu/drm/msm/msm_drv.c
>> > @@ -124,7 +124,7 @@ struct clk *msm_clk_get(struct platform_device
>> > *pdev, const char *name)
>> >  }
>> >
>> >  static void __iomem *_msm_ioremap(struct platform_device *pdev, const
>> > char *name,
>> > -                               const char *dbgname, bool quiet)
>> > +                               const char *dbgname, bool quiet, phys_addr_t *psize)
>> >  {
>> >       struct resource *res;
>> >       unsigned long size;
>> > @@ -153,37 +153,28 @@ static void __iomem *_msm_ioremap(struct
>> > platform_device *pdev, const char *name
>> >       if (reglog)
>> >               printk(KERN_DEBUG "IO:region %s %p %08lx\n", dbgname, ptr, size);
>> >
>> > +     if (psize)
>> > +             *psize = size;
>> > +
>> >       return ptr;
>> >  }
>> >
>> >  void __iomem *msm_ioremap(struct platform_device *pdev, const char
>> > *name,
>> >                         const char *dbgname)
>> >  {
>> > -     return _msm_ioremap(pdev, name, dbgname, false);
>> > +     return _msm_ioremap(pdev, name, dbgname, false, NULL);
>> >  }
>> >
>> >  void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const
>> > char *name,
>> >                               const char *dbgname)
>> >  {
>> > -     return _msm_ioremap(pdev, name, dbgname, true);
>> > +     return _msm_ioremap(pdev, name, dbgname, true, NULL);
>> >  }
>> >
>> > -unsigned long msm_iomap_size(struct platform_device *pdev, const char
>> > *name)
>> > +void __iomem *msm_ioremap_size(struct platform_device *pdev, const
>> > char *name,
>> > +                       const char *dbgname, phys_addr_t *psize)
>> >  {
>> > -     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);
>> > +     return _msm_ioremap(pdev, name, dbgname, false, psize);
>> >  }
>> >
>> >  void msm_writel(u32 data, void __iomem *addr)
>> > diff --git a/drivers/gpu/drm/msm/msm_drv.h
>> > b/drivers/gpu/drm/msm/msm_drv.h
>> > index 15cb34451ded..c33fc1293789 100644
>> > --- a/drivers/gpu/drm/msm/msm_drv.h
>> > +++ b/drivers/gpu/drm/msm/msm_drv.h
>> > @@ -450,9 +450,10 @@ struct clk *msm_clk_bulk_get_clock(struct
>> > clk_bulk_data *bulk, int count,
>> >       const char *name);
>> >  void __iomem *msm_ioremap(struct platform_device *pdev, const char
>> > *name,
>> >               const char *dbgname);
>> > +void __iomem *msm_ioremap_size(struct platform_device *pdev, const
>> > char *name,
>> > +             const char *dbgname, phys_addr_t *size);
>> >  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);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/4] drm/msm/dsi: add DSI PHY registers to snapshot data

[abhinavk]

On 2021-04-26 17:18, Dmitry Baryshkov wrote:
> Add DSI PHY registers to the msm state snapshots to be able to check
> their contents.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/dsi/dsi.c         |  1 +
>  drivers/gpu/drm/msm/dsi/dsi.h         |  1 +
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 31 +++++++++++++++++++++++----
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy.h |  4 ++++
>  4 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c 
> b/drivers/gpu/drm/msm/dsi/dsi.c
> index 322d2e535df0..75afc12a7b25 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> @@ -269,5 +269,6 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi,
> struct drm_device *dev,
>  void msm_dsi_snapshot(struct msm_disp_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 b5679cf89413..cea73f9c4be9 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -176,6 +176,7 @@ int msm_dsi_phy_get_clk_provider(struct msm_dsi_phy 
> *phy,
>  	struct clk **byte_clk_provider, struct clk **pixel_clk_provider);
>  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 f0a2ddf96a4b..bf7a4c20c13c 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> @@ -658,14 +658,14 @@ static int dsi_phy_driver_probe(struct
> platform_device *pdev)
>  	phy->regulator_ldo_mode = of_property_read_bool(dev->of_node,
>  				"qcom,dsi-phy-regulator-ldo-mode");
> 
> -	phy->base = msm_ioremap(pdev, "dsi_phy", "DSI_PHY");
> +	phy->base = msm_ioremap_size(pdev, "dsi_phy", "DSI_PHY", 
> &phy->base_size);
>  	if (IS_ERR(phy->base)) {
>  		DRM_DEV_ERROR(dev, "%s: failed to map phy base\n", __func__);
>  		ret = -ENOMEM;
>  		goto fail;
>  	}
> 
> -	phy->pll_base = msm_ioremap(pdev, "dsi_pll", "DSI_PLL");
> +	phy->pll_base = msm_ioremap_size(pdev, "dsi_pll", "DSI_PLL", 
> &phy->pll_size);
>  	if (IS_ERR(phy->pll_base)) {
>  		DRM_DEV_ERROR(&pdev->dev, "%s: failed to map pll base\n", __func__);
>  		ret = -ENOMEM;
> @@ -673,7 +673,7 @@ static int dsi_phy_driver_probe(struct
> platform_device *pdev)
>  	}
> 
>  	if (phy->cfg->has_phy_lane) {
> -		phy->lane_base = msm_ioremap(pdev, "dsi_phy_lane", "DSI_PHY_LANE");
> +		phy->lane_base = msm_ioremap_size(pdev, "dsi_phy_lane",
> "DSI_PHY_LANE", &phy->lane_size);
>  		if (IS_ERR(phy->lane_base)) {
>  			DRM_DEV_ERROR(&pdev->dev, "%s: failed to map phy lane base\n", 
> __func__);
>  			ret = -ENOMEM;
> @@ -682,7 +682,7 @@ static int dsi_phy_driver_probe(struct
> platform_device *pdev)
>  	}
> 
>  	if (phy->cfg->has_phy_regulator) {
> -		phy->reg_base = msm_ioremap(pdev, "dsi_phy_regulator", 
> "DSI_PHY_REG");
> +		phy->reg_base = msm_ioremap_size(pdev, "dsi_phy_regulator",
> "DSI_PHY_REG", &phy->reg_size);
>  		if (IS_ERR(phy->reg_base)) {
>  			DRM_DEV_ERROR(&pdev->dev, "%s: failed to map phy regulator
> base\n", __func__);
>  			ret = -ENOMEM;
> @@ -868,3 +868,26 @@ int msm_dsi_phy_pll_restore_state(struct 
> msm_dsi_phy *phy)
> 
>  	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,
> +			phy->base_size, phy->base,
> +			"dsi%d_phy", phy->id);
> +
> +	/* Do not try accessing PLL registers if it is switched off */
> +	if (phy->pll_on)
> +		msm_disp_snapshot_add_block(disp_state,
> +			phy->pll_size, phy->pll_base,
> +			"dsi%d_pll", phy->id);
> +
> +	if (phy->lane_base)
> +		msm_disp_snapshot_add_block(disp_state,
> +			phy->lane_size, phy->lane_base,
> +			"dsi%d_lane", phy->id);
> +
> +	if (phy->reg_base)
> +		msm_disp_snapshot_add_block(disp_state,
> +			phy->reg_size, phy->reg_base,
> +			"dsi%d_reg", phy->id);
> +}
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> index 94a77ac364d3..5b0feef87127 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> @@ -85,6 +85,10 @@ struct msm_dsi_phy {
>  	void __iomem *pll_base;
>  	void __iomem *reg_base;
>  	void __iomem *lane_base;
> +	phys_addr_t base_size;
> +	phys_addr_t pll_size;
> +	phys_addr_t reg_size;
> +	phys_addr_t lane_size;
>  	int id;
> 
>  	struct clk *ahb_clk;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/4] drm/msm: get rid of msm_iomap_size

[Bjorn Andersson]

On Mon 26 Apr 19:18 CDT 2021, Dmitry Baryshkov wrote:
[..]
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 92fe844b517b..be578fc4e54f 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -124,7 +124,7 @@ struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
>  }
>  
>  static void __iomem *_msm_ioremap(struct platform_device *pdev, const char *name,
> -				  const char *dbgname, bool quiet)
> +				  const char *dbgname, bool quiet, phys_addr_t *psize)

size_t sounds like a better fit for psize...

Regards,
Bjorn
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/4] drm/msm: get rid of msm_iomap_size

[Dmitry Baryshkov]

On 28/04/2021 05:47, Bjorn Andersson wrote:
> On Mon 26 Apr 19:18 CDT 2021, Dmitry Baryshkov wrote:
> [..]
>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>> index 92fe844b517b..be578fc4e54f 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.c
>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>> @@ -124,7 +124,7 @@ struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
>>   }
>>   
>>   static void __iomem *_msm_ioremap(struct platform_device *pdev, const char *name,
>> -				  const char *dbgname, bool quiet)
>> +				  const char *dbgname, bool quiet, phys_addr_t *psize)
> 
> size_t sounds like a better fit for psize...

I was trying to select between size_t and phys_addr_t, settling on the 
latter one because it is used for resource size.


-- 
With best wishes
Dmitry
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/4] drm/msm: get rid of msm_iomap_size

[Bjorn Andersson]

On Wed 28 Apr 08:41 CDT 2021, Dmitry Baryshkov wrote:

> On 28/04/2021 05:47, Bjorn Andersson wrote:
> > On Mon 26 Apr 19:18 CDT 2021, Dmitry Baryshkov wrote:
> > [..]
> > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > > index 92fe844b517b..be578fc4e54f 100644
> > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > @@ -124,7 +124,7 @@ struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
> > >   }
> > >   static void __iomem *_msm_ioremap(struct platform_device *pdev, const char *name,
> > > -				  const char *dbgname, bool quiet)
> > > +				  const char *dbgname, bool quiet, phys_addr_t *psize)
> > 
> > size_t sounds like a better fit for psize...
> 
> I was trying to select between size_t and phys_addr_t, settling on the
> latter one because it is used for resource size.
> 

I always thought resource_size_t was an alias for size_t, now I know :)

That said, I still think that size_t (in line with resource_size_t)
gives a better hint about what the parameter represents...

Regards,
Bjorn
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/4] drm/msm: get rid of msm_iomap_size

[Dmitry Baryshkov]

On 28/04/2021 16:59, Bjorn Andersson wrote:
> On Wed 28 Apr 08:41 CDT 2021, Dmitry Baryshkov wrote:
> 
>> On 28/04/2021 05:47, Bjorn Andersson wrote:
>>> On Mon 26 Apr 19:18 CDT 2021, Dmitry Baryshkov wrote:
>>> [..]
>>>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>>>> index 92fe844b517b..be578fc4e54f 100644
>>>> --- a/drivers/gpu/drm/msm/msm_drv.c
>>>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>>>> @@ -124,7 +124,7 @@ struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
>>>>    }
>>>>    static void __iomem *_msm_ioremap(struct platform_device *pdev, const char *name,
>>>> -				  const char *dbgname, bool quiet)
>>>> +				  const char *dbgname, bool quiet, phys_addr_t *psize)
>>>
>>> size_t sounds like a better fit for psize...
>>
>> I was trying to select between size_t and phys_addr_t, settling on the
>> latter one because it is used for resource size.
>>
> 
> I always thought resource_size_t was an alias for size_t, now I know :)
> 
> That said, I still think that size_t (in line with resource_size_t)
> gives a better hint about what the parameter represents...

Indeed, I'll change that in the next version.

> 
> Regards,
> Bjorn
> 


-- 
With best wishes
Dmitry
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel