Re: [PATCH v2] drm/meson: switch to a managed drm device

Hi Anastasia,

thank you for picking this up and apologies for my late reply.

On Tue, Sep 10, 2024 at 5:17 PM Anastasia Belova abelova@astralinux.ru wrote: [...]

@@ -215,7 +210,7 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) regs = devm_platform_ioremap_resource_byname(pdev, "vpu"); if (IS_ERR(regs)) { ret = PTR_ERR(regs);

•           goto free_drm;
  

•           goto remove_encoders;
  

can't we just return PTR_ERR(regs) here since all resources above are now automatically free (as they are devm_* managed)?

    }

    priv->io_base = regs;

@@ -223,13 +218,13 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hhi"); if (!res) { ret = -EINVAL;

•           goto free_drm;
  

•           goto remove_encoders;
  

same here, can't we just return -EINVAL directly?

    }
    /* Simply ioremap since it may be a shared register zone */
    regs = devm_ioremap(dev, res->start, resource_size(res));
    if (!regs) {
            ret = -EADDRNOTAVAIL;

•           goto free_drm;
  

•           goto remove_encoders;
  

also just return -EADDRNOTAVAIL directly

    }

    priv->hhi = devm_regmap_init_mmio(dev, regs,

@@ -237,18 +232,18 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) if (IS_ERR(priv->hhi)) { dev_err(&pdev->dev, "Couldn't create the HHI regmap\n"); ret = PTR_ERR(priv->hhi);

either return PTR_ERR(priv->hhi) here or convert the dev_err to dev_err_probe and return it's value

•           goto free_drm;
  

•           goto remove_encoders;
    }
  
    priv->canvas = meson_canvas_get(dev);
    if (IS_ERR(priv->canvas)) {
            ret = PTR_ERR(priv->canvas);
  

•           goto free_drm;
  

•           goto remove_encoders;
  

return PTR_ERR(priv->canvas);

    }

    ret = meson_canvas_alloc(priv->canvas, &priv->canvas_id_osd1);
    if (ret)

•           goto free_drm;
  

•           goto remove_encoders;
  

meson_canvas_alloc() is the first non devm_* managed allocation. so this is the last time we can simply "return ret;", starting with the next error case we need goto for cleaning up resources.

    ret = meson_canvas_alloc(priv->canvas, &priv->canvas_id_vd1_0);
    if (ret)
            goto free_canvas_osd1;

(starting from here the goto free_... is needed and this one is actually already correct)

@@ -261,7 +256,7 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)

    priv->vsync_irq = platform_get_irq(pdev, 0);

•   ret = drm_vblank_init(drm, 1);
  

•   ret = drm_vblank_init(&priv->drm, 1);
    if (ret)
            goto free_canvas_vd1_2;
  
  

@@ -284,10 +279,10 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) ret = drmm_mode_config_init(drm); if (ret) goto free_canvas_vd1_2;

•   drm->mode_config.max_width = 3840;
  

•   drm->mode_config.max_height = 2160;
  

•   drm->mode_config.funcs = &meson_mode_config_funcs;
  

•   drm->mode_config.helper_private = &meson_mode_config_helpers;
  

•   priv->drm.mode_config.max_width = 3840;
  

•   priv->drm.mode_config.max_height = 2160;
  

•   priv->drm.mode_config.funcs = &meson_mode_config_funcs;
  

•   priv->drm.mode_config.helper_private = &meson_mode_config_helpers;
  
    /* Hardware Initialization */
  
  

@@ -308,9 +303,9 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) goto exit_afbcd;

    if (has_components) {

•           ret = component_bind_all(dev, drm);
  

•           ret = component_bind_all(dev, &priv->drm);
            if (ret) {
  

•                   dev_err(drm->dev, "Couldn't bind all components\n");
  

•                   dev_err(priv->drm.dev, "Couldn't bind all components\n");
                    /* Do not try to unbind */
                    has_components = false;
                    goto exit_afbcd;
  

just below this we have: ret = meson_encoder_hdmi_probe(priv); if (ret) goto exit_afbcd; I think this is the place where we need to call component_unbind_all, so we need some kind of "goto unbind_components;" here. All other "goto exit_afbcd;" below will need to be converted to "goto unbind_components;" (or whichever name you choose) as well.

Also the ordering where component_unbind_all() is incorrect. It's been incorrect even before your patch - but maybe now is the right time to clean it up?

I had to double check because I was surprised about the calls to meson_encoder_{cvbs,dsi,hdmi}_remove(priv); It turns out that it's safe to call these three functions at any time because they only ever do something if their _probe() counterpart has been called.

Best regards, Martin