* [PATCH v3 0/2] drm/msm: Fix dsi/bridge probe
@ 2021-12-01 10:52 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-12-01 10:52 UTC (permalink / raw)
To: robdclark
Cc: freedreno, airlied, linux-arm-msm, konrad.dybcio, linux-kernel,
dri-devel, jami.kettunen, martin.botka, maxime, dmitry.baryshkov,
marijn.suijten, kernel, sean, AngeloGioacchino Del Regno
Context, from patch 2/2:
Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
DSI host gets initialized earlier, but this caused unability to probe
the entire stack of components because they all depend on interrupts
coming from the main `mdss` node (mdp5, or dpu1).
Changes in v3:
- Removed a forgotten (and wrong) kfree() call.
Series v2:
After a very nice conversation with Dmitry, it turned out that my first
approach to solve this issue wasn't great: even though it appeared to
actually work, it was introducing a number of issues, one of which was
critical as it was not removing down the DRM device when it's supposed to.
Instead of actually fixing that patch, I went for "simplifying" the
approach by not initializing the entire MDSS, but just the interrupt
controller, which still untangles the infinite probe deferrals, but
actually doesn't even touch most of the already present logic in place.
AngeloGioacchino Del Regno (2):
drm/msm: Allocate msm_drm_private early and pass it as driver data
drm/msm: Initialize MDSS irq domain at probe time
drivers/gpu/drm/msm/adreno/adreno_device.c | 16 ++---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 50 ++++++++++------
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 3 +-
drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c | 58 ++++++++++++------
drivers/gpu/drm/msm/dp/dp_display.c | 10 +---
drivers/gpu/drm/msm/dsi/dsi.c | 6 +-
drivers/gpu/drm/msm/edp/edp.c | 6 +-
drivers/gpu/drm/msm/hdmi/hdmi.c | 7 +--
drivers/gpu/drm/msm/msm_drv.c | 68 +++++++++++++---------
drivers/gpu/drm/msm/msm_kms.h | 3 +
11 files changed, 133 insertions(+), 98 deletions(-)
--
2.33.1
^ permalink raw reply [flat|nested] 36+ messages in thread* [PATCH v3 1/2] drm/msm: Allocate msm_drm_private early and pass it as driver data
2021-12-01 10:52 ` AngeloGioacchino Del Regno
@ 2021-12-01 10:52 ` AngeloGioacchino Del Regno
-1 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-12-01 10:52 UTC (permalink / raw)
To: robdclark
Cc: dmitry.baryshkov, sean, airlied, daniel, maxime, linux-arm-msm,
dri-devel, freedreno, linux-kernel, kernel, konrad.dybcio,
marijn.suijten, jami.kettunen, martin.botka,
AngeloGioacchino Del Regno
In preparation for registering the mdss interrupt controller earlier,
move the allocation of msm_drm_private from component bind time to
msm_drv probe; this also allows us to use the devm variant of kzalloc.
Since it is not right to allocate the drm_device at probe time (as
it should exist only when all components are bound, and taken down
when components get cleaned up), the only way to make this happen is
to pass a pointer to msm_drm_private as driver data (like done in
many other DRM drivers), instead of one to drm_device like it's
currently done in this driver.
This is also simplifying some bind/unbind functions around drm/msm,
as some of them are using drm_device just to grab a pointer to the
msm_drm_private structure, which we now retrieve in one call.
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
drivers/gpu/drm/msm/adreno/adreno_device.c | 16 +++-----
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +-
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 3 +-
drivers/gpu/drm/msm/dp/dp_display.c | 10 ++---
drivers/gpu/drm/msm/dsi/dsi.c | 6 +--
drivers/gpu/drm/msm/edp/edp.c | 6 +--
drivers/gpu/drm/msm/hdmi/hdmi.c | 7 ++--
drivers/gpu/drm/msm/msm_drv.c | 46 +++++++++-------------
8 files changed, 38 insertions(+), 60 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 2a6ce76656aa..6b113881aefb 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -427,13 +427,6 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev)
return gpu;
}
-static void set_gpu_pdev(struct drm_device *dev,
- struct platform_device *pdev)
-{
- struct msm_drm_private *priv = dev->dev_private;
- priv->gpu_pdev = pdev;
-}
-
static int find_chipid(struct device *dev, struct adreno_rev *rev)
{
struct device_node *node = dev->of_node;
@@ -482,8 +475,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
{
static struct adreno_platform_config config = {};
const struct adreno_info *info;
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(master);
+ struct drm_device *drm = priv->dev;
struct msm_gpu *gpu;
int ret;
@@ -492,7 +485,7 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
return ret;
dev->platform_data = &config;
- set_gpu_pdev(drm, to_platform_device(dev));
+ priv->gpu_pdev = to_platform_device(dev);
info = adreno_info(config.rev);
@@ -521,12 +514,13 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
static void adreno_unbind(struct device *dev, struct device *master,
void *data)
{
+ struct msm_drm_private *priv = dev_get_drvdata(master);
struct msm_gpu *gpu = dev_to_gpu(dev);
pm_runtime_force_suspend(dev);
gpu->funcs->destroy(gpu);
- set_gpu_pdev(dev_get_drvdata(master), NULL);
+ priv->gpu_pdev = NULL;
}
static const struct component_ops a3xx_ops = {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index a15b26428280..8b038690d9ce 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1153,9 +1153,9 @@ struct msm_kms *dpu_kms_init(struct drm_device *dev)
static int dpu_bind(struct device *dev, struct device *master, void *data)
{
- struct drm_device *ddev = dev_get_drvdata(master);
+ struct msm_drm_private *priv = dev_get_drvdata(master);
struct platform_device *pdev = to_platform_device(dev);
- struct msm_drm_private *priv = ddev->dev_private;
+ struct drm_device *ddev = priv->dev;
struct dpu_kms *dpu_kms;
struct dss_module_power *mp;
int ret = 0;
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 7b242246d4e7..cab6451661b2 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -936,7 +936,8 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev)
static int mdp5_bind(struct device *dev, struct device *master, void *data)
{
- struct drm_device *ddev = dev_get_drvdata(master);
+ struct msm_drm_private *priv = dev_get_drvdata(master);
+ struct drm_device *ddev = priv->dev;
struct platform_device *pdev = to_platform_device(dev);
DBG("");
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 0eb3c007d503..ac29a8a99450 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -224,13 +224,10 @@ static int dp_display_bind(struct device *dev, struct device *master,
{
int rc = 0;
struct dp_display_private *dp = dev_get_dp_display_private(dev);
- struct msm_drm_private *priv;
- struct drm_device *drm;
-
- drm = dev_get_drvdata(master);
+ struct msm_drm_private *priv = dev_get_drvdata(master);
+ struct drm_device *drm = priv->dev;
dp->dp_display.drm_dev = drm;
- priv = drm->dev_private;
priv->dp[dp->id] = &dp->dp_display;
rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
@@ -266,8 +263,7 @@ static void dp_display_unbind(struct device *dev, struct device *master,
void *data)
{
struct dp_display_private *dp = dev_get_dp_display_private(dev);
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(master);
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 5cd230a5d5d3..9670e548b3e9 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -110,8 +110,7 @@ static struct msm_dsi *dsi_init(struct platform_device *pdev)
static int dsi_bind(struct device *dev, struct device *master, void *data)
{
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(master);
struct msm_dsi *msm_dsi = dev_get_drvdata(dev);
priv->dsi[msm_dsi->id] = msm_dsi;
@@ -122,8 +121,7 @@ static int dsi_bind(struct device *dev, struct device *master, void *data)
static void dsi_unbind(struct device *dev, struct device *master,
void *data)
{
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(master);
struct msm_dsi *msm_dsi = dev_get_drvdata(dev);
priv->dsi[msm_dsi->id] = NULL;
diff --git a/drivers/gpu/drm/msm/edp/edp.c b/drivers/gpu/drm/msm/edp/edp.c
index 106a67473af5..101cf1671472 100644
--- a/drivers/gpu/drm/msm/edp/edp.c
+++ b/drivers/gpu/drm/msm/edp/edp.c
@@ -66,8 +66,7 @@ static struct msm_edp *edp_init(struct platform_device *pdev)
static int edp_bind(struct device *dev, struct device *master, void *data)
{
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(master);
struct msm_edp *edp;
DBG("");
@@ -81,8 +80,7 @@ static int edp_bind(struct device *dev, struct device *master, void *data)
static void edp_unbind(struct device *dev, struct device *master, void *data)
{
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(master);
DBG("");
if (priv->edp) {
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 75b64e6ae035..64ad73a01edd 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -514,8 +514,7 @@ static int msm_hdmi_register_audio_driver(struct hdmi *hdmi, struct device *dev)
static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
{
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(master);
struct hdmi_platform_config *hdmi_cfg;
struct hdmi *hdmi;
struct device_node *of_node = dev->of_node;
@@ -586,8 +585,8 @@ static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
static void msm_hdmi_unbind(struct device *dev, struct device *master,
void *data)
{
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(master);
+
if (priv->hdmi) {
if (priv->hdmi->audio_pdev)
platform_device_unregister(priv->hdmi->audio_pdev);
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 892c04365239..64230e473a34 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -339,8 +339,8 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
static int msm_drm_uninit(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
- struct drm_device *ddev = platform_get_drvdata(pdev);
- struct msm_drm_private *priv = ddev->dev_private;
+ struct msm_drm_private *priv = platform_get_drvdata(pdev);
+ struct drm_device *ddev = priv->dev;
struct msm_kms *kms = priv->kms;
struct msm_mdss *mdss = priv->mdss;
int i;
@@ -409,7 +409,6 @@ static int msm_drm_uninit(struct device *dev)
drm_dev_put(ddev);
destroy_workqueue(priv->wq);
- kfree(priv);
return 0;
}
@@ -512,8 +511,8 @@ static int msm_init_vram(struct drm_device *dev)
static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
{
struct platform_device *pdev = to_platform_device(dev);
+ struct msm_drm_private *priv = dev_get_drvdata(dev);
struct drm_device *ddev;
- struct msm_drm_private *priv;
struct msm_kms *kms;
struct msm_mdss *mdss;
int ret, i;
@@ -523,15 +522,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
DRM_DEV_ERROR(dev, "failed to allocate drm_device\n");
return PTR_ERR(ddev);
}
-
- platform_set_drvdata(pdev, ddev);
-
- priv = kzalloc(sizeof(*priv), GFP_KERNEL);
- if (!priv) {
- ret = -ENOMEM;
- goto err_put_drm_dev;
- }
-
ddev->dev_private = priv;
priv->dev = ddev;
@@ -547,7 +537,7 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
break;
}
if (ret)
- goto err_free_priv;
+ goto err_put_drm_dev;
mdss = priv->mdss;
@@ -685,11 +675,8 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
err_destroy_mdss:
if (mdss && mdss->funcs)
mdss->funcs->destroy(ddev);
-err_free_priv:
- kfree(priv);
err_put_drm_dev:
drm_dev_put(ddev);
- platform_set_drvdata(pdev, NULL);
return ret;
}
@@ -1142,8 +1129,7 @@ static const struct drm_driver msm_driver = {
static int __maybe_unused msm_runtime_suspend(struct device *dev)
{
- struct drm_device *ddev = dev_get_drvdata(dev);
- struct msm_drm_private *priv = ddev->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(dev);
struct msm_mdss *mdss = priv->mdss;
DBG("");
@@ -1156,8 +1142,7 @@ static int __maybe_unused msm_runtime_suspend(struct device *dev)
static int __maybe_unused msm_runtime_resume(struct device *dev)
{
- struct drm_device *ddev = dev_get_drvdata(dev);
- struct msm_drm_private *priv = ddev->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(dev);
struct msm_mdss *mdss = priv->mdss;
DBG("");
@@ -1187,8 +1172,8 @@ static int __maybe_unused msm_pm_resume(struct device *dev)
static int __maybe_unused msm_pm_prepare(struct device *dev)
{
- struct drm_device *ddev = dev_get_drvdata(dev);
- struct msm_drm_private *priv = ddev ? ddev->dev_private : NULL;
+ struct msm_drm_private *priv = dev_get_drvdata(dev);
+ struct drm_device *ddev = priv ? priv->dev : NULL;
if (!priv || !priv->kms)
return 0;
@@ -1198,8 +1183,8 @@ static int __maybe_unused msm_pm_prepare(struct device *dev)
static void __maybe_unused msm_pm_complete(struct device *dev)
{
- struct drm_device *ddev = dev_get_drvdata(dev);
- struct msm_drm_private *priv = ddev ? ddev->dev_private : NULL;
+ struct msm_drm_private *priv = dev_get_drvdata(dev);
+ struct drm_device *ddev = priv ? priv->dev : NULL;
if (!priv || !priv->kms)
return;
@@ -1397,8 +1382,15 @@ static const struct component_master_ops msm_drm_ops = {
static int msm_pdev_probe(struct platform_device *pdev)
{
struct component_match *match = NULL;
+ struct msm_drm_private *priv;
int ret;
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, priv);
+
if (get_mdp_ver(pdev)) {
ret = add_display_components(pdev, &match);
if (ret)
@@ -1437,8 +1429,8 @@ static int msm_pdev_remove(struct platform_device *pdev)
static void msm_pdev_shutdown(struct platform_device *pdev)
{
- struct drm_device *drm = platform_get_drvdata(pdev);
- struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
+ struct msm_drm_private *priv = platform_get_drvdata(pdev);
+ struct drm_device *drm = priv ? priv->dev : NULL;
if (!priv || !priv->kms)
return;
--
2.33.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 1/2] drm/msm: Allocate msm_drm_private early and pass it as driver data
@ 2021-12-01 10:52 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-12-01 10:52 UTC (permalink / raw)
To: robdclark
Cc: freedreno, airlied, linux-arm-msm, konrad.dybcio, linux-kernel,
dri-devel, jami.kettunen, martin.botka, maxime, dmitry.baryshkov,
marijn.suijten, kernel, sean, AngeloGioacchino Del Regno
In preparation for registering the mdss interrupt controller earlier,
move the allocation of msm_drm_private from component bind time to
msm_drv probe; this also allows us to use the devm variant of kzalloc.
Since it is not right to allocate the drm_device at probe time (as
it should exist only when all components are bound, and taken down
when components get cleaned up), the only way to make this happen is
to pass a pointer to msm_drm_private as driver data (like done in
many other DRM drivers), instead of one to drm_device like it's
currently done in this driver.
This is also simplifying some bind/unbind functions around drm/msm,
as some of them are using drm_device just to grab a pointer to the
msm_drm_private structure, which we now retrieve in one call.
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
drivers/gpu/drm/msm/adreno/adreno_device.c | 16 +++-----
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +-
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 3 +-
drivers/gpu/drm/msm/dp/dp_display.c | 10 ++---
drivers/gpu/drm/msm/dsi/dsi.c | 6 +--
drivers/gpu/drm/msm/edp/edp.c | 6 +--
drivers/gpu/drm/msm/hdmi/hdmi.c | 7 ++--
drivers/gpu/drm/msm/msm_drv.c | 46 +++++++++-------------
8 files changed, 38 insertions(+), 60 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 2a6ce76656aa..6b113881aefb 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -427,13 +427,6 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev)
return gpu;
}
-static void set_gpu_pdev(struct drm_device *dev,
- struct platform_device *pdev)
-{
- struct msm_drm_private *priv = dev->dev_private;
- priv->gpu_pdev = pdev;
-}
-
static int find_chipid(struct device *dev, struct adreno_rev *rev)
{
struct device_node *node = dev->of_node;
@@ -482,8 +475,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
{
static struct adreno_platform_config config = {};
const struct adreno_info *info;
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(master);
+ struct drm_device *drm = priv->dev;
struct msm_gpu *gpu;
int ret;
@@ -492,7 +485,7 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
return ret;
dev->platform_data = &config;
- set_gpu_pdev(drm, to_platform_device(dev));
+ priv->gpu_pdev = to_platform_device(dev);
info = adreno_info(config.rev);
@@ -521,12 +514,13 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
static void adreno_unbind(struct device *dev, struct device *master,
void *data)
{
+ struct msm_drm_private *priv = dev_get_drvdata(master);
struct msm_gpu *gpu = dev_to_gpu(dev);
pm_runtime_force_suspend(dev);
gpu->funcs->destroy(gpu);
- set_gpu_pdev(dev_get_drvdata(master), NULL);
+ priv->gpu_pdev = NULL;
}
static const struct component_ops a3xx_ops = {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index a15b26428280..8b038690d9ce 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1153,9 +1153,9 @@ struct msm_kms *dpu_kms_init(struct drm_device *dev)
static int dpu_bind(struct device *dev, struct device *master, void *data)
{
- struct drm_device *ddev = dev_get_drvdata(master);
+ struct msm_drm_private *priv = dev_get_drvdata(master);
struct platform_device *pdev = to_platform_device(dev);
- struct msm_drm_private *priv = ddev->dev_private;
+ struct drm_device *ddev = priv->dev;
struct dpu_kms *dpu_kms;
struct dss_module_power *mp;
int ret = 0;
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 7b242246d4e7..cab6451661b2 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -936,7 +936,8 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev)
static int mdp5_bind(struct device *dev, struct device *master, void *data)
{
- struct drm_device *ddev = dev_get_drvdata(master);
+ struct msm_drm_private *priv = dev_get_drvdata(master);
+ struct drm_device *ddev = priv->dev;
struct platform_device *pdev = to_platform_device(dev);
DBG("");
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 0eb3c007d503..ac29a8a99450 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -224,13 +224,10 @@ static int dp_display_bind(struct device *dev, struct device *master,
{
int rc = 0;
struct dp_display_private *dp = dev_get_dp_display_private(dev);
- struct msm_drm_private *priv;
- struct drm_device *drm;
-
- drm = dev_get_drvdata(master);
+ struct msm_drm_private *priv = dev_get_drvdata(master);
+ struct drm_device *drm = priv->dev;
dp->dp_display.drm_dev = drm;
- priv = drm->dev_private;
priv->dp[dp->id] = &dp->dp_display;
rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
@@ -266,8 +263,7 @@ static void dp_display_unbind(struct device *dev, struct device *master,
void *data)
{
struct dp_display_private *dp = dev_get_dp_display_private(dev);
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(master);
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 5cd230a5d5d3..9670e548b3e9 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -110,8 +110,7 @@ static struct msm_dsi *dsi_init(struct platform_device *pdev)
static int dsi_bind(struct device *dev, struct device *master, void *data)
{
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(master);
struct msm_dsi *msm_dsi = dev_get_drvdata(dev);
priv->dsi[msm_dsi->id] = msm_dsi;
@@ -122,8 +121,7 @@ static int dsi_bind(struct device *dev, struct device *master, void *data)
static void dsi_unbind(struct device *dev, struct device *master,
void *data)
{
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(master);
struct msm_dsi *msm_dsi = dev_get_drvdata(dev);
priv->dsi[msm_dsi->id] = NULL;
diff --git a/drivers/gpu/drm/msm/edp/edp.c b/drivers/gpu/drm/msm/edp/edp.c
index 106a67473af5..101cf1671472 100644
--- a/drivers/gpu/drm/msm/edp/edp.c
+++ b/drivers/gpu/drm/msm/edp/edp.c
@@ -66,8 +66,7 @@ static struct msm_edp *edp_init(struct platform_device *pdev)
static int edp_bind(struct device *dev, struct device *master, void *data)
{
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(master);
struct msm_edp *edp;
DBG("");
@@ -81,8 +80,7 @@ static int edp_bind(struct device *dev, struct device *master, void *data)
static void edp_unbind(struct device *dev, struct device *master, void *data)
{
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(master);
DBG("");
if (priv->edp) {
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 75b64e6ae035..64ad73a01edd 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -514,8 +514,7 @@ static int msm_hdmi_register_audio_driver(struct hdmi *hdmi, struct device *dev)
static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
{
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(master);
struct hdmi_platform_config *hdmi_cfg;
struct hdmi *hdmi;
struct device_node *of_node = dev->of_node;
@@ -586,8 +585,8 @@ static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
static void msm_hdmi_unbind(struct device *dev, struct device *master,
void *data)
{
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(master);
+
if (priv->hdmi) {
if (priv->hdmi->audio_pdev)
platform_device_unregister(priv->hdmi->audio_pdev);
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 892c04365239..64230e473a34 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -339,8 +339,8 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
static int msm_drm_uninit(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
- struct drm_device *ddev = platform_get_drvdata(pdev);
- struct msm_drm_private *priv = ddev->dev_private;
+ struct msm_drm_private *priv = platform_get_drvdata(pdev);
+ struct drm_device *ddev = priv->dev;
struct msm_kms *kms = priv->kms;
struct msm_mdss *mdss = priv->mdss;
int i;
@@ -409,7 +409,6 @@ static int msm_drm_uninit(struct device *dev)
drm_dev_put(ddev);
destroy_workqueue(priv->wq);
- kfree(priv);
return 0;
}
@@ -512,8 +511,8 @@ static int msm_init_vram(struct drm_device *dev)
static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
{
struct platform_device *pdev = to_platform_device(dev);
+ struct msm_drm_private *priv = dev_get_drvdata(dev);
struct drm_device *ddev;
- struct msm_drm_private *priv;
struct msm_kms *kms;
struct msm_mdss *mdss;
int ret, i;
@@ -523,15 +522,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
DRM_DEV_ERROR(dev, "failed to allocate drm_device\n");
return PTR_ERR(ddev);
}
-
- platform_set_drvdata(pdev, ddev);
-
- priv = kzalloc(sizeof(*priv), GFP_KERNEL);
- if (!priv) {
- ret = -ENOMEM;
- goto err_put_drm_dev;
- }
-
ddev->dev_private = priv;
priv->dev = ddev;
@@ -547,7 +537,7 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
break;
}
if (ret)
- goto err_free_priv;
+ goto err_put_drm_dev;
mdss = priv->mdss;
@@ -685,11 +675,8 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
err_destroy_mdss:
if (mdss && mdss->funcs)
mdss->funcs->destroy(ddev);
-err_free_priv:
- kfree(priv);
err_put_drm_dev:
drm_dev_put(ddev);
- platform_set_drvdata(pdev, NULL);
return ret;
}
@@ -1142,8 +1129,7 @@ static const struct drm_driver msm_driver = {
static int __maybe_unused msm_runtime_suspend(struct device *dev)
{
- struct drm_device *ddev = dev_get_drvdata(dev);
- struct msm_drm_private *priv = ddev->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(dev);
struct msm_mdss *mdss = priv->mdss;
DBG("");
@@ -1156,8 +1142,7 @@ static int __maybe_unused msm_runtime_suspend(struct device *dev)
static int __maybe_unused msm_runtime_resume(struct device *dev)
{
- struct drm_device *ddev = dev_get_drvdata(dev);
- struct msm_drm_private *priv = ddev->dev_private;
+ struct msm_drm_private *priv = dev_get_drvdata(dev);
struct msm_mdss *mdss = priv->mdss;
DBG("");
@@ -1187,8 +1172,8 @@ static int __maybe_unused msm_pm_resume(struct device *dev)
static int __maybe_unused msm_pm_prepare(struct device *dev)
{
- struct drm_device *ddev = dev_get_drvdata(dev);
- struct msm_drm_private *priv = ddev ? ddev->dev_private : NULL;
+ struct msm_drm_private *priv = dev_get_drvdata(dev);
+ struct drm_device *ddev = priv ? priv->dev : NULL;
if (!priv || !priv->kms)
return 0;
@@ -1198,8 +1183,8 @@ static int __maybe_unused msm_pm_prepare(struct device *dev)
static void __maybe_unused msm_pm_complete(struct device *dev)
{
- struct drm_device *ddev = dev_get_drvdata(dev);
- struct msm_drm_private *priv = ddev ? ddev->dev_private : NULL;
+ struct msm_drm_private *priv = dev_get_drvdata(dev);
+ struct drm_device *ddev = priv ? priv->dev : NULL;
if (!priv || !priv->kms)
return;
@@ -1397,8 +1382,15 @@ static const struct component_master_ops msm_drm_ops = {
static int msm_pdev_probe(struct platform_device *pdev)
{
struct component_match *match = NULL;
+ struct msm_drm_private *priv;
int ret;
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, priv);
+
if (get_mdp_ver(pdev)) {
ret = add_display_components(pdev, &match);
if (ret)
@@ -1437,8 +1429,8 @@ static int msm_pdev_remove(struct platform_device *pdev)
static void msm_pdev_shutdown(struct platform_device *pdev)
{
- struct drm_device *drm = platform_get_drvdata(pdev);
- struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
+ struct msm_drm_private *priv = platform_get_drvdata(pdev);
+ struct drm_device *drm = priv ? priv->dev : NULL;
if (!priv || !priv->kms)
return;
--
2.33.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v3 2/2] drm/msm: Initialize MDSS irq domain at probe time
2021-12-01 10:52 ` AngeloGioacchino Del Regno
@ 2021-12-01 10:52 ` AngeloGioacchino Del Regno
-1 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-12-01 10:52 UTC (permalink / raw)
To: robdclark
Cc: dmitry.baryshkov, sean, airlied, daniel, maxime, linux-arm-msm,
dri-devel, freedreno, linux-kernel, kernel, konrad.dybcio,
marijn.suijten, jami.kettunen, martin.botka,
AngeloGioacchino Del Regno
Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
DSI host gets initialized earlier, but this caused unability to probe
the entire stack of components because they all depend on interrupts
coming from the main `mdss` node (mdp5, or dpu1).
To fix this issue, anticipate registering the irq domain from mdp5/dpu1
at msm_mdev_probe() time, as to make sure that the interrupt controller
is available before dsi and/or other components try to initialize,
finally satisfying the dependency.
Moreover, to balance this operation while avoiding to always check if
the irq domain is registered everytime we call bind() on msm_pdev, add
a new *remove function pointer to msm_mdss_funcs, used to remove the
irq domain only at msm_pdev_remove() time.
Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 50 ++++++++++++-------
drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c | 58 +++++++++++++++--------
drivers/gpu/drm/msm/msm_drv.c | 22 ++++++++-
drivers/gpu/drm/msm/msm_kms.h | 3 ++
4 files changed, 95 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
index b466784d9822..6c2569175633 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
@@ -106,13 +106,10 @@ static const struct irq_domain_ops dpu_mdss_irqdomain_ops = {
.xlate = irq_domain_xlate_onecell,
};
-static int _dpu_mdss_irq_domain_add(struct dpu_mdss *dpu_mdss)
+static int _dpu_mdss_irq_domain_add(struct device *dev, struct dpu_mdss *dpu_mdss)
{
- struct device *dev;
struct irq_domain *domain;
- dev = dpu_mdss->base.dev->dev;
-
domain = irq_domain_add_linear(dev->of_node, 32,
&dpu_mdss_irqdomain_ops, dpu_mdss);
if (!domain) {
@@ -194,7 +191,6 @@ static void dpu_mdss_destroy(struct drm_device *dev)
pm_runtime_suspend(dev->dev);
pm_runtime_disable(dev->dev);
- _dpu_mdss_irq_domain_fini(dpu_mdss);
irq = platform_get_irq(pdev, 0);
irq_set_chained_handler_and_data(irq, NULL, NULL);
msm_dss_put_clk(mp->clk_config, mp->num_clk);
@@ -203,15 +199,43 @@ static void dpu_mdss_destroy(struct drm_device *dev)
if (dpu_mdss->mmio)
devm_iounmap(&pdev->dev, dpu_mdss->mmio);
dpu_mdss->mmio = NULL;
- priv->mdss = NULL;
+}
+
+static void dpu_mdss_remove(struct msm_mdss *mdss)
+{
+ _dpu_mdss_irq_domain_fini(to_dpu_mdss(mdss));
}
static const struct msm_mdss_funcs mdss_funcs = {
.enable = dpu_mdss_enable,
.disable = dpu_mdss_disable,
.destroy = dpu_mdss_destroy,
+ .remove = dpu_mdss_remove,
};
+int dpu_mdss_early_init(struct device *dev, struct msm_drm_private *priv)
+{
+ struct dpu_mdss *dpu_mdss;
+ int ret;
+
+ dpu_mdss = devm_kzalloc(dev, sizeof(*dpu_mdss), GFP_KERNEL);
+ if (!dpu_mdss)
+ return -ENOMEM;
+
+ ret = _dpu_mdss_irq_domain_add(dev, dpu_mdss);
+ if (ret)
+ return ret;
+
+ /*
+ * Here we have no drm_device yet, but still do the assignment
+ * so that we can retrieve our struct dpu_mdss from the main
+ * init function, since we allocate it here.
+ */
+ priv->mdss = &dpu_mdss->base;
+
+ return 0;
+}
+
int dpu_mdss_init(struct drm_device *dev)
{
struct platform_device *pdev = to_platform_device(dev->dev);
@@ -221,9 +245,9 @@ int dpu_mdss_init(struct drm_device *dev)
int ret;
int irq;
- dpu_mdss = devm_kzalloc(dev->dev, sizeof(*dpu_mdss), GFP_KERNEL);
+ dpu_mdss = to_dpu_mdss(priv->mdss);
if (!dpu_mdss)
- return -ENOMEM;
+ return -ENODATA;
dpu_mdss->mmio = msm_ioremap(pdev, "mdss", "mdss");
if (IS_ERR(dpu_mdss->mmio))
@@ -241,10 +265,6 @@ int dpu_mdss_init(struct drm_device *dev)
dpu_mdss->base.dev = dev;
dpu_mdss->base.funcs = &mdss_funcs;
- ret = _dpu_mdss_irq_domain_add(dpu_mdss);
- if (ret)
- goto irq_domain_error;
-
irq = platform_get_irq(pdev, 0);
if (irq < 0) {
ret = irq;
@@ -253,16 +273,10 @@ int dpu_mdss_init(struct drm_device *dev)
irq_set_chained_handler_and_data(irq, dpu_mdss_irq,
dpu_mdss);
-
- priv->mdss = &dpu_mdss->base;
-
pm_runtime_enable(dev->dev);
-
return 0;
irq_error:
- _dpu_mdss_irq_domain_fini(dpu_mdss);
-irq_domain_error:
msm_dss_put_clk(mp->clk_config, mp->num_clk);
clk_parse_err:
devm_kfree(&pdev->dev, mp->clk_config);
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
index 0ea53420bc40..a99538ae4182 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
@@ -112,9 +112,8 @@ static const struct irq_domain_ops mdss_hw_irqdomain_ops = {
};
-static int mdss_irq_domain_init(struct mdp5_mdss *mdp5_mdss)
+static int mdss_irq_domain_init(struct device *dev, struct mdp5_mdss *mdp5_mdss)
{
- struct device *dev = mdp5_mdss->base.dev->dev;
struct irq_domain *d;
d = irq_domain_add_linear(dev->of_node, 32, &mdss_hw_irqdomain_ops,
@@ -182,20 +181,52 @@ static void mdp5_mdss_destroy(struct drm_device *dev)
if (!mdp5_mdss)
return;
- irq_domain_remove(mdp5_mdss->irqcontroller.domain);
- mdp5_mdss->irqcontroller.domain = NULL;
-
regulator_disable(mdp5_mdss->vdd);
pm_runtime_disable(dev->dev);
}
+static void mdp5_mdss_remove(struct msm_mdss *mdss)
+{
+ struct mdp5_mdss *mdp5_mdss = to_mdp5_mdss(mdss);
+
+ irq_domain_remove(mdp5_mdss->irqcontroller.domain);
+ mdp5_mdss->irqcontroller.domain = NULL;
+}
+
static const struct msm_mdss_funcs mdss_funcs = {
.enable = mdp5_mdss_enable,
.disable = mdp5_mdss_disable,
.destroy = mdp5_mdss_destroy,
+ .remove = mdp5_mdss_remove,
};
+int mdp5_mdss_early_init(struct device *dev, struct msm_drm_private *priv)
+{
+ struct mdp5_mdss *mdp5_mdss;
+ int ret;
+
+ if (!of_device_is_compatible(dev->of_node, "qcom,mdss"))
+ return 0;
+
+ mdp5_mdss = devm_kzalloc(dev, sizeof(*mdp5_mdss), GFP_KERNEL);
+ if (!mdp5_mdss)
+ return -ENOMEM;
+
+ ret = mdss_irq_domain_init(dev, mdp5_mdss);
+ if (ret)
+ return ret;
+
+ /*
+ * Here we have no drm_device yet, but still do the assignment
+ * so that we can retrieve our struct mdp5_mdss from the main
+ * init function, since we allocate it here.
+ */
+ priv->mdss = &mdp5_mdss->base;
+
+ return 0;
+}
+
int mdp5_mdss_init(struct drm_device *dev)
{
struct platform_device *pdev = to_platform_device(dev->dev);
@@ -208,11 +239,9 @@ int mdp5_mdss_init(struct drm_device *dev)
if (!of_device_is_compatible(dev->dev->of_node, "qcom,mdss"))
return 0;
- mdp5_mdss = devm_kzalloc(dev->dev, sizeof(*mdp5_mdss), GFP_KERNEL);
- if (!mdp5_mdss) {
- ret = -ENOMEM;
- goto fail;
- }
+ mdp5_mdss = to_mdp5_mdss(priv->mdss);
+ if (!mdp5_mdss)
+ return -ENODATA;
mdp5_mdss->base.dev = dev;
@@ -255,17 +284,8 @@ int mdp5_mdss_init(struct drm_device *dev)
goto fail_irq;
}
- ret = mdss_irq_domain_init(mdp5_mdss);
- if (ret) {
- DRM_DEV_ERROR(dev->dev, "failed to init sub-block irqs: %d\n", ret);
- goto fail_irq;
- }
-
mdp5_mdss->base.funcs = &mdss_funcs;
- priv->mdss = &mdp5_mdss->base;
-
pm_runtime_enable(dev->dev);
-
return 0;
fail_irq:
regulator_disable(mdp5_mdss->vdd);
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 64230e473a34..ded4f4d6545f 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1389,6 +1389,20 @@ static int msm_pdev_probe(struct platform_device *pdev)
if (!priv)
return -ENOMEM;
+ switch (get_mdp_ver(pdev)) {
+ case KMS_MDP5:
+ ret = mdp5_mdss_early_init(&pdev->dev, priv);
+ break;
+ case KMS_DPU:
+ ret = dpu_mdss_early_init(&pdev->dev, priv);
+ break;
+ default:
+ ret = 0;
+ break;
+ }
+ if (ret)
+ return ret;
+
platform_set_drvdata(pdev, priv);
if (get_mdp_ver(pdev)) {
@@ -1421,6 +1435,12 @@ static int msm_pdev_probe(struct platform_device *pdev)
static int msm_pdev_remove(struct platform_device *pdev)
{
+ struct msm_drm_private *priv = platform_get_drvdata(pdev);
+
+ if (priv->mdss && priv->mdss->funcs)
+ priv->mdss->funcs->remove(priv->mdss);
+
+ priv->mdss = NULL;
component_master_del(&pdev->dev, &msm_drm_ops);
of_platform_depopulate(&pdev->dev);
@@ -1432,7 +1452,7 @@ static void msm_pdev_shutdown(struct platform_device *pdev)
struct msm_drm_private *priv = platform_get_drvdata(pdev);
struct drm_device *drm = priv ? priv->dev : NULL;
- if (!priv || !priv->kms)
+ if (!priv || !priv->kms || !drm->mode_config.funcs)
return;
drm_atomic_helper_shutdown(drm);
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 6a42b819abc4..2c539a228156 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -202,6 +202,7 @@ struct msm_mdss_funcs {
int (*enable)(struct msm_mdss *mdss);
int (*disable)(struct msm_mdss *mdss);
void (*destroy)(struct drm_device *dev);
+ void (*remove)(struct msm_mdss *mdss);
};
struct msm_mdss {
@@ -209,7 +210,9 @@ struct msm_mdss {
const struct msm_mdss_funcs *funcs;
};
+int mdp5_mdss_early_init(struct device *dev, struct msm_drm_private *priv);
int mdp5_mdss_init(struct drm_device *dev);
+int dpu_mdss_early_init(struct device *dev, struct msm_drm_private *priv);
int dpu_mdss_init(struct drm_device *dev);
#define for_each_crtc_mask(dev, crtc, crtc_mask) \
--
2.33.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 2/2] drm/msm: Initialize MDSS irq domain at probe time
@ 2021-12-01 10:52 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-12-01 10:52 UTC (permalink / raw)
To: robdclark
Cc: freedreno, airlied, linux-arm-msm, konrad.dybcio, linux-kernel,
dri-devel, jami.kettunen, martin.botka, maxime, dmitry.baryshkov,
marijn.suijten, kernel, sean, AngeloGioacchino Del Regno
Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
DSI host gets initialized earlier, but this caused unability to probe
the entire stack of components because they all depend on interrupts
coming from the main `mdss` node (mdp5, or dpu1).
To fix this issue, anticipate registering the irq domain from mdp5/dpu1
at msm_mdev_probe() time, as to make sure that the interrupt controller
is available before dsi and/or other components try to initialize,
finally satisfying the dependency.
Moreover, to balance this operation while avoiding to always check if
the irq domain is registered everytime we call bind() on msm_pdev, add
a new *remove function pointer to msm_mdss_funcs, used to remove the
irq domain only at msm_pdev_remove() time.
Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 50 ++++++++++++-------
drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c | 58 +++++++++++++++--------
drivers/gpu/drm/msm/msm_drv.c | 22 ++++++++-
drivers/gpu/drm/msm/msm_kms.h | 3 ++
4 files changed, 95 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
index b466784d9822..6c2569175633 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
@@ -106,13 +106,10 @@ static const struct irq_domain_ops dpu_mdss_irqdomain_ops = {
.xlate = irq_domain_xlate_onecell,
};
-static int _dpu_mdss_irq_domain_add(struct dpu_mdss *dpu_mdss)
+static int _dpu_mdss_irq_domain_add(struct device *dev, struct dpu_mdss *dpu_mdss)
{
- struct device *dev;
struct irq_domain *domain;
- dev = dpu_mdss->base.dev->dev;
-
domain = irq_domain_add_linear(dev->of_node, 32,
&dpu_mdss_irqdomain_ops, dpu_mdss);
if (!domain) {
@@ -194,7 +191,6 @@ static void dpu_mdss_destroy(struct drm_device *dev)
pm_runtime_suspend(dev->dev);
pm_runtime_disable(dev->dev);
- _dpu_mdss_irq_domain_fini(dpu_mdss);
irq = platform_get_irq(pdev, 0);
irq_set_chained_handler_and_data(irq, NULL, NULL);
msm_dss_put_clk(mp->clk_config, mp->num_clk);
@@ -203,15 +199,43 @@ static void dpu_mdss_destroy(struct drm_device *dev)
if (dpu_mdss->mmio)
devm_iounmap(&pdev->dev, dpu_mdss->mmio);
dpu_mdss->mmio = NULL;
- priv->mdss = NULL;
+}
+
+static void dpu_mdss_remove(struct msm_mdss *mdss)
+{
+ _dpu_mdss_irq_domain_fini(to_dpu_mdss(mdss));
}
static const struct msm_mdss_funcs mdss_funcs = {
.enable = dpu_mdss_enable,
.disable = dpu_mdss_disable,
.destroy = dpu_mdss_destroy,
+ .remove = dpu_mdss_remove,
};
+int dpu_mdss_early_init(struct device *dev, struct msm_drm_private *priv)
+{
+ struct dpu_mdss *dpu_mdss;
+ int ret;
+
+ dpu_mdss = devm_kzalloc(dev, sizeof(*dpu_mdss), GFP_KERNEL);
+ if (!dpu_mdss)
+ return -ENOMEM;
+
+ ret = _dpu_mdss_irq_domain_add(dev, dpu_mdss);
+ if (ret)
+ return ret;
+
+ /*
+ * Here we have no drm_device yet, but still do the assignment
+ * so that we can retrieve our struct dpu_mdss from the main
+ * init function, since we allocate it here.
+ */
+ priv->mdss = &dpu_mdss->base;
+
+ return 0;
+}
+
int dpu_mdss_init(struct drm_device *dev)
{
struct platform_device *pdev = to_platform_device(dev->dev);
@@ -221,9 +245,9 @@ int dpu_mdss_init(struct drm_device *dev)
int ret;
int irq;
- dpu_mdss = devm_kzalloc(dev->dev, sizeof(*dpu_mdss), GFP_KERNEL);
+ dpu_mdss = to_dpu_mdss(priv->mdss);
if (!dpu_mdss)
- return -ENOMEM;
+ return -ENODATA;
dpu_mdss->mmio = msm_ioremap(pdev, "mdss", "mdss");
if (IS_ERR(dpu_mdss->mmio))
@@ -241,10 +265,6 @@ int dpu_mdss_init(struct drm_device *dev)
dpu_mdss->base.dev = dev;
dpu_mdss->base.funcs = &mdss_funcs;
- ret = _dpu_mdss_irq_domain_add(dpu_mdss);
- if (ret)
- goto irq_domain_error;
-
irq = platform_get_irq(pdev, 0);
if (irq < 0) {
ret = irq;
@@ -253,16 +273,10 @@ int dpu_mdss_init(struct drm_device *dev)
irq_set_chained_handler_and_data(irq, dpu_mdss_irq,
dpu_mdss);
-
- priv->mdss = &dpu_mdss->base;
-
pm_runtime_enable(dev->dev);
-
return 0;
irq_error:
- _dpu_mdss_irq_domain_fini(dpu_mdss);
-irq_domain_error:
msm_dss_put_clk(mp->clk_config, mp->num_clk);
clk_parse_err:
devm_kfree(&pdev->dev, mp->clk_config);
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
index 0ea53420bc40..a99538ae4182 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
@@ -112,9 +112,8 @@ static const struct irq_domain_ops mdss_hw_irqdomain_ops = {
};
-static int mdss_irq_domain_init(struct mdp5_mdss *mdp5_mdss)
+static int mdss_irq_domain_init(struct device *dev, struct mdp5_mdss *mdp5_mdss)
{
- struct device *dev = mdp5_mdss->base.dev->dev;
struct irq_domain *d;
d = irq_domain_add_linear(dev->of_node, 32, &mdss_hw_irqdomain_ops,
@@ -182,20 +181,52 @@ static void mdp5_mdss_destroy(struct drm_device *dev)
if (!mdp5_mdss)
return;
- irq_domain_remove(mdp5_mdss->irqcontroller.domain);
- mdp5_mdss->irqcontroller.domain = NULL;
-
regulator_disable(mdp5_mdss->vdd);
pm_runtime_disable(dev->dev);
}
+static void mdp5_mdss_remove(struct msm_mdss *mdss)
+{
+ struct mdp5_mdss *mdp5_mdss = to_mdp5_mdss(mdss);
+
+ irq_domain_remove(mdp5_mdss->irqcontroller.domain);
+ mdp5_mdss->irqcontroller.domain = NULL;
+}
+
static const struct msm_mdss_funcs mdss_funcs = {
.enable = mdp5_mdss_enable,
.disable = mdp5_mdss_disable,
.destroy = mdp5_mdss_destroy,
+ .remove = mdp5_mdss_remove,
};
+int mdp5_mdss_early_init(struct device *dev, struct msm_drm_private *priv)
+{
+ struct mdp5_mdss *mdp5_mdss;
+ int ret;
+
+ if (!of_device_is_compatible(dev->of_node, "qcom,mdss"))
+ return 0;
+
+ mdp5_mdss = devm_kzalloc(dev, sizeof(*mdp5_mdss), GFP_KERNEL);
+ if (!mdp5_mdss)
+ return -ENOMEM;
+
+ ret = mdss_irq_domain_init(dev, mdp5_mdss);
+ if (ret)
+ return ret;
+
+ /*
+ * Here we have no drm_device yet, but still do the assignment
+ * so that we can retrieve our struct mdp5_mdss from the main
+ * init function, since we allocate it here.
+ */
+ priv->mdss = &mdp5_mdss->base;
+
+ return 0;
+}
+
int mdp5_mdss_init(struct drm_device *dev)
{
struct platform_device *pdev = to_platform_device(dev->dev);
@@ -208,11 +239,9 @@ int mdp5_mdss_init(struct drm_device *dev)
if (!of_device_is_compatible(dev->dev->of_node, "qcom,mdss"))
return 0;
- mdp5_mdss = devm_kzalloc(dev->dev, sizeof(*mdp5_mdss), GFP_KERNEL);
- if (!mdp5_mdss) {
- ret = -ENOMEM;
- goto fail;
- }
+ mdp5_mdss = to_mdp5_mdss(priv->mdss);
+ if (!mdp5_mdss)
+ return -ENODATA;
mdp5_mdss->base.dev = dev;
@@ -255,17 +284,8 @@ int mdp5_mdss_init(struct drm_device *dev)
goto fail_irq;
}
- ret = mdss_irq_domain_init(mdp5_mdss);
- if (ret) {
- DRM_DEV_ERROR(dev->dev, "failed to init sub-block irqs: %d\n", ret);
- goto fail_irq;
- }
-
mdp5_mdss->base.funcs = &mdss_funcs;
- priv->mdss = &mdp5_mdss->base;
-
pm_runtime_enable(dev->dev);
-
return 0;
fail_irq:
regulator_disable(mdp5_mdss->vdd);
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 64230e473a34..ded4f4d6545f 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1389,6 +1389,20 @@ static int msm_pdev_probe(struct platform_device *pdev)
if (!priv)
return -ENOMEM;
+ switch (get_mdp_ver(pdev)) {
+ case KMS_MDP5:
+ ret = mdp5_mdss_early_init(&pdev->dev, priv);
+ break;
+ case KMS_DPU:
+ ret = dpu_mdss_early_init(&pdev->dev, priv);
+ break;
+ default:
+ ret = 0;
+ break;
+ }
+ if (ret)
+ return ret;
+
platform_set_drvdata(pdev, priv);
if (get_mdp_ver(pdev)) {
@@ -1421,6 +1435,12 @@ static int msm_pdev_probe(struct platform_device *pdev)
static int msm_pdev_remove(struct platform_device *pdev)
{
+ struct msm_drm_private *priv = platform_get_drvdata(pdev);
+
+ if (priv->mdss && priv->mdss->funcs)
+ priv->mdss->funcs->remove(priv->mdss);
+
+ priv->mdss = NULL;
component_master_del(&pdev->dev, &msm_drm_ops);
of_platform_depopulate(&pdev->dev);
@@ -1432,7 +1452,7 @@ static void msm_pdev_shutdown(struct platform_device *pdev)
struct msm_drm_private *priv = platform_get_drvdata(pdev);
struct drm_device *drm = priv ? priv->dev : NULL;
- if (!priv || !priv->kms)
+ if (!priv || !priv->kms || !drm->mode_config.funcs)
return;
drm_atomic_helper_shutdown(drm);
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 6a42b819abc4..2c539a228156 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -202,6 +202,7 @@ struct msm_mdss_funcs {
int (*enable)(struct msm_mdss *mdss);
int (*disable)(struct msm_mdss *mdss);
void (*destroy)(struct drm_device *dev);
+ void (*remove)(struct msm_mdss *mdss);
};
struct msm_mdss {
@@ -209,7 +210,9 @@ struct msm_mdss {
const struct msm_mdss_funcs *funcs;
};
+int mdp5_mdss_early_init(struct device *dev, struct msm_drm_private *priv);
int mdp5_mdss_init(struct drm_device *dev);
+int dpu_mdss_early_init(struct device *dev, struct msm_drm_private *priv);
int dpu_mdss_init(struct drm_device *dev);
#define for_each_crtc_mask(dev, crtc, crtc_mask) \
--
2.33.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH] drm/msm: Initialize MDSS irq domain at probe time
2021-12-01 10:52 ` AngeloGioacchino Del Regno
@ 2021-12-01 20:20 ` Dmitry Baryshkov
-1 siblings, 0 replies; 36+ messages in thread
From: Dmitry Baryshkov @ 2021-12-01 20:20 UTC (permalink / raw)
To: Bjorn Andersson, Rob Clark, Sean Paul, Abhinav Kumar
Cc: Stephen Boyd, David Airlie, Daniel Vetter, linux-arm-msm,
dri-devel, freedreno, AngeloGioacchino Del Regno
Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
DSI host gets initialized earlier, but this caused unability to probe
the entire stack of components because they all depend on interrupts
coming from the main `mdss` node (mdp5, or dpu1).
To fix this issue, move mdss device initialization (which include irq
domain setup) to msm_mdev_probe() time, as to make sure that the
interrupt controller is available before dsi and/or other components try
to initialize, finally satisfying the dependency.
Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
Co-Developed-By: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
When checking your patch, I noticed that IRQ domain is created before
respective MDSS clocks are enabled. This does not look like causing any
issues at this time, but it did not look good. So I started moving
clocks parsing to early_init() callbacks. And at some point it looked
like we can drop the init()/destroy() callbacks in favour of
early_init() and remove(). Which promted me to move init()/destroy() in
place of early_init()/remove() with few minor fixes here and there.
---
drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 25 +++++-----
drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c | 32 ++++++-------
drivers/gpu/drm/msm/msm_drv.c | 56 ++++++++++++-----------
drivers/gpu/drm/msm/msm_kms.h | 8 ++--
4 files changed, 59 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
index b466784d9822..131c1f1a869c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
@@ -111,7 +111,7 @@ static int _dpu_mdss_irq_domain_add(struct dpu_mdss *dpu_mdss)
struct device *dev;
struct irq_domain *domain;
- dev = dpu_mdss->base.dev->dev;
+ dev = dpu_mdss->base.dev;
domain = irq_domain_add_linear(dev->of_node, 32,
&dpu_mdss_irqdomain_ops, dpu_mdss);
@@ -184,16 +184,15 @@ static int dpu_mdss_disable(struct msm_mdss *mdss)
return ret;
}
-static void dpu_mdss_destroy(struct drm_device *dev)
+static void dpu_mdss_destroy(struct msm_mdss *mdss)
{
- struct platform_device *pdev = to_platform_device(dev->dev);
- struct msm_drm_private *priv = dev->dev_private;
- struct dpu_mdss *dpu_mdss = to_dpu_mdss(priv->mdss);
+ struct platform_device *pdev = to_platform_device(mdss->dev);
+ struct dpu_mdss *dpu_mdss = to_dpu_mdss(mdss);
struct dss_module_power *mp = &dpu_mdss->mp;
int irq;
- pm_runtime_suspend(dev->dev);
- pm_runtime_disable(dev->dev);
+ pm_runtime_suspend(mdss->dev);
+ pm_runtime_disable(mdss->dev);
_dpu_mdss_irq_domain_fini(dpu_mdss);
irq = platform_get_irq(pdev, 0);
irq_set_chained_handler_and_data(irq, NULL, NULL);
@@ -203,7 +202,6 @@ static void dpu_mdss_destroy(struct drm_device *dev)
if (dpu_mdss->mmio)
devm_iounmap(&pdev->dev, dpu_mdss->mmio);
dpu_mdss->mmio = NULL;
- priv->mdss = NULL;
}
static const struct msm_mdss_funcs mdss_funcs = {
@@ -212,16 +210,15 @@ static const struct msm_mdss_funcs mdss_funcs = {
.destroy = dpu_mdss_destroy,
};
-int dpu_mdss_init(struct drm_device *dev)
+int dpu_mdss_init(struct platform_device *pdev)
{
- struct platform_device *pdev = to_platform_device(dev->dev);
- struct msm_drm_private *priv = dev->dev_private;
+ struct msm_drm_private *priv = platform_get_drvdata(pdev);
struct dpu_mdss *dpu_mdss;
struct dss_module_power *mp;
int ret;
int irq;
- dpu_mdss = devm_kzalloc(dev->dev, sizeof(*dpu_mdss), GFP_KERNEL);
+ dpu_mdss = devm_kzalloc(&pdev->dev, sizeof(*dpu_mdss), GFP_KERNEL);
if (!dpu_mdss)
return -ENOMEM;
@@ -238,7 +235,7 @@ int dpu_mdss_init(struct drm_device *dev)
goto clk_parse_err;
}
- dpu_mdss->base.dev = dev;
+ dpu_mdss->base.dev = &pdev->dev;
dpu_mdss->base.funcs = &mdss_funcs;
ret = _dpu_mdss_irq_domain_add(dpu_mdss);
@@ -256,7 +253,7 @@ int dpu_mdss_init(struct drm_device *dev)
priv->mdss = &dpu_mdss->base;
- pm_runtime_enable(dev->dev);
+ pm_runtime_enable(&pdev->dev);
return 0;
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
index c34760d981b8..b3f79c2277e9 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
@@ -112,7 +112,7 @@ static const struct irq_domain_ops mdss_hw_irqdomain_ops = {
static int mdss_irq_domain_init(struct mdp5_mdss *mdp5_mdss)
{
- struct device *dev = mdp5_mdss->base.dev->dev;
+ struct device *dev = mdp5_mdss->base.dev;
struct irq_domain *d;
d = irq_domain_add_linear(dev->of_node, 32, &mdss_hw_irqdomain_ops,
@@ -155,7 +155,7 @@ static int mdp5_mdss_disable(struct msm_mdss *mdss)
static int msm_mdss_get_clocks(struct mdp5_mdss *mdp5_mdss)
{
struct platform_device *pdev =
- to_platform_device(mdp5_mdss->base.dev->dev);
+ to_platform_device(mdp5_mdss->base.dev);
mdp5_mdss->ahb_clk = msm_clk_get(pdev, "iface");
if (IS_ERR(mdp5_mdss->ahb_clk))
@@ -172,10 +172,9 @@ static int msm_mdss_get_clocks(struct mdp5_mdss *mdp5_mdss)
return 0;
}
-static void mdp5_mdss_destroy(struct drm_device *dev)
+static void mdp5_mdss_destroy(struct msm_mdss *mdss)
{
- struct msm_drm_private *priv = dev->dev_private;
- struct mdp5_mdss *mdp5_mdss = to_mdp5_mdss(priv->mdss);
+ struct mdp5_mdss *mdp5_mdss = to_mdp5_mdss(mdss);
if (!mdp5_mdss)
return;
@@ -183,7 +182,7 @@ static void mdp5_mdss_destroy(struct drm_device *dev)
irq_domain_remove(mdp5_mdss->irqcontroller.domain);
mdp5_mdss->irqcontroller.domain = NULL;
- pm_runtime_disable(dev->dev);
+ pm_runtime_disable(mdss->dev);
}
static const struct msm_mdss_funcs mdss_funcs = {
@@ -192,25 +191,24 @@ static const struct msm_mdss_funcs mdss_funcs = {
.destroy = mdp5_mdss_destroy,
};
-int mdp5_mdss_init(struct drm_device *dev)
+int mdp5_mdss_init(struct platform_device *pdev)
{
- struct platform_device *pdev = to_platform_device(dev->dev);
- struct msm_drm_private *priv = dev->dev_private;
+ struct msm_drm_private *priv = platform_get_drvdata(pdev);
struct mdp5_mdss *mdp5_mdss;
int ret;
DBG("");
- if (!of_device_is_compatible(dev->dev->of_node, "qcom,mdss"))
+ if (!of_device_is_compatible(pdev->dev.of_node, "qcom,mdss"))
return 0;
- mdp5_mdss = devm_kzalloc(dev->dev, sizeof(*mdp5_mdss), GFP_KERNEL);
+ mdp5_mdss = devm_kzalloc(&pdev->dev, sizeof(*mdp5_mdss), GFP_KERNEL);
if (!mdp5_mdss) {
ret = -ENOMEM;
goto fail;
}
- mdp5_mdss->base.dev = dev;
+ mdp5_mdss->base.dev = &pdev->dev;
mdp5_mdss->mmio = msm_ioremap(pdev, "mdss_phys", "MDSS");
if (IS_ERR(mdp5_mdss->mmio)) {
@@ -226,27 +224,27 @@ int mdp5_mdss_init(struct drm_device *dev)
ret = msm_mdss_get_clocks(mdp5_mdss);
if (ret) {
- DRM_DEV_ERROR(dev->dev, "failed to get clocks: %d\n", ret);
+ DRM_DEV_ERROR(&pdev->dev, "failed to get clocks: %d\n", ret);
goto fail;
}
- ret = devm_request_irq(dev->dev, platform_get_irq(pdev, 0),
+ ret = devm_request_irq(&pdev->dev, platform_get_irq(pdev, 0),
mdss_irq, 0, "mdss_isr", mdp5_mdss);
if (ret) {
- DRM_DEV_ERROR(dev->dev, "failed to init irq: %d\n", ret);
+ DRM_DEV_ERROR(&pdev->dev, "failed to init irq: %d\n", ret);
goto fail;
}
ret = mdss_irq_domain_init(mdp5_mdss);
if (ret) {
- DRM_DEV_ERROR(dev->dev, "failed to init sub-block irqs: %d\n", ret);
+ DRM_DEV_ERROR(&pdev->dev, "failed to init sub-block irqs: %d\n", ret);
goto fail;
}
mdp5_mdss->base.funcs = &mdss_funcs;
priv->mdss = &mdp5_mdss->base;
- pm_runtime_enable(dev->dev);
+ pm_runtime_enable(&pdev->dev);
return 0;
fail:
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index f5596efd3819..ad35a5d94053 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -342,7 +342,6 @@ static int msm_drm_uninit(struct device *dev)
struct msm_drm_private *priv = platform_get_drvdata(pdev);
struct drm_device *ddev = priv->dev;
struct msm_kms *kms = priv->kms;
- struct msm_mdss *mdss = priv->mdss;
int i;
/*
@@ -402,9 +401,6 @@ static int msm_drm_uninit(struct device *dev)
component_unbind_all(dev, ddev);
- if (mdss && mdss->funcs)
- mdss->funcs->destroy(ddev);
-
ddev->dev_private = NULL;
drm_dev_put(ddev);
@@ -525,20 +521,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
ddev->dev_private = priv;
priv->dev = ddev;
- switch (get_mdp_ver(pdev)) {
- case KMS_MDP5:
- ret = mdp5_mdss_init(ddev);
- break;
- case KMS_DPU:
- ret = dpu_mdss_init(ddev);
- break;
- default:
- ret = 0;
- break;
- }
- if (ret)
- goto err_put_drm_dev;
-
mdss = priv->mdss;
priv->wq = alloc_ordered_workqueue("msm", 0);
@@ -561,12 +543,12 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
ret = msm_init_vram(ddev);
if (ret)
- goto err_destroy_mdss;
+ return ret;
/* Bind all our sub-components: */
ret = component_bind_all(dev, ddev);
if (ret)
- goto err_destroy_mdss;
+ return ret;
dma_set_max_seg_size(dev, UINT_MAX);
@@ -672,12 +654,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
err_msm_uninit:
msm_drm_uninit(dev);
return ret;
-err_destroy_mdss:
- if (mdss && mdss->funcs)
- mdss->funcs->destroy(ddev);
-err_put_drm_dev:
- drm_dev_put(ddev);
- return ret;
}
/*
@@ -1386,10 +1362,26 @@ static int msm_pdev_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, priv);
+ switch (get_mdp_ver(pdev)) {
+ case KMS_MDP5:
+ ret = mdp5_mdss_init(pdev);
+ break;
+ case KMS_DPU:
+ ret = dpu_mdss_init(pdev);
+ break;
+ default:
+ ret = 0;
+ break;
+ }
+ if (ret) {
+ platform_set_drvdata(pdev, NULL);
+ return ret;
+ }
+
if (get_mdp_ver(pdev)) {
ret = add_display_components(pdev, &match);
if (ret)
- return ret;
+ goto fail;
}
ret = add_gpu_components(&pdev->dev, &match);
@@ -1411,14 +1403,24 @@ static int msm_pdev_probe(struct platform_device *pdev)
fail:
of_platform_depopulate(&pdev->dev);
+
+ if (priv->mdss && priv->mdss->funcs)
+ priv->mdss->funcs->destroy(priv->mdss);
+
return ret;
}
static int msm_pdev_remove(struct platform_device *pdev)
{
+ struct msm_drm_private *priv = platform_get_drvdata(pdev);
+ struct msm_mdss *mdss = priv->mdss;
+
component_master_del(&pdev->dev, &msm_drm_ops);
of_platform_depopulate(&pdev->dev);
+ if (mdss && mdss->funcs)
+ mdss->funcs->destroy(mdss);
+
return 0;
}
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 8b132c8b1513..2a4f0526cb98 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -204,16 +204,16 @@ extern const struct of_device_id mdp5_dt_match[];
struct msm_mdss_funcs {
int (*enable)(struct msm_mdss *mdss);
int (*disable)(struct msm_mdss *mdss);
- void (*destroy)(struct drm_device *dev);
+ void (*destroy)(struct msm_mdss *mdss);
};
struct msm_mdss {
- struct drm_device *dev;
+ struct device *dev;
const struct msm_mdss_funcs *funcs;
};
-int mdp5_mdss_init(struct drm_device *dev);
-int dpu_mdss_init(struct drm_device *dev);
+int mdp5_mdss_init(struct platform_device *dev);
+int dpu_mdss_init(struct platform_device *dev);
#define for_each_crtc_mask(dev, crtc, crtc_mask) \
drm_for_each_crtc(crtc, dev) \
--
2.33.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH] drm/msm: Initialize MDSS irq domain at probe time
@ 2021-12-01 20:20 ` Dmitry Baryshkov
0 siblings, 0 replies; 36+ messages in thread
From: Dmitry Baryshkov @ 2021-12-01 20:20 UTC (permalink / raw)
To: Bjorn Andersson, Rob Clark, Sean Paul, Abhinav Kumar
Cc: Stephen Boyd, linux-arm-msm, dri-devel, David Airlie, freedreno,
AngeloGioacchino Del Regno
Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
DSI host gets initialized earlier, but this caused unability to probe
the entire stack of components because they all depend on interrupts
coming from the main `mdss` node (mdp5, or dpu1).
To fix this issue, move mdss device initialization (which include irq
domain setup) to msm_mdev_probe() time, as to make sure that the
interrupt controller is available before dsi and/or other components try
to initialize, finally satisfying the dependency.
Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
Co-Developed-By: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
When checking your patch, I noticed that IRQ domain is created before
respective MDSS clocks are enabled. This does not look like causing any
issues at this time, but it did not look good. So I started moving
clocks parsing to early_init() callbacks. And at some point it looked
like we can drop the init()/destroy() callbacks in favour of
early_init() and remove(). Which promted me to move init()/destroy() in
place of early_init()/remove() with few minor fixes here and there.
---
drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 25 +++++-----
drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c | 32 ++++++-------
drivers/gpu/drm/msm/msm_drv.c | 56 ++++++++++++-----------
drivers/gpu/drm/msm/msm_kms.h | 8 ++--
4 files changed, 59 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
index b466784d9822..131c1f1a869c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
@@ -111,7 +111,7 @@ static int _dpu_mdss_irq_domain_add(struct dpu_mdss *dpu_mdss)
struct device *dev;
struct irq_domain *domain;
- dev = dpu_mdss->base.dev->dev;
+ dev = dpu_mdss->base.dev;
domain = irq_domain_add_linear(dev->of_node, 32,
&dpu_mdss_irqdomain_ops, dpu_mdss);
@@ -184,16 +184,15 @@ static int dpu_mdss_disable(struct msm_mdss *mdss)
return ret;
}
-static void dpu_mdss_destroy(struct drm_device *dev)
+static void dpu_mdss_destroy(struct msm_mdss *mdss)
{
- struct platform_device *pdev = to_platform_device(dev->dev);
- struct msm_drm_private *priv = dev->dev_private;
- struct dpu_mdss *dpu_mdss = to_dpu_mdss(priv->mdss);
+ struct platform_device *pdev = to_platform_device(mdss->dev);
+ struct dpu_mdss *dpu_mdss = to_dpu_mdss(mdss);
struct dss_module_power *mp = &dpu_mdss->mp;
int irq;
- pm_runtime_suspend(dev->dev);
- pm_runtime_disable(dev->dev);
+ pm_runtime_suspend(mdss->dev);
+ pm_runtime_disable(mdss->dev);
_dpu_mdss_irq_domain_fini(dpu_mdss);
irq = platform_get_irq(pdev, 0);
irq_set_chained_handler_and_data(irq, NULL, NULL);
@@ -203,7 +202,6 @@ static void dpu_mdss_destroy(struct drm_device *dev)
if (dpu_mdss->mmio)
devm_iounmap(&pdev->dev, dpu_mdss->mmio);
dpu_mdss->mmio = NULL;
- priv->mdss = NULL;
}
static const struct msm_mdss_funcs mdss_funcs = {
@@ -212,16 +210,15 @@ static const struct msm_mdss_funcs mdss_funcs = {
.destroy = dpu_mdss_destroy,
};
-int dpu_mdss_init(struct drm_device *dev)
+int dpu_mdss_init(struct platform_device *pdev)
{
- struct platform_device *pdev = to_platform_device(dev->dev);
- struct msm_drm_private *priv = dev->dev_private;
+ struct msm_drm_private *priv = platform_get_drvdata(pdev);
struct dpu_mdss *dpu_mdss;
struct dss_module_power *mp;
int ret;
int irq;
- dpu_mdss = devm_kzalloc(dev->dev, sizeof(*dpu_mdss), GFP_KERNEL);
+ dpu_mdss = devm_kzalloc(&pdev->dev, sizeof(*dpu_mdss), GFP_KERNEL);
if (!dpu_mdss)
return -ENOMEM;
@@ -238,7 +235,7 @@ int dpu_mdss_init(struct drm_device *dev)
goto clk_parse_err;
}
- dpu_mdss->base.dev = dev;
+ dpu_mdss->base.dev = &pdev->dev;
dpu_mdss->base.funcs = &mdss_funcs;
ret = _dpu_mdss_irq_domain_add(dpu_mdss);
@@ -256,7 +253,7 @@ int dpu_mdss_init(struct drm_device *dev)
priv->mdss = &dpu_mdss->base;
- pm_runtime_enable(dev->dev);
+ pm_runtime_enable(&pdev->dev);
return 0;
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
index c34760d981b8..b3f79c2277e9 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
@@ -112,7 +112,7 @@ static const struct irq_domain_ops mdss_hw_irqdomain_ops = {
static int mdss_irq_domain_init(struct mdp5_mdss *mdp5_mdss)
{
- struct device *dev = mdp5_mdss->base.dev->dev;
+ struct device *dev = mdp5_mdss->base.dev;
struct irq_domain *d;
d = irq_domain_add_linear(dev->of_node, 32, &mdss_hw_irqdomain_ops,
@@ -155,7 +155,7 @@ static int mdp5_mdss_disable(struct msm_mdss *mdss)
static int msm_mdss_get_clocks(struct mdp5_mdss *mdp5_mdss)
{
struct platform_device *pdev =
- to_platform_device(mdp5_mdss->base.dev->dev);
+ to_platform_device(mdp5_mdss->base.dev);
mdp5_mdss->ahb_clk = msm_clk_get(pdev, "iface");
if (IS_ERR(mdp5_mdss->ahb_clk))
@@ -172,10 +172,9 @@ static int msm_mdss_get_clocks(struct mdp5_mdss *mdp5_mdss)
return 0;
}
-static void mdp5_mdss_destroy(struct drm_device *dev)
+static void mdp5_mdss_destroy(struct msm_mdss *mdss)
{
- struct msm_drm_private *priv = dev->dev_private;
- struct mdp5_mdss *mdp5_mdss = to_mdp5_mdss(priv->mdss);
+ struct mdp5_mdss *mdp5_mdss = to_mdp5_mdss(mdss);
if (!mdp5_mdss)
return;
@@ -183,7 +182,7 @@ static void mdp5_mdss_destroy(struct drm_device *dev)
irq_domain_remove(mdp5_mdss->irqcontroller.domain);
mdp5_mdss->irqcontroller.domain = NULL;
- pm_runtime_disable(dev->dev);
+ pm_runtime_disable(mdss->dev);
}
static const struct msm_mdss_funcs mdss_funcs = {
@@ -192,25 +191,24 @@ static const struct msm_mdss_funcs mdss_funcs = {
.destroy = mdp5_mdss_destroy,
};
-int mdp5_mdss_init(struct drm_device *dev)
+int mdp5_mdss_init(struct platform_device *pdev)
{
- struct platform_device *pdev = to_platform_device(dev->dev);
- struct msm_drm_private *priv = dev->dev_private;
+ struct msm_drm_private *priv = platform_get_drvdata(pdev);
struct mdp5_mdss *mdp5_mdss;
int ret;
DBG("");
- if (!of_device_is_compatible(dev->dev->of_node, "qcom,mdss"))
+ if (!of_device_is_compatible(pdev->dev.of_node, "qcom,mdss"))
return 0;
- mdp5_mdss = devm_kzalloc(dev->dev, sizeof(*mdp5_mdss), GFP_KERNEL);
+ mdp5_mdss = devm_kzalloc(&pdev->dev, sizeof(*mdp5_mdss), GFP_KERNEL);
if (!mdp5_mdss) {
ret = -ENOMEM;
goto fail;
}
- mdp5_mdss->base.dev = dev;
+ mdp5_mdss->base.dev = &pdev->dev;
mdp5_mdss->mmio = msm_ioremap(pdev, "mdss_phys", "MDSS");
if (IS_ERR(mdp5_mdss->mmio)) {
@@ -226,27 +224,27 @@ int mdp5_mdss_init(struct drm_device *dev)
ret = msm_mdss_get_clocks(mdp5_mdss);
if (ret) {
- DRM_DEV_ERROR(dev->dev, "failed to get clocks: %d\n", ret);
+ DRM_DEV_ERROR(&pdev->dev, "failed to get clocks: %d\n", ret);
goto fail;
}
- ret = devm_request_irq(dev->dev, platform_get_irq(pdev, 0),
+ ret = devm_request_irq(&pdev->dev, platform_get_irq(pdev, 0),
mdss_irq, 0, "mdss_isr", mdp5_mdss);
if (ret) {
- DRM_DEV_ERROR(dev->dev, "failed to init irq: %d\n", ret);
+ DRM_DEV_ERROR(&pdev->dev, "failed to init irq: %d\n", ret);
goto fail;
}
ret = mdss_irq_domain_init(mdp5_mdss);
if (ret) {
- DRM_DEV_ERROR(dev->dev, "failed to init sub-block irqs: %d\n", ret);
+ DRM_DEV_ERROR(&pdev->dev, "failed to init sub-block irqs: %d\n", ret);
goto fail;
}
mdp5_mdss->base.funcs = &mdss_funcs;
priv->mdss = &mdp5_mdss->base;
- pm_runtime_enable(dev->dev);
+ pm_runtime_enable(&pdev->dev);
return 0;
fail:
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index f5596efd3819..ad35a5d94053 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -342,7 +342,6 @@ static int msm_drm_uninit(struct device *dev)
struct msm_drm_private *priv = platform_get_drvdata(pdev);
struct drm_device *ddev = priv->dev;
struct msm_kms *kms = priv->kms;
- struct msm_mdss *mdss = priv->mdss;
int i;
/*
@@ -402,9 +401,6 @@ static int msm_drm_uninit(struct device *dev)
component_unbind_all(dev, ddev);
- if (mdss && mdss->funcs)
- mdss->funcs->destroy(ddev);
-
ddev->dev_private = NULL;
drm_dev_put(ddev);
@@ -525,20 +521,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
ddev->dev_private = priv;
priv->dev = ddev;
- switch (get_mdp_ver(pdev)) {
- case KMS_MDP5:
- ret = mdp5_mdss_init(ddev);
- break;
- case KMS_DPU:
- ret = dpu_mdss_init(ddev);
- break;
- default:
- ret = 0;
- break;
- }
- if (ret)
- goto err_put_drm_dev;
-
mdss = priv->mdss;
priv->wq = alloc_ordered_workqueue("msm", 0);
@@ -561,12 +543,12 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
ret = msm_init_vram(ddev);
if (ret)
- goto err_destroy_mdss;
+ return ret;
/* Bind all our sub-components: */
ret = component_bind_all(dev, ddev);
if (ret)
- goto err_destroy_mdss;
+ return ret;
dma_set_max_seg_size(dev, UINT_MAX);
@@ -672,12 +654,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
err_msm_uninit:
msm_drm_uninit(dev);
return ret;
-err_destroy_mdss:
- if (mdss && mdss->funcs)
- mdss->funcs->destroy(ddev);
-err_put_drm_dev:
- drm_dev_put(ddev);
- return ret;
}
/*
@@ -1386,10 +1362,26 @@ static int msm_pdev_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, priv);
+ switch (get_mdp_ver(pdev)) {
+ case KMS_MDP5:
+ ret = mdp5_mdss_init(pdev);
+ break;
+ case KMS_DPU:
+ ret = dpu_mdss_init(pdev);
+ break;
+ default:
+ ret = 0;
+ break;
+ }
+ if (ret) {
+ platform_set_drvdata(pdev, NULL);
+ return ret;
+ }
+
if (get_mdp_ver(pdev)) {
ret = add_display_components(pdev, &match);
if (ret)
- return ret;
+ goto fail;
}
ret = add_gpu_components(&pdev->dev, &match);
@@ -1411,14 +1403,24 @@ static int msm_pdev_probe(struct platform_device *pdev)
fail:
of_platform_depopulate(&pdev->dev);
+
+ if (priv->mdss && priv->mdss->funcs)
+ priv->mdss->funcs->destroy(priv->mdss);
+
return ret;
}
static int msm_pdev_remove(struct platform_device *pdev)
{
+ struct msm_drm_private *priv = platform_get_drvdata(pdev);
+ struct msm_mdss *mdss = priv->mdss;
+
component_master_del(&pdev->dev, &msm_drm_ops);
of_platform_depopulate(&pdev->dev);
+ if (mdss && mdss->funcs)
+ mdss->funcs->destroy(mdss);
+
return 0;
}
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 8b132c8b1513..2a4f0526cb98 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -204,16 +204,16 @@ extern const struct of_device_id mdp5_dt_match[];
struct msm_mdss_funcs {
int (*enable)(struct msm_mdss *mdss);
int (*disable)(struct msm_mdss *mdss);
- void (*destroy)(struct drm_device *dev);
+ void (*destroy)(struct msm_mdss *mdss);
};
struct msm_mdss {
- struct drm_device *dev;
+ struct device *dev;
const struct msm_mdss_funcs *funcs;
};
-int mdp5_mdss_init(struct drm_device *dev);
-int dpu_mdss_init(struct drm_device *dev);
+int mdp5_mdss_init(struct platform_device *dev);
+int dpu_mdss_init(struct platform_device *dev);
#define for_each_crtc_mask(dev, crtc, crtc_mask) \
drm_for_each_crtc(crtc, dev) \
--
2.33.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
2021-12-01 20:20 ` Dmitry Baryshkov
@ 2021-12-03 10:43 ` AngeloGioacchino Del Regno
-1 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-12-03 10:43 UTC (permalink / raw)
To: Dmitry Baryshkov, Bjorn Andersson, Rob Clark, Sean Paul, Abhinav Kumar
Cc: Stephen Boyd, linux-arm-msm, dri-devel, David Airlie, freedreno
Il 01/12/21 21:20, Dmitry Baryshkov ha scritto:
> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
> DSI host gets initialized earlier, but this caused unability to probe
> the entire stack of components because they all depend on interrupts
> coming from the main `mdss` node (mdp5, or dpu1).
>
> To fix this issue, move mdss device initialization (which include irq
> domain setup) to msm_mdev_probe() time, as to make sure that the
> interrupt controller is available before dsi and/or other components try
> to initialize, finally satisfying the dependency.
>
> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
> Co-Developed-By: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>
> When checking your patch, I noticed that IRQ domain is created before
> respective MDSS clocks are enabled. This does not look like causing any
> issues at this time, but it did not look good. So I started moving
> clocks parsing to early_init() callbacks. And at some point it looked
> like we can drop the init()/destroy() callbacks in favour of
> early_init() and remove(). Which promted me to move init()/destroy() in
> place of early_init()/remove() with few minor fixes here and there.
>
Hey Dmitry,
I wanted to make the least amount of changes to Rob's logic... I know that
the clocks aren't up before registering the domain, but my logic was implying
that if the handlers aren't registered, then there's no interrupt coming, hence
no risk of getting issues. Same if the hardware is down, you can't get any
interrupt, because it can't generate any (but if bootloader leaves it up.. eh.)
I recognize that such approach is "fragile enough", lastly, I agree with this
patch which is, in the end, something that is in the middle between my first
and last approach.
I've tested this one on trogdor-lazor-limozeen and seems to be working in an
analogous way to my v2/v3, so on my side it's validated.
Let's go for this one!
How do we proceeed now? Are you sending a new series with the new patches, or
should I?
Also, I don't think this is relevant, since I'm in co-dev, but in case it is:
Tested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
P.S.: Sorry for the 1-day delay, got busy with other tasks!
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
@ 2021-12-03 10:43 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-12-03 10:43 UTC (permalink / raw)
To: Dmitry Baryshkov, Bjorn Andersson, Rob Clark, Sean Paul, Abhinav Kumar
Cc: Stephen Boyd, David Airlie, Daniel Vetter, linux-arm-msm,
dri-devel, freedreno
Il 01/12/21 21:20, Dmitry Baryshkov ha scritto:
> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
> DSI host gets initialized earlier, but this caused unability to probe
> the entire stack of components because they all depend on interrupts
> coming from the main `mdss` node (mdp5, or dpu1).
>
> To fix this issue, move mdss device initialization (which include irq
> domain setup) to msm_mdev_probe() time, as to make sure that the
> interrupt controller is available before dsi and/or other components try
> to initialize, finally satisfying the dependency.
>
> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
> Co-Developed-By: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>
> When checking your patch, I noticed that IRQ domain is created before
> respective MDSS clocks are enabled. This does not look like causing any
> issues at this time, but it did not look good. So I started moving
> clocks parsing to early_init() callbacks. And at some point it looked
> like we can drop the init()/destroy() callbacks in favour of
> early_init() and remove(). Which promted me to move init()/destroy() in
> place of early_init()/remove() with few minor fixes here and there.
>
Hey Dmitry,
I wanted to make the least amount of changes to Rob's logic... I know that
the clocks aren't up before registering the domain, but my logic was implying
that if the handlers aren't registered, then there's no interrupt coming, hence
no risk of getting issues. Same if the hardware is down, you can't get any
interrupt, because it can't generate any (but if bootloader leaves it up.. eh.)
I recognize that such approach is "fragile enough", lastly, I agree with this
patch which is, in the end, something that is in the middle between my first
and last approach.
I've tested this one on trogdor-lazor-limozeen and seems to be working in an
analogous way to my v2/v3, so on my side it's validated.
Let's go for this one!
How do we proceeed now? Are you sending a new series with the new patches, or
should I?
Also, I don't think this is relevant, since I'm in co-dev, but in case it is:
Tested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
P.S.: Sorry for the 1-day delay, got busy with other tasks!
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
2021-12-03 10:43 ` AngeloGioacchino Del Regno
@ 2021-12-03 13:14 ` Dmitry Baryshkov
-1 siblings, 0 replies; 36+ messages in thread
From: Dmitry Baryshkov @ 2021-12-03 13:14 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Bjorn Andersson, Rob Clark,
Sean Paul, Abhinav Kumar
Cc: Stephen Boyd, David Airlie, Daniel Vetter, linux-arm-msm,
dri-devel, freedreno
On 03/12/2021 13:43, AngeloGioacchino Del Regno wrote:
> Il 01/12/21 21:20, Dmitry Baryshkov ha scritto:
>> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
>> DSI host gets initialized earlier, but this caused unability to probe
>> the entire stack of components because they all depend on interrupts
>> coming from the main `mdss` node (mdp5, or dpu1).
>>
>> To fix this issue, move mdss device initialization (which include irq
>> domain setup) to msm_mdev_probe() time, as to make sure that the
>> interrupt controller is available before dsi and/or other components try
>> to initialize, finally satisfying the dependency.
>>
>> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
>> Co-Developed-By: AngeloGioacchino Del Regno
>> <angelogioacchino.delregno@collabora.com>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>
>> When checking your patch, I noticed that IRQ domain is created before
>> respective MDSS clocks are enabled. This does not look like causing any
>> issues at this time, but it did not look good. So I started moving
>> clocks parsing to early_init() callbacks. And at some point it looked
>> like we can drop the init()/destroy() callbacks in favour of
>> early_init() and remove(). Which promted me to move init()/destroy() in
>> place of early_init()/remove() with few minor fixes here and there.
>>
>
>
> Hey Dmitry,
> I wanted to make the least amount of changes to Rob's logic... I know that
> the clocks aren't up before registering the domain, but my logic was
> implying
> that if the handlers aren't registered, then there's no interrupt
> coming, hence
> no risk of getting issues. Same if the hardware is down, you can't get any
> interrupt, because it can't generate any (but if bootloader leaves it
> up.. eh.)
We can get spurious interrupts for any reason, which puts us at risk of
peeking into unpowered registers. So, while your approach was working,
it did not seem fully correct.
>
> I recognize that such approach is "fragile enough", lastly, I agree with
> this
> patch which is, in the end, something that is in the middle between my
> first
> and last approach.
>
> I've tested this one on trogdor-lazor-limozeen and seems to be working
> in an
> analogous way to my v2/v3, so on my side it's validated.
>
>
> Let's go for this one!
> How do we proceeed now? Are you sending a new series with the new
> patches, or
> should I?
I'll run a few more tests and then I'll probably include both patches
into the next series to be sent to Rob.
>
> Also, I don't think this is relevant, since I'm in co-dev, but in case
> it is:
> Tested-by: AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com>
>
> P.S.: Sorry for the 1-day delay, got busy with other tasks!
No problem, it was just single day, no worries.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
@ 2021-12-03 13:14 ` Dmitry Baryshkov
0 siblings, 0 replies; 36+ messages in thread
From: Dmitry Baryshkov @ 2021-12-03 13:14 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Bjorn Andersson, Rob Clark,
Sean Paul, Abhinav Kumar
Cc: Stephen Boyd, linux-arm-msm, dri-devel, David Airlie, freedreno
On 03/12/2021 13:43, AngeloGioacchino Del Regno wrote:
> Il 01/12/21 21:20, Dmitry Baryshkov ha scritto:
>> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
>> DSI host gets initialized earlier, but this caused unability to probe
>> the entire stack of components because they all depend on interrupts
>> coming from the main `mdss` node (mdp5, or dpu1).
>>
>> To fix this issue, move mdss device initialization (which include irq
>> domain setup) to msm_mdev_probe() time, as to make sure that the
>> interrupt controller is available before dsi and/or other components try
>> to initialize, finally satisfying the dependency.
>>
>> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
>> Co-Developed-By: AngeloGioacchino Del Regno
>> <angelogioacchino.delregno@collabora.com>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>
>> When checking your patch, I noticed that IRQ domain is created before
>> respective MDSS clocks are enabled. This does not look like causing any
>> issues at this time, but it did not look good. So I started moving
>> clocks parsing to early_init() callbacks. And at some point it looked
>> like we can drop the init()/destroy() callbacks in favour of
>> early_init() and remove(). Which promted me to move init()/destroy() in
>> place of early_init()/remove() with few minor fixes here and there.
>>
>
>
> Hey Dmitry,
> I wanted to make the least amount of changes to Rob's logic... I know that
> the clocks aren't up before registering the domain, but my logic was
> implying
> that if the handlers aren't registered, then there's no interrupt
> coming, hence
> no risk of getting issues. Same if the hardware is down, you can't get any
> interrupt, because it can't generate any (but if bootloader leaves it
> up.. eh.)
We can get spurious interrupts for any reason, which puts us at risk of
peeking into unpowered registers. So, while your approach was working,
it did not seem fully correct.
>
> I recognize that such approach is "fragile enough", lastly, I agree with
> this
> patch which is, in the end, something that is in the middle between my
> first
> and last approach.
>
> I've tested this one on trogdor-lazor-limozeen and seems to be working
> in an
> analogous way to my v2/v3, so on my side it's validated.
>
>
> Let's go for this one!
> How do we proceeed now? Are you sending a new series with the new
> patches, or
> should I?
I'll run a few more tests and then I'll probably include both patches
into the next series to be sent to Rob.
>
> Also, I don't think this is relevant, since I'm in co-dev, but in case
> it is:
> Tested-by: AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com>
>
> P.S.: Sorry for the 1-day delay, got busy with other tasks!
No problem, it was just single day, no worries.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
2021-12-03 13:14 ` Dmitry Baryshkov
@ 2021-12-03 13:17 ` AngeloGioacchino Del Regno
-1 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-12-03 13:17 UTC (permalink / raw)
To: Dmitry Baryshkov, Bjorn Andersson, Rob Clark, Sean Paul, Abhinav Kumar
Cc: Stephen Boyd, David Airlie, Daniel Vetter, linux-arm-msm,
dri-devel, freedreno
Il 03/12/21 14:14, Dmitry Baryshkov ha scritto:
> On 03/12/2021 13:43, AngeloGioacchino Del Regno wrote:
>> Il 01/12/21 21:20, Dmitry Baryshkov ha scritto:
>>> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
>>> DSI host gets initialized earlier, but this caused unability to probe
>>> the entire stack of components because they all depend on interrupts
>>> coming from the main `mdss` node (mdp5, or dpu1).
>>>
>>> To fix this issue, move mdss device initialization (which include irq
>>> domain setup) to msm_mdev_probe() time, as to make sure that the
>>> interrupt controller is available before dsi and/or other components try
>>> to initialize, finally satisfying the dependency.
>>>
>>> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
>>> Co-Developed-By: AngeloGioacchino Del Regno
>>> <angelogioacchino.delregno@collabora.com>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>
>>> When checking your patch, I noticed that IRQ domain is created before
>>> respective MDSS clocks are enabled. This does not look like causing any
>>> issues at this time, but it did not look good. So I started moving
>>> clocks parsing to early_init() callbacks. And at some point it looked
>>> like we can drop the init()/destroy() callbacks in favour of
>>> early_init() and remove(). Which promted me to move init()/destroy() in
>>> place of early_init()/remove() with few minor fixes here and there.
>>>
>>
>>
>> Hey Dmitry,
>> I wanted to make the least amount of changes to Rob's logic... I know that
>> the clocks aren't up before registering the domain, but my logic was implying
>> that if the handlers aren't registered, then there's no interrupt coming, hence
>> no risk of getting issues. Same if the hardware is down, you can't get any
>> interrupt, because it can't generate any (but if bootloader leaves it up.. eh.)
>
> We can get spurious interrupts for any reason, which puts us at risk of peeking
> into unpowered registers. So, while your approach was working, it did not seem
> fully correct.
>
Yeah, that's right and I totally agree.
>>
>> I recognize that such approach is "fragile enough", lastly, I agree with this
>> patch which is, in the end, something that is in the middle between my first
>> and last approach.
>>
>> I've tested this one on trogdor-lazor-limozeen and seems to be working in an
>> analogous way to my v2/v3, so on my side it's validated.
>>
>>
>> Let's go for this one!
>> How do we proceeed now? Are you sending a new series with the new patches, or
>> should I?
>
> I'll run a few more tests and then I'll probably include both patches into the next
> series to be sent to Rob.
>
That's perfect!
>>
>> Also, I don't think this is relevant, since I'm in co-dev, but in case it is:
>> Tested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>
>> P.S.: Sorry for the 1-day delay, got busy with other tasks!
>
> No problem, it was just single day, no worries.
>
Alright, thank you! :D
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
@ 2021-12-03 13:17 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-12-03 13:17 UTC (permalink / raw)
To: Dmitry Baryshkov, Bjorn Andersson, Rob Clark, Sean Paul, Abhinav Kumar
Cc: Stephen Boyd, linux-arm-msm, dri-devel, David Airlie, freedreno
Il 03/12/21 14:14, Dmitry Baryshkov ha scritto:
> On 03/12/2021 13:43, AngeloGioacchino Del Regno wrote:
>> Il 01/12/21 21:20, Dmitry Baryshkov ha scritto:
>>> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
>>> DSI host gets initialized earlier, but this caused unability to probe
>>> the entire stack of components because they all depend on interrupts
>>> coming from the main `mdss` node (mdp5, or dpu1).
>>>
>>> To fix this issue, move mdss device initialization (which include irq
>>> domain setup) to msm_mdev_probe() time, as to make sure that the
>>> interrupt controller is available before dsi and/or other components try
>>> to initialize, finally satisfying the dependency.
>>>
>>> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
>>> Co-Developed-By: AngeloGioacchino Del Regno
>>> <angelogioacchino.delregno@collabora.com>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>
>>> When checking your patch, I noticed that IRQ domain is created before
>>> respective MDSS clocks are enabled. This does not look like causing any
>>> issues at this time, but it did not look good. So I started moving
>>> clocks parsing to early_init() callbacks. And at some point it looked
>>> like we can drop the init()/destroy() callbacks in favour of
>>> early_init() and remove(). Which promted me to move init()/destroy() in
>>> place of early_init()/remove() with few minor fixes here and there.
>>>
>>
>>
>> Hey Dmitry,
>> I wanted to make the least amount of changes to Rob's logic... I know that
>> the clocks aren't up before registering the domain, but my logic was implying
>> that if the handlers aren't registered, then there's no interrupt coming, hence
>> no risk of getting issues. Same if the hardware is down, you can't get any
>> interrupt, because it can't generate any (but if bootloader leaves it up.. eh.)
>
> We can get spurious interrupts for any reason, which puts us at risk of peeking
> into unpowered registers. So, while your approach was working, it did not seem
> fully correct.
>
Yeah, that's right and I totally agree.
>>
>> I recognize that such approach is "fragile enough", lastly, I agree with this
>> patch which is, in the end, something that is in the middle between my first
>> and last approach.
>>
>> I've tested this one on trogdor-lazor-limozeen and seems to be working in an
>> analogous way to my v2/v3, so on my side it's validated.
>>
>>
>> Let's go for this one!
>> How do we proceeed now? Are you sending a new series with the new patches, or
>> should I?
>
> I'll run a few more tests and then I'll probably include both patches into the next
> series to be sent to Rob.
>
That's perfect!
>>
>> Also, I don't think this is relevant, since I'm in co-dev, but in case it is:
>> Tested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>
>> P.S.: Sorry for the 1-day delay, got busy with other tasks!
>
> No problem, it was just single day, no worries.
>
Alright, thank you! :D
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] drm/msm: Initialize MDSS irq domain at probe time
@ 2021-11-25 15:09 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-11-25 15:09 UTC (permalink / raw)
To: robdclark
Cc: sean, airlied, daniel, maxime, linux-arm-msm, dri-devel,
freedreno, linux-kernel, kernel, konrad.dybcio, marijn.suijten,
jami.kettunen, AngeloGioacchino Del Regno
Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
DSI host gets initialized earlier, but this caused unability to probe
the entire stack of components because they all depend on interrupts
coming from the main `mdss` node (mdp5, or dpu1).
To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
them at msm_pdev_probe() time: this will make sure that we add the
required interrupt controller mapping before dsi and/or other components
try to initialize, finally satisfying the dependency.
While at it, also change the allocation of msm_drm_private to use the
devm variant of kzalloc().
Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
drivers/gpu/drm/msm/msm_drv.c | 81 ++++++++++++++++-------------------
1 file changed, 38 insertions(+), 43 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 7936e8d498dd..790acf4993c0 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -512,45 +512,12 @@ static int msm_init_vram(struct drm_device *dev)
static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
{
struct platform_device *pdev = to_platform_device(dev);
- struct drm_device *ddev;
- struct msm_drm_private *priv;
- struct msm_kms *kms;
- struct msm_mdss *mdss;
+ struct drm_device *ddev = platform_get_drvdata(pdev);
+ struct msm_drm_private *priv = ddev->dev_private;
+ struct msm_kms *kms = priv->kms;
+ struct msm_mdss *mdss = priv->mdss;
int ret, i;
- ddev = drm_dev_alloc(drv, dev);
- if (IS_ERR(ddev)) {
- DRM_DEV_ERROR(dev, "failed to allocate drm_device\n");
- return PTR_ERR(ddev);
- }
-
- platform_set_drvdata(pdev, ddev);
-
- priv = kzalloc(sizeof(*priv), GFP_KERNEL);
- if (!priv) {
- ret = -ENOMEM;
- goto err_put_drm_dev;
- }
-
- ddev->dev_private = priv;
- priv->dev = ddev;
-
- switch (get_mdp_ver(pdev)) {
- case KMS_MDP5:
- ret = mdp5_mdss_init(ddev);
- break;
- case KMS_DPU:
- ret = dpu_mdss_init(ddev);
- break;
- default:
- ret = 0;
- break;
- }
- if (ret)
- goto err_free_priv;
-
- mdss = priv->mdss;
-
priv->wq = alloc_ordered_workqueue("msm", 0);
priv->hangcheck_period = DRM_MSM_HANGCHECK_DEFAULT_PERIOD;
@@ -685,11 +652,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
err_destroy_mdss:
if (mdss && mdss->funcs)
mdss->funcs->destroy(ddev);
-err_free_priv:
- kfree(priv);
-err_put_drm_dev:
- drm_dev_put(ddev);
- platform_set_drvdata(pdev, NULL);
return ret;
}
@@ -1382,12 +1344,42 @@ static const struct component_master_ops msm_drm_ops = {
static int msm_pdev_probe(struct platform_device *pdev)
{
struct component_match *match = NULL;
+ struct msm_drm_private *priv;
+ struct drm_device *ddev;
int ret;
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ ddev = drm_dev_alloc(&msm_driver, &pdev->dev);
+ if (IS_ERR(ddev)) {
+ DRM_DEV_ERROR(&pdev->dev, "failed to allocate drm_device\n");
+ return PTR_ERR(ddev);
+ }
+
+ platform_set_drvdata(pdev, ddev);
+ ddev->dev_private = priv;
+ priv->dev = ddev;
+
+ switch (get_mdp_ver(pdev)) {
+ case KMS_MDP5:
+ ret = mdp5_mdss_init(ddev);
+ break;
+ case KMS_DPU:
+ ret = dpu_mdss_init(ddev);
+ break;
+ default:
+ ret = 0;
+ break;
+ }
+ if (ret)
+ goto err_put_drm_dev;
+
if (get_mdp_ver(pdev)) {
ret = add_display_components(pdev, &match);
if (ret)
- return ret;
+ goto fail;
}
ret = add_gpu_components(&pdev->dev, &match);
@@ -1409,6 +1401,9 @@ static int msm_pdev_probe(struct platform_device *pdev)
fail:
of_platform_depopulate(&pdev->dev);
+err_put_drm_dev:
+ drm_dev_put(ddev);
+ platform_set_drvdata(pdev, NULL);
return ret;
}
--
2.33.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
2021-11-25 15:09 ` AngeloGioacchino Del Regno
@ 2021-11-25 15:23 ` Dmitry Baryshkov
-1 siblings, 0 replies; 36+ messages in thread
From: Dmitry Baryshkov @ 2021-11-25 15:23 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, robdclark
Cc: freedreno, airlied, linux-arm-msm, konrad.dybcio, linux-kernel,
dri-devel, jami.kettunen, maxime, marijn.suijten, kernel, sean
On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
> DSI host gets initialized earlier, but this caused unability to probe
> the entire stack of components because they all depend on interrupts
> coming from the main `mdss` node (mdp5, or dpu1).
>
> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
> them at msm_pdev_probe() time: this will make sure that we add the
> required interrupt controller mapping before dsi and/or other components
> try to initialize, finally satisfying the dependency.
>
> While at it, also change the allocation of msm_drm_private to use the
> devm variant of kzalloc().
>
> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/msm_drv.c | 81 ++++++++++++++++-------------------
> 1 file changed, 38 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 7936e8d498dd..790acf4993c0 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -512,45 +512,12 @@ static int msm_init_vram(struct drm_device *dev)
> static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> {
> struct platform_device *pdev = to_platform_device(dev);
> - struct drm_device *ddev;
> - struct msm_drm_private *priv;
> - struct msm_kms *kms;
> - struct msm_mdss *mdss;
> + struct drm_device *ddev = platform_get_drvdata(pdev);
> + struct msm_drm_private *priv = ddev->dev_private;
> + struct msm_kms *kms = priv->kms;
> + struct msm_mdss *mdss = priv->mdss;
> int ret, i;
>
> - ddev = drm_dev_alloc(drv, dev);
> - if (IS_ERR(ddev)) {
> - DRM_DEV_ERROR(dev, "failed to allocate drm_device\n");
> - return PTR_ERR(ddev);
> - }
> -
> - platform_set_drvdata(pdev, ddev);
> -
> - priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> - if (!priv) {
> - ret = -ENOMEM;
> - goto err_put_drm_dev;
> - }
> -
> - ddev->dev_private = priv;
> - priv->dev = ddev;
> -
> - switch (get_mdp_ver(pdev)) {
> - case KMS_MDP5:
> - ret = mdp5_mdss_init(ddev);
> - break;
> - case KMS_DPU:
> - ret = dpu_mdss_init(ddev);
> - break;
> - default:
> - ret = 0;
> - break;
> - }
> - if (ret)
> - goto err_free_priv;
> -
> - mdss = priv->mdss;
> -
> priv->wq = alloc_ordered_workqueue("msm", 0);
> priv->hangcheck_period = DRM_MSM_HANGCHECK_DEFAULT_PERIOD;
>
> @@ -685,11 +652,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> err_destroy_mdss:
> if (mdss && mdss->funcs)
> mdss->funcs->destroy(ddev);
> -err_free_priv:
> - kfree(priv);
> -err_put_drm_dev:
> - drm_dev_put(ddev);
> - platform_set_drvdata(pdev, NULL);
> return ret;
> }
>
> @@ -1382,12 +1344,42 @@ static const struct component_master_ops msm_drm_ops = {
> static int msm_pdev_probe(struct platform_device *pdev)
> {
> struct component_match *match = NULL;
> + struct msm_drm_private *priv;
> + struct drm_device *ddev;
> int ret;
>
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + ddev = drm_dev_alloc(&msm_driver, &pdev->dev);
> + if (IS_ERR(ddev)) {
> + DRM_DEV_ERROR(&pdev->dev, "failed to allocate drm_device\n");
> + return PTR_ERR(ddev);
> + }
> +
> + platform_set_drvdata(pdev, ddev);
> + ddev->dev_private = priv;
> + priv->dev = ddev;
> +
> + switch (get_mdp_ver(pdev)) {
> + case KMS_MDP5:
> + ret = mdp5_mdss_init(ddev);
> + break;
> + case KMS_DPU:
> + ret = dpu_mdss_init(ddev);
> + break;
> + default:
> + ret = 0;
> + break;
> + }
> + if (ret)
> + goto err_put_drm_dev;
> +
> if (get_mdp_ver(pdev)) {
> ret = add_display_components(pdev, &match);
> if (ret)
> - return ret;
> + goto fail;
> }
>
> ret = add_gpu_components(&pdev->dev, &match);
> @@ -1409,6 +1401,9 @@ static int msm_pdev_probe(struct platform_device *pdev)
>
> fail:
> of_platform_depopulate(&pdev->dev);
> +err_put_drm_dev:
> + drm_dev_put(ddev);
> + platform_set_drvdata(pdev, NULL);
> return ret;
> }
>
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
@ 2021-11-25 15:23 ` Dmitry Baryshkov
0 siblings, 0 replies; 36+ messages in thread
From: Dmitry Baryshkov @ 2021-11-25 15:23 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, robdclark
Cc: sean, airlied, daniel, maxime, linux-arm-msm, dri-devel,
freedreno, linux-kernel, kernel, konrad.dybcio, marijn.suijten,
jami.kettunen
On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
> DSI host gets initialized earlier, but this caused unability to probe
> the entire stack of components because they all depend on interrupts
> coming from the main `mdss` node (mdp5, or dpu1).
>
> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
> them at msm_pdev_probe() time: this will make sure that we add the
> required interrupt controller mapping before dsi and/or other components
> try to initialize, finally satisfying the dependency.
>
> While at it, also change the allocation of msm_drm_private to use the
> devm variant of kzalloc().
>
> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/msm_drv.c | 81 ++++++++++++++++-------------------
> 1 file changed, 38 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 7936e8d498dd..790acf4993c0 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -512,45 +512,12 @@ static int msm_init_vram(struct drm_device *dev)
> static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> {
> struct platform_device *pdev = to_platform_device(dev);
> - struct drm_device *ddev;
> - struct msm_drm_private *priv;
> - struct msm_kms *kms;
> - struct msm_mdss *mdss;
> + struct drm_device *ddev = platform_get_drvdata(pdev);
> + struct msm_drm_private *priv = ddev->dev_private;
> + struct msm_kms *kms = priv->kms;
> + struct msm_mdss *mdss = priv->mdss;
> int ret, i;
>
> - ddev = drm_dev_alloc(drv, dev);
> - if (IS_ERR(ddev)) {
> - DRM_DEV_ERROR(dev, "failed to allocate drm_device\n");
> - return PTR_ERR(ddev);
> - }
> -
> - platform_set_drvdata(pdev, ddev);
> -
> - priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> - if (!priv) {
> - ret = -ENOMEM;
> - goto err_put_drm_dev;
> - }
> -
> - ddev->dev_private = priv;
> - priv->dev = ddev;
> -
> - switch (get_mdp_ver(pdev)) {
> - case KMS_MDP5:
> - ret = mdp5_mdss_init(ddev);
> - break;
> - case KMS_DPU:
> - ret = dpu_mdss_init(ddev);
> - break;
> - default:
> - ret = 0;
> - break;
> - }
> - if (ret)
> - goto err_free_priv;
> -
> - mdss = priv->mdss;
> -
> priv->wq = alloc_ordered_workqueue("msm", 0);
> priv->hangcheck_period = DRM_MSM_HANGCHECK_DEFAULT_PERIOD;
>
> @@ -685,11 +652,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> err_destroy_mdss:
> if (mdss && mdss->funcs)
> mdss->funcs->destroy(ddev);
> -err_free_priv:
> - kfree(priv);
> -err_put_drm_dev:
> - drm_dev_put(ddev);
> - platform_set_drvdata(pdev, NULL);
> return ret;
> }
>
> @@ -1382,12 +1344,42 @@ static const struct component_master_ops msm_drm_ops = {
> static int msm_pdev_probe(struct platform_device *pdev)
> {
> struct component_match *match = NULL;
> + struct msm_drm_private *priv;
> + struct drm_device *ddev;
> int ret;
>
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + ddev = drm_dev_alloc(&msm_driver, &pdev->dev);
> + if (IS_ERR(ddev)) {
> + DRM_DEV_ERROR(&pdev->dev, "failed to allocate drm_device\n");
> + return PTR_ERR(ddev);
> + }
> +
> + platform_set_drvdata(pdev, ddev);
> + ddev->dev_private = priv;
> + priv->dev = ddev;
> +
> + switch (get_mdp_ver(pdev)) {
> + case KMS_MDP5:
> + ret = mdp5_mdss_init(ddev);
> + break;
> + case KMS_DPU:
> + ret = dpu_mdss_init(ddev);
> + break;
> + default:
> + ret = 0;
> + break;
> + }
> + if (ret)
> + goto err_put_drm_dev;
> +
> if (get_mdp_ver(pdev)) {
> ret = add_display_components(pdev, &match);
> if (ret)
> - return ret;
> + goto fail;
> }
>
> ret = add_gpu_components(&pdev->dev, &match);
> @@ -1409,6 +1401,9 @@ static int msm_pdev_probe(struct platform_device *pdev)
>
> fail:
> of_platform_depopulate(&pdev->dev);
> +err_put_drm_dev:
> + drm_dev_put(ddev);
> + platform_set_drvdata(pdev, NULL);
> return ret;
> }
>
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
2021-11-25 15:09 ` AngeloGioacchino Del Regno
@ 2021-11-25 22:39 ` Dmitry Baryshkov
-1 siblings, 0 replies; 36+ messages in thread
From: Dmitry Baryshkov @ 2021-11-25 22:39 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, robdclark
Cc: freedreno, airlied, linux-arm-msm, konrad.dybcio, linux-kernel,
dri-devel, jami.kettunen, maxime, marijn.suijten, kernel, sean
On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
> DSI host gets initialized earlier, but this caused unability to probe
> the entire stack of components because they all depend on interrupts
> coming from the main `mdss` node (mdp5, or dpu1).
>
> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
> them at msm_pdev_probe() time: this will make sure that we add the
> required interrupt controller mapping before dsi and/or other components
> try to initialize, finally satisfying the dependency.
>
> While at it, also change the allocation of msm_drm_private to use the
> devm variant of kzalloc().
>
> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
If device is not fully bound (e.g. DSI host could not bind the panel),
this patch causes the following oops on reboot:
[ 75.011942] Unable to handle kernel NULL pointer dereference at
virtual address 0000000000000030
[ 75.020974] Mem abort info:
[ 75.023859] ESR = 0x96000006
[ 75.027013] EC = 0x25: DABT (current EL), IL = 32 bits
[ 75.032480] SET = 0, FnV = 0
[ 75.035627] EA = 0, S1PTW = 0
[ 75.038861] FSC = 0x06: level 2 translation fault
[ 75.043876] Data abort info:
[ 75.046847] ISV = 0, ISS = 0x00000006
[ 75.050796] CM = 0, WnR = 0
[ 75.053857] user pgtable: 4k pages, 48-bit VAs, pgdp=00000001102b3000
[ 75.060478] [0000000000000030] pgd=080000011035d003,
p4d=080000011035d003, pud=080000011035f003, pmd=0000000000000000
[ 75.071380] Internal error: Oops: 96000006 [#1] SMP
[ 75.076388] Modules linked in:
[ 75.079530] CPU: 0 PID: 1442 Comm: reboot Not tainted
5.16.0-rc1-00046-g2207fd610cf4-dirty #185
[ 75.088460] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
[ 75.095345] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS
BTYPE=--)
[ 75.102496] pc : drm_atomic_state_alloc+0x14/0x74
[ 75.107335] lr : drm_atomic_helper_disable_all+0x20/0x210
[ 75.112885] sp : ffff80001480bb70
[ 75.116298] x29: ffff80001480bb70 x28: ffff0c8753505400 x27:
0000000000000000
[ 75.123626] x26: ffff0c874097d890 x25: ffffaa357b610e00 x24:
0000000000000000
[ 75.130954] x23: ffffaa357bdaa030 x22: ffffaa357bdfd2d8 x21:
ffff80001480bbf8
[ 75.138282] x20: ffff0c87469bd800 x19: ffff0c87469bd800 x18:
ffffffffffffffff
[ 75.145608] x17: 000000000000000e x16: 0000000000000001 x15:
ffff80009480ba3d
[ 75.152934] x14: 0000000000000004 x13: 0000000000000000 x12:
ffff0c87452c1288
[ 75.160261] x11: 0000000000000003 x10: ffff0c87452c1240 x9 :
0000000000000001
[ 75.167588] x8 : ffff80001480bc38 x7 : 0000000000000000 x6 :
ffff0c874f63d300
[ 75.174914] x5 : 0000000000000000 x4 : ffffaa357b582d30 x3 :
0000000000000000
[ 75.182240] x2 : ffff80001480bc20 x1 : 0000000000000000 x0 :
ffff0c87469bd800
[ 75.189568] Call trace:
[ 75.192092] drm_atomic_state_alloc+0x14/0x74
[ 75.196571] drm_atomic_helper_disable_all+0x20/0x210
[ 75.201765] drm_atomic_helper_shutdown+0x80/0x130
[ 75.206683] msm_pdev_shutdown+0x2c/0x40
[ 75.210717] platform_shutdown+0x28/0x40
[ 75.214751] device_shutdown+0x15c/0x450
[ 75.218785] __do_sys_reboot+0x218/0x2a0
[ 75.222819] __arm64_sys_reboot+0x28/0x34
[ 75.226937] invoke_syscall+0x48/0x114
[ 75.230794] el0_svc_common.constprop.0+0xd4/0xfc
[ 75.235626] do_el0_svc+0x28/0x90
[ 75.239030] el0_svc+0x28/0x80
[ 75.242174] el0t_64_sync_handler+0xa4/0x130
[ 75.246567] el0t_64_sync+0x1a0/0x1a4
[ 75.250338] Code: a9be7bfd 910003fd a90153f3 f9418c01 (f9401821)
[ 75.256599] ---[ end trace d90b41486de58d22 ]---
The following patch fixes it:
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 41c6a9f9dd34..5a92417d21d0 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1435,7 +1435,7 @@ static void msm_pdev_shutdown(struct
struct drm_device *drm = platform_get_drvdata(pdev);
struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
- if (!priv || !priv->kms)
+ if (!priv || !priv->kms || !drm->mode_config.funcs)
return;
drm_atomic_helper_shutdown(drm);
> ---
> drivers/gpu/drm/msm/msm_drv.c | 81 ++++++++++++++++-------------------
> 1 file changed, 38 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 7936e8d498dd..790acf4993c0 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -512,45 +512,12 @@ static int msm_init_vram(struct drm_device *dev)
> static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> {
> struct platform_device *pdev = to_platform_device(dev);
> - struct drm_device *ddev;
> - struct msm_drm_private *priv;
> - struct msm_kms *kms;
> - struct msm_mdss *mdss;
> + struct drm_device *ddev = platform_get_drvdata(pdev);
> + struct msm_drm_private *priv = ddev->dev_private;
> + struct msm_kms *kms = priv->kms;
> + struct msm_mdss *mdss = priv->mdss;
> int ret, i;
>
> - ddev = drm_dev_alloc(drv, dev);
> - if (IS_ERR(ddev)) {
> - DRM_DEV_ERROR(dev, "failed to allocate drm_device\n");
> - return PTR_ERR(ddev);
> - }
> -
> - platform_set_drvdata(pdev, ddev);
> -
> - priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> - if (!priv) {
> - ret = -ENOMEM;
> - goto err_put_drm_dev;
> - }
> -
> - ddev->dev_private = priv;
> - priv->dev = ddev;
> -
> - switch (get_mdp_ver(pdev)) {
> - case KMS_MDP5:
> - ret = mdp5_mdss_init(ddev);
> - break;
> - case KMS_DPU:
> - ret = dpu_mdss_init(ddev);
> - break;
> - default:
> - ret = 0;
> - break;
> - }
> - if (ret)
> - goto err_free_priv;
> -
> - mdss = priv->mdss;
> -
> priv->wq = alloc_ordered_workqueue("msm", 0);
> priv->hangcheck_period = DRM_MSM_HANGCHECK_DEFAULT_PERIOD;
>
> @@ -685,11 +652,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> err_destroy_mdss:
> if (mdss && mdss->funcs)
> mdss->funcs->destroy(ddev);
> -err_free_priv:
> - kfree(priv);
> -err_put_drm_dev:
> - drm_dev_put(ddev);
> - platform_set_drvdata(pdev, NULL);
> return ret;
> }
>
> @@ -1382,12 +1344,42 @@ static const struct component_master_ops msm_drm_ops = {
> static int msm_pdev_probe(struct platform_device *pdev)
> {
> struct component_match *match = NULL;
> + struct msm_drm_private *priv;
> + struct drm_device *ddev;
> int ret;
>
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + ddev = drm_dev_alloc(&msm_driver, &pdev->dev);
> + if (IS_ERR(ddev)) {
> + DRM_DEV_ERROR(&pdev->dev, "failed to allocate drm_device\n");
> + return PTR_ERR(ddev);
> + }
> +
> + platform_set_drvdata(pdev, ddev);
> + ddev->dev_private = priv;
> + priv->dev = ddev;
> +
> + switch (get_mdp_ver(pdev)) {
> + case KMS_MDP5:
> + ret = mdp5_mdss_init(ddev);
> + break;
> + case KMS_DPU:
> + ret = dpu_mdss_init(ddev);
> + break;
> + default:
> + ret = 0;
> + break;
> + }
> + if (ret)
> + goto err_put_drm_dev;
> +
> if (get_mdp_ver(pdev)) {
> ret = add_display_components(pdev, &match);
> if (ret)
> - return ret;
> + goto fail;
> }
>
> ret = add_gpu_components(&pdev->dev, &match);
> @@ -1409,6 +1401,9 @@ static int msm_pdev_probe(struct platform_device *pdev)
>
> fail:
> of_platform_depopulate(&pdev->dev);
> +err_put_drm_dev:
> + drm_dev_put(ddev);
> + platform_set_drvdata(pdev, NULL);
> return ret;
> }
>
>
--
With best wishes
Dmitry
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
@ 2021-11-25 22:39 ` Dmitry Baryshkov
0 siblings, 0 replies; 36+ messages in thread
From: Dmitry Baryshkov @ 2021-11-25 22:39 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, robdclark
Cc: sean, airlied, daniel, maxime, linux-arm-msm, dri-devel,
freedreno, linux-kernel, kernel, konrad.dybcio, marijn.suijten,
jami.kettunen
On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
> DSI host gets initialized earlier, but this caused unability to probe
> the entire stack of components because they all depend on interrupts
> coming from the main `mdss` node (mdp5, or dpu1).
>
> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
> them at msm_pdev_probe() time: this will make sure that we add the
> required interrupt controller mapping before dsi and/or other components
> try to initialize, finally satisfying the dependency.
>
> While at it, also change the allocation of msm_drm_private to use the
> devm variant of kzalloc().
>
> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
If device is not fully bound (e.g. DSI host could not bind the panel),
this patch causes the following oops on reboot:
[ 75.011942] Unable to handle kernel NULL pointer dereference at
virtual address 0000000000000030
[ 75.020974] Mem abort info:
[ 75.023859] ESR = 0x96000006
[ 75.027013] EC = 0x25: DABT (current EL), IL = 32 bits
[ 75.032480] SET = 0, FnV = 0
[ 75.035627] EA = 0, S1PTW = 0
[ 75.038861] FSC = 0x06: level 2 translation fault
[ 75.043876] Data abort info:
[ 75.046847] ISV = 0, ISS = 0x00000006
[ 75.050796] CM = 0, WnR = 0
[ 75.053857] user pgtable: 4k pages, 48-bit VAs, pgdp=00000001102b3000
[ 75.060478] [0000000000000030] pgd=080000011035d003,
p4d=080000011035d003, pud=080000011035f003, pmd=0000000000000000
[ 75.071380] Internal error: Oops: 96000006 [#1] SMP
[ 75.076388] Modules linked in:
[ 75.079530] CPU: 0 PID: 1442 Comm: reboot Not tainted
5.16.0-rc1-00046-g2207fd610cf4-dirty #185
[ 75.088460] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
[ 75.095345] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS
BTYPE=--)
[ 75.102496] pc : drm_atomic_state_alloc+0x14/0x74
[ 75.107335] lr : drm_atomic_helper_disable_all+0x20/0x210
[ 75.112885] sp : ffff80001480bb70
[ 75.116298] x29: ffff80001480bb70 x28: ffff0c8753505400 x27:
0000000000000000
[ 75.123626] x26: ffff0c874097d890 x25: ffffaa357b610e00 x24:
0000000000000000
[ 75.130954] x23: ffffaa357bdaa030 x22: ffffaa357bdfd2d8 x21:
ffff80001480bbf8
[ 75.138282] x20: ffff0c87469bd800 x19: ffff0c87469bd800 x18:
ffffffffffffffff
[ 75.145608] x17: 000000000000000e x16: 0000000000000001 x15:
ffff80009480ba3d
[ 75.152934] x14: 0000000000000004 x13: 0000000000000000 x12:
ffff0c87452c1288
[ 75.160261] x11: 0000000000000003 x10: ffff0c87452c1240 x9 :
0000000000000001
[ 75.167588] x8 : ffff80001480bc38 x7 : 0000000000000000 x6 :
ffff0c874f63d300
[ 75.174914] x5 : 0000000000000000 x4 : ffffaa357b582d30 x3 :
0000000000000000
[ 75.182240] x2 : ffff80001480bc20 x1 : 0000000000000000 x0 :
ffff0c87469bd800
[ 75.189568] Call trace:
[ 75.192092] drm_atomic_state_alloc+0x14/0x74
[ 75.196571] drm_atomic_helper_disable_all+0x20/0x210
[ 75.201765] drm_atomic_helper_shutdown+0x80/0x130
[ 75.206683] msm_pdev_shutdown+0x2c/0x40
[ 75.210717] platform_shutdown+0x28/0x40
[ 75.214751] device_shutdown+0x15c/0x450
[ 75.218785] __do_sys_reboot+0x218/0x2a0
[ 75.222819] __arm64_sys_reboot+0x28/0x34
[ 75.226937] invoke_syscall+0x48/0x114
[ 75.230794] el0_svc_common.constprop.0+0xd4/0xfc
[ 75.235626] do_el0_svc+0x28/0x90
[ 75.239030] el0_svc+0x28/0x80
[ 75.242174] el0t_64_sync_handler+0xa4/0x130
[ 75.246567] el0t_64_sync+0x1a0/0x1a4
[ 75.250338] Code: a9be7bfd 910003fd a90153f3 f9418c01 (f9401821)
[ 75.256599] ---[ end trace d90b41486de58d22 ]---
The following patch fixes it:
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 41c6a9f9dd34..5a92417d21d0 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1435,7 +1435,7 @@ static void msm_pdev_shutdown(struct
struct drm_device *drm = platform_get_drvdata(pdev);
struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
- if (!priv || !priv->kms)
+ if (!priv || !priv->kms || !drm->mode_config.funcs)
return;
drm_atomic_helper_shutdown(drm);
> ---
> drivers/gpu/drm/msm/msm_drv.c | 81 ++++++++++++++++-------------------
> 1 file changed, 38 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 7936e8d498dd..790acf4993c0 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -512,45 +512,12 @@ static int msm_init_vram(struct drm_device *dev)
> static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> {
> struct platform_device *pdev = to_platform_device(dev);
> - struct drm_device *ddev;
> - struct msm_drm_private *priv;
> - struct msm_kms *kms;
> - struct msm_mdss *mdss;
> + struct drm_device *ddev = platform_get_drvdata(pdev);
> + struct msm_drm_private *priv = ddev->dev_private;
> + struct msm_kms *kms = priv->kms;
> + struct msm_mdss *mdss = priv->mdss;
> int ret, i;
>
> - ddev = drm_dev_alloc(drv, dev);
> - if (IS_ERR(ddev)) {
> - DRM_DEV_ERROR(dev, "failed to allocate drm_device\n");
> - return PTR_ERR(ddev);
> - }
> -
> - platform_set_drvdata(pdev, ddev);
> -
> - priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> - if (!priv) {
> - ret = -ENOMEM;
> - goto err_put_drm_dev;
> - }
> -
> - ddev->dev_private = priv;
> - priv->dev = ddev;
> -
> - switch (get_mdp_ver(pdev)) {
> - case KMS_MDP5:
> - ret = mdp5_mdss_init(ddev);
> - break;
> - case KMS_DPU:
> - ret = dpu_mdss_init(ddev);
> - break;
> - default:
> - ret = 0;
> - break;
> - }
> - if (ret)
> - goto err_free_priv;
> -
> - mdss = priv->mdss;
> -
> priv->wq = alloc_ordered_workqueue("msm", 0);
> priv->hangcheck_period = DRM_MSM_HANGCHECK_DEFAULT_PERIOD;
>
> @@ -685,11 +652,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> err_destroy_mdss:
> if (mdss && mdss->funcs)
> mdss->funcs->destroy(ddev);
> -err_free_priv:
> - kfree(priv);
> -err_put_drm_dev:
> - drm_dev_put(ddev);
> - platform_set_drvdata(pdev, NULL);
> return ret;
> }
>
> @@ -1382,12 +1344,42 @@ static const struct component_master_ops msm_drm_ops = {
> static int msm_pdev_probe(struct platform_device *pdev)
> {
> struct component_match *match = NULL;
> + struct msm_drm_private *priv;
> + struct drm_device *ddev;
> int ret;
>
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + ddev = drm_dev_alloc(&msm_driver, &pdev->dev);
> + if (IS_ERR(ddev)) {
> + DRM_DEV_ERROR(&pdev->dev, "failed to allocate drm_device\n");
> + return PTR_ERR(ddev);
> + }
> +
> + platform_set_drvdata(pdev, ddev);
> + ddev->dev_private = priv;
> + priv->dev = ddev;
> +
> + switch (get_mdp_ver(pdev)) {
> + case KMS_MDP5:
> + ret = mdp5_mdss_init(ddev);
> + break;
> + case KMS_DPU:
> + ret = dpu_mdss_init(ddev);
> + break;
> + default:
> + ret = 0;
> + break;
> + }
> + if (ret)
> + goto err_put_drm_dev;
> +
> if (get_mdp_ver(pdev)) {
> ret = add_display_components(pdev, &match);
> if (ret)
> - return ret;
> + goto fail;
> }
>
> ret = add_gpu_components(&pdev->dev, &match);
> @@ -1409,6 +1401,9 @@ static int msm_pdev_probe(struct platform_device *pdev)
>
> fail:
> of_platform_depopulate(&pdev->dev);
> +err_put_drm_dev:
> + drm_dev_put(ddev);
> + platform_set_drvdata(pdev, NULL);
> return ret;
> }
>
>
--
With best wishes
Dmitry
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
2021-11-25 15:09 ` AngeloGioacchino Del Regno
@ 2021-11-26 0:06 ` Dmitry Baryshkov
-1 siblings, 0 replies; 36+ messages in thread
From: Dmitry Baryshkov @ 2021-11-26 0:06 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, robdclark
Cc: freedreno, airlied, linux-arm-msm, konrad.dybcio, linux-kernel,
dri-devel, jami.kettunen, maxime, marijn.suijten, kernel, sean
On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
> DSI host gets initialized earlier, but this caused unability to probe
> the entire stack of components because they all depend on interrupts
> coming from the main `mdss` node (mdp5, or dpu1).
>
> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
> them at msm_pdev_probe() time: this will make sure that we add the
> required interrupt controller mapping before dsi and/or other components
> try to initialize, finally satisfying the dependency.
>
> While at it, also change the allocation of msm_drm_private to use the
> devm variant of kzalloc().
>
> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Another issue (or a pack of issues):
Now the msm_drm_init() is unbalanced with msm_drm_uninit(). Bits of code
(putting the drm dev, removing the IRQ domain, etc) have to be called
now from the msm_pdev_remove() function rather than from the unbind path.
The following changes fix the observed issues here, however additional
care should be taken.
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 5a92417d21d0..0abb16256b61 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -342,7 +342,6 @@ static int msm_drm_uninit(struct device *dev)
struct drm_device *ddev = platform_get_drvdata(pdev);
struct msm_drm_private *priv = ddev->dev_private;
struct msm_kms *kms = priv->kms;
- struct msm_mdss *mdss = priv->mdss;
int i;
/*
@@ -402,14 +401,7 @@ static int msm_drm_uninit(struct device *dev)
component_unbind_all(dev, ddev);
- if (mdss && mdss->funcs)
- mdss->funcs->destroy(ddev);
-
- ddev->dev_private = NULL;
- drm_dev_put(ddev);
-
destroy_workqueue(priv->wq);
- kfree(priv);
return 0;
}
@@ -515,7 +507,6 @@ static int msm_drm_init(struct device *dev, const
struct drm_device *ddev = platform_get_drvdata(pdev);
struct msm_drm_private *priv = ddev->dev_private;
struct msm_kms *kms = priv->kms;
- struct msm_mdss *mdss = priv->mdss;
int ret, i;
priv->wq = alloc_ordered_workqueue("msm", 0);
@@ -538,12 +529,12 @@ static int msm_drm_init(struct device *dev, const
ret = msm_init_vram(ddev);
if (ret)
- goto err_destroy_mdss;
+ return ret;
/* Bind all our sub-components: */
ret = component_bind_all(dev, ddev);
if (ret)
- goto err_destroy_mdss;
+ return ret;
dma_set_max_seg_size(dev, UINT_MAX);
@@ -649,10 +640,6 @@ static int msm_drm_init(struct device *dev, const
err_msm_uninit:
msm_drm_uninit(dev);
return ret;
-err_destroy_mdss:
- if (mdss && mdss->funcs)
- mdss->funcs->destroy(ddev);
- return ret;
}
/*
@@ -1424,9 +1411,20 @@ static int msm_pdev_probe(struct platform_device
static int msm_pdev_remove(struct platform_device *pdev)
{
+ struct drm_device *ddev = platform_get_drvdata(pdev);
+ struct msm_drm_private *priv = ddev->dev_private;
+ struct msm_mdss *mdss = priv->mdss;
+
component_master_del(&pdev->dev, &msm_drm_ops);
+
of_platform_depopulate(&pdev->dev);
+ if (mdss && mdss->funcs)
+ mdss->funcs->destroy(ddev);
+
+ ddev->dev_private = NULL;
+ drm_dev_put(ddev);
+
return 0;
}
> ---
> drivers/gpu/drm/msm/msm_drv.c | 81 ++++++++++++++++-------------------
> 1 file changed, 38 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 7936e8d498dd..790acf4993c0 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -512,45 +512,12 @@ static int msm_init_vram(struct drm_device *dev)
> static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> {
> struct platform_device *pdev = to_platform_device(dev);
> - struct drm_device *ddev;
> - struct msm_drm_private *priv;
> - struct msm_kms *kms;
> - struct msm_mdss *mdss;
> + struct drm_device *ddev = platform_get_drvdata(pdev);
> + struct msm_drm_private *priv = ddev->dev_private;
> + struct msm_kms *kms = priv->kms;
> + struct msm_mdss *mdss = priv->mdss;
> int ret, i;
>
> - ddev = drm_dev_alloc(drv, dev);
> - if (IS_ERR(ddev)) {
> - DRM_DEV_ERROR(dev, "failed to allocate drm_device\n");
> - return PTR_ERR(ddev);
> - }
> -
> - platform_set_drvdata(pdev, ddev);
> -
> - priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> - if (!priv) {
> - ret = -ENOMEM;
> - goto err_put_drm_dev;
> - }
> -
> - ddev->dev_private = priv;
> - priv->dev = ddev;
> -
> - switch (get_mdp_ver(pdev)) {
> - case KMS_MDP5:
> - ret = mdp5_mdss_init(ddev);
> - break;
> - case KMS_DPU:
> - ret = dpu_mdss_init(ddev);
> - break;
> - default:
> - ret = 0;
> - break;
> - }
> - if (ret)
> - goto err_free_priv;
> -
> - mdss = priv->mdss;
> -
> priv->wq = alloc_ordered_workqueue("msm", 0);
> priv->hangcheck_period = DRM_MSM_HANGCHECK_DEFAULT_PERIOD;
>
> @@ -685,11 +652,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> err_destroy_mdss:
> if (mdss && mdss->funcs)
> mdss->funcs->destroy(ddev);
> -err_free_priv:
> - kfree(priv);
> -err_put_drm_dev:
> - drm_dev_put(ddev);
> - platform_set_drvdata(pdev, NULL);
> return ret;
> }
>
> @@ -1382,12 +1344,42 @@ static const struct component_master_ops msm_drm_ops = {
> static int msm_pdev_probe(struct platform_device *pdev)
> {
> struct component_match *match = NULL;
> + struct msm_drm_private *priv;
> + struct drm_device *ddev;
> int ret;
>
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + ddev = drm_dev_alloc(&msm_driver, &pdev->dev);
> + if (IS_ERR(ddev)) {
> + DRM_DEV_ERROR(&pdev->dev, "failed to allocate drm_device\n");
> + return PTR_ERR(ddev);
> + }
> +
> + platform_set_drvdata(pdev, ddev);
> + ddev->dev_private = priv;
> + priv->dev = ddev;
> +
> + switch (get_mdp_ver(pdev)) {
> + case KMS_MDP5:
> + ret = mdp5_mdss_init(ddev);
> + break;
> + case KMS_DPU:
> + ret = dpu_mdss_init(ddev);
> + break;
> + default:
> + ret = 0;
> + break;
> + }
> + if (ret)
> + goto err_put_drm_dev;
> +
> if (get_mdp_ver(pdev)) {
> ret = add_display_components(pdev, &match);
> if (ret)
> - return ret;
> + goto fail;
> }
>
> ret = add_gpu_components(&pdev->dev, &match);
> @@ -1409,6 +1401,9 @@ static int msm_pdev_probe(struct platform_device *pdev)
>
> fail:
> of_platform_depopulate(&pdev->dev);
> +err_put_drm_dev:
> + drm_dev_put(ddev);
> + platform_set_drvdata(pdev, NULL);
> return ret;
> }
>
>
--
With best wishes
Dmitry
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
@ 2021-11-26 0:06 ` Dmitry Baryshkov
0 siblings, 0 replies; 36+ messages in thread
From: Dmitry Baryshkov @ 2021-11-26 0:06 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, robdclark
Cc: sean, airlied, daniel, maxime, linux-arm-msm, dri-devel,
freedreno, linux-kernel, kernel, konrad.dybcio, marijn.suijten,
jami.kettunen
On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
> DSI host gets initialized earlier, but this caused unability to probe
> the entire stack of components because they all depend on interrupts
> coming from the main `mdss` node (mdp5, or dpu1).
>
> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
> them at msm_pdev_probe() time: this will make sure that we add the
> required interrupt controller mapping before dsi and/or other components
> try to initialize, finally satisfying the dependency.
>
> While at it, also change the allocation of msm_drm_private to use the
> devm variant of kzalloc().
>
> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Another issue (or a pack of issues):
Now the msm_drm_init() is unbalanced with msm_drm_uninit(). Bits of code
(putting the drm dev, removing the IRQ domain, etc) have to be called
now from the msm_pdev_remove() function rather than from the unbind path.
The following changes fix the observed issues here, however additional
care should be taken.
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 5a92417d21d0..0abb16256b61 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -342,7 +342,6 @@ static int msm_drm_uninit(struct device *dev)
struct drm_device *ddev = platform_get_drvdata(pdev);
struct msm_drm_private *priv = ddev->dev_private;
struct msm_kms *kms = priv->kms;
- struct msm_mdss *mdss = priv->mdss;
int i;
/*
@@ -402,14 +401,7 @@ static int msm_drm_uninit(struct device *dev)
component_unbind_all(dev, ddev);
- if (mdss && mdss->funcs)
- mdss->funcs->destroy(ddev);
-
- ddev->dev_private = NULL;
- drm_dev_put(ddev);
-
destroy_workqueue(priv->wq);
- kfree(priv);
return 0;
}
@@ -515,7 +507,6 @@ static int msm_drm_init(struct device *dev, const
struct drm_device *ddev = platform_get_drvdata(pdev);
struct msm_drm_private *priv = ddev->dev_private;
struct msm_kms *kms = priv->kms;
- struct msm_mdss *mdss = priv->mdss;
int ret, i;
priv->wq = alloc_ordered_workqueue("msm", 0);
@@ -538,12 +529,12 @@ static int msm_drm_init(struct device *dev, const
ret = msm_init_vram(ddev);
if (ret)
- goto err_destroy_mdss;
+ return ret;
/* Bind all our sub-components: */
ret = component_bind_all(dev, ddev);
if (ret)
- goto err_destroy_mdss;
+ return ret;
dma_set_max_seg_size(dev, UINT_MAX);
@@ -649,10 +640,6 @@ static int msm_drm_init(struct device *dev, const
err_msm_uninit:
msm_drm_uninit(dev);
return ret;
-err_destroy_mdss:
- if (mdss && mdss->funcs)
- mdss->funcs->destroy(ddev);
- return ret;
}
/*
@@ -1424,9 +1411,20 @@ static int msm_pdev_probe(struct platform_device
static int msm_pdev_remove(struct platform_device *pdev)
{
+ struct drm_device *ddev = platform_get_drvdata(pdev);
+ struct msm_drm_private *priv = ddev->dev_private;
+ struct msm_mdss *mdss = priv->mdss;
+
component_master_del(&pdev->dev, &msm_drm_ops);
+
of_platform_depopulate(&pdev->dev);
+ if (mdss && mdss->funcs)
+ mdss->funcs->destroy(ddev);
+
+ ddev->dev_private = NULL;
+ drm_dev_put(ddev);
+
return 0;
}
> ---
> drivers/gpu/drm/msm/msm_drv.c | 81 ++++++++++++++++-------------------
> 1 file changed, 38 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 7936e8d498dd..790acf4993c0 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -512,45 +512,12 @@ static int msm_init_vram(struct drm_device *dev)
> static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> {
> struct platform_device *pdev = to_platform_device(dev);
> - struct drm_device *ddev;
> - struct msm_drm_private *priv;
> - struct msm_kms *kms;
> - struct msm_mdss *mdss;
> + struct drm_device *ddev = platform_get_drvdata(pdev);
> + struct msm_drm_private *priv = ddev->dev_private;
> + struct msm_kms *kms = priv->kms;
> + struct msm_mdss *mdss = priv->mdss;
> int ret, i;
>
> - ddev = drm_dev_alloc(drv, dev);
> - if (IS_ERR(ddev)) {
> - DRM_DEV_ERROR(dev, "failed to allocate drm_device\n");
> - return PTR_ERR(ddev);
> - }
> -
> - platform_set_drvdata(pdev, ddev);
> -
> - priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> - if (!priv) {
> - ret = -ENOMEM;
> - goto err_put_drm_dev;
> - }
> -
> - ddev->dev_private = priv;
> - priv->dev = ddev;
> -
> - switch (get_mdp_ver(pdev)) {
> - case KMS_MDP5:
> - ret = mdp5_mdss_init(ddev);
> - break;
> - case KMS_DPU:
> - ret = dpu_mdss_init(ddev);
> - break;
> - default:
> - ret = 0;
> - break;
> - }
> - if (ret)
> - goto err_free_priv;
> -
> - mdss = priv->mdss;
> -
> priv->wq = alloc_ordered_workqueue("msm", 0);
> priv->hangcheck_period = DRM_MSM_HANGCHECK_DEFAULT_PERIOD;
>
> @@ -685,11 +652,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> err_destroy_mdss:
> if (mdss && mdss->funcs)
> mdss->funcs->destroy(ddev);
> -err_free_priv:
> - kfree(priv);
> -err_put_drm_dev:
> - drm_dev_put(ddev);
> - platform_set_drvdata(pdev, NULL);
> return ret;
> }
>
> @@ -1382,12 +1344,42 @@ static const struct component_master_ops msm_drm_ops = {
> static int msm_pdev_probe(struct platform_device *pdev)
> {
> struct component_match *match = NULL;
> + struct msm_drm_private *priv;
> + struct drm_device *ddev;
> int ret;
>
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + ddev = drm_dev_alloc(&msm_driver, &pdev->dev);
> + if (IS_ERR(ddev)) {
> + DRM_DEV_ERROR(&pdev->dev, "failed to allocate drm_device\n");
> + return PTR_ERR(ddev);
> + }
> +
> + platform_set_drvdata(pdev, ddev);
> + ddev->dev_private = priv;
> + priv->dev = ddev;
> +
> + switch (get_mdp_ver(pdev)) {
> + case KMS_MDP5:
> + ret = mdp5_mdss_init(ddev);
> + break;
> + case KMS_DPU:
> + ret = dpu_mdss_init(ddev);
> + break;
> + default:
> + ret = 0;
> + break;
> + }
> + if (ret)
> + goto err_put_drm_dev;
> +
> if (get_mdp_ver(pdev)) {
> ret = add_display_components(pdev, &match);
> if (ret)
> - return ret;
> + goto fail;
> }
>
> ret = add_gpu_components(&pdev->dev, &match);
> @@ -1409,6 +1401,9 @@ static int msm_pdev_probe(struct platform_device *pdev)
>
> fail:
> of_platform_depopulate(&pdev->dev);
> +err_put_drm_dev:
> + drm_dev_put(ddev);
> + platform_set_drvdata(pdev, NULL);
> return ret;
> }
>
>
--
With best wishes
Dmitry
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
2021-11-26 0:06 ` Dmitry Baryshkov
@ 2021-11-26 9:26 ` AngeloGioacchino Del Regno
-1 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-11-26 9:26 UTC (permalink / raw)
To: Dmitry Baryshkov, robdclark
Cc: freedreno, airlied, linux-arm-msm, konrad.dybcio, linux-kernel,
dri-devel, jami.kettunen, maxime, marijn.suijten, kernel, sean
Il 26/11/21 01:06, Dmitry Baryshkov ha scritto:
> On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
>> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
>> DSI host gets initialized earlier, but this caused unability to probe
>> the entire stack of components because they all depend on interrupts
>> coming from the main `mdss` node (mdp5, or dpu1).
>>
>> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
>> them at msm_pdev_probe() time: this will make sure that we add the
>> required interrupt controller mapping before dsi and/or other components
>> try to initialize, finally satisfying the dependency.
>>
>> While at it, also change the allocation of msm_drm_private to use the
>> devm variant of kzalloc().
>>
>> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>
> Another issue (or a pack of issues):
> Now the msm_drm_init() is unbalanced with msm_drm_uninit(). Bits of code (putting
> the drm dev, removing the IRQ domain, etc) have to be called now from the
> msm_pdev_remove() function rather than from the unbind path.
>
> The following changes fix the observed issues here, however additional care should
> be taken.
>
Hello Dmitry,
thanks for the thorough review (and solutions!).
Are you going to push your changes on top, or should I send a V2?
Cheers,
- Angelo
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
@ 2021-11-26 9:26 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-11-26 9:26 UTC (permalink / raw)
To: Dmitry Baryshkov, robdclark
Cc: sean, airlied, daniel, maxime, linux-arm-msm, dri-devel,
freedreno, linux-kernel, kernel, konrad.dybcio, marijn.suijten,
jami.kettunen
Il 26/11/21 01:06, Dmitry Baryshkov ha scritto:
> On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
>> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
>> DSI host gets initialized earlier, but this caused unability to probe
>> the entire stack of components because they all depend on interrupts
>> coming from the main `mdss` node (mdp5, or dpu1).
>>
>> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
>> them at msm_pdev_probe() time: this will make sure that we add the
>> required interrupt controller mapping before dsi and/or other components
>> try to initialize, finally satisfying the dependency.
>>
>> While at it, also change the allocation of msm_drm_private to use the
>> devm variant of kzalloc().
>>
>> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>
> Another issue (or a pack of issues):
> Now the msm_drm_init() is unbalanced with msm_drm_uninit(). Bits of code (putting
> the drm dev, removing the IRQ domain, etc) have to be called now from the
> msm_pdev_remove() function rather than from the unbind path.
>
> The following changes fix the observed issues here, however additional care should
> be taken.
>
Hello Dmitry,
thanks for the thorough review (and solutions!).
Are you going to push your changes on top, or should I send a V2?
Cheers,
- Angelo
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
2021-11-26 9:26 ` AngeloGioacchino Del Regno
@ 2021-11-26 21:12 ` Dmitry Baryshkov
-1 siblings, 0 replies; 36+ messages in thread
From: Dmitry Baryshkov @ 2021-11-26 21:12 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, robdclark
Cc: freedreno, airlied, linux-arm-msm, konrad.dybcio, linux-kernel,
dri-devel, jami.kettunen, maxime, marijn.suijten, kernel, sean
On 26/11/2021 12:26, AngeloGioacchino Del Regno wrote:
> Il 26/11/21 01:06, Dmitry Baryshkov ha scritto:
>> On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
>>> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
>>> DSI host gets initialized earlier, but this caused unability to probe
>>> the entire stack of components because they all depend on interrupts
>>> coming from the main `mdss` node (mdp5, or dpu1).
>>>
>>> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
>>> them at msm_pdev_probe() time: this will make sure that we add the
>>> required interrupt controller mapping before dsi and/or other components
>>> try to initialize, finally satisfying the dependency.
>>>
>>> While at it, also change the allocation of msm_drm_private to use the
>>> devm variant of kzalloc().
>>>
>>> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
>>> Signed-off-by: AngeloGioacchino Del Regno
>>> <angelogioacchino.delregno@collabora.com>
>>
>> Another issue (or a pack of issues):
>> Now the msm_drm_init() is unbalanced with msm_drm_uninit(). Bits of
>> code (putting the drm dev, removing the IRQ domain, etc) have to be
>> called now from the msm_pdev_remove() function rather than from the
>> unbind path.
>>
>> The following changes fix the observed issues here, however additional
>> care should be taken.
>>
>
>
> Hello Dmitry,
>
> thanks for the thorough review (and solutions!).
> Are you going to push your changes on top, or should I send a V2?
Please send a v2. As you see, my suggestions have to be validated too
(and they were based on crashes/issues observed locally).
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
@ 2021-11-26 21:12 ` Dmitry Baryshkov
0 siblings, 0 replies; 36+ messages in thread
From: Dmitry Baryshkov @ 2021-11-26 21:12 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, robdclark
Cc: sean, airlied, daniel, maxime, linux-arm-msm, dri-devel,
freedreno, linux-kernel, kernel, konrad.dybcio, marijn.suijten,
jami.kettunen
On 26/11/2021 12:26, AngeloGioacchino Del Regno wrote:
> Il 26/11/21 01:06, Dmitry Baryshkov ha scritto:
>> On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
>>> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
>>> DSI host gets initialized earlier, but this caused unability to probe
>>> the entire stack of components because they all depend on interrupts
>>> coming from the main `mdss` node (mdp5, or dpu1).
>>>
>>> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
>>> them at msm_pdev_probe() time: this will make sure that we add the
>>> required interrupt controller mapping before dsi and/or other components
>>> try to initialize, finally satisfying the dependency.
>>>
>>> While at it, also change the allocation of msm_drm_private to use the
>>> devm variant of kzalloc().
>>>
>>> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
>>> Signed-off-by: AngeloGioacchino Del Regno
>>> <angelogioacchino.delregno@collabora.com>
>>
>> Another issue (or a pack of issues):
>> Now the msm_drm_init() is unbalanced with msm_drm_uninit(). Bits of
>> code (putting the drm dev, removing the IRQ domain, etc) have to be
>> called now from the msm_pdev_remove() function rather than from the
>> unbind path.
>>
>> The following changes fix the observed issues here, however additional
>> care should be taken.
>>
>
>
> Hello Dmitry,
>
> thanks for the thorough review (and solutions!).
> Are you going to push your changes on top, or should I send a V2?
Please send a v2. As you see, my suggestions have to be validated too
(and they were based on crashes/issues observed locally).
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
2021-11-26 0:06 ` Dmitry Baryshkov
@ 2021-11-26 16:08 ` AngeloGioacchino Del Regno
-1 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-11-26 16:08 UTC (permalink / raw)
To: Dmitry Baryshkov, robdclark
Cc: freedreno, airlied, linux-arm-msm, konrad.dybcio, linux-kernel,
dri-devel, jami.kettunen, maxime, marijn.suijten, kernel, sean
Il 26/11/21 01:06, Dmitry Baryshkov ha scritto:
> On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
>> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
>> DSI host gets initialized earlier, but this caused unability to probe
>> the entire stack of components because they all depend on interrupts
>> coming from the main `mdss` node (mdp5, or dpu1).
>>
>> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
>> them at msm_pdev_probe() time: this will make sure that we add the
>> required interrupt controller mapping before dsi and/or other components
>> try to initialize, finally satisfying the dependency.
>>
>> While at it, also change the allocation of msm_drm_private to use the
>> devm variant of kzalloc().
>>
>> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>
> Another issue (or a pack of issues):
> Now the msm_drm_init() is unbalanced with msm_drm_uninit(). Bits of code (putting
> the drm dev, removing the IRQ domain, etc) have to be called now from the
> msm_pdev_remove() function rather than from the unbind path.
>
> The following changes fix the observed issues here, however additional care should
> be taken.
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 5a92417d21d0..0abb16256b61 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -342,7 +342,6 @@ static int msm_drm_uninit(struct device *dev)
> struct drm_device *ddev = platform_get_drvdata(pdev);
> struct msm_drm_private *priv = ddev->dev_private;
> struct msm_kms *kms = priv->kms;
> - struct msm_mdss *mdss = priv->mdss;
> int i;
>
> /*
> @@ -402,14 +401,7 @@ static int msm_drm_uninit(struct device *dev)
>
> component_unbind_all(dev, ddev);
>
> - if (mdss && mdss->funcs)
> - mdss->funcs->destroy(ddev);
> -
> - ddev->dev_private = NULL;
> - drm_dev_put(ddev);
> -
> destroy_workqueue(priv->wq);
> - kfree(priv);
>
> return 0;
> }
> @@ -515,7 +507,6 @@ static int msm_drm_init(struct device *dev, const
> struct drm_device *ddev = platform_get_drvdata(pdev);
> struct msm_drm_private *priv = ddev->dev_private;
> struct msm_kms *kms = priv->kms;
> - struct msm_mdss *mdss = priv->mdss;
> int ret, i;
>
> priv->wq = alloc_ordered_workqueue("msm", 0);
> @@ -538,12 +529,12 @@ static int msm_drm_init(struct device *dev, const
>
> ret = msm_init_vram(ddev);
> if (ret)
> - goto err_destroy_mdss;
> + return ret;
>
> /* Bind all our sub-components: */
> ret = component_bind_all(dev, ddev);
> if (ret)
> - goto err_destroy_mdss;
> + return ret;
>
> dma_set_max_seg_size(dev, UINT_MAX);
>
> @@ -649,10 +640,6 @@ static int msm_drm_init(struct device *dev, const
> err_msm_uninit:
> msm_drm_uninit(dev);
> return ret;
> -err_destroy_mdss:
> - if (mdss && mdss->funcs)
> - mdss->funcs->destroy(ddev);
> - return ret;
> }
>
> /*
> @@ -1424,9 +1411,20 @@ static int msm_pdev_probe(struct platform_device
>
> static int msm_pdev_remove(struct platform_device *pdev)
> {
> + struct drm_device *ddev = platform_get_drvdata(pdev);
> + struct msm_drm_private *priv = ddev->dev_private;
> + struct msm_mdss *mdss = priv->mdss;
> +
> component_master_del(&pdev->dev, &msm_drm_ops);
> +
> of_platform_depopulate(&pdev->dev);
>
> + if (mdss && mdss->funcs)
> + mdss->funcs->destroy(ddev);
> +
> + ddev->dev_private = NULL;
> + drm_dev_put(ddev);
> +
> return 0;
> }
>
>
>
Hello,
I had a chance to get back to this patch... and there's a bit more to do...
Applying your suggestion makes the kernel crash when removing the DSI panel:
[ 92.084668] Unable to handle kernel paging request at virtual address
ffffdd7f137945d8
[ 92.092848] Mem abort info:
[ 92.095758] ESR = 0x96000007
[ 92.098918] EC = 0x25: DABT (current EL), IL = 32 bits
[ 92.104395] SET = 0, FnV = 0
[ 92.107545] EA = 0, S1PTW = 0
[ 92.110785] FSC = 0x07: level 3 translation fault
[ 92.115802] Data abort info:
[ 92.118767] ISV = 0, ISS = 0x00000007
[ 92.122720] CM = 0, WnR = 0
[ 92.125770] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000082466000
[ 92.132668] [ffffdd7f137945d8] pgd=100000017ffff003, p4d=100000017ffff003,
pud=10000001034bb003, pmd=1000000108ec2003, pte=0000000000000000
[ 92.145530] Internal error: Oops: 96000007 [#1] SMP
[ 92.150557] Modules linked in: af_alg ipv6 uvcvideo videobuf2_vmalloc
snd_soc_hdmi_codec venus_enc venus_dec videobuf2_dma_contig videobuf2_memops
cdc_ether usbnet hci_uart ath10k_snoc msm venus_core r8152 ath10k_core btqca btbcm
v4l2_mem2mem ti_sn65dsi86(-) ath gpu_sched videobuf2_v4l2 sx9310 cros_ec_typec
drm_dp_aux_bus bluetooth mac80211 snd_soc_rt5682_i2c qrtr typec drm_kms_helper
sbs_battery snd_soc_rt5682 videobuf2_common cros_usbpd_charger cros_usbpd_logger
cros_ec_chardev elan_i2c pwm_cros_ec industrialio_triggered_buffer qcom_q6v5_mss
videodev snd_soc_rl6231 kfifo_buf qcom_spmi_adc5 drm snd_soc_sc7180
qcom_vadc_common qcom_pil_info qcom_spmi_temp_alarm ecdh_generic crct10dif_ce
libarc4 mc snd_soc_qcom_common ecc qcom_stats qcom_q6v5 ipa cfg80211
snd_soc_lpass_sc7180 i2c_qcom_geni reset_qcom_pdc qcom_sysmon dispcc_sc7180
spi_geni_qcom snd_soc_lpass_hdmi videocc_sc7180 qcom_common qcom_glink_smem
snd_soc_lpass_cpu spi_qcom_qspi lpasscorecc_sc7180 qmi_helpers gpucc_sc7180
[ 92.150710] snd_soc_lpass_platform icc_osm_l3 mdt_loader qcom_wdt
snd_soc_max98357a socinfo rmtfs_mem pwm_bl rfkill uinput btrfs blake2b_generic
libcrc32c xor xor_neon raid6_pq zstd_compress cuse fuse [last unloaded: panel_edp]
[ 92.239499] CPU: 2 PID: 1627 Comm: rmmod Not tainted 5.16.0-rc2-next-20211125+ #29
[ 92.239508] Hardware name: Google Lazor Limozeen without Touchscreen (rev5 -
rev8) (DT)
[ 92.239512] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 92.239518] pc : drm_panel_disable+0x80/0xe0 [drm]
[ 92.268628] rfkill: input handler enabled
[ 92.276336] lr : drm_panel_disable+0x6c/0xe0 [drm]
[ 92.297469] sp : ffff80000c7c38a0
[ 92.297474] x29: ffff80000c7c38a0 x28: ffffdd7f136e6e68 x27: ffff280ed1801400
[ 92.308207] x26: ffff280ec46c5080 x25: ffff280ec46b3880 x24: ffffdd7f1341e6f8
[ 92.315534] x23: 0000000000000038 x22: ffff280ec46c50d8 x21: ffff280ec0a28e00
[ 92.322858] x20: 0000000000000000 x19: ffff280ec3de7c80 x18: 0000000000000020
[ 92.330191] x17: 0000000000000000 x16: ffffdd7f4b7acec0 x15: 0000000000000000
[ 92.337531] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
[ 92.344859] x11: 0000000000000000 x10: 0000000000000000 x9 : ffffdd7f13335000
[ 92.352182] x8 : ffff280f03fed280 x7 : 0000000000000001 x6 : ffff280edf1c2600
[ 92.359504] x5 : 0000000000000002 x4 : ffff280f03fed280 x3 : ffff280ec4689820
[ 92.366835] x2 : 0000000000000000 x1 : ffffdd7f137945c8 x0 : 0000000000000000
[ 92.374159] Call trace:
[ 92.376682] drm_panel_disable+0x80/0xe0 [drm]
[ 92.381367] panel_bridge_disable+0x18/0x2c [drm_kms_helper]
[ 92.387281] drm_atomic_bridge_chain_disable+0x98/0xd0 [drm]
[ 92.393194] disable_outputs+0xfc/0x31c [drm_kms_helper]
[ 92.398738] drm_atomic_helper_commit_modeset_disables+0x20/0x50 [drm_kms_helper]
[ 92.406482] msm_atomic_commit_tail+0x188/0x500 [msm]
[ 92.411772] commit_tail+0xa4/0x184 [drm_kms_helper]
[ 92.416954] drm_atomic_helper_commit+0x164/0x3fc [drm_kms_helper]
[ 92.423373] drm_atomic_commit+0x50/0x60 [drm]
[ 92.428064] drm_atomic_helper_disable_all+0x1f8/0x20c [drm_kms_helper]
[ 92.434989] drm_atomic_helper_shutdown+0x80/0x130 [drm_kms_helper]
[ 92.441497] msm_drm_uninit.isra.0+0x14c/0x174 [msm]
[ 92.446729] msm_drm_unbind+0x14/0x20 [msm]
[ 92.451125] component_del+0xa8/0x160
[ 92.454898] dsi_dev_detach+0x24/0x30 [msm]
[ 92.459294] dsi_host_detach+0x20/0x64 [msm]
[ 92.463764] devm_mipi_dsi_detach+0x2c/0x40
[ 92.468069] devm_action_release+0x18/0x24
[ 92.472278] devres_release_group+0x100/0x1b0
[ 92.476755] i2c_device_remove+0x48/0xf0
[ 92.480790] __device_release_driver+0x188/0x23c
[ 92.485534] driver_detach+0xfc/0x1e0
[ 92.489303] bus_remove_driver+0x5c/0xd0
[ 92.493333] driver_unregister+0x34/0x64
[ 92.497363] i2c_del_driver+0x58/0x70
[ 92.501134] ti_sn65dsi86_exit+0x44/0x98c [ti_sn65dsi86]
[ 92.506611] __arm64_sys_delete_module+0x198/0x22c
[ 92.511535] invoke_syscall+0x48/0x114
[ 92.515389] el0_svc_common.constprop.0+0x44/0xec
[ 92.520223] do_el0_svc+0x28/0x90
[ 92.523629] el0_svc+0x20/0x60
[ 92.526780] el0t_64_sync_handler+0xec/0xf0
[ 92.531085] el0t_64_sync+0x1a0/0x1a4
[ 92.534879] Code: f94013f5 52800000 f9400a61 b40000a1 (f9400821)
[ 92.541134] ---[ end trace 1bc553757c40a199 ]---
I'll look into this.
In the meanwhile, if anyone has any suggestion before I solve this issue,
as to speed up getting this fix done (as it's pretty much critical), you're
welcome.
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
@ 2021-11-26 16:08 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-11-26 16:08 UTC (permalink / raw)
To: Dmitry Baryshkov, robdclark
Cc: sean, airlied, daniel, maxime, linux-arm-msm, dri-devel,
freedreno, linux-kernel, kernel, konrad.dybcio, marijn.suijten,
jami.kettunen
Il 26/11/21 01:06, Dmitry Baryshkov ha scritto:
> On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
>> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
>> DSI host gets initialized earlier, but this caused unability to probe
>> the entire stack of components because they all depend on interrupts
>> coming from the main `mdss` node (mdp5, or dpu1).
>>
>> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
>> them at msm_pdev_probe() time: this will make sure that we add the
>> required interrupt controller mapping before dsi and/or other components
>> try to initialize, finally satisfying the dependency.
>>
>> While at it, also change the allocation of msm_drm_private to use the
>> devm variant of kzalloc().
>>
>> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>
> Another issue (or a pack of issues):
> Now the msm_drm_init() is unbalanced with msm_drm_uninit(). Bits of code (putting
> the drm dev, removing the IRQ domain, etc) have to be called now from the
> msm_pdev_remove() function rather than from the unbind path.
>
> The following changes fix the observed issues here, however additional care should
> be taken.
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 5a92417d21d0..0abb16256b61 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -342,7 +342,6 @@ static int msm_drm_uninit(struct device *dev)
> struct drm_device *ddev = platform_get_drvdata(pdev);
> struct msm_drm_private *priv = ddev->dev_private;
> struct msm_kms *kms = priv->kms;
> - struct msm_mdss *mdss = priv->mdss;
> int i;
>
> /*
> @@ -402,14 +401,7 @@ static int msm_drm_uninit(struct device *dev)
>
> component_unbind_all(dev, ddev);
>
> - if (mdss && mdss->funcs)
> - mdss->funcs->destroy(ddev);
> -
> - ddev->dev_private = NULL;
> - drm_dev_put(ddev);
> -
> destroy_workqueue(priv->wq);
> - kfree(priv);
>
> return 0;
> }
> @@ -515,7 +507,6 @@ static int msm_drm_init(struct device *dev, const
> struct drm_device *ddev = platform_get_drvdata(pdev);
> struct msm_drm_private *priv = ddev->dev_private;
> struct msm_kms *kms = priv->kms;
> - struct msm_mdss *mdss = priv->mdss;
> int ret, i;
>
> priv->wq = alloc_ordered_workqueue("msm", 0);
> @@ -538,12 +529,12 @@ static int msm_drm_init(struct device *dev, const
>
> ret = msm_init_vram(ddev);
> if (ret)
> - goto err_destroy_mdss;
> + return ret;
>
> /* Bind all our sub-components: */
> ret = component_bind_all(dev, ddev);
> if (ret)
> - goto err_destroy_mdss;
> + return ret;
>
> dma_set_max_seg_size(dev, UINT_MAX);
>
> @@ -649,10 +640,6 @@ static int msm_drm_init(struct device *dev, const
> err_msm_uninit:
> msm_drm_uninit(dev);
> return ret;
> -err_destroy_mdss:
> - if (mdss && mdss->funcs)
> - mdss->funcs->destroy(ddev);
> - return ret;
> }
>
> /*
> @@ -1424,9 +1411,20 @@ static int msm_pdev_probe(struct platform_device
>
> static int msm_pdev_remove(struct platform_device *pdev)
> {
> + struct drm_device *ddev = platform_get_drvdata(pdev);
> + struct msm_drm_private *priv = ddev->dev_private;
> + struct msm_mdss *mdss = priv->mdss;
> +
> component_master_del(&pdev->dev, &msm_drm_ops);
> +
> of_platform_depopulate(&pdev->dev);
>
> + if (mdss && mdss->funcs)
> + mdss->funcs->destroy(ddev);
> +
> + ddev->dev_private = NULL;
> + drm_dev_put(ddev);
> +
> return 0;
> }
>
>
>
Hello,
I had a chance to get back to this patch... and there's a bit more to do...
Applying your suggestion makes the kernel crash when removing the DSI panel:
[ 92.084668] Unable to handle kernel paging request at virtual address
ffffdd7f137945d8
[ 92.092848] Mem abort info:
[ 92.095758] ESR = 0x96000007
[ 92.098918] EC = 0x25: DABT (current EL), IL = 32 bits
[ 92.104395] SET = 0, FnV = 0
[ 92.107545] EA = 0, S1PTW = 0
[ 92.110785] FSC = 0x07: level 3 translation fault
[ 92.115802] Data abort info:
[ 92.118767] ISV = 0, ISS = 0x00000007
[ 92.122720] CM = 0, WnR = 0
[ 92.125770] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000082466000
[ 92.132668] [ffffdd7f137945d8] pgd=100000017ffff003, p4d=100000017ffff003,
pud=10000001034bb003, pmd=1000000108ec2003, pte=0000000000000000
[ 92.145530] Internal error: Oops: 96000007 [#1] SMP
[ 92.150557] Modules linked in: af_alg ipv6 uvcvideo videobuf2_vmalloc
snd_soc_hdmi_codec venus_enc venus_dec videobuf2_dma_contig videobuf2_memops
cdc_ether usbnet hci_uart ath10k_snoc msm venus_core r8152 ath10k_core btqca btbcm
v4l2_mem2mem ti_sn65dsi86(-) ath gpu_sched videobuf2_v4l2 sx9310 cros_ec_typec
drm_dp_aux_bus bluetooth mac80211 snd_soc_rt5682_i2c qrtr typec drm_kms_helper
sbs_battery snd_soc_rt5682 videobuf2_common cros_usbpd_charger cros_usbpd_logger
cros_ec_chardev elan_i2c pwm_cros_ec industrialio_triggered_buffer qcom_q6v5_mss
videodev snd_soc_rl6231 kfifo_buf qcom_spmi_adc5 drm snd_soc_sc7180
qcom_vadc_common qcom_pil_info qcom_spmi_temp_alarm ecdh_generic crct10dif_ce
libarc4 mc snd_soc_qcom_common ecc qcom_stats qcom_q6v5 ipa cfg80211
snd_soc_lpass_sc7180 i2c_qcom_geni reset_qcom_pdc qcom_sysmon dispcc_sc7180
spi_geni_qcom snd_soc_lpass_hdmi videocc_sc7180 qcom_common qcom_glink_smem
snd_soc_lpass_cpu spi_qcom_qspi lpasscorecc_sc7180 qmi_helpers gpucc_sc7180
[ 92.150710] snd_soc_lpass_platform icc_osm_l3 mdt_loader qcom_wdt
snd_soc_max98357a socinfo rmtfs_mem pwm_bl rfkill uinput btrfs blake2b_generic
libcrc32c xor xor_neon raid6_pq zstd_compress cuse fuse [last unloaded: panel_edp]
[ 92.239499] CPU: 2 PID: 1627 Comm: rmmod Not tainted 5.16.0-rc2-next-20211125+ #29
[ 92.239508] Hardware name: Google Lazor Limozeen without Touchscreen (rev5 -
rev8) (DT)
[ 92.239512] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 92.239518] pc : drm_panel_disable+0x80/0xe0 [drm]
[ 92.268628] rfkill: input handler enabled
[ 92.276336] lr : drm_panel_disable+0x6c/0xe0 [drm]
[ 92.297469] sp : ffff80000c7c38a0
[ 92.297474] x29: ffff80000c7c38a0 x28: ffffdd7f136e6e68 x27: ffff280ed1801400
[ 92.308207] x26: ffff280ec46c5080 x25: ffff280ec46b3880 x24: ffffdd7f1341e6f8
[ 92.315534] x23: 0000000000000038 x22: ffff280ec46c50d8 x21: ffff280ec0a28e00
[ 92.322858] x20: 0000000000000000 x19: ffff280ec3de7c80 x18: 0000000000000020
[ 92.330191] x17: 0000000000000000 x16: ffffdd7f4b7acec0 x15: 0000000000000000
[ 92.337531] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
[ 92.344859] x11: 0000000000000000 x10: 0000000000000000 x9 : ffffdd7f13335000
[ 92.352182] x8 : ffff280f03fed280 x7 : 0000000000000001 x6 : ffff280edf1c2600
[ 92.359504] x5 : 0000000000000002 x4 : ffff280f03fed280 x3 : ffff280ec4689820
[ 92.366835] x2 : 0000000000000000 x1 : ffffdd7f137945c8 x0 : 0000000000000000
[ 92.374159] Call trace:
[ 92.376682] drm_panel_disable+0x80/0xe0 [drm]
[ 92.381367] panel_bridge_disable+0x18/0x2c [drm_kms_helper]
[ 92.387281] drm_atomic_bridge_chain_disable+0x98/0xd0 [drm]
[ 92.393194] disable_outputs+0xfc/0x31c [drm_kms_helper]
[ 92.398738] drm_atomic_helper_commit_modeset_disables+0x20/0x50 [drm_kms_helper]
[ 92.406482] msm_atomic_commit_tail+0x188/0x500 [msm]
[ 92.411772] commit_tail+0xa4/0x184 [drm_kms_helper]
[ 92.416954] drm_atomic_helper_commit+0x164/0x3fc [drm_kms_helper]
[ 92.423373] drm_atomic_commit+0x50/0x60 [drm]
[ 92.428064] drm_atomic_helper_disable_all+0x1f8/0x20c [drm_kms_helper]
[ 92.434989] drm_atomic_helper_shutdown+0x80/0x130 [drm_kms_helper]
[ 92.441497] msm_drm_uninit.isra.0+0x14c/0x174 [msm]
[ 92.446729] msm_drm_unbind+0x14/0x20 [msm]
[ 92.451125] component_del+0xa8/0x160
[ 92.454898] dsi_dev_detach+0x24/0x30 [msm]
[ 92.459294] dsi_host_detach+0x20/0x64 [msm]
[ 92.463764] devm_mipi_dsi_detach+0x2c/0x40
[ 92.468069] devm_action_release+0x18/0x24
[ 92.472278] devres_release_group+0x100/0x1b0
[ 92.476755] i2c_device_remove+0x48/0xf0
[ 92.480790] __device_release_driver+0x188/0x23c
[ 92.485534] driver_detach+0xfc/0x1e0
[ 92.489303] bus_remove_driver+0x5c/0xd0
[ 92.493333] driver_unregister+0x34/0x64
[ 92.497363] i2c_del_driver+0x58/0x70
[ 92.501134] ti_sn65dsi86_exit+0x44/0x98c [ti_sn65dsi86]
[ 92.506611] __arm64_sys_delete_module+0x198/0x22c
[ 92.511535] invoke_syscall+0x48/0x114
[ 92.515389] el0_svc_common.constprop.0+0x44/0xec
[ 92.520223] do_el0_svc+0x28/0x90
[ 92.523629] el0_svc+0x20/0x60
[ 92.526780] el0t_64_sync_handler+0xec/0xf0
[ 92.531085] el0t_64_sync+0x1a0/0x1a4
[ 92.534879] Code: f94013f5 52800000 f9400a61 b40000a1 (f9400821)
[ 92.541134] ---[ end trace 1bc553757c40a199 ]---
I'll look into this.
In the meanwhile, if anyone has any suggestion before I solve this issue,
as to speed up getting this fix done (as it's pretty much critical), you're
welcome.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
2021-11-25 15:09 ` AngeloGioacchino Del Regno
@ 2021-11-29 2:20 ` Dmitry Baryshkov
-1 siblings, 0 replies; 36+ messages in thread
From: Dmitry Baryshkov @ 2021-11-29 2:20 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, robdclark
Cc: freedreno, airlied, linux-arm-msm, konrad.dybcio, linux-kernel,
dri-devel, jami.kettunen, maxime, marijn.suijten, kernel, sean
Hi,
On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
> DSI host gets initialized earlier, but this caused unability to probe
> the entire stack of components because they all depend on interrupts
> coming from the main `mdss` node (mdp5, or dpu1).
>
> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
> them at msm_pdev_probe() time: this will make sure that we add the
> required interrupt controller mapping before dsi and/or other components
> try to initialize, finally satisfying the dependency.
>
> While at it, also change the allocation of msm_drm_private to use the
> devm variant of kzalloc().
>
> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
I have been thinking about this. I do not feel that this is the correct
approach. Currently DRM device exists only when all components are
bound. If any of the subdevices is removed, corresponding component is
delteted (and thus all components are unbound), the DRM device is taken
down. This results in the state cleanup, userspace notifications, etc.
With your changes, DRM device will continue to exist even after one of
subdevices is removed. This is not an expected behaviour, since
subdrivers do not perform full cleanup, delegating that to DRM device
takedown.
I suppose that proper solution would be to split msm_drv.c into into:
- generic components & drm code to be called from mdp4/mdp5/dpu driver
(making mdp4, mdp5 or dpu1 the components master)
- bare mdss driver, taking care only about IRQs, OF devices population -
calling proper mdss_init/mdss_destroy functions. Most probably we can
drop this part altogether and just make md5_mdss.c/dpu_mdss.c proper
platform drivers.
> ---
> drivers/gpu/drm/msm/msm_drv.c | 81 ++++++++++++++++-------------------
> 1 file changed, 38 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 7936e8d498dd..790acf4993c0 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -512,45 +512,12 @@ static int msm_init_vram(struct drm_device *dev)
> static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> {
> struct platform_device *pdev = to_platform_device(dev);
> - struct drm_device *ddev;
> - struct msm_drm_private *priv;
> - struct msm_kms *kms;
> - struct msm_mdss *mdss;
> + struct drm_device *ddev = platform_get_drvdata(pdev);
> + struct msm_drm_private *priv = ddev->dev_private;
> + struct msm_kms *kms = priv->kms;
> + struct msm_mdss *mdss = priv->mdss;
> int ret, i;
>
> - ddev = drm_dev_alloc(drv, dev);
> - if (IS_ERR(ddev)) {
> - DRM_DEV_ERROR(dev, "failed to allocate drm_device\n");
> - return PTR_ERR(ddev);
> - }
> -
> - platform_set_drvdata(pdev, ddev);
> -
> - priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> - if (!priv) {
> - ret = -ENOMEM;
> - goto err_put_drm_dev;
> - }
> -
> - ddev->dev_private = priv;
> - priv->dev = ddev;
> -
> - switch (get_mdp_ver(pdev)) {
> - case KMS_MDP5:
> - ret = mdp5_mdss_init(ddev);
> - break;
> - case KMS_DPU:
> - ret = dpu_mdss_init(ddev);
> - break;
> - default:
> - ret = 0;
> - break;
> - }
> - if (ret)
> - goto err_free_priv;
> -
> - mdss = priv->mdss;
> -
> priv->wq = alloc_ordered_workqueue("msm", 0);
> priv->hangcheck_period = DRM_MSM_HANGCHECK_DEFAULT_PERIOD;
>
> @@ -685,11 +652,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> err_destroy_mdss:
> if (mdss && mdss->funcs)
> mdss->funcs->destroy(ddev);
> -err_free_priv:
> - kfree(priv);
> -err_put_drm_dev:
> - drm_dev_put(ddev);
> - platform_set_drvdata(pdev, NULL);
> return ret;
> }
>
> @@ -1382,12 +1344,42 @@ static const struct component_master_ops msm_drm_ops = {
> static int msm_pdev_probe(struct platform_device *pdev)
> {
> struct component_match *match = NULL;
> + struct msm_drm_private *priv;
> + struct drm_device *ddev;
> int ret;
>
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + ddev = drm_dev_alloc(&msm_driver, &pdev->dev);
> + if (IS_ERR(ddev)) {
> + DRM_DEV_ERROR(&pdev->dev, "failed to allocate drm_device\n");
> + return PTR_ERR(ddev);
> + }
> +
> + platform_set_drvdata(pdev, ddev);
> + ddev->dev_private = priv;
> + priv->dev = ddev;
> +
> + switch (get_mdp_ver(pdev)) {
> + case KMS_MDP5:
> + ret = mdp5_mdss_init(ddev);
> + break;
> + case KMS_DPU:
> + ret = dpu_mdss_init(ddev);
> + break;
> + default:
> + ret = 0;
> + break;
> + }
> + if (ret)
> + goto err_put_drm_dev;
> +
> if (get_mdp_ver(pdev)) {
> ret = add_display_components(pdev, &match);
> if (ret)
> - return ret;
> + goto fail;
> }
>
> ret = add_gpu_components(&pdev->dev, &match);
> @@ -1409,6 +1401,9 @@ static int msm_pdev_probe(struct platform_device *pdev)
>
> fail:
> of_platform_depopulate(&pdev->dev);
> +err_put_drm_dev:
> + drm_dev_put(ddev);
> + platform_set_drvdata(pdev, NULL);
> return ret;
> }
>
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
@ 2021-11-29 2:20 ` Dmitry Baryshkov
0 siblings, 0 replies; 36+ messages in thread
From: Dmitry Baryshkov @ 2021-11-29 2:20 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, robdclark
Cc: sean, airlied, daniel, maxime, linux-arm-msm, dri-devel,
freedreno, linux-kernel, kernel, konrad.dybcio, marijn.suijten,
jami.kettunen
Hi,
On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
> DSI host gets initialized earlier, but this caused unability to probe
> the entire stack of components because they all depend on interrupts
> coming from the main `mdss` node (mdp5, or dpu1).
>
> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
> them at msm_pdev_probe() time: this will make sure that we add the
> required interrupt controller mapping before dsi and/or other components
> try to initialize, finally satisfying the dependency.
>
> While at it, also change the allocation of msm_drm_private to use the
> devm variant of kzalloc().
>
> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
I have been thinking about this. I do not feel that this is the correct
approach. Currently DRM device exists only when all components are
bound. If any of the subdevices is removed, corresponding component is
delteted (and thus all components are unbound), the DRM device is taken
down. This results in the state cleanup, userspace notifications, etc.
With your changes, DRM device will continue to exist even after one of
subdevices is removed. This is not an expected behaviour, since
subdrivers do not perform full cleanup, delegating that to DRM device
takedown.
I suppose that proper solution would be to split msm_drv.c into into:
- generic components & drm code to be called from mdp4/mdp5/dpu driver
(making mdp4, mdp5 or dpu1 the components master)
- bare mdss driver, taking care only about IRQs, OF devices population -
calling proper mdss_init/mdss_destroy functions. Most probably we can
drop this part altogether and just make md5_mdss.c/dpu_mdss.c proper
platform drivers.
> ---
> drivers/gpu/drm/msm/msm_drv.c | 81 ++++++++++++++++-------------------
> 1 file changed, 38 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 7936e8d498dd..790acf4993c0 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -512,45 +512,12 @@ static int msm_init_vram(struct drm_device *dev)
> static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> {
> struct platform_device *pdev = to_platform_device(dev);
> - struct drm_device *ddev;
> - struct msm_drm_private *priv;
> - struct msm_kms *kms;
> - struct msm_mdss *mdss;
> + struct drm_device *ddev = platform_get_drvdata(pdev);
> + struct msm_drm_private *priv = ddev->dev_private;
> + struct msm_kms *kms = priv->kms;
> + struct msm_mdss *mdss = priv->mdss;
> int ret, i;
>
> - ddev = drm_dev_alloc(drv, dev);
> - if (IS_ERR(ddev)) {
> - DRM_DEV_ERROR(dev, "failed to allocate drm_device\n");
> - return PTR_ERR(ddev);
> - }
> -
> - platform_set_drvdata(pdev, ddev);
> -
> - priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> - if (!priv) {
> - ret = -ENOMEM;
> - goto err_put_drm_dev;
> - }
> -
> - ddev->dev_private = priv;
> - priv->dev = ddev;
> -
> - switch (get_mdp_ver(pdev)) {
> - case KMS_MDP5:
> - ret = mdp5_mdss_init(ddev);
> - break;
> - case KMS_DPU:
> - ret = dpu_mdss_init(ddev);
> - break;
> - default:
> - ret = 0;
> - break;
> - }
> - if (ret)
> - goto err_free_priv;
> -
> - mdss = priv->mdss;
> -
> priv->wq = alloc_ordered_workqueue("msm", 0);
> priv->hangcheck_period = DRM_MSM_HANGCHECK_DEFAULT_PERIOD;
>
> @@ -685,11 +652,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> err_destroy_mdss:
> if (mdss && mdss->funcs)
> mdss->funcs->destroy(ddev);
> -err_free_priv:
> - kfree(priv);
> -err_put_drm_dev:
> - drm_dev_put(ddev);
> - platform_set_drvdata(pdev, NULL);
> return ret;
> }
>
> @@ -1382,12 +1344,42 @@ static const struct component_master_ops msm_drm_ops = {
> static int msm_pdev_probe(struct platform_device *pdev)
> {
> struct component_match *match = NULL;
> + struct msm_drm_private *priv;
> + struct drm_device *ddev;
> int ret;
>
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + ddev = drm_dev_alloc(&msm_driver, &pdev->dev);
> + if (IS_ERR(ddev)) {
> + DRM_DEV_ERROR(&pdev->dev, "failed to allocate drm_device\n");
> + return PTR_ERR(ddev);
> + }
> +
> + platform_set_drvdata(pdev, ddev);
> + ddev->dev_private = priv;
> + priv->dev = ddev;
> +
> + switch (get_mdp_ver(pdev)) {
> + case KMS_MDP5:
> + ret = mdp5_mdss_init(ddev);
> + break;
> + case KMS_DPU:
> + ret = dpu_mdss_init(ddev);
> + break;
> + default:
> + ret = 0;
> + break;
> + }
> + if (ret)
> + goto err_put_drm_dev;
> +
> if (get_mdp_ver(pdev)) {
> ret = add_display_components(pdev, &match);
> if (ret)
> - return ret;
> + goto fail;
> }
>
> ret = add_gpu_components(&pdev->dev, &match);
> @@ -1409,6 +1401,9 @@ static int msm_pdev_probe(struct platform_device *pdev)
>
> fail:
> of_platform_depopulate(&pdev->dev);
> +err_put_drm_dev:
> + drm_dev_put(ddev);
> + platform_set_drvdata(pdev, NULL);
> return ret;
> }
>
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
2021-11-29 2:20 ` Dmitry Baryshkov
@ 2021-11-29 14:14 ` AngeloGioacchino Del Regno
-1 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-11-29 14:14 UTC (permalink / raw)
To: Dmitry Baryshkov, robdclark
Cc: freedreno, airlied, linux-arm-msm, konrad.dybcio, linux-kernel,
dri-devel, jami.kettunen, maxime, marijn.suijten, kernel, sean
Il 29/11/21 03:20, Dmitry Baryshkov ha scritto:
> Hi,
>
> On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
>> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
>> DSI host gets initialized earlier, but this caused unability to probe
>> the entire stack of components because they all depend on interrupts
>> coming from the main `mdss` node (mdp5, or dpu1).
>>
>> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
>> them at msm_pdev_probe() time: this will make sure that we add the
>> required interrupt controller mapping before dsi and/or other components
>> try to initialize, finally satisfying the dependency.
>>
>> While at it, also change the allocation of msm_drm_private to use the
>> devm variant of kzalloc().
>>
>> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>
> I have been thinking about this. I do not feel that this is the correct approach.
> Currently DRM device exists only when all components are bound. If any of the
> subdevices is removed, corresponding component is delteted (and thus all components
> are unbound), the DRM device is taken down. This results in the state cleanup,
> userspace notifications, etc.
>
> With your changes, DRM device will continue to exist even after one of subdevices
> is removed. This is not an expected behaviour, since subdrivers do not perform full
> cleanup, delegating that to DRM device takedown.
>
> I suppose that proper solution would be to split msm_drv.c into into:
> - generic components & drm code to be called from mdp4/mdp5/dpu driver (making
> mdp4, mdp5 or dpu1 the components master)
>
> - bare mdss driver, taking care only about IRQs, OF devices population - calling
> proper mdss_init/mdss_destroy functions. Most probably we can drop this part
> altogether and just make md5_mdss.c/dpu_mdss.c proper platform drivers.
>
Hmm... getting a better look on how things are structured... yes, I mostly agree
with you, though I'm not sure about making MDP{4,5}/DPU1 the component master; that
would result in a major change in drm-msm, which may be "a bit too much".
Don't misunderstand me here, please, major changes are fine - but I feel urgency
to get this bug solved ASAP (since drm-msm is currently broken at least for drm
bridges) and, if we do anything major, that would require a very careful slow
review process that will leave this driver broken for a lot of time.
I actually tried something else that "simplifies" the former approach, so here's
my proposal:
* we introduce {mdp5,dpu}_mdss_early_init(struct device, struct msm_drm_private)
* allocate only msm_drm_private in msm_pdev_probe, leaving the drm_dev_alloc call
into msm_drm_init(), so that the drm_dev_put() stays in msm_drm_uninit()
* pass msm_drm_private as drvdata instead of drm_device
* change all the drvdata users to get drm_device from priv->dev, instead of getting
msm_drm_private from drm_device->dev_private (like many other drm drivers are
currently doing)
This way, we keep the current flow of creating the DRM device at msm_drm_init time
and tearing it down at msm_drm_unbind time, solving the issue that you are
describing.
If you're okay with this kind of approach, I have two patches here that are 95%
ready, can finish them off and send briefly.
Though, something else must be noted here... in the last mail where I'm pasting
a crash that happens when running 'rmmod panel_edp ti_sn65dsi86', I have implied
that this is happening due to the patch that I've sent: after some more research,
I'm not convinced anymore that this is a consequence of that. That crash may not
be related to my fix at all, but to something else (perhaps also related to commit
8f59ee9a570c, the one that we're fixing here).
Of course, that crash still happens even with the approach that I've just proposed.
Looking forward for your opinion!
Cheers,
- Angelo
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
@ 2021-11-29 14:14 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-11-29 14:14 UTC (permalink / raw)
To: Dmitry Baryshkov, robdclark
Cc: sean, airlied, daniel, maxime, linux-arm-msm, dri-devel,
freedreno, linux-kernel, kernel, konrad.dybcio, marijn.suijten,
jami.kettunen
Il 29/11/21 03:20, Dmitry Baryshkov ha scritto:
> Hi,
>
> On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
>> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
>> DSI host gets initialized earlier, but this caused unability to probe
>> the entire stack of components because they all depend on interrupts
>> coming from the main `mdss` node (mdp5, or dpu1).
>>
>> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
>> them at msm_pdev_probe() time: this will make sure that we add the
>> required interrupt controller mapping before dsi and/or other components
>> try to initialize, finally satisfying the dependency.
>>
>> While at it, also change the allocation of msm_drm_private to use the
>> devm variant of kzalloc().
>>
>> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>
> I have been thinking about this. I do not feel that this is the correct approach.
> Currently DRM device exists only when all components are bound. If any of the
> subdevices is removed, corresponding component is delteted (and thus all components
> are unbound), the DRM device is taken down. This results in the state cleanup,
> userspace notifications, etc.
>
> With your changes, DRM device will continue to exist even after one of subdevices
> is removed. This is not an expected behaviour, since subdrivers do not perform full
> cleanup, delegating that to DRM device takedown.
>
> I suppose that proper solution would be to split msm_drv.c into into:
> - generic components & drm code to be called from mdp4/mdp5/dpu driver (making
> mdp4, mdp5 or dpu1 the components master)
>
> - bare mdss driver, taking care only about IRQs, OF devices population - calling
> proper mdss_init/mdss_destroy functions. Most probably we can drop this part
> altogether and just make md5_mdss.c/dpu_mdss.c proper platform drivers.
>
Hmm... getting a better look on how things are structured... yes, I mostly agree
with you, though I'm not sure about making MDP{4,5}/DPU1 the component master; that
would result in a major change in drm-msm, which may be "a bit too much".
Don't misunderstand me here, please, major changes are fine - but I feel urgency
to get this bug solved ASAP (since drm-msm is currently broken at least for drm
bridges) and, if we do anything major, that would require a very careful slow
review process that will leave this driver broken for a lot of time.
I actually tried something else that "simplifies" the former approach, so here's
my proposal:
* we introduce {mdp5,dpu}_mdss_early_init(struct device, struct msm_drm_private)
* allocate only msm_drm_private in msm_pdev_probe, leaving the drm_dev_alloc call
into msm_drm_init(), so that the drm_dev_put() stays in msm_drm_uninit()
* pass msm_drm_private as drvdata instead of drm_device
* change all the drvdata users to get drm_device from priv->dev, instead of getting
msm_drm_private from drm_device->dev_private (like many other drm drivers are
currently doing)
This way, we keep the current flow of creating the DRM device at msm_drm_init time
and tearing it down at msm_drm_unbind time, solving the issue that you are
describing.
If you're okay with this kind of approach, I have two patches here that are 95%
ready, can finish them off and send briefly.
Though, something else must be noted here... in the last mail where I'm pasting
a crash that happens when running 'rmmod panel_edp ti_sn65dsi86', I have implied
that this is happening due to the patch that I've sent: after some more research,
I'm not convinced anymore that this is a consequence of that. That crash may not
be related to my fix at all, but to something else (perhaps also related to commit
8f59ee9a570c, the one that we're fixing here).
Of course, that crash still happens even with the approach that I've just proposed.
Looking forward for your opinion!
Cheers,
- Angelo
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
2021-11-29 14:14 ` AngeloGioacchino Del Regno
@ 2021-11-29 14:53 ` Dmitry Baryshkov
-1 siblings, 0 replies; 36+ messages in thread
From: Dmitry Baryshkov @ 2021-11-29 14:53 UTC (permalink / raw)
To: AngeloGioacchino Del Regno
Cc: freedreno, airlied, linux-arm-msm, konrad.dybcio, linux-kernel,
dri-devel, jami.kettunen, maxime, marijn.suijten, kernel, sean
Hi,
On Mon, 29 Nov 2021 at 17:15, AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 29/11/21 03:20, Dmitry Baryshkov ha scritto:
> > Hi,
> >
> > On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
> >> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
> >> DSI host gets initialized earlier, but this caused unability to probe
> >> the entire stack of components because they all depend on interrupts
> >> coming from the main `mdss` node (mdp5, or dpu1).
> >>
> >> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
> >> them at msm_pdev_probe() time: this will make sure that we add the
> >> required interrupt controller mapping before dsi and/or other components
> >> try to initialize, finally satisfying the dependency.
> >>
> >> While at it, also change the allocation of msm_drm_private to use the
> >> devm variant of kzalloc().
> >>
> >> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> >
> > I have been thinking about this. I do not feel that this is the correct approach.
> > Currently DRM device exists only when all components are bound. If any of the
> > subdevices is removed, corresponding component is delteted (and thus all components
> > are unbound), the DRM device is taken down. This results in the state cleanup,
> > userspace notifications, etc.
> >
> > With your changes, DRM device will continue to exist even after one of subdevices
> > is removed. This is not an expected behaviour, since subdrivers do not perform full
> > cleanup, delegating that to DRM device takedown.
> >
> > I suppose that proper solution would be to split msm_drv.c into into:
> > - generic components & drm code to be called from mdp4/mdp5/dpu driver (making
> > mdp4, mdp5 or dpu1 the components master)
> >
> > - bare mdss driver, taking care only about IRQs, OF devices population - calling
> > proper mdss_init/mdss_destroy functions. Most probably we can drop this part
> > altogether and just make md5_mdss.c/dpu_mdss.c proper platform drivers.
> >
>
>
> Hmm... getting a better look on how things are structured... yes, I mostly agree
> with you, though I'm not sure about making MDP{4,5}/DPU1 the component master; that
> would result in a major change in drm-msm, which may be "a bit too much".
>
> Don't misunderstand me here, please, major changes are fine - but I feel urgency
> to get this bug solved ASAP (since drm-msm is currently broken at least for drm
> bridges) and, if we do anything major, that would require a very careful slow
> review process that will leave this driver broken for a lot of time.
Yep. I wanted to hear your and Rob's opinion, that's why I did not
rush into implementing it.
I still think this is the way to go in the long term. What I really
liked in your patchset was untying the knot between component binds
returning EPROBE_DEFER and mdss subdevices being in place. This solved
the "dsi host registration" infinite loop for me.
>
> I actually tried something else that "simplifies" the former approach, so here's
> my proposal:
> * we introduce {mdp5,dpu}_mdss_early_init(struct device, struct msm_drm_private)
> * allocate only msm_drm_private in msm_pdev_probe, leaving the drm_dev_alloc call
> into msm_drm_init(), so that the drm_dev_put() stays in msm_drm_uninit()
> * pass msm_drm_private as drvdata instead of drm_device
> * change all the drvdata users to get drm_device from priv->dev, instead of getting
> msm_drm_private from drm_device->dev_private (like many other drm drivers are
> currently doing)
This sounds in a way that it should work. I'm looking forward to
seeing (and testing) your patches.
>
> This way, we keep the current flow of creating the DRM device at msm_drm_init time
> and tearing it down at msm_drm_unbind time, solving the issue that you are
> describing.
>
> If you're okay with this kind of approach, I have two patches here that are 95%
> ready, can finish them off and send briefly.
>
> Though, something else must be noted here... in the last mail where I'm pasting
> a crash that happens when running 'rmmod panel_edp ti_sn65dsi86', I have implied
> that this is happening due to the patch that I've sent: after some more research,
> I'm not convinced anymore that this is a consequence of that. That crash may not
> be related to my fix at all, but to something else (perhaps also related to commit
> 8f59ee9a570c, the one that we're fixing here).
>
> Of course, that crash still happens even with the approach that I've just proposed.
>
>
> Looking forward for your opinion!
>
> Cheers,
> - Angelo
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
@ 2021-11-29 14:53 ` Dmitry Baryshkov
0 siblings, 0 replies; 36+ messages in thread
From: Dmitry Baryshkov @ 2021-11-29 14:53 UTC (permalink / raw)
To: AngeloGioacchino Del Regno
Cc: robdclark, sean, airlied, daniel, maxime, linux-arm-msm,
dri-devel, freedreno, linux-kernel, kernel, konrad.dybcio,
marijn.suijten, jami.kettunen
Hi,
On Mon, 29 Nov 2021 at 17:15, AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 29/11/21 03:20, Dmitry Baryshkov ha scritto:
> > Hi,
> >
> > On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
> >> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
> >> DSI host gets initialized earlier, but this caused unability to probe
> >> the entire stack of components because they all depend on interrupts
> >> coming from the main `mdss` node (mdp5, or dpu1).
> >>
> >> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
> >> them at msm_pdev_probe() time: this will make sure that we add the
> >> required interrupt controller mapping before dsi and/or other components
> >> try to initialize, finally satisfying the dependency.
> >>
> >> While at it, also change the allocation of msm_drm_private to use the
> >> devm variant of kzalloc().
> >>
> >> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> >
> > I have been thinking about this. I do not feel that this is the correct approach.
> > Currently DRM device exists only when all components are bound. If any of the
> > subdevices is removed, corresponding component is delteted (and thus all components
> > are unbound), the DRM device is taken down. This results in the state cleanup,
> > userspace notifications, etc.
> >
> > With your changes, DRM device will continue to exist even after one of subdevices
> > is removed. This is not an expected behaviour, since subdrivers do not perform full
> > cleanup, delegating that to DRM device takedown.
> >
> > I suppose that proper solution would be to split msm_drv.c into into:
> > - generic components & drm code to be called from mdp4/mdp5/dpu driver (making
> > mdp4, mdp5 or dpu1 the components master)
> >
> > - bare mdss driver, taking care only about IRQs, OF devices population - calling
> > proper mdss_init/mdss_destroy functions. Most probably we can drop this part
> > altogether and just make md5_mdss.c/dpu_mdss.c proper platform drivers.
> >
>
>
> Hmm... getting a better look on how things are structured... yes, I mostly agree
> with you, though I'm not sure about making MDP{4,5}/DPU1 the component master; that
> would result in a major change in drm-msm, which may be "a bit too much".
>
> Don't misunderstand me here, please, major changes are fine - but I feel urgency
> to get this bug solved ASAP (since drm-msm is currently broken at least for drm
> bridges) and, if we do anything major, that would require a very careful slow
> review process that will leave this driver broken for a lot of time.
Yep. I wanted to hear your and Rob's opinion, that's why I did not
rush into implementing it.
I still think this is the way to go in the long term. What I really
liked in your patchset was untying the knot between component binds
returning EPROBE_DEFER and mdss subdevices being in place. This solved
the "dsi host registration" infinite loop for me.
>
> I actually tried something else that "simplifies" the former approach, so here's
> my proposal:
> * we introduce {mdp5,dpu}_mdss_early_init(struct device, struct msm_drm_private)
> * allocate only msm_drm_private in msm_pdev_probe, leaving the drm_dev_alloc call
> into msm_drm_init(), so that the drm_dev_put() stays in msm_drm_uninit()
> * pass msm_drm_private as drvdata instead of drm_device
> * change all the drvdata users to get drm_device from priv->dev, instead of getting
> msm_drm_private from drm_device->dev_private (like many other drm drivers are
> currently doing)
This sounds in a way that it should work. I'm looking forward to
seeing (and testing) your patches.
>
> This way, we keep the current flow of creating the DRM device at msm_drm_init time
> and tearing it down at msm_drm_unbind time, solving the issue that you are
> describing.
>
> If you're okay with this kind of approach, I have two patches here that are 95%
> ready, can finish them off and send briefly.
>
> Though, something else must be noted here... in the last mail where I'm pasting
> a crash that happens when running 'rmmod panel_edp ti_sn65dsi86', I have implied
> that this is happening due to the patch that I've sent: after some more research,
> I'm not convinced anymore that this is a consequence of that. That crash may not
> be related to my fix at all, but to something else (perhaps also related to commit
> 8f59ee9a570c, the one that we're fixing here).
>
> Of course, that crash still happens even with the approach that I've just proposed.
>
>
> Looking forward for your opinion!
>
> Cheers,
> - Angelo
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
2021-11-29 14:53 ` Dmitry Baryshkov
@ 2021-11-29 14:56 ` AngeloGioacchino Del Regno
-1 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-11-29 14:56 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, airlied, linux-arm-msm, konrad.dybcio, linux-kernel,
dri-devel, jami.kettunen, maxime, marijn.suijten, kernel, sean
Il 29/11/21 15:53, Dmitry Baryshkov ha scritto:
> Hi,
>
> On Mon, 29 Nov 2021 at 17:15, AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 29/11/21 03:20, Dmitry Baryshkov ha scritto:
>>> Hi,
>>>
>>> On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
>>>> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
>>>> DSI host gets initialized earlier, but this caused unability to probe
>>>> the entire stack of components because they all depend on interrupts
>>>> coming from the main `mdss` node (mdp5, or dpu1).
>>>>
>>>> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
>>>> them at msm_pdev_probe() time: this will make sure that we add the
>>>> required interrupt controller mapping before dsi and/or other components
>>>> try to initialize, finally satisfying the dependency.
>>>>
>>>> While at it, also change the allocation of msm_drm_private to use the
>>>> devm variant of kzalloc().
>>>>
>>>> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>
>>> I have been thinking about this. I do not feel that this is the correct approach.
>>> Currently DRM device exists only when all components are bound. If any of the
>>> subdevices is removed, corresponding component is delteted (and thus all components
>>> are unbound), the DRM device is taken down. This results in the state cleanup,
>>> userspace notifications, etc.
>>>
>>> With your changes, DRM device will continue to exist even after one of subdevices
>>> is removed. This is not an expected behaviour, since subdrivers do not perform full
>>> cleanup, delegating that to DRM device takedown.
>>>
>>> I suppose that proper solution would be to split msm_drv.c into into:
>>> - generic components & drm code to be called from mdp4/mdp5/dpu driver (making
>>> mdp4, mdp5 or dpu1 the components master)
>>>
>>> - bare mdss driver, taking care only about IRQs, OF devices population - calling
>>> proper mdss_init/mdss_destroy functions. Most probably we can drop this part
>>> altogether and just make md5_mdss.c/dpu_mdss.c proper platform drivers.
>>>
>>
>>
>> Hmm... getting a better look on how things are structured... yes, I mostly agree
>> with you, though I'm not sure about making MDP{4,5}/DPU1 the component master; that
>> would result in a major change in drm-msm, which may be "a bit too much".
>>
>> Don't misunderstand me here, please, major changes are fine - but I feel urgency
>> to get this bug solved ASAP (since drm-msm is currently broken at least for drm
>> bridges) and, if we do anything major, that would require a very careful slow
>> review process that will leave this driver broken for a lot of time.
>
> Yep. I wanted to hear your and Rob's opinion, that's why I did not
> rush into implementing it.
> I still think this is the way to go in the long term. What I really
> liked in your patchset was untying the knot between component binds
> returning EPROBE_DEFER and mdss subdevices being in place. This solved
> the "dsi host registration" infinite loop for me.
>
Thanks. I'm also curious about Rob's opinion on this, as that'd be very valuable.
>>
>> I actually tried something else that "simplifies" the former approach, so here's
>> my proposal:
>> * we introduce {mdp5,dpu}_mdss_early_init(struct device, struct msm_drm_private)
>> * allocate only msm_drm_private in msm_pdev_probe, leaving the drm_dev_alloc call
>> into msm_drm_init(), so that the drm_dev_put() stays in msm_drm_uninit()
>> * pass msm_drm_private as drvdata instead of drm_device
>> * change all the drvdata users to get drm_device from priv->dev, instead of getting
>> msm_drm_private from drm_device->dev_private (like many other drm drivers are
>> currently doing)
>
> This sounds in a way that it should work. I'm looking forward to
> seeing (and testing) your patches.
>
Alright then, I'm running some more tests and cleaning up the patches.
Expect a v2 between today and tomorrow at max! :))
>>
>> This way, we keep the current flow of creating the DRM device at msm_drm_init time
>> and tearing it down at msm_drm_unbind time, solving the issue that you are
>> describing.
>>
>> If you're okay with this kind of approach, I have two patches here that are 95%
>> ready, can finish them off and send briefly.
>>
>> Though, something else must be noted here... in the last mail where I'm pasting
>> a crash that happens when running 'rmmod panel_edp ti_sn65dsi86', I have implied
>> that this is happening due to the patch that I've sent: after some more research,
>> I'm not convinced anymore that this is a consequence of that. That crash may not
>> be related to my fix at all, but to something else (perhaps also related to commit
>> 8f59ee9a570c, the one that we're fixing here).
>>
>> Of course, that crash still happens even with the approach that I've just proposed.
>>
>>
>> Looking forward for your opinion!
>>
>> Cheers,
>> - Angelo
>
>
>
--
AngeloGioacchino Del Regno
Software Engineer
Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time
@ 2021-11-29 14:56 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 36+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-11-29 14:56 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: robdclark, sean, airlied, daniel, maxime, linux-arm-msm,
dri-devel, freedreno, linux-kernel, kernel, konrad.dybcio,
marijn.suijten, jami.kettunen
Il 29/11/21 15:53, Dmitry Baryshkov ha scritto:
> Hi,
>
> On Mon, 29 Nov 2021 at 17:15, AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 29/11/21 03:20, Dmitry Baryshkov ha scritto:
>>> Hi,
>>>
>>> On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
>>>> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
>>>> DSI host gets initialized earlier, but this caused unability to probe
>>>> the entire stack of components because they all depend on interrupts
>>>> coming from the main `mdss` node (mdp5, or dpu1).
>>>>
>>>> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
>>>> them at msm_pdev_probe() time: this will make sure that we add the
>>>> required interrupt controller mapping before dsi and/or other components
>>>> try to initialize, finally satisfying the dependency.
>>>>
>>>> While at it, also change the allocation of msm_drm_private to use the
>>>> devm variant of kzalloc().
>>>>
>>>> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>
>>> I have been thinking about this. I do not feel that this is the correct approach.
>>> Currently DRM device exists only when all components are bound. If any of the
>>> subdevices is removed, corresponding component is delteted (and thus all components
>>> are unbound), the DRM device is taken down. This results in the state cleanup,
>>> userspace notifications, etc.
>>>
>>> With your changes, DRM device will continue to exist even after one of subdevices
>>> is removed. This is not an expected behaviour, since subdrivers do not perform full
>>> cleanup, delegating that to DRM device takedown.
>>>
>>> I suppose that proper solution would be to split msm_drv.c into into:
>>> - generic components & drm code to be called from mdp4/mdp5/dpu driver (making
>>> mdp4, mdp5 or dpu1 the components master)
>>>
>>> - bare mdss driver, taking care only about IRQs, OF devices population - calling
>>> proper mdss_init/mdss_destroy functions. Most probably we can drop this part
>>> altogether and just make md5_mdss.c/dpu_mdss.c proper platform drivers.
>>>
>>
>>
>> Hmm... getting a better look on how things are structured... yes, I mostly agree
>> with you, though I'm not sure about making MDP{4,5}/DPU1 the component master; that
>> would result in a major change in drm-msm, which may be "a bit too much".
>>
>> Don't misunderstand me here, please, major changes are fine - but I feel urgency
>> to get this bug solved ASAP (since drm-msm is currently broken at least for drm
>> bridges) and, if we do anything major, that would require a very careful slow
>> review process that will leave this driver broken for a lot of time.
>
> Yep. I wanted to hear your and Rob's opinion, that's why I did not
> rush into implementing it.
> I still think this is the way to go in the long term. What I really
> liked in your patchset was untying the knot between component binds
> returning EPROBE_DEFER and mdss subdevices being in place. This solved
> the "dsi host registration" infinite loop for me.
>
Thanks. I'm also curious about Rob's opinion on this, as that'd be very valuable.
>>
>> I actually tried something else that "simplifies" the former approach, so here's
>> my proposal:
>> * we introduce {mdp5,dpu}_mdss_early_init(struct device, struct msm_drm_private)
>> * allocate only msm_drm_private in msm_pdev_probe, leaving the drm_dev_alloc call
>> into msm_drm_init(), so that the drm_dev_put() stays in msm_drm_uninit()
>> * pass msm_drm_private as drvdata instead of drm_device
>> * change all the drvdata users to get drm_device from priv->dev, instead of getting
>> msm_drm_private from drm_device->dev_private (like many other drm drivers are
>> currently doing)
>
> This sounds in a way that it should work. I'm looking forward to
> seeing (and testing) your patches.
>
Alright then, I'm running some more tests and cleaning up the patches.
Expect a v2 between today and tomorrow at max! :))
>>
>> This way, we keep the current flow of creating the DRM device at msm_drm_init time
>> and tearing it down at msm_drm_unbind time, solving the issue that you are
>> describing.
>>
>> If you're okay with this kind of approach, I have two patches here that are 95%
>> ready, can finish them off and send briefly.
>>
>> Though, something else must be noted here... in the last mail where I'm pasting
>> a crash that happens when running 'rmmod panel_edp ti_sn65dsi86', I have implied
>> that this is happening due to the patch that I've sent: after some more research,
>> I'm not convinced anymore that this is a consequence of that. That crash may not
>> be related to my fix at all, but to something else (perhaps also related to commit
>> 8f59ee9a570c, the one that we're fixing here).
>>
>> Of course, that crash still happens even with the approach that I've just proposed.
>>
>>
>> Looking forward for your opinion!
>>
>> Cheers,
>> - Angelo
>
>
>
--
AngeloGioacchino Del Regno
Software Engineer
Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718
^ permalink raw reply [flat|nested] 36+ messages in thread