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?
> 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.
> +}
> +
> /**
> * drm_atomic_get_existing_connector_state - get connector state, if it exists
> * @state: global atomic state object
> --
> 2.37.3
--
Ville Syrjälä
Intel
