[PATCH 2/5] drm/simpledrm: Use drm_atomic_get_new_plane_state()

[Ruhl, Michael J]

>-----Original Message-----
>From: Thomas Zimmermann <tzimmermann at suse.de>
>Sent: Friday, September 23, 2022 3:53 AM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>; javierm at redhat.com;
>airlied at linux.ie; daniel at ffwll.ch
>Cc: dri-devel at lists.freedesktop.org
>Subject: Re: [PATCH 2/5] drm/simpledrm: Use
>drm_atomic_get_new_plane_state()
>
>Hi
>
>Am 22.09.22 um 18:12 schrieb Ruhl, Michael J:
>>> -----Original Message-----
>>> From: dri-devel <dri-devel-bounces at lists.freedesktop.org> On Behalf Of
>>> Thomas Zimmermann
>>> Sent: Thursday, September 22, 2022 9:10 AM
>>> To: javierm at redhat.com; airlied at linux.ie; daniel at ffwll.ch
>>> Cc: Thomas Zimmermann <tzimmermann at suse.de>; dri-
>>> devel at lists.freedesktop.org
>>> Subject: [PATCH 2/5] drm/simpledrm: Use
>>> drm_atomic_get_new_plane_state()
>>>
>>> Lookup the plane's state in atomic_update with the helper
>>> drm_atomic_get_new_plane_state(). Also rename the helpers'
>>> state arguments. No functional changes.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>>> ---
>>> drivers/gpu/drm/tiny/simpledrm.c | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c
>>> b/drivers/gpu/drm/tiny/simpledrm.c
>>> index 51d01e34d5eb..14782a50f816 100644
>>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>>> @@ -470,10 +470,10 @@ static const uint64_t
>>> simpledrm_primary_plane_format_modifiers[] = {
>>> };
>>>
>>> static void simpledrm_primary_plane_helper_atomic_update(struct
>>> drm_plane *plane,
>>> -							 struct
>>> drm_atomic_state *old_state)
>>> +							 struct
>>> drm_atomic_state *state)
>>> {
>>> -	struct drm_plane_state *plane_state = plane->state;
>>> -	struct drm_plane_state *old_plane_state =
>drm_atomic_get_old_plane_state(old_state, plane);
>>> +	struct drm_plane_state *plane_state =
>drm_atomic_get_new_plane_state(state, plane);
>>
>> Going from plane->state to drm_atomic_get_new_plane_state seems like a
>slight function change.
>>
>> If this is the equivalent and the "right" way to do this, should the ->state
>part of the data
>> structure be pruned?
>
>The plane->state field is still relevant. I recently learned that the
>state-lookup helpers are supposed to be used in all atomic_check/commit
>functions. Ville gave a description here:
>
>   https://lore.kernel.org/dri-devel/Yx9pij4LmFHrq81V@intel.com/
>
>>
>> The comment for drm_atomic_get_new_plane_state also says that it can
>return NULL.
>>
>> would plane->state be NULL in this case?
>
>I don't think so. In atomic_update, we know that we're supposed to
>change the plane. That requires the state. Maybe it's different in the
>plane's atomic_disable helper, we don't access the state there.

Ok, that makes sense.

If you need it:

Reviewed-by: Michael J. Ruhl <michael.j.ruhl at intel.com>

M


>Best regards
>Thomas
>
>>
>> Thanks,
>>
>> M
>>
>>> +	struct drm_plane_state *old_plane_state =
>drm_atomic_get_old_plane_state(state, plane);
>>> 	struct drm_shadow_plane_state *shadow_plane_state =
>to_drm_shadow_plane_state(plane_state);
>>> 	struct drm_framebuffer *fb = plane_state->fb;
>>> 	struct drm_device *dev = plane->dev;
>>> @@ -503,7 +503,7 @@ static void
>>> simpledrm_primary_plane_helper_atomic_update(struct drm_plane
>*plane
>>> }
>>>
>>> static void simpledrm_primary_plane_helper_atomic_disable(struct
>>> drm_plane *plane,
>>> -							  struct
>>> drm_atomic_state *old_state)
>>> +							  struct
>>> drm_atomic_state *state)
>>> {
>>> 	struct drm_device *dev = plane->dev;
>>> 	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
>>> --
>>> 2.37.3
>>
>
>--
>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