Messages in this thread | | | Date | Mon, 12 Sep 2022 16:22:49 +0200 | Subject | Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper | From | Thomas Zimmermann <> |
| |
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.
If you call drm_atomic_get_old_plane_state() from atomic_check(), what will it return?
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 [unhandled content-type:application/pgp-signature] | |