lkml.org 
[lkml]   [2022]   [Sep]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper
From
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]
\
 
 \ /
  Last update: 2022-09-12 16:23    [W:0.073 / U:1.444 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site