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

From: Thomas Zimmermann <tzimmermann@suse.de>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: David Airlie <airlied@linux.ie>,
	jfalempe@redhat.com, javierm@redhat.com,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/atomic-helper: Don't allocated plane state in CRTC check
Date: Fri, 30 Sep 2022 12:36:45 +0200	[thread overview]
Message-ID: <20a4ba1c-a813-151e-9118-702fbf9c1b3e@suse.de> (raw)
In-Reply-To: <YzbEYyUQIJX1ut1q@intel.com>

[-- Attachment #1.1: Type: text/plain, Size: 3723 bytes --]

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@suse.de>
>>>>> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()")
>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> Cc: Jocelyn Falempe <jfalempe@redhat.com>
>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>>> Cc: David Airlie <airlied@linux.ie>
>>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>>> Cc: dri-devel@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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]