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

[Javier Martinez Canillas <javierm@redhat.com>]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> Hi Javier
>
> Am 05.10.23 um 13:37 schrieb Javier Martinez Canillas:

[...]

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

Sure, let's continue this discussion there.

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

Thanks for the explanation.

> So can you disable the plane now?
>

It does not get disabled now indeed.

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

You are correct, there's no NULL-deref and in fact you are fixing the
plane not disable bug, so your fix is correct and should be applied.

I still prefer as mentioned keeping the drm_plane_helper_atomic_check()
call instead of open coding it, but regardless of what is decided:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat