Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxxxxx> -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
