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

[Ville Syrjälä]

On Fri, Sep 30, 2022 at 12:36:45PM +0200, Thomas Zimmermann wrote:
> 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.

Not sure what you mean by enabled. Usually a fully clipped plane
will actually be disabled in hardware.

-- 
Ville Syrjälä
Intel