[PATCH v4 6/7] drm/ssd130x: Fix atomic_check for disabled planes

[Geert Uytterhoeven]

Hi Thomas,

On Thu, Oct 5, 2023 at 11:05 AM Thomas Zimmermann <tzimmermann at suse.de> wrote:
> The plane's atomic_check returns -EINVAL if the CRTC has not been
> set. This is the case for disabled planes, for which atomic_check
> should return 0. For disabled planes, it also omits the mandatory
> call to drm_atomic_helper_check_plane_state().
>
> Replace the test with the boiler-plate code that first invokes
> drm_atomic_helper_check_plane_state() and then tests for the plane
> to be visible. Return early for non-visible planes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> Fixes: d51f9fbd98b6 ("drm/ssd130x: Store the HW buffer in the driver-private CRTC state")

Thanks for your patch!

> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -639,21 +639,22 @@ static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
>         struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
>         struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
>         struct drm_crtc *crtc = plane_state->crtc;
> -       struct drm_crtc_state *crtc_state;
> +       struct drm_crtc_state *crtc_state = NULL;
>         const struct drm_format_info *fi;
>         unsigned int pitch;
>         int ret;
>
> -       if (!crtc)
> -               return -EINVAL;
> -
> -       crtc_state = drm_atomic_get_crtc_state(state, crtc);
> -       if (IS_ERR(crtc_state))
> -               return PTR_ERR(crtc_state);
> +       if (crtc)
> +               crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>
> -       ret = drm_plane_helper_atomic_check(plane, state);
> +       ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> +                                                 DRM_PLANE_NO_SCALING,
> +                                                 DRM_PLANE_NO_SCALING,
> +                                                 false, false);

Aren't the above 6 new lines identical to the call to
drm_plane_helper_atomic_check()? So why duplicate that?

>         if (ret)
>                 return ret;

Insert blank line?

> +       else if (!plane_state->visible)

"else" not needed.

> +               return 0;
>
>         fi = drm_format_info(DRM_FORMAT_R1);
>         if (!fi)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds