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

[Thomas Zimmermann]

Hi

Am 30.09.22 um 12:26 schrieb Ville Syrjälä:
> On Fri, Sep 30, 2022 at 12:12:09PM +0300, Ville Syrjälä wrote:
>> 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.
> 
> Oh and btw, fb != NULL doesn't guarantee the plane is actually
> visible (could have been fully clipped), if that is what you're
> trying to check here.
> 

No, it's really just about having a primary plane enabled on the CRTC.

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
-------------- 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/84f7468f/attachment.sig>