Messages in this thread | | | Date | Mon, 12 Sep 2022 14:05:36 +0200 | Subject | Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper | From | Thomas Zimmermann <> |
| |
Hi
Am 12.09.22 um 13:18 schrieb Ville Syrjälä: > On Mon, Sep 12, 2022 at 01:05:45PM +0200, Thomas Zimmermann wrote: >> 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. > > There is no old or new drm_atomic_state. It contains both.
I (vaguely) remember a bug where a driver tried drm_atomic_get_new_plane_state() with the (old) state that's passed to atomic_update. It didn't return the expected results and modesetting gave slightly wrong results. So we began to be more precise about new and old. And whatever is stored in 'plane->state' is then just 'the state'.
I understand that the semantics of atomic_check are different from atomic_update, but it still doesn't hurt to talk of new_state IMHO.
Best regards Thomas
> >> >> 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 > > > >
-- 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] | |