Hi Javier Am 05.10.23 um 13:37 schrieb Javier Martinez Canillas:
Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxx> Fixes: d51f9fbd98b6 ("drm/ssd130x: Store the HW buffer in the driver-private CRTC state") Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> Cc: Javier Martinez Canillas <javierm@xxxxxxxxxx> Cc: Maxime Ripard <mripard@xxxxxxxxxx> --- 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)
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
