Messages in this thread | | | Date | Tue, 13 Sep 2022 09:40:27 +0200 | Subject | Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper | From | Thomas Zimmermann <> |
| |
Hi
Am 12.09.22 um 19:16 schrieb Ville Syrjälä: > On Mon, Sep 12, 2022 at 04:22:49PM +0200, Thomas Zimmermann wrote: >> Hi >> >> Am 12.09.22 um 14:34 schrieb Ville Syrjälä: >>> On Mon, Sep 12, 2022 at 02:05:36PM +0200, Thomas Zimmermann wrote: >>>> 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. >>> >>> As there is no wrong drm_atomic_state to pass I don't think it could >>> have been the case. >>> >>>> So we began to be more precise about new >>>> and old. And whatever is stored in 'plane->state' is then just 'the state'. >>> >>> There were certainly a lot of confusion before the explicit new/old >>> state stuff was added whether foo->state/etc. was the old or the >>> new state. And labeling things as explicitly old vs. new when passing >>> in individual object states certainly makes sense. But that doesn't >>> really have anything to do with mislabeling the overall drm_atomic_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. >>> >>> IMO it's just confusing. Makes the reader think there is somehow >>> different drm_atomic_states for old vs. new states when there isn't. >>> I also wouldn't call it new_state for .atomic_update() either. >>> >>> In both cases you have the old and new states in there and how >>> exactly they get used in the hooks is more of an implementation >>> detail. The only rules you would have to follow is that at the >>> end of .atomic_update() the hardware state matches the new state, >>> and .atomic_check() makes sure the transition from the old to the >>> new state is possible. >> >> From what I understand: >> >> In atomic_check(), plane->state is the current state and the state >> argument is the state to be validated. Calling >> drm_atomic_get_new_plane_state() will return the plane's new state. > > You should pretty much never use plane->state anywhere. Just use > drm_atomic_get_{,old,new}_plane_state() & co. Outside of exceptional > cases plane->state should only be accessed by duplicate_state() > and swap_state(). > >> >> If you call drm_atomic_get_old_plane_state() from atomic_check(), what >> will it return? > > Before swap state: > - drm_atomic_get_old_plane_state() points to the same thing > as plane->state, or NULL if the plane is not part of the > drm_atomic_state > - drm_atomic_get_new_plane_state() points to the newly > duplicated state only tracked within drm_atomic_state, > or NULL if the plane is not part of the drm_atomic_state > > After swap state: > - drm_atomic_get_old_plane_state() still points to the same > thing as before, even though plane->state no longer points there. > This old state is no longer visible outside the drm_atomic_state > and will get destoyed when the drm_atomic_state gets nuked > once the commit has been done > - drm_atomic_get_new_plane_state() still points to the same > thing as before, and now plane->state also points to it
This is exactly what I always assumed, but I remember finding a situation where this didn't work as expected. (If only I could find it again.) Anyway, as it's supposed to be the correct let's do exactly this.
Best regards Thomas
> > But all you really need to know is you have a transaction > (drm_atomic_state) and each object taking part in it > will have an old state (= the object's state before the > transaction has been commited), and new state (= the object's > state after the transaction has been commited). > >> >> In atomic_update() plane->state is the state to be committed and the >> state argument is the old state before the start of the atomic commit. >> And calling drm_atomic_get_new_plane_state() will *not* the return the >> plane's new state (i.e., the one in plane->state) IIRC. (As I mentioned, >> there was a related bug in one of the drivers.) So we began to call this >> 'old_state'. >> >> My point is: the state passed to the check and commit functions are >> different things, even though they appear to be the same. >> >>> >>> I've proposed renaming drm_atomic_state to eg. drm_atomic_transaction >>> a few times before but no one took the bait so far... >>> >> >> If you really don't like new_state, then let's call it state_tx. >> >> 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 > > > >
-- 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] | |