Hi,
On Thu, Jul 22, 2021 at 08:22:42AM +0200, Sam Ravnborg wrote:
> drm_bridge_new_crtc_state() will be used by bridge drivers to provide
> easy access to the mode from the drm_bridge_funcs operations.
>
> The helper will be useful in the atomic operations of
> struct drm_bridge_funcs.
>
> Signed-off-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
> Suggested-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> Cc: Robert Foss <robert.foss@xxxxxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> ---
> drivers/gpu/drm/drm_atomic.c | 34 ++++++++++++++++++++++++++++++++++
> include/drm/drm_atomic.h | 3 +++
> 2 files changed, 37 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a8bbb021684b..93d513078e9a 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1133,6 +1133,40 @@ drm_atomic_get_new_bridge_state(struct drm_atomic_state *state,
> }
> EXPORT_SYMBOL(drm_atomic_get_new_bridge_state);
>
> +/**
> + * drm_bridge_new_crtc_state - get crtc_state for the bridge
> + * @bridge: bridge object
> + * @old_bridge_state: state of the bridge
> + *
> + * This function returns the &struct drm_crtc_state for the given bridge/state,
> + * or NULL if no crtc_state could be looked up. In case no crtc_state then this is
> + * logged using WARN as the crtc_state is always expected to be present.
> + * This function is often used in the &struct drm_bridge_funcs operations.
> + */
> +const struct drm_crtc_state *
> +drm_bridge_new_crtc_state(struct drm_bridge *bridge,
Shouldn't we call this drm_atomic_bridge_get_new_crtc_state for consistency?
> + struct drm_bridge_state *old_bridge_state)
It seems odd to me to name it old_bridge_state, I guess it would make
more sense to pass in the new bridge state?
> +{
> + struct drm_atomic_state *state = old_bridge_state->base.state;
> + const struct drm_connector_state *conn_state;
> + const struct drm_crtc_state *crtc_state;
> + struct drm_connector *connector;
> +
> + connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
> + if (WARN_ON(!connector))
> + return NULL;
> +
> + conn_state = drm_atomic_get_new_connector_state(state, connector);
> + if (WARN_ON(!conn_state || !conn_state->crtc))
> + return NULL;
> +
> + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
> + if (WARN_ON(!crtc_state))
> + return NULL;
> +
> + return crtc_state;
You don't even seem to use the bridge state itself, so maybe we just
need to pass the drm_atomic_state? And thus we end up with something
like drm_atomic_get_new_connector_for_encoder, so maybe we should just
call it drm_atomic_get_new_crtc_for_bridge?
Also, can we end up with a commit that affects the bridge state but not
the crtc state (like a connector property change)? In such a case
drm_atomic_get_new_crtc_state would return NULL.
I'm not sure if it's a big deal or not, but we should make it clear in
the documentation and remove the WARN_ON.
Maxime
