| Message ID | 20240420-mdp4-fixes-v1-2-96a70f64fa85@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/mdp4: fix probe deferral issues | expand |
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 --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 = {
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(-)