[PATCH v3 6/6] drm/msm: make mdp5/dpu devices master components
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Fri Mar 25 09:34:45 UTC 2022
On 25/03/2022 00:37, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-03-23 02:25:38)
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 38627ccf3068..ab8a35e09bc9 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -381,8 +381,8 @@ static int dpu_kms_parse_data_bus_icc_path(struct dpu_kms *dpu_kms)
>> struct icc_path *path1;
>> struct drm_device *dev = dpu_kms->dev;
>>
>> - path0 = of_icc_get(dev->dev, "mdp0-mem");
>> - path1 = of_icc_get(dev->dev, "mdp1-mem");
>> + path0 = of_icc_get(dev->dev->parent, "mdp0-mem");
>
> dev->dev->parent is long
>
>> + path1 = of_icc_get(dev->dev->parent, "mdp1-mem");
>>
>> if (IS_ERR_OR_NULL(path0))
>> return PTR_ERR_OR_ZERO(path0);
>> @@ -837,6 +837,9 @@ static void dpu_kms_destroy(struct msm_kms *kms)
>> _dpu_kms_hw_destroy(dpu_kms);
>>
>> msm_kms_destroy(&dpu_kms->base);
>> +
>> + if (dpu_kms->rpm_enabled)
>> + pm_runtime_disable(&dpu_kms->pdev->dev);
>> }
>>
>> static irqreturn_t dpu_irq(struct msm_kms *kms)
>> @@ -978,7 +981,7 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
>> if (!domain)
>> return 0;
>>
>> - mmu = msm_iommu_new(dpu_kms->dev->dev, domain);
>> + mmu = msm_iommu_new(dpu_kms->dev->dev->parent, domain);
>
> And dpu_kms->dev->dev->parent is longer. Can we get some local variable
> or something that is more descriptive? I guess it is an 'mdss_dev'?
Yes, I'll fix these two usages.
>
>> if (IS_ERR(mmu)) {
>> iommu_domain_free(domain);
>> return PTR_ERR(mmu);
>> @@ -1172,40 +1175,15 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>> return rc;
>> }
>>
>> -static int dpu_kms_init(struct drm_device *dev)
>> -{
>> - struct msm_drm_private *priv;
>> - struct dpu_kms *dpu_kms;
>> - int irq;
>> -
>> - if (!dev) {
>> - DPU_ERROR("drm device node invalid\n");
>> - return -EINVAL;
>> - }
>> -
>> - priv = dev->dev_private;
>> - dpu_kms = to_dpu_kms(priv->kms);
>> -
>> - irq = irq_of_parse_and_map(dpu_kms->pdev->dev.of_node, 0);
>> - if (irq < 0) {
>> - DPU_ERROR("failed to get irq: %d\n", irq);
>> - return irq;
>> - }
>> - dpu_kms->base.irq = irq;
>> -
>> - return 0;
>> -}
>> -
>> -static int dpu_bind(struct device *dev, struct device *master, void *data)
>> +static int dpu_kms_init(struct drm_device *ddev)
>> {
>> - struct msm_drm_private *priv = dev_get_drvdata(master);
>> + struct msm_drm_private *priv = ddev->dev_private;
>> + struct device *dev = ddev->dev;
>> struct platform_device *pdev = to_platform_device(dev);
>> - struct drm_device *ddev = priv->dev;
>> struct dpu_kms *dpu_kms;
>> + int irq;
>> int ret = 0;
>>
>> - priv->kms_init = dpu_kms_init;
>> -
>> dpu_kms = devm_kzalloc(&pdev->dev, sizeof(*dpu_kms), GFP_KERNEL);
>> if (!dpu_kms)
>> return -ENOMEM;
>> @@ -1227,8 +1205,6 @@ static int dpu_bind(struct device *dev, struct device *master, void *data)
>> }
>> dpu_kms->num_clocks = ret;
>>
>> - platform_set_drvdata(pdev, dpu_kms);
>> -
>> ret = msm_kms_init(&dpu_kms->base, &kms_funcs);
>> if (ret) {
>> DPU_ERROR("failed to init kms, ret=%d\n", ret);
>> @@ -1242,31 +1218,25 @@ static int dpu_bind(struct device *dev, struct device *master, void *data)
>>
>> priv->kms = &dpu_kms->base;
>>
>> - return ret;
>> -}
>> -
>> -static void dpu_unbind(struct device *dev, struct device *master, void *data)
>> -{
>> - struct platform_device *pdev = to_platform_device(dev);
>> - struct dpu_kms *dpu_kms = platform_get_drvdata(pdev);
>> + irq = irq_of_parse_and_map(dpu_kms->pdev->dev.of_node, 0);
>
> Why doesn't platform_get_irq() work? This is code movement but I'm
> trying to understand why OF APIs are required.
Good question, I'll take a look separately (in a followup patch).
>
>> + if (irq < 0) {
>> + DPU_ERROR("failed to get irq: %d\n", irq);
>> + return irq;
>> + }
>> + dpu_kms->base.irq = irq;
>>
>> - if (dpu_kms->rpm_enabled)
>> - pm_runtime_disable(&pdev->dev);
>> + return 0;
>> }
>>
>> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
>> index 1f571372e928..ab25fff271f9 100644
>> --- a/drivers/gpu/drm/msm/msm_kms.h
>> +++ b/drivers/gpu/drm/msm/msm_kms.h
>> @@ -194,9 +194,6 @@ static inline void msm_kms_destroy(struct msm_kms *kms)
>> msm_atomic_destroy_pending_timer(&kms->pending_timers[i]);
>> }
>>
>> -extern const struct of_device_id dpu_dt_match[];
>> -extern const struct of_device_id mdp5_dt_match[];
>> -
>> #define for_each_crtc_mask(dev, crtc, crtc_mask) \
>> drm_for_each_crtc(crtc, dev) \
>> for_each_if (drm_crtc_mask(crtc) & (crtc_mask))
>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
>> index 7451105cbf01..9ecae833037d 100644
>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>> @@ -329,14 +310,7 @@ static int mdss_probe(struct platform_device *pdev)
>> if (IS_ERR(mdss))
>> return PTR_ERR(mdss);
>>
>> - priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> - if (!priv) {
>> - ret = -ENOMEM;
>> - goto fail;
>> - }
>> -
>> - priv->mdss = mdss;
>> - platform_set_drvdata(pdev, priv);
>> + platform_set_drvdata(pdev, mdss);
>>
>> /*
>> * MDP5/DPU based devices don't have a flat hierarchy. There is a top
>> @@ -350,39 +324,18 @@ static int mdss_probe(struct platform_device *pdev)
>> goto fail;
>
> Can the goto fail be removed? And replaced with
Ack, I'll do this.
>
> if (ret)
> msm_mdss_destroy(mdss)
>
> return ret;
>
>> }
>>
>> - mdp_dev = device_find_child(dev, NULL, find_mdp_node);
>> - if (!mdp_dev) {
>> - DRM_DEV_ERROR(dev, "failed to find MDSS MDP node\n");
>> - of_platform_depopulate(dev);
>> - ret = -ENODEV;
>> - goto fail;
>> - }
>> -
>> - /*
>> - * on MDP5 based platforms, the MDSS platform device is the component
>> - * that adds MDP5 and other display interface components to
>> - * itself.
>> - */
>> - ret = msm_drv_probe(dev, mdp_dev);
>> - put_device(mdp_dev);
>> - if (ret)
>> - goto fail;
>> -
>
> I see a lot of removal of 'goto fail'.
>
>> return 0;
>>
>> fail:
>> - of_platform_depopulate(dev);
>> - msm_mdss_destroy(priv->mdss);
>> + msm_mdss_destroy(mdss);
>>
>> return ret;
>> }
>>
--
With best wishes
Dmitry
More information about the dri-devel
mailing list