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

Fri Sep 30 10:27:58 UTC 2022

Hi

Am 30.09.22 um 11:12 schrieb Ville Syrjälä:
> On Fri, Sep 30, 2022 at 11:01:52AM +0200, Thomas Zimmermann wrote:
>> 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.
> 
> What I'm asking how can you have crtc!=NULL && fb==NULL at all here?
> Some other plane state check function (can't remember which one
> specifically) should have rejected that. So either you're checking
> for impossible things, or there is a bug somewhere else.
> 
> So to me it looks like it should be enough to just check the plane_mask
> and not even bother looking at the plane state at all.

Indeed. Somehow I confused plane_mask with 'mask of all planes possible'.

Best regards
Thomas

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

-- 
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/37f26dd0/attachment.sig>