Re: [PATCH 2/3] drm/msm/mdp4: don't destroy mdp4_kms in mdp4_kms_init error path
On Mon, Apr 22, 2024 at 11:17:15AM -0700, Abhinav Kumar wrote:
>
>
> On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote:
> > Since commit 3c74682637e6 ("drm/msm/mdp4: move resource allocation to
> > the _probe function") the mdp4_kms data is allocated during probe. It is
> > an error to destroy it during mdp4_kms_init(), as the data is still
> > referenced by the drivers's data and can be used later in case of probe
> > deferral.
> >
>
> mdp4_destroy() currently detaches mmu, calls msm_kms_destroy() which
> destroys pending timers and releases refcount on the aspace.
>
> It does not touch the mdp4_kms as that one is devm managed.
>
> In the comment
> https://patchwork.freedesktop.org/patch/590411/?series=132664=1#comment_1074306,
> we had discussed that the last component's reprobe attempt is affected
> (which is not dpu or mdp4 or mdp5 right? )
>
> If it was an interface (such as DSI OR DP), is it the aspace detach which is
> causing the crash?
I should have retained the trace log. I'll trigger the issue and post the trace.
>
> Another note is, mdp5 needs the same fix in that case.
>
> dpu_kms_init() looks fine.
>
> > Fixes: 3c74682637e6 ("drm/msm/mdp4: move resource allocation to the _probe
> > function")
> > Signed-off-by: Dmitry Baryshkov
> > ---
> > drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 28 +---
> > 1 file changed, 9 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > index 4ba1cb74ad76..4c5f72b7e0e5 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > @@ -392,7 +392,7 @@ static int mdp4_kms_init(struct drm_device *dev)
> > ret = mdp_kms_init(_kms->base, _funcs);
> > if (ret) {
> > DRM_DEV_ERROR(dev->dev, "failed to init kms\n");
> > - goto fail;
> > + return ret;
> > }
> > kms = priv->kms;
> > @@ -403,7 +403,7 @@ static int mdp4_kms_init(struct drm_device *dev)
> > ret = regulator_enable(mdp4_kms->vdd);
> > if (ret) {
> > DRM_DEV_ERROR(dev->dev, "failed to enable regulator
> > vdd: %d\n", ret);
> > - goto fail;
> > + return ret;
> > }
> > }
> > @@ -414,8 +414,7 @@ static int mdp4_kms_init(struct drm_device *dev)
> > if (major != 4) {
> > DRM_DEV_ERROR(dev->dev, "unexpected MDP version: v%d.%d\n",
> > major, minor);
> > - ret = -ENXIO;
> > - goto fail;
> > + return -ENXIO;
> > }
> > mdp4_kms->rev = minor;
> > @@ -423,8 +422,7 @@ static int mdp4_kms_init(struct drm_device *dev)
> > if (mdp4_kms->rev >= 2) {
> > if (!mdp4_kms->lut_clk) {
> > DRM_DEV_ERROR(dev->dev, "failed to get lut_clk\n");
> > - ret = -ENODEV;
> > - goto fail;
> > + return -ENODEV;
> > }
> > clk_set_rate(mdp4_kms->lut_clk, max_clk);
> > }
> > @@ -445,8 +443,7 @@ static int mdp4_kms_init(struct drm_device *dev)
> > mmu = msm_iommu_new(>dev, 0);
> > if (IS_ERR(mmu)) {
> > - ret = PTR_ERR(mmu);
> > - goto fail;
> > + return PTR_ERR(mmu);
> > } else if (!mmu) {
> > DRM_DEV_INFO(dev->dev, "no iommu, fallback to phys "
> > "contig buffers for scanout\n");
> > @@ -458,8 +455,7 @@ static int mdp4_kms_init(struct drm_device *dev)
> > if (IS_ERR(aspace)) {
> > if (!IS_ERR(mmu))
> > mmu->funcs->destroy(mmu);
> > - ret = PTR_ERR(aspace);
> > - goto fail;
> > + return PTR_ERR(aspace);
> > }
> > kms->aspace = aspace;
> > @@ -468,7 +464,7 @@ static int mdp4_kms_init(struct drm_device *dev)
> > ret = modeset_init(mdp4_kms);
> > if (ret) {
> > DRM_DEV_ERROR(dev->dev, "modeset_init failed: %d\n", ret);
> > - goto fail;
> > + return ret;
> > }
> > mdp4_kms->blank_cursor_bo = msm_gem_new(dev, SZ_16K, MSM_BO_WC |
> > MSM_BO_SCANOUT);
> > @@ -476,14 +472,14 @@ static int mdp4_kms_init(struct drm_device *dev)
> > ret = PTR_ERR(mdp4_kms->blank_cursor_bo);
> > DRM_DEV_ERROR(dev->dev, "could not allocate blank-cursor bo:
> > %d\n", ret);
> > mdp4_kms->blank_cursor_bo = NULL;
> > - goto fail;
> > + return ret;
> > }
> > ret = msm_gem_get_and_pin_iova(mdp4_kms->blank_cursor_bo, kms->aspace,
> > _kms->blank_cursor_iova);
> > if (ret) {
> > DRM_DEV_ERROR(dev->dev, "could not pin blank-cursor bo: %d\n",
> > ret);
> > - goto fail;
> > + return ret;
> > }
> > dev->mode_config.min_width = 0;
> > @@ -492,12 +488,6 @@ static int mdp4_kms_init(struct
Re: [PATCH 2/3] drm/msm/mdp4: don't destroy mdp4_kms in mdp4_kms_init error path
On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote:
Since commit 3c74682637e6 ("drm/msm/mdp4: move resource allocation to
the _probe function") the mdp4_kms data is allocated during probe. It is
an error to destroy it during mdp4_kms_init(), as the data is still
referenced by the drivers's data and can be used later in case of probe
deferral.
mdp4_destroy() currently detaches mmu, calls msm_kms_destroy() which
destroys pending timers and releases refcount on the aspace.
It does not touch the mdp4_kms as that one is devm managed.
In the comment
https://patchwork.freedesktop.org/patch/590411/?series=132664=1#comment_1074306,
we had discussed that the last component's reprobe attempt is affected
(which is not dpu or mdp4 or mdp5 right? )
If it was an interface (such as DSI OR DP), is it the aspace detach
which is causing the crash?
Another note is, mdp5 needs the same fix in that case.
dpu_kms_init() looks fine.
Fixes: 3c74682637e6 ("drm/msm/mdp4: move resource allocation to the _probe
function")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 28 +---
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 4ba1cb74ad76..4c5f72b7e0e5 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -392,7 +392,7 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = mdp_kms_init(_kms->base, _funcs);
if (ret) {
DRM_DEV_ERROR(dev->dev, "failed to init kms\n");
- goto fail;
+ return ret;
}
kms = priv->kms;
@@ -403,7 +403,7 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = regulator_enable(mdp4_kms->vdd);
if (ret) {
DRM_DEV_ERROR(dev->dev, "failed to enable regulator vdd:
%d\n", ret);
- goto fail;
+ return ret;
}
}
@@ -414,8 +414,7 @@ static int mdp4_kms_init(struct drm_device *dev)
if (major != 4) {
DRM_DEV_ERROR(dev->dev, "unexpected MDP version: v%d.%d\n",
major, minor);
- ret = -ENXIO;
- goto fail;
+ return -ENXIO;
}
mdp4_kms->rev = minor;
@@ -423,8 +422,7 @@ static int mdp4_kms_init(struct drm_device *dev)
if (mdp4_kms->rev >= 2) {
if (!mdp4_kms->lut_clk) {
DRM_DEV_ERROR(dev->dev, "failed to get lut_clk\n");
- ret = -ENODEV;
- goto fail;
+ return -ENODEV;
}
clk_set_rate(mdp4_kms->lut_clk, max_clk);
}
@@ -445,8 +443,7 @@ static int mdp4_kms_init(struct drm_device *dev)
mmu = msm_iommu_new(>dev, 0);
if (IS_ERR(mmu)) {
- ret = PTR_ERR(mmu);
- goto fail;
+ return PTR_ERR(mmu);
} else if (!mmu) {
DRM_DEV_INFO(dev->dev, "no iommu, fallback to phys "
"contig buffers for scanout\n");
@@ -458,8 +455,7 @@ static int mdp4_kms_init(struct drm_device *dev)
if (IS_ERR(aspace)) {
if (!IS_ERR(mmu))
mmu->funcs->destroy(mmu);
- ret = PTR_ERR(aspace);
- goto fail;
+ return PTR_ERR(aspace);
}
kms->aspace = aspace;
@@ -468,7 +464,7 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = modeset_init(mdp4_kms);
if (ret) {
DRM_DEV_ERROR(dev->dev, "modeset_init failed: %d\n", ret);
- goto fail;
+ return ret;
}
mdp4_kms->blank_cursor_bo = msm_gem_new(dev, SZ_16K, MSM_BO_WC | MSM_BO_SCANOUT);
@@ -476,14 +472,14 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = PTR_ERR(mdp4_kms->blank_cursor_bo);
DRM_DEV_ERROR(dev->dev, "could not allocate blank-cursor bo:
%d\n", ret);
mdp4_kms->blank_cursor_bo = NULL;
- goto fail;
+ return ret;
}
ret = msm_gem_get_and_pin_iova(mdp4_kms->blank_cursor_bo, kms->aspace,
_kms->blank_cursor_iova);
if (ret) {
DRM_DEV_ERROR(dev->dev, "could not pin blank-cursor bo: %d\n",
ret);
- goto fail;
+ return ret;
}
dev->mode_config.min_width = 0;
@@ -492,12 +488,6 @@ static int mdp4_kms_init(struct drm_device *dev)
dev->mode_config.max_height = 2048;
return 0;
-
-fail:
- if (kms)
- mdp4_destroy(kms);
-
- return ret;
}
static const struct dev_pm_ops mdp4_pm_ops = {
[PATCH 2/3] drm/msm/mdp4: don't destroy mdp4_kms in mdp4_kms_init error path
Since commit 3c74682637e6 ("drm/msm/mdp4: move resource allocation to
the _probe function") the mdp4_kms data is allocated during probe. It is
an error to destroy it during mdp4_kms_init(), as the data is still
referenced by the drivers's data and can be used later in case of probe
deferral.
Fixes: 3c74682637e6 ("drm/msm/mdp4: move resource allocation to the _probe
function")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 28 +---
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 4ba1cb74ad76..4c5f72b7e0e5 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -392,7 +392,7 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = mdp_kms_init(_kms->base, _funcs);
if (ret) {
DRM_DEV_ERROR(dev->dev, "failed to init kms\n");
- goto fail;
+ return ret;
}
kms = priv->kms;
@@ -403,7 +403,7 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = regulator_enable(mdp4_kms->vdd);
if (ret) {
DRM_DEV_ERROR(dev->dev, "failed to enable regulator
vdd: %d\n", ret);
- goto fail;
+ return ret;
}
}
@@ -414,8 +414,7 @@ static int mdp4_kms_init(struct drm_device *dev)
if (major != 4) {
DRM_DEV_ERROR(dev->dev, "unexpected MDP version: v%d.%d\n",
major, minor);
- ret = -ENXIO;
- goto fail;
+ return -ENXIO;
}
mdp4_kms->rev = minor;
@@ -423,8 +422,7 @@ static int mdp4_kms_init(struct drm_device *dev)
if (mdp4_kms->rev >= 2) {
if (!mdp4_kms->lut_clk) {
DRM_DEV_ERROR(dev->dev, "failed to get lut_clk\n");
- ret = -ENODEV;
- goto fail;
+ return -ENODEV;
}
clk_set_rate(mdp4_kms->lut_clk, max_clk);
}
@@ -445,8 +443,7 @@ static int mdp4_kms_init(struct drm_device *dev)
mmu = msm_iommu_new(>dev, 0);
if (IS_ERR(mmu)) {
- ret = PTR_ERR(mmu);
- goto fail;
+ return PTR_ERR(mmu);
} else if (!mmu) {
DRM_DEV_INFO(dev->dev, "no iommu, fallback to phys "
"contig buffers for scanout\n");
@@ -458,8 +455,7 @@ static int mdp4_kms_init(struct drm_device *dev)
if (IS_ERR(aspace)) {
if (!IS_ERR(mmu))
mmu->funcs->destroy(mmu);
- ret = PTR_ERR(aspace);
- goto fail;
+ return PTR_ERR(aspace);
}
kms->aspace = aspace;
@@ -468,7 +464,7 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = modeset_init(mdp4_kms);
if (ret) {
DRM_DEV_ERROR(dev->dev, "modeset_init failed: %d\n", ret);
- goto fail;
+ return ret;
}
mdp4_kms->blank_cursor_bo = msm_gem_new(dev, SZ_16K, MSM_BO_WC |
MSM_BO_SCANOUT);
@@ -476,14 +472,14 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = PTR_ERR(mdp4_kms->blank_cursor_bo);
DRM_DEV_ERROR(dev->dev, "could not allocate blank-cursor bo:
%d\n", ret);
mdp4_kms->blank_cursor_bo = NULL;
- goto fail;
+ return ret;
}
ret = msm_gem_get_and_pin_iova(mdp4_kms->blank_cursor_bo, kms->aspace,
_kms->blank_cursor_iova);
if (ret) {
DRM_DEV_ERROR(dev->dev, "could not pin blank-cursor bo: %d\n",
ret);
- goto fail;
+ return ret;
}
dev->mode_config.min_width = 0;
@@ -492,12 +488,6 @@ static int mdp4_kms_init(struct drm_device *dev)
dev->mode_config.max_height = 2048;
return 0;
-
-fail:
- if (kms)
- mdp4_destroy(kms);
-
- return ret;
}
static const struct dev_pm_ops mdp4_pm_ops = {
--
2.39.2
