Message-ID: <CAFBinCCVL5idjip8NtDTimid+H0xmoMT1807SBj0-dFaH+hbkQ@mail.gmail.com>
Date: Wed, 25 Sep 2024 22:10:25 +0200
From: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To: Anastasia Belova <abelova@...ralinux.ru>
Cc: Neil Armstrong <neil.armstrong@...aro.org>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, 
	Kevin Hilman <khilman@...libre.com>, Jerome Brunet <jbrunet@...libre.com>, 
	dri-devel@...ts.freedesktop.org, linux-amlogic@...ts.infradead.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	lvc-project@...uxtesting.org, stable@...r.kernel.org
Subject: 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@...ralinux.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

Powered by blists - more mailing lists