Date:   Tue, 13 Sep 2022 09:40:27 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc:     David Airlie <airlied@...ux.ie>,
        Javier Martinez Canillas <javierm@...hat.com>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check()
 helper

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@...e.de>
>>>>>>>> Signed-off-by: Javier Martinez Canillas <javierm@...hat.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

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists