* [PATCH 0/2] drm/msm: rework display snapshotting
@ 2021-04-25 16:07 Dmitry Baryshkov
2021-04-25 16:07 ` [PATCH 1/2] drm/msm: pass dump state as a function argument Dmitry Baryshkov
2021-04-25 16:08 ` [PATCH 2/2] drm/msm: make msm_disp_state transient data struct Dmitry Baryshkov
0 siblings, 2 replies; 9+ messages in thread
From: Dmitry Baryshkov @ 2021-04-25 16:07 UTC (permalink / raw)
To: Bjorn Andersson, Rob Clark, Sean Paul, Abhinav Kumar
Cc: Jonathan Marek, Stephen Boyd, David Airlie, Daniel Vetter,
linux-arm-msm, dri-devel, freedreno
Rework display subsystem snapshotting by making msm_disp_state transient
data struct. This simplifies handling of data.
Dependencies:
https://lore.kernel.org/linux-arm-msm/1618606645-19695-1-git-send-email-abhinavk@codeaurora.org/
----------------------------------------------------------------
Dmitry Baryshkov (2):
drm/msm: pass dump state as a function argument
drm/msm: make msm_disp_state transient data struct
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 5 +-
drivers/gpu/drm/msm/disp/msm_disp_snapshot.c | 87 +++++------------------
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 | 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 | 7 +-
10 files changed, 41 insertions(+), 122 deletions(-)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] drm/msm: pass dump state as a function argument
2021-04-25 16:07 [PATCH 0/2] drm/msm: rework display snapshotting Dmitry Baryshkov
@ 2021-04-25 16:07 ` Dmitry Baryshkov
2021-04-26 20:13 ` abhinavk
2021-04-25 16:08 ` [PATCH 2/2] drm/msm: make msm_disp_state transient data struct Dmitry Baryshkov
1 sibling, 1 reply; 9+ messages in thread
From: Dmitry Baryshkov @ 2021-04-25 16:07 UTC (permalink / raw)
To: Bjorn Andersson, Rob Clark, Sean Paul, Abhinav Kumar
Cc: Jonathan Marek, Stephen Boyd, David Airlie, Daniel Vetter,
linux-arm-msm, dri-devel, freedreno
Instead of always getting the disp_state from drm device, pass it as an
argument.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] drm/msm: make msm_disp_state transient data struct
2021-04-25 16:07 [PATCH 0/2] drm/msm: rework display snapshotting Dmitry Baryshkov
2021-04-25 16:07 ` [PATCH 1/2] drm/msm: pass dump state as a function argument Dmitry Baryshkov
@ 2021-04-25 16:08 ` Dmitry Baryshkov
2021-04-26 20:50 ` abhinavk
1 sibling, 1 reply; 9+ messages in thread
From: Dmitry Baryshkov @ 2021-04-25 16:08 UTC (permalink / raw)
To: Bjorn Andersson, Rob Clark, Sean Paul, Abhinav Kumar
Cc: Jonathan Marek, Stephen Boyd, David Airlie, Daniel Vetter,
linux-arm-msm, dri-devel, freedreno
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 | 87 ++++---------------
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 | 5 +-
4 files changed, 28 insertions(+), 82 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..371358893716 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,24 +28,21 @@ 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 *msm_kms = container_of(work, struct msm_kms, dump_work);
+ struct drm_device *drm_dev = msm_kms->dev;
struct msm_disp_state *disp_state;
+ struct drm_printer p;
- disp_state = data;
-
- msm_disp_state_free(disp_state);
-
- disp_state->coredump_pending = false;
-}
-#endif /* CONFIG_DEV_COREDUMP */
+ disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
+ if (!disp_state)
+ return;
-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;
+ disp_state->dev = drm_dev->dev;
+ disp_state->drm_dev = drm_dev;
- mutex_lock(&disp_state->mutex);
+ INIT_LIST_HEAD(&disp_state->blocks);
msm_disp_snapshot_capture_state(disp_state);
@@ -55,26 +51,15 @@ static void _msm_disp_snapshot_work(struct kthread_work *work)
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.
- */
-#ifdef CONFIG_DEV_COREDUMP
+ /* If COREDUMP is disabled, the stub will call the free function. */
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 +68,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 +85,11 @@ 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);
-
- 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 +98,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 +106,7 @@ 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);
- mutex_destroy(&disp_state->mutex);
+ if (kms->dump_worker)
+ kthread_destroy_worker(kms->dump_worker);
}
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..529b9dacf7ca 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -156,8 +156,9 @@ 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;
/*
* For async commit, where ->flush_commit() and later happens
--
2.30.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm/msm: pass dump state as a function argument
2021-04-25 16:07 ` [PATCH 1/2] drm/msm: pass dump state as a function argument Dmitry Baryshkov
@ 2021-04-26 20:13 ` abhinavk
0 siblings, 0 replies; 9+ messages in thread
From: abhinavk @ 2021-04-26 20:13 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Bjorn Andersson, Rob Clark, Sean Paul, Jonathan Marek,
Stephen Boyd, David Airlie, Daniel Vetter, linux-arm-msm,
dri-devel, freedreno
On 2021-04-25 09:07, Dmitry Baryshkov wrote:
> Instead of always getting the disp_state from drm device, pass it as an
> argument.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
This certainly reduces some amount of code, I am onboard with this,
hence:
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: */
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/msm: make msm_disp_state transient data struct
2021-04-25 16:08 ` [PATCH 2/2] drm/msm: make msm_disp_state transient data struct Dmitry Baryshkov
@ 2021-04-26 20:50 ` abhinavk
2021-04-26 21:23 ` Dmitry Baryshkov
0 siblings, 1 reply; 9+ messages in thread
From: abhinavk @ 2021-04-26 20:50 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Bjorn Andersson, Rob Clark, Sean Paul, Jonathan Marek,
Stephen Boyd, linux-arm-msm, dri-devel, David Airlie, freedreno
On 2021-04-25 09:08, 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.
the purpose of the mutex is that when there are two coredumps being
triggered
by two consecutive errors, we want to make sure that till one coredump
is completely
read and/or processed, we do not allow triggering of another one as we
want to capture
the accurate hardware/software state.
So moving disp_state out of kms might be okay and just allocated it when
actually capturing the state
but we will need the mutex and some sort of pending flag in my opinion.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/disp/msm_disp_snapshot.c | 87 ++++---------------
> 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 | 5 +-
> 4 files changed, 28 insertions(+), 82 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..371358893716 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,24 +28,21 @@ 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 *msm_kms = container_of(work, struct msm_kms,
> dump_work);
> + struct drm_device *drm_dev = msm_kms->dev;
> struct msm_disp_state *disp_state;
> + struct drm_printer p;
>
> - disp_state = data;
> -
> - msm_disp_state_free(disp_state);
> -
> - disp_state->coredump_pending = false;
> -}
> -#endif /* CONFIG_DEV_COREDUMP */
> + disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
> + if (!disp_state)
> + return;
>
> -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;
> + disp_state->dev = drm_dev->dev;
> + disp_state->drm_dev = drm_dev;
>
> - mutex_lock(&disp_state->mutex);
> + INIT_LIST_HEAD(&disp_state->blocks);
>
> msm_disp_snapshot_capture_state(disp_state);
>
> @@ -55,26 +51,15 @@ static void _msm_disp_snapshot_work(struct
> kthread_work *work)
> 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.
> - */
> -#ifdef CONFIG_DEV_COREDUMP
> + /* If COREDUMP is disabled, the stub will call the free function. */
This is a good cleanup, I just checked that the free() is called even if
the config is not enabled
> 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 +68,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 +85,11 @@ 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);
> -
> - 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 +98,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 +106,7 @@ 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);
>
> - mutex_destroy(&disp_state->mutex);
> + if (kms->dump_worker)
> + kthread_destroy_worker(kms->dump_worker);
> }
> 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..529b9dacf7ca 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -156,8 +156,9 @@ 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;
>
> /*
> * For async commit, where ->flush_commit() and later happens
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/msm: make msm_disp_state transient data struct
2021-04-26 20:50 ` abhinavk
@ 2021-04-26 21:23 ` Dmitry Baryshkov
2021-04-26 22:03 ` [Freedreno] " abhinavk
0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Baryshkov @ 2021-04-26 21:23 UTC (permalink / raw)
To: abhinavk
Cc: Bjorn Andersson, Rob Clark, Sean Paul, Jonathan Marek,
Stephen Boyd, linux-arm-msm, dri-devel, David Airlie, freedreno
On 26/04/2021 23:50, abhinavk@codeaurora.org wrote:
> On 2021-04-25 09:08, 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.
>
> the purpose of the mutex is that when there are two coredumps being
> triggered
> by two consecutive errors, we want to make sure that till one coredump
> is completely
> read and/or processed, we do not allow triggering of another one as we
> want to capture
> the accurate hardware/software state.
>
> So moving disp_state out of kms might be okay and just allocated it when
> actually capturing the state
> but we will need the mutex and some sort of pending flag in my opinion.
I'll return the mutex to the kms struct, so that we won't start another
snapshot untill previous one is complete.
Concerning the pending flag, I think it is also handled by the coredump
code: dev_coredumpm() will not create another device if there is one
already associated with the device being dumped. I should probably
mention this in the commit message.
If you agree with this, I'll send v2 then (also adding plls dumping).
>
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>> drivers/gpu/drm/msm/disp/msm_disp_snapshot.c | 87 ++++---------------
>> 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 | 5 +-
>> 4 files changed, 28 insertions(+), 82 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..371358893716 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,24 +28,21 @@ 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 *msm_kms = container_of(work, struct msm_kms,
>> dump_work);
>> + struct drm_device *drm_dev = msm_kms->dev;
>> struct msm_disp_state *disp_state;
>> + struct drm_printer p;
>>
>> - disp_state = data;
>> -
>> - msm_disp_state_free(disp_state);
>> -
>> - disp_state->coredump_pending = false;
>> -}
>> -#endif /* CONFIG_DEV_COREDUMP */
>> + disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
>> + if (!disp_state)
>> + return;
>>
>> -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;
>> + disp_state->dev = drm_dev->dev;
>> + disp_state->drm_dev = drm_dev;
>>
>> - mutex_lock(&disp_state->mutex);
>> + INIT_LIST_HEAD(&disp_state->blocks);
>>
>> msm_disp_snapshot_capture_state(disp_state);
>>
>> @@ -55,26 +51,15 @@ static void _msm_disp_snapshot_work(struct
>> kthread_work *work)
>> 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.
>> - */
>> -#ifdef CONFIG_DEV_COREDUMP
>> + /* If COREDUMP is disabled, the stub will call the free function. */
> This is a good cleanup, I just checked that the free() is called even if
> the config is not enabled
>> 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 +68,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 +85,11 @@ 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);
>> -
>> - 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 +98,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 +106,7 @@ 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);
>>
>> - mutex_destroy(&disp_state->mutex);
>> + if (kms->dump_worker)
>> + kthread_destroy_worker(kms->dump_worker);
>> }
>> 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..529b9dacf7ca 100644
>> --- a/drivers/gpu/drm/msm/msm_kms.h
>> +++ b/drivers/gpu/drm/msm/msm_kms.h
>> @@ -156,8 +156,9 @@ 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;
>>
>> /*
>> * For async commit, where ->flush_commit() and later happens
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Freedreno] [PATCH 2/2] drm/msm: make msm_disp_state transient data struct
2021-04-26 21:23 ` Dmitry Baryshkov
@ 2021-04-26 22:03 ` abhinavk
2021-04-26 22:14 ` Dmitry Baryshkov
0 siblings, 1 reply; 9+ messages in thread
From: abhinavk @ 2021-04-26 22:03 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, Jonathan Marek, Stephen Boyd, linux-arm-msm,
dri-devel, Bjorn Andersson, David Airlie, Rob Clark, Sean Paul
On 2021-04-26 14:23, Dmitry Baryshkov wrote:
> On 26/04/2021 23:50, abhinavk@codeaurora.org wrote:
>> On 2021-04-25 09:08, 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.
>>
>> the purpose of the mutex is that when there are two coredumps being
>> triggered
>> by two consecutive errors, we want to make sure that till one coredump
>> is completely
>> read and/or processed, we do not allow triggering of another one as we
>> want to capture
>> the accurate hardware/software state.
>>
>> So moving disp_state out of kms might be okay and just allocated it
>> when actually capturing the state
>> but we will need the mutex and some sort of pending flag in my
>> opinion.
>
> I'll return the mutex to the kms struct, so that we won't start
> another snapshot untill previous one is complete.
I think mutex should remain as part of snapshot so that they go
together. Since this mutex is not used
by any other module, I thought its better to keep it there.
>
> Concerning the pending flag, I think it is also handled by the
> coredump code: dev_coredumpm() will not create another device if there
> is one already associated with the device being dumped. I should
> probably mention this in the commit message.
Thats a good point, I checked that now as well but I still think we need
this flag because
it also makes sure devcoredump is taken and read together within our
driver itself instead of relying
on the devcoredump. Its not a strong preference.
But if you would like to go ahead with this, might have to retest its
robustness.
With the older logic how i verified this was that I relaxed the underrun
cnt check in this patch
( https://patchwork.freedesktop.org/patch/429112/?series=89181&rev=1 )
here and simulated an error:
@@ -1336,6 +1337,11 @@ static void dpu_encoder_underrun_callback(struct
drm_encoder *drm_enc,
DPU_ATRACE_BEGIN("encoder_underrun_callback");
atomic_inc(&phy_enc->underrun_cnt);
+
+ /* trigger dump only on the first underrun */
+ if (atomic_read(&phy_enc->underrun_cnt) == 1)
+ msm_disp_snapshot_state(drm_enc->dev);
+
And verified that across various underrun interrupts, the devcoredump is
stable.
>
> If you agree with this, I'll send v2 then (also adding plls dumping).
Looking fwd to seeing the pll dumping, that will be a great addition to
this.
>
>>
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>> drivers/gpu/drm/msm/disp/msm_disp_snapshot.c | 87
>>> ++++---------------
>>> 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 | 5 +-
>>> 4 files changed, 28 insertions(+), 82 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..371358893716 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,24 +28,21 @@ 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 *msm_kms = container_of(work, struct msm_kms,
>>> dump_work);
>>> + struct drm_device *drm_dev = msm_kms->dev;
>>> struct msm_disp_state *disp_state;
>>> + struct drm_printer p;
>>>
>>> - disp_state = data;
>>> -
>>> - msm_disp_state_free(disp_state);
>>> -
>>> - disp_state->coredump_pending = false;
>>> -}
>>> -#endif /* CONFIG_DEV_COREDUMP */
>>> + disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
>>> + if (!disp_state)
>>> + return;
>>>
>>> -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;
>>> + disp_state->dev = drm_dev->dev;
>>> + disp_state->drm_dev = drm_dev;
>>>
>>> - mutex_lock(&disp_state->mutex);
>>> + INIT_LIST_HEAD(&disp_state->blocks);
>>>
>>> msm_disp_snapshot_capture_state(disp_state);
>>>
>>> @@ -55,26 +51,15 @@ static void _msm_disp_snapshot_work(struct
>>> kthread_work *work)
>>> 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.
>>> - */
>>> -#ifdef CONFIG_DEV_COREDUMP
>>> + /* If COREDUMP is disabled, the stub will call the free
>>> function. */
>> This is a good cleanup, I just checked that the free() is called even
>> if the config is not enabled
>>> 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 +68,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 +85,11 @@ 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);
>>> -
>>> - 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 +98,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 +106,7 @@ 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);
>>>
>>> - mutex_destroy(&disp_state->mutex);
>>> + if (kms->dump_worker)
>>> + kthread_destroy_worker(kms->dump_worker);
>>> }
>>> 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..529b9dacf7ca 100644
>>> --- a/drivers/gpu/drm/msm/msm_kms.h
>>> +++ b/drivers/gpu/drm/msm/msm_kms.h
>>> @@ -156,8 +156,9 @@ 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;
>>>
>>> /*
>>> * For async commit, where ->flush_commit() and later happens
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Freedreno] [PATCH 2/2] drm/msm: make msm_disp_state transient data struct
2021-04-26 22:03 ` [Freedreno] " abhinavk
@ 2021-04-26 22:14 ` Dmitry Baryshkov
2021-04-26 22:56 ` abhinavk
0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Baryshkov @ 2021-04-26 22:14 UTC (permalink / raw)
To: Abhinav Kumar
Cc: freedreno, Jonathan Marek, Stephen Boyd,
open list:DRM DRIVER FOR MSM ADRENO GPU,
open list:DRM DRIVER FOR MSM ADRENO GPU, Bjorn Andersson,
David Airlie, Rob Clark, Sean Paul
On Tue, 27 Apr 2021 at 01:03, <abhinavk@codeaurora.org> wrote:
>
> On 2021-04-26 14:23, Dmitry Baryshkov wrote:
> > On 26/04/2021 23:50, abhinavk@codeaurora.org wrote:
> >> On 2021-04-25 09:08, 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.
> >>
> >> the purpose of the mutex is that when there are two coredumps being
> >> triggered
> >> by two consecutive errors, we want to make sure that till one coredump
> >> is completely
> >> read and/or processed, we do not allow triggering of another one as we
> >> want to capture
> >> the accurate hardware/software state.
> >>
> >> So moving disp_state out of kms might be okay and just allocated it
> >> when actually capturing the state
> >> but we will need the mutex and some sort of pending flag in my
> >> opinion.
> >
> > I'll return the mutex to the kms struct, so that we won't start
> > another snapshot untill previous one is complete.
>
> I think mutex should remain as part of snapshot so that they go
> together. Since this mutex is not used
> by any other module, I thought its better to keep it there.
I don't think so: we will serialize access to the snapshot, but not
dumping between snapshots. Note, that your code also serializes
writing to snapshot, not reading from it.
With disp_state being allocated on demand we do not have to protect
its contents (since it is not available before registration using
dev_codedumpm).
I thought that you wanted to be sure that one snapshot is fully
captured before next error triggers the next snapshot. And for this
we'd need 'global' mutex (in kms).
> > Concerning the pending flag, I think it is also handled by the
> > coredump code: dev_coredumpm() will not create another device if there
> > is one already associated with the device being dumped. I should
> > probably mention this in the commit message.
>
> Thats a good point, I checked that now as well but I still think we need
> this flag because
> it also makes sure devcoredump is taken and read together within our
> driver itself instead of relying
> on the devcoredump. Its not a strong preference.
>
> But if you would like to go ahead with this, might have to retest its
> robustness.
> With the older logic how i verified this was that I relaxed the underrun
> cnt check in this patch
> ( https://patchwork.freedesktop.org/patch/429112/?series=89181&rev=1 )
> here and simulated an error:
>
> @@ -1336,6 +1337,11 @@ static void dpu_encoder_underrun_callback(struct
> drm_encoder *drm_enc,
>
> DPU_ATRACE_BEGIN("encoder_underrun_callback");
> atomic_inc(&phy_enc->underrun_cnt);
> +
> + /* trigger dump only on the first underrun */
> + if (atomic_read(&phy_enc->underrun_cnt) == 1)
> + msm_disp_snapshot_state(drm_enc->dev);
> +
>
> And verified that across various underrun interrupts, the devcoredump is
> stable.
I'll try testing it this way, thank you for the pointer!
>
> >
> > If you agree with this, I'll send v2 then (also adding plls dumping).
> Looking fwd to seeing the pll dumping, that will be a great addition to
> this.
> >
> >>
> >>>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> ---
> >>> drivers/gpu/drm/msm/disp/msm_disp_snapshot.c | 87
> >>> ++++---------------
> >>> 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 | 5 +-
> >>> 4 files changed, 28 insertions(+), 82 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..371358893716 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,24 +28,21 @@ 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 *msm_kms = container_of(work, struct msm_kms,
> >>> dump_work);
> >>> + struct drm_device *drm_dev = msm_kms->dev;
> >>> struct msm_disp_state *disp_state;
> >>> + struct drm_printer p;
> >>>
> >>> - disp_state = data;
> >>> -
> >>> - msm_disp_state_free(disp_state);
> >>> -
> >>> - disp_state->coredump_pending = false;
> >>> -}
> >>> -#endif /* CONFIG_DEV_COREDUMP */
> >>> + disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
> >>> + if (!disp_state)
> >>> + return;
> >>>
> >>> -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;
> >>> + disp_state->dev = drm_dev->dev;
> >>> + disp_state->drm_dev = drm_dev;
> >>>
> >>> - mutex_lock(&disp_state->mutex);
> >>> + INIT_LIST_HEAD(&disp_state->blocks);
> >>>
> >>> msm_disp_snapshot_capture_state(disp_state);
> >>>
> >>> @@ -55,26 +51,15 @@ static void _msm_disp_snapshot_work(struct
> >>> kthread_work *work)
> >>> 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.
> >>> - */
> >>> -#ifdef CONFIG_DEV_COREDUMP
> >>> + /* If COREDUMP is disabled, the stub will call the free
> >>> function. */
> >> This is a good cleanup, I just checked that the free() is called even
> >> if the config is not enabled
> >>> 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 +68,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 +85,11 @@ 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);
> >>> -
> >>> - 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 +98,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 +106,7 @@ 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);
> >>>
> >>> - mutex_destroy(&disp_state->mutex);
> >>> + if (kms->dump_worker)
> >>> + kthread_destroy_worker(kms->dump_worker);
> >>> }
> >>> 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..529b9dacf7ca 100644
> >>> --- a/drivers/gpu/drm/msm/msm_kms.h
> >>> +++ b/drivers/gpu/drm/msm/msm_kms.h
> >>> @@ -156,8 +156,9 @@ 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;
> >>>
> >>> /*
> >>> * For async commit, where ->flush_commit() and later happens
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Freedreno] [PATCH 2/2] drm/msm: make msm_disp_state transient data struct
2021-04-26 22:14 ` Dmitry Baryshkov
@ 2021-04-26 22:56 ` abhinavk
0 siblings, 0 replies; 9+ messages in thread
From: abhinavk @ 2021-04-26 22:56 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Sean Paul, Jonathan Marek, Stephen Boyd,
open list:DRM DRIVER FOR MSM ADRENO GPU,
open list:DRM DRIVER FOR MSM ADRENO GPU, Bjorn Andersson,
David Airlie, freedreno
On 2021-04-26 15:14, Dmitry Baryshkov wrote:
> On Tue, 27 Apr 2021 at 01:03, <abhinavk@codeaurora.org> wrote:
>>
>> On 2021-04-26 14:23, Dmitry Baryshkov wrote:
>> > On 26/04/2021 23:50, abhinavk@codeaurora.org wrote:
>> >> On 2021-04-25 09:08, 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.
>> >>
>> >> the purpose of the mutex is that when there are two coredumps being
>> >> triggered
>> >> by two consecutive errors, we want to make sure that till one coredump
>> >> is completely
>> >> read and/or processed, we do not allow triggering of another one as we
>> >> want to capture
>> >> the accurate hardware/software state.
>> >>
>> >> So moving disp_state out of kms might be okay and just allocated it
>> >> when actually capturing the state
>> >> but we will need the mutex and some sort of pending flag in my
>> >> opinion.
>> >
>> > I'll return the mutex to the kms struct, so that we won't start
>> > another snapshot untill previous one is complete.
>>
>> I think mutex should remain as part of snapshot so that they go
>> together. Since this mutex is not used
>> by any other module, I thought its better to keep it there.
>
> I don't think so: we will serialize access to the snapshot, but not
> dumping between snapshots. Note, that your code also serializes
> writing to snapshot, not reading from it.
> With disp_state being allocated on demand we do not have to protect
> its contents (since it is not available before registration using
> dev_codedumpm).
> I thought that you wanted to be sure that one snapshot is fully
> captured before next error triggers the next snapshot. And for this
> we'd need 'global' mutex (in kms).
True, the intention of the mutex is to serialize access to snapshot.
the coredump_pending flag is the one which will serialize the write and
read
because its set to false only in the free() method of the coredump.
I was referring to the pending flag when I meant that it will serialize
read/write.
Regarding the mutex, well, if you word it that way that it protects one
snapshot
from being overwritten by the other and hence we need a 'global' mutex
in kms, it convinced
me :)
The only reason it was within the disp_state itself was because its not
used outside the module
but I am okay with moving it to kms if Rob is okay with that.
>
>> > Concerning the pending flag, I think it is also handled by the
>> > coredump code: dev_coredumpm() will not create another device if there
>> > is one already associated with the device being dumped. I should
>> > probably mention this in the commit message.
>>
>> Thats a good point, I checked that now as well but I still think we
>> need
>> this flag because
>> it also makes sure devcoredump is taken and read together within our
>> driver itself instead of relying
>> on the devcoredump. Its not a strong preference.
>>
>> But if you would like to go ahead with this, might have to retest its
>> robustness.
>> With the older logic how i verified this was that I relaxed the
>> underrun
>> cnt check in this patch
>> ( https://patchwork.freedesktop.org/patch/429112/?series=89181&rev=1 )
>> here and simulated an error:
>>
>> @@ -1336,6 +1337,11 @@ static void
>> dpu_encoder_underrun_callback(struct
>> drm_encoder *drm_enc,
>>
>> DPU_ATRACE_BEGIN("encoder_underrun_callback");
>> atomic_inc(&phy_enc->underrun_cnt);
>> +
>> + /* trigger dump only on the first underrun */
>> + if (atomic_read(&phy_enc->underrun_cnt) == 1)
>> + msm_disp_snapshot_state(drm_enc->dev);
>> +
>>
>> And verified that across various underrun interrupts, the devcoredump
>> is
>> stable.
>
> I'll try testing it this way, thank you for the pointer!
>
>>
>> >
>> > If you agree with this, I'll send v2 then (also adding plls dumping).
>> Looking fwd to seeing the pll dumping, that will be a great addition
>> to
>> this.
>> >
>> >>
>> >>>
>> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> >>> ---
>> >>> drivers/gpu/drm/msm/disp/msm_disp_snapshot.c | 87
>> >>> ++++---------------
>> >>> 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 | 5 +-
>> >>> 4 files changed, 28 insertions(+), 82 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..371358893716 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,24 +28,21 @@ 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 *msm_kms = container_of(work, struct msm_kms,
>> >>> dump_work);
>> >>> + struct drm_device *drm_dev = msm_kms->dev;
>> >>> struct msm_disp_state *disp_state;
>> >>> + struct drm_printer p;
>> >>>
>> >>> - disp_state = data;
>> >>> -
>> >>> - msm_disp_state_free(disp_state);
>> >>> -
>> >>> - disp_state->coredump_pending = false;
>> >>> -}
>> >>> -#endif /* CONFIG_DEV_COREDUMP */
>> >>> + disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
>> >>> + if (!disp_state)
>> >>> + return;
>> >>>
>> >>> -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;
>> >>> + disp_state->dev = drm_dev->dev;
>> >>> + disp_state->drm_dev = drm_dev;
>> >>>
>> >>> - mutex_lock(&disp_state->mutex);
>> >>> + INIT_LIST_HEAD(&disp_state->blocks);
>> >>>
>> >>> msm_disp_snapshot_capture_state(disp_state);
>> >>>
>> >>> @@ -55,26 +51,15 @@ static void _msm_disp_snapshot_work(struct
>> >>> kthread_work *work)
>> >>> 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.
>> >>> - */
>> >>> -#ifdef CONFIG_DEV_COREDUMP
>> >>> + /* If COREDUMP is disabled, the stub will call the free
>> >>> function. */
>> >> This is a good cleanup, I just checked that the free() is called even
>> >> if the config is not enabled
>> >>> 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 +68,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 +85,11 @@ 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);
>> >>> -
>> >>> - 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 +98,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 +106,7 @@ 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);
>> >>>
>> >>> - mutex_destroy(&disp_state->mutex);
>> >>> + if (kms->dump_worker)
>> >>> + kthread_destroy_worker(kms->dump_worker);
>> >>> }
>> >>> 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..529b9dacf7ca 100644
>> >>> --- a/drivers/gpu/drm/msm/msm_kms.h
>> >>> +++ b/drivers/gpu/drm/msm/msm_kms.h
>> >>> @@ -156,8 +156,9 @@ 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;
>> >>>
>> >>> /*
>> >>> * For async commit, where ->flush_commit() and later happens
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-04-26 22:56 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-25 16:07 [PATCH 0/2] drm/msm: rework display snapshotting Dmitry Baryshkov
2021-04-25 16:07 ` [PATCH 1/2] drm/msm: pass dump state as a function argument Dmitry Baryshkov
2021-04-26 20:13 ` abhinavk
2021-04-25 16:08 ` [PATCH 2/2] drm/msm: make msm_disp_state transient data struct Dmitry Baryshkov
2021-04-26 20:50 ` abhinavk
2021-04-26 21:23 ` Dmitry Baryshkov
2021-04-26 22:03 ` [Freedreno] " abhinavk
2021-04-26 22:14 ` Dmitry Baryshkov
2021-04-26 22:56 ` abhinavk
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).