Re: [PATCH 24/29] drm/bridge: Provide a helper to get the global state from a bridge state

[Simona Vetter <simona.vetter@ffwll.ch>]

On Wed, Jan 15, 2025 at 10:05:31PM +0100, Maxime Ripard wrote:
> We have access to the global drm_atomic_state from a drm_bridge_state,
> but since it's fairly indirect it's not as obvious as it can be for
> other KMS entities.
> 
> Provide a helper to make it easier to figure out.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  include/drm/drm_atomic.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 31ca88deb10d262fb3a3f8e14d2afe24f8410cb1..bd7959ae312c99c0a0034d36378ae44f04f6a374 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -1183,10 +1183,26 @@ static inline struct drm_bridge_state *
>  drm_priv_to_bridge_state(struct drm_private_state *priv)
>  {
>  	return container_of(priv, struct drm_bridge_state, base);
>  }
>  
> +/**
> + * @drm_bridge_state_get_atomic_state() - Get the atomic state from a bridge state
> + * @bridge_state: bridge state object
> + *
> + * RETURNS:
> + * The global atomic state @bridge_state is a part of, or NULL if there is none.
> + */
> +static inline struct drm_atomic_state *
> +drm_bridge_state_get_atomic_state(struct drm_bridge_state *bridge_state)

So this one is nasty, because we clear out these backpointers once we push
the states into obj->state (because they can then outlive the
drm_atomic_state). Which means you can't use this in commit callbacks. Or
the bridge code has a really bad use-after-free when it doesn't clear out
these backpointers when we swap in the new states in
drm_atomic_helper_swap_state().

The better pattern is to just ditch passing individual states to callbacks
and just pass the entire drm_atomic_state container, and let callbacks
fish out what exactly they need. And also provide all necessary helpers to
find the right states and all that stuff.

We should probably also document that design approach in the kerneldoc for
drm_atomic_state, or wherever there's a good place for that.

See also my other reply for some of the history of why we have this mess.

Cheers, Sima

> +{
> +	if (!bridge_state)
> +		return NULL;
> +
> +	return bridge_state->base.state;
> +}
> +
>  struct drm_bridge_state *
>  drm_atomic_get_bridge_state(struct drm_atomic_state *state,
>  			    struct drm_bridge *bridge);
>  struct drm_bridge_state *
>  drm_atomic_get_old_bridge_state(const struct drm_atomic_state *state,
> 
> -- 
> 2.47.1
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch