Messages in this thread | | | Date | Mon, 12 Sep 2022 13:05:45 +0200 | Subject | Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper | From | Thomas Zimmermann <> |
| |
Hi
Am 12.09.22 um 12:40 schrieb Ville Syrjälä: > On Mon, Sep 12, 2022 at 12:15:22PM +0200, Javier Martinez Canillas wrote: >> Provides a default plane state check handler for primary planes that are a >> fullscreen scanout buffer and whose state scale and position can't change. >> >> There are some drivers that duplicate this logic in their helpers, such as >> simpledrm and ssd130x. Factor out this common code into a plane helper and >> make drivers use it. >> >> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >> --- >> >> drivers/gpu/drm/drm_plane_helper.c | 29 +++++++++++++++++++++++++++++ >> drivers/gpu/drm/solomon/ssd130x.c | 18 +----------------- >> drivers/gpu/drm/tiny/simpledrm.c | 25 +------------------------ >> include/drm/drm_plane_helper.h | 2 ++ >> 4 files changed, 33 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c >> index c7785967f5bf..fb41eee74693 100644 >> --- a/drivers/gpu/drm/drm_plane_helper.c >> +++ b/drivers/gpu/drm/drm_plane_helper.c >> @@ -278,3 +278,32 @@ void drm_plane_helper_destroy(struct drm_plane *plane) >> kfree(plane); >> } >> EXPORT_SYMBOL(drm_plane_helper_destroy); >> + >> +/** >> + * drm_plane_helper_atomic_check() - Helper to check primary planes states >> + * @plane: plane to check >> + * @new_state: plane state to check > > That is not a plane state. Also should s/new_// since it's just > the overall atomic state thing rather than some new or old state.
Using only 'state' is non-intuitive and has lead to bugs where sub-state was retrieved from the wrong state information. So we've been using 'new_state' and 'old_state' explicitly in several places now.
Best regards Thomas
> >> + * >> + * Provides a default plane state check handler for primary planes whose atomic >> + * state scale and position is not expected to change because the primary plane >> + * is always a fullscreen scanout buffer. >> + * >> + * RETURNS: >> + * Zero on success, or an errno code otherwise. >> + */ >> +int drm_plane_helper_atomic_check(struct drm_plane *plane, >> + struct drm_atomic_state *new_state) >> +{ >> + 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; >> + >> + if (new_crtc) >> + new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc); >> + >> + return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state, >> + DRM_PLANE_NO_SCALING, >> + DRM_PLANE_NO_SCALING, >> + false, false); >> +} >> +EXPORT_SYMBOL(drm_plane_helper_atomic_check); >> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c >> index 79e8e2017c68..28cf9c87f86d 100644 >> --- a/drivers/gpu/drm/solomon/ssd130x.c >> +++ b/drivers/gpu/drm/solomon/ssd130x.c >> @@ -565,22 +565,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m >> return ret; >> } >> >> -static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane, >> - struct drm_atomic_state *new_state) >> -{ >> - 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; >> - >> - if (new_crtc) >> - new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc); >> - >> - return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state, >> - DRM_PLANE_NO_SCALING, >> - DRM_PLANE_NO_SCALING, >> - false, false); >> -} >> - >> static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane, >> struct drm_atomic_state *old_state) >> { >> @@ -623,7 +607,7 @@ static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane, >> >> static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = { >> DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, >> - .atomic_check = ssd130x_primary_plane_helper_atomic_check, >> + .atomic_check = drm_plane_helper_atomic_check, >> .atomic_update = ssd130x_primary_plane_helper_atomic_update, >> .atomic_disable = ssd130x_primary_plane_helper_atomic_disable, >> }; >> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c >> index 777ccd250871..ea5b3239a659 100644 >> --- a/drivers/gpu/drm/tiny/simpledrm.c >> +++ b/drivers/gpu/drm/tiny/simpledrm.c >> @@ -469,29 +469,6 @@ static const uint64_t simpledrm_primary_plane_format_modifiers[] = { >> DRM_FORMAT_MOD_INVALID >> }; >> >> -static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane, >> - struct drm_atomic_state *new_state) >> -{ >> - 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; >> -} >> - >> static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane, >> struct drm_atomic_state *old_state) >> { >> @@ -543,7 +520,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan >> >> static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = { >> DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, >> - .atomic_check = simpledrm_primary_plane_helper_atomic_check, >> + .atomic_check = drm_plane_helper_atomic_check, >> .atomic_update = simpledrm_primary_plane_helper_atomic_update, >> .atomic_disable = simpledrm_primary_plane_helper_atomic_disable, >> }; >> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h >> index 1781fab24dd6..7ba414655d69 100644 >> --- a/include/drm/drm_plane_helper.h >> +++ b/include/drm/drm_plane_helper.h >> @@ -41,5 +41,7 @@ int drm_plane_helper_update_primary(struct drm_plane *plane, struct drm_crtc *cr >> int drm_plane_helper_disable_primary(struct drm_plane *plane, >> struct drm_modeset_acquire_ctx *ctx); >> void drm_plane_helper_destroy(struct drm_plane *plane); >> +int drm_plane_helper_atomic_check(struct drm_plane *plane, >> + struct drm_atomic_state *new_state); >> >> #endif >> -- >> 2.37.1 >
-- 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 [unhandled content-type:application/pgp-signature] | |