Hi Am 05.09.22 um 13:06 schrieb Javier Martinez Canillas:
Hello Thomas, On 9/5/22 12:57, Thomas Zimmermann wrote:Hi Javier Am 31.08.22 um 13:12 schrieb Javier Martinez Canillas:The simpledrm_primary_plane_helper_atomic_check() function is more complex than needed. It first checks drm_atomic_helper_check_plane_state() returns value to decide whether to return this or zero. But it could just return that function return value directly. It also does a check if new_plane_state->visible isn't set, but returns zero regardless. Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> --- drivers/gpu/drm/tiny/simpledrm.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c index a81f91814595..0be47f40247a 100644 --- a/drivers/gpu/drm/tiny/simpledrm.c +++ b/drivers/gpu/drm/tiny/simpledrm.c @@ -485,21 +485,14 @@ static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane, struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane); struct drm_crtc *new_crtc = new_plane_state->crtc; struct drm_crtc_state *new_crtc_state = NULL; - int ret;if (new_crtc)new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);- ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,- DRM_PLANE_NO_SCALING, - DRM_PLANE_NO_SCALING, - false, false); - if (ret) - return ret; - else if (!new_plane_state->visible) - return 0; - - return 0; + return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state, + DRM_PLANE_NO_SCALING, + DRM_PLANE_NO_SCALING, + false, false);I'm undecided on this change. I know it's correct and more to the point. But the call's logic is non-intuitive: the call either returns an error or we have to test ->visible afterwards. So I wrote it explicitly.Yes, but the check has no effect so I found it even less intuitive. Maybe add a comment then if you wan to keep the current code?I saw that your change to ssd130x also uses the pattern. If we find more such drivers, we could implement the atomic check as a helper. I suggest drm_plane_helper_atomic_check_fixed() in drm_plane_helper.cSure. I can add a preparatory change in v2 that adds that helper and then use it in the follow-up patch.
Maybe wait for your ssd130x changes to land and then you can convert both drivers to the new helper.
Best regards Thomas -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
