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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 5143 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]