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

[Thomas Zimmermann]

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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220923/3118bac2/attachment.sig>