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@xxxxxxx>
> > >> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()")
> > >> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > >> Cc: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
> > >> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > >> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> > >> Cc: David Airlie <airlied@xxxxxxxx>
> > >> Cc: Daniel Vetter <daniel@xxxxxxxx>
> > >> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > >> ---
> > >> 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
