Re: [Freedreno] [PATCH v1 3/4] drm/msm/mdp4: move resource allocation to the _probe function

Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> · Fri, 2 Sep 2022 12:10:14 -0700

On 9/2/2022 12:05 PM, Dmitry Baryshkov wrote:
On Fri, 2 Sept 2022 at 21:07, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:



On 9/1/2022 11:06 PM, Dmitry Baryshkov wrote:


On 2 September 2022 03:24:17 GMT+03:00, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:


On 6/20/2022 2:30 PM, Dmitry Baryshkov wrote:
To let the probe function bail early if any of the resources is
unavailable, move resource allocattion from kms_init directly to the
probe callback. While we are at it, replace irq_of_parse_and_map() with
platform_get_irq().

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
    drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 107 +++++++++++------------
    1 file changed, 51 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 41dc60784847..6499713eccf6 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -139,8 +139,6 @@ static void mdp4_destroy(struct msm_kms *kms)
             pm_runtime_disable(dev);
             mdp_kms_destroy(&mdp4_kms->base);
-
-   kfree(mdp4_kms);
    }
      static const struct mdp_kms_funcs kms_funcs = {
@@ -383,57 +381,27 @@ static int mdp4_kms_init(struct drm_device *dev)
    {
     struct platform_device *pdev = to_platform_device(dev->dev);
     struct msm_drm_private *priv = dev->dev_private;
-   struct mdp4_kms *mdp4_kms;
+   struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(priv->kms));
     struct msm_kms *kms = NULL;
     struct iommu_domain *iommu;
     struct msm_gem_address_space *aspace;
-   int irq, ret;
+   int ret;
     u32 major, minor;
     unsigned long max_clk;
             /* TODO: Chips that aren't apq8064 have a 200 Mhz max_clk */
     max_clk = 266667000;
    -        mdp4_kms = kzalloc(sizeof(*mdp4_kms), GFP_KERNEL);
-   if (!mdp4_kms) {
-           DRM_DEV_ERROR(dev->dev, "failed to allocate kms\n");
-           return -ENOMEM;
-   }
-
     ret = mdp_kms_init(&mdp4_kms->base, &kms_funcs);
     if (ret) {
             DRM_DEV_ERROR(dev->dev, "failed to init kms\n");
             goto fail;
     }
    -        priv->kms = &mdp4_kms->base.base;
     kms = priv->kms;
             mdp4_kms->dev = dev;
    -        mdp4_kms->mmio = msm_ioremap(pdev, NULL);
-   if (IS_ERR(mdp4_kms->mmio)) {
-           ret = PTR_ERR(mdp4_kms->mmio);
-           goto fail;
-   }
-
-   irq = platform_get_irq(pdev, 0);
-   if (irq < 0) {
-           ret = irq;
-           DRM_DEV_ERROR(dev->dev, "failed to get irq: %d\n", ret);
-           goto fail;
-   }
-
-   kms->irq = irq;
-
-   /* NOTE: driver for this regulator still missing upstream.. use
-    * _get_exclusive() and ignore the error if it does not exist
-    * (and hope that the bootloader left it on for us)
-    */
-   mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd");
-   if (IS_ERR(mdp4_kms->vdd))
-           mdp4_kms->vdd = NULL;
-
     if (mdp4_kms->vdd) {
             ret = regulator_enable(mdp4_kms->vdd);
             if (ret) {
@@ -442,24 +410,6 @@ static int mdp4_kms_init(struct drm_device *dev)
             }
     }
    -        mdp4_kms->clk = devm_clk_get(&pdev->dev, "core_clk");
-   if (IS_ERR(mdp4_kms->clk)) {
-           DRM_DEV_ERROR(dev->dev, "failed to get core_clk\n");
-           ret = PTR_ERR(mdp4_kms->clk);
-           goto fail;
-   }
-
-   mdp4_kms->pclk = devm_clk_get(&pdev->dev, "iface_clk");
-   if (IS_ERR(mdp4_kms->pclk))
-           mdp4_kms->pclk = NULL;
-
-   mdp4_kms->axi_clk = devm_clk_get(&pdev->dev, "bus_clk");
-   if (IS_ERR(mdp4_kms->axi_clk)) {
-           DRM_DEV_ERROR(dev->dev, "failed to get axi_clk\n");
-           ret = PTR_ERR(mdp4_kms->axi_clk);
-           goto fail;
-   }
-
     clk_set_rate(mdp4_kms->clk, max_clk);
             read_mdp_hw_revision(mdp4_kms, &major, &minor);
@@ -474,10 +424,9 @@ static int mdp4_kms_init(struct drm_device *dev)
     mdp4_kms->rev = minor;
             if (mdp4_kms->rev >= 2) {
-           mdp4_kms->lut_clk = devm_clk_get(&pdev->dev, "lut_clk");
-           if (IS_ERR(mdp4_kms->lut_clk)) {
+           if (!mdp4_kms->lut_clk) {
                     DRM_DEV_ERROR(dev->dev, "failed to get lut_clk\n");
-                   ret = PTR_ERR(mdp4_kms->lut_clk);
+                   ret = -ENODEV;
                     goto fail;
             }
             clk_set_rate(mdp4_kms->lut_clk, max_clk);
@@ -560,7 +509,53 @@ static const struct dev_pm_ops mdp4_pm_ops = {
      static int mdp4_probe(struct platform_device *pdev)
    {
-   return msm_drv_probe(&pdev->dev, mdp4_kms_init, NULL);
+   struct device *dev = &pdev->dev;
+   struct mdp4_kms *mdp4_kms;
+   int irq;
+
+   mdp4_kms = devm_kzalloc(dev, sizeof(*mdp4_kms), GFP_KERNEL);
+   if (!mdp4_kms)
+           return dev_err_probe(dev, -ENOMEM, "failed to allocate kms\n");
+
+   mdp4_kms->mmio = msm_ioremap(pdev, NULL);
+   if (IS_ERR(mdp4_kms->mmio))
+           return PTR_ERR(mdp4_kms->mmio);
+
+   irq = platform_get_irq(pdev, 0);
+   if (irq < 0)
+           return dev_err_probe(dev, irq, "failed to get irq\n");
+
+   mdp4_kms->base.base.irq = irq;
+
+   /* NOTE: driver for this regulator still missing upstream.. use
+    * _get_exclusive() and ignore the error if it does not exist
+    * (and hope that the bootloader left it on for us)
+    */
+   mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd");
+   if (IS_ERR(mdp4_kms->vdd))
+           mdp4_kms->vdd = NULL;
+
+   mdp4_kms->clk = devm_clk_get(&pdev->dev, "core_clk");
+   if (IS_ERR(mdp4_kms->clk))
+           return dev_err_probe(dev, PTR_ERR(mdp4_kms->clk), "failed to get core_clk\n");
+
+   mdp4_kms->pclk = devm_clk_get(&pdev->dev, "iface_clk");
+   if (IS_ERR(mdp4_kms->pclk))
+           mdp4_kms->pclk = NULL;
+
+   mdp4_kms->axi_clk = devm_clk_get(&pdev->dev, "bus_clk");
+   if (IS_ERR(mdp4_kms->axi_clk))
+           return dev_err_probe(dev, PTR_ERR(mdp4_kms->axi_clk), "failed to get axi_clk\n");
+
+   /*
+    * This is required for revn >= 2. Handle errors here and let the kms
+    * init bail out if the clock is not provided.
+    */
+   mdp4_kms->lut_clk = devm_clk_get_optional(&pdev->dev, "lut_clk");
+   if (IS_ERR(mdp4_kms->lut_clk))
+           return dev_err_probe(dev, PTR_ERR(mdp4_kms->lut_clk), "failed to get lut_clk\n");

I can see that you have moved this from init to probe and only rev >=2 needs it.

But, your check here will end up returning from probe because you have a return. So I guess you means just having dev_err_probe without the return and let the init fail if the clk is not found because we have the hw_rev only in init.

No. The function called here is the devm_clk_get_optional(). So the driver will get NULL if the clock is not present in the DT and an error only in an error case (e.g. EINVAL, EPROBE_DEFER).

Later on the mdp4_kms_init() will check hw_rev and return -ENODEV if the clock is required, but is set to NULL (not present in DT).


Ok, I have understood. But dont you think this is too much convolution
just for this check? Why not leave the lut_clk in the init instead of
trying to move it to probe and add all this?

No, I don't. I saw no point in leaving the lut_clk getter unconverted.
Having everything handled in a similar way is a neat bonus from my
point of view.


Alright, its a small enough change and you have left a comment too, hence

Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>







