[PATCH] drm/atomic-helper: Don't allocated plane state in CRTC check

[Thomas Zimmermann]

Hi,

thanks for reviewing.

Am 29.09.22 um 21:03 schrieb Ville Syrjälä:
> On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote:
>> In drm_atomic_helper_check_crtc_state(), do not add a new plane state
>> to the global state if it does not exist already. Adding a new plane
>> state will results in overhead for the plane during the atomic-commit
>> step.
>>
>> For the test in drm_atomic_helper_check_crtc_state() to succeed, it is
>> important that the CRTC has an enabled primary plane after the commit.
>> This can be a newly enabled plane or an already enabled plane. So if a
>> plane is not part of the commit, use the plane's existing state. The new
>> helper drm_atomic_get_next_plane_state() returns the correct instance.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()")
>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>> Cc: Jocelyn Falempe <jfalempe at redhat.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Cc: Maxime Ripard <mripard at kernel.org>
>> Cc: David Airlie <airlied at linux.ie>
>> Cc: Daniel Vetter <daniel at ffwll.ch>
>> Cc: dri-devel at lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/drm_atomic_helper.c |  4 +---
>>   include/drm/drm_atomic.h            | 20 ++++++++++++++++++++
>>   2 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 98cc3137c062..463d4f3fa443 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state,
>>   
>>   			if (plane->type != DRM_PLANE_TYPE_PRIMARY)
>>   				continue;
>> -			plane_state = drm_atomic_get_plane_state(state, plane);
>> -			if (IS_ERR(plane_state))
>> -				return PTR_ERR(plane_state);
>> +			plane_state = drm_atomic_get_next_plane_state(state, plane);
>>   			if (plane_state->fb && plane_state->crtc) {
> 
> Hmm. Why this is even looking at these. If the plane is in the crtc's
> plane_mask then surely plane_state->crtc must already point to this
> crtc? And I don't think ->fb and ->crtc are allowed to disagree, so
> if one is set then surely the other one must be as well or we'd
> return an error at some point somewhere?

Yeah, the crtc test is done for keeping consistency. Other places also 
sometimes validate that these fields don't disagree. I'll remove the 
crtc test in the next version. The fb test is the important one.

> 
>>   				has_primary_plane = true;
>>   				break;
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index 10b1990bc1f6..4006f2d459e3 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -623,6 +623,26 @@ drm_atomic_get_new_plane_state(struct drm_atomic_state *state,
>>   	return state->planes[drm_plane_index(plane)].new_state;
>>   }
>>   
>> +/**
>> + * drm_atomic_get_next_plane_state - get plane state after atomic commit
>> + * @state: global atomic state object
>> + * @plane: plane to grab
>> + *
>> + * This function returns the plane state that the given plane will have
>> + * after the atomic commit. This will be the new plane state if the plane
>> + * is part of the global atomic state, or the current state otherwise.
>> + */
>> +static inline struct drm_plane_state *
>> +drm_atomic_get_next_plane_state(struct drm_atomic_state *state,
>> +				struct drm_plane *plane)
>> +{
>> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
>> +
>> +	if (plane_state)
>> +		return plane_state;
>> +	return plane->state;
> 
> You're not holding the appropriate lock so that does not look safe. Even
> if you know somewhere, somehow this sort of thing to be safe I don't
> think it's a good idea to promote this with any kind of official
> function because someone will shoot themselves in the foot with it.

OK. It'll be inlined with locking added.

Best regards
Thomas

> 
>> +}
>> +
>>   /**
>>    * drm_atomic_get_existing_connector_state - get connector state, if it exists
>>    * @state: global atomic state object
>> -- 
>> 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/20220930/b72ebd1a/attachment.sig>