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

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann at suse.de> writes:

Hello Thomas,

Thanks for your patch.

> 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")
> Cc: Geert Uytterhoeven <geert at linux-m68k.org>
> Cc: Javier Martinez Canillas <javierm at redhat.com>
> Cc: Maxime Ripard <mripard at kernel.org>
> ---
>  drivers/gpu/drm/solomon/ssd130x.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 3dd8e8a444b6f..dccbfe33edb5e 100644
> --- 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);

As Geert mentioned you are open coding here what the called helper already
does. I prefer to keep doing that, instead of adding boiler plate code.

One question, the reason to return -EINVAL was to prevent the callback
ssd130x_primary_plane_atomic_update() to be executed, since that attempts
to get the CRTC state to pass the HW buffer to ssd130x_fb_blit_rect().

I believe this patch will introduce a regression and cause a NULL pointer
dereference when !plane_state->crtc and you should also add a check for
plane_state->visible in ssd130x_primary_plane_atomic_update() to bail ?

I haven't tested your patch yet though, so maybe I'm wrong about this.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat