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