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

[Javier Martinez Canillas <javierm@redhat.com>]

Hello Ville and Thomas,

On 9/12/22 16:22, Thomas Zimmermann wrote:

[...]

>>>>>>> +
>>>>>>> +/**
>>>>>>> + * 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

Right. What's the proper name? hardware state, modeset state, atomic state ?

>>>>>> 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.
>

That's my understanding as well.
 
> 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.
>

Agree.

Maybe instead of new and old `current_state` and `commit_state` would
be more clear ?
 
>>
>> 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.
>

I would prefer if for this patch we call it either `new_state` or just
`state` to be consistent with the other helpers. And then we can as a
follow-up change the naming used by all the helpers.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat