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

[Thomas Zimmermann]

Hi Javier

Am 05.10.23 um 13:37 schrieb 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.

Please see my other email.

> 
> 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().

Returning an errno code aborts the commit. [1] The CRTC can (maybe 
should?) be NULL to disable the plane. (It is in sync with 
plane_state->fb IIRC.)

So can you disable the plane now?

[1] 
https://elixir.bootlin.com/linux/v6.5/source/drivers/gpu/drm/drm_atomic_helper.c#L997

> 
> 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 ?

You have a atomic_disable in that plane, so you're taking the branch at 
[2] for disabling the plane. No atomic_update then. If the plane has 
been enabled, you should take the branch at [3]. Without being able to 
move/scale the primary plane, I don't see how plane_state->visible could 
be false here. Right?

AFAIKT there should not be a NULL-deref here. Can you do a test?

[2] 
https://elixir.bootlin.com/linux/v6.5/source/drivers/gpu/drm/drm_atomic_helper.c#L2745
[3] 
https://elixir.bootlin.com/linux/v6.5/source/drivers/gpu/drm/drm_atomic_helper.c#L2755

Best regards
Thomas

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231005/a0640217/attachment.sig>