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

[Maxime Ripard <maxime@cerno.tech>]

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