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

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

-- 
Ville Syrjälä
Intel