diff mbox series

[2/3] drm/msm/mdp4: don't destroy mdp4_kms in mdp4_kms_init error path

Message ID 20240420-mdp4-fixes-v1-2-96a70f64fa85@linaro.org (mailing list archive)
State New, archived
Headers show
Series drm/msm/mdp4: fix probe deferral issues | expand

Commit Message

Dmitry Baryshkov April 20, 2024, 2:33 a.m. UTC 
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 <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

Comments

Abhinav Kumar April 22, 2024, 6:17 p.m. UTC | #1 
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&rev=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 <dmitry.baryshkov@linaro.org>
> ---
>   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(&mdp4_kms->base, &kms_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(&pdev->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,
>   			&mdp4_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 = {
>
Dmitry Baryshkov April 22, 2024, 7:45 p.m. UTC | #2 
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&rev=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 <dmitry.baryshkov@linaro.org>
> > ---
> >   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(&mdp4_kms->base, &kms_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(&pdev->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,
> >   			&mdp4_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 = {
> >
diff mbox series

Patch

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(&mdp4_kms->base, &kms_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(&pdev->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,
 			&mdp4_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 = {