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.
So to me it looks like it should be enough to just check the plane_mask
and not even bother looking at the plane state at all.
>
> >
> >> has_primary_plane = true;
> >> break;
> >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> >> index 10b1990bc1f6..4006f2d459e3 100644
> >> --- a/include/drm/drm_atomic.h
> >> +++ b/include/drm/drm_atomic.h
> >> @@ -623,6 +623,26 @@ drm_atomic_get_new_plane_state(struct drm_atomic_state *state,
> >> return state->planes[drm_plane_index(plane)].new_state;
> >> }
> >>
> >> +/**
> >> + * drm_atomic_get_next_plane_state - get plane state after atomic commit
> >> + * @state: global atomic state object
> >> + * @plane: plane to grab
> >> + *
> >> + * This function returns the plane state that the given plane will have
> >> + * after the atomic commit. This will be the new plane state if the plane
> >> + * is part of the global atomic state, or the current state otherwise.
> >> + */
> >> +static inline struct drm_plane_state *
> >> +drm_atomic_get_next_plane_state(struct drm_atomic_state *state,
> >> + struct drm_plane *plane)
> >> +{
> >> + struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> >> +
> >> + if (plane_state)
> >> + return plane_state;
> >> + return plane->state;
> >
> > You're not holding the appropriate lock so that does not look safe. Even
> > if you know somewhere, somehow this sort of thing to be safe I don't
> > think it's a good idea to promote this with any kind of official
> > function because someone will shoot themselves in the foot with it.
>
> OK. It'll be inlined with locking added.
>
> Best regards
> Thomas
>
> >
> >> +}
> >> +
> >> /**
> >> * drm_atomic_get_existing_connector_state - get connector state, if it exists
> >> * @state: global atomic state object
> >> --
> >> 2.37.3
> >
>
> --
> 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
--
Ville Syrjälä
Intel
