Re: [PATCH v1 3/7] drm/bridge: Add drm_bridge_new_crtc_state() helper

Maxime Ripard
2021-07-22T07:32:38Z

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@ravnborg.org>
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
> 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