Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[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@xxxxxxx>
> >>>>>> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> >>>>>> ---
> >>>>>>
> >>>>>>     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

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




-- 
Ville Syrjälä
Intel