Re: [PATCH 6/7] drm/bridge: Introduce early_enable and late disable

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4716 bytes --]

On Sat, May 11, 2024 at 09:00:50PM +0530, Aradhya Bhatia wrote:
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 4baca0d9107b..40f93230abb2 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -206,6 +206,20 @@ struct drm_bridge_funcs {
>  	 */
>  	void (*post_disable)(struct drm_bridge *bridge);
>  
> +	/**
> +	 * @late_disable:
> +	 *
> +	 * This callback should disable the bridge. It is called right after the
> +	 * preceding element in the display pipe is disabled. If the preceding
> +	 * element is a bridge this means it's called after that bridge's
> +	 * @atomic_post_disable. If the preceding element is a &drm_crtc it's
> +	 * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable
> +	 * hook.
> +	 *
> +	 * The @late_disable callback is optional.
> +	 */
> +	void (*late_disable)(struct drm_bridge *bridge);
> +
>  	/**
>  	 * @mode_set:
>  	 *
> @@ -235,6 +249,26 @@ struct drm_bridge_funcs {
>  	void (*mode_set)(struct drm_bridge *bridge,
>  			 const struct drm_display_mode *mode,
>  			 const struct drm_display_mode *adjusted_mode);
> +
> +	/**
> +	 * @early_enable:
> +	 *
> +	 * This callback should enable the bridge. It is called right before
> +	 * the preceding element in the display pipe is enabled. If the
> +	 * preceding element is a bridge this means it's called before that
> +	 * bridge's @early_enable. If the preceding element is a &drm_crtc it's
> +	 * called right before the crtc's &drm_crtc_helper_funcs.atomic_enable
> +	 * hook.
> +	 *
> +	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
> +	 * will not yet be running when this callback is called. The bridge can
> +	 * enable the display link feeding the next bridge in the chain (if
> +	 * there is one) when this callback is called.
> +	 *
> +	 * The @early_enable callback is optional.
> +	 */
> +	void (*early_enable)(struct drm_bridge *bridge);
> +

You don't need the legacy option here, just go straight for the atomic one.

>  	/**
>  	 * @pre_enable:
>  	 *
> @@ -285,6 +319,26 @@ struct drm_bridge_funcs {
>  	 */
>  	void (*enable)(struct drm_bridge *bridge);
>  
> +	/**
> +	 * @atomic_early_enable:
> +	 *
> +	 * This callback should enable the bridge. It is called right before
> +	 * the preceding element in the display pipe is enabled. If the
> +	 * preceding element is a bridge this means it's called before that
> +	 * bridge's @atomic_early_enable. If the preceding element is a
> +	 * &drm_crtc it's called right before the crtc's
> +	 * &drm_crtc_helper_funcs.atomic_enable hook.
> +	 *
> +	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
> +	 * will not yet be running when this callback is called. The bridge can
> +	 * enable the display link feeding the next bridge in the chain (if
> +	 * there is one) when this callback is called.
> +	 *
> +	 * The @early_enable callback is optional.
> +	 */
> +	void (*atomic_early_enable)(struct drm_bridge *bridge,
> +				    struct drm_bridge_state *old_bridge_state);
> +
>  	/**
>  	 * @atomic_pre_enable:
>  	 *
> @@ -361,6 +415,21 @@ struct drm_bridge_funcs {
>  	void (*atomic_post_disable)(struct drm_bridge *bridge,
>  				    struct drm_bridge_state *old_bridge_state);
>  
> +	/**
> +	 * @atomic_late_disable:
> +	 *
> +	 * This callback should disable the bridge. It is called right after the
> +	 * preceding element in the display pipe is disabled. If the preceding
> +	 * element is a bridge this means it's called after that bridge's
> +	 * @atomic_late_disable. If the preceding element is a &drm_crtc it's
> +	 * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable
> +	 * hook.
> +	 *
> +	 * The @atomic_late_disable callback is optional.
> +	 */
> +	void (*atomic_late_disable)(struct drm_bridge *bridge,
> +				    struct drm_bridge_state *old_bridge_state);
> +

But more importantly, I don't quite get the use case you're trying to
solve here.

If I got the rest of your series, the Cadence DSI bridge needs to be
powered up before its source is started. You can't use atomic_enable or
atomic_pre_enable because it would start the source before the DSI
bridge. Is that correct?

If it is, then how is it different from what
drm_atomic_bridge_chain_pre_enable is doing? The assumption there is
that it starts enabling bridges last to first, to it should be enabled
before anything starts.

The whole bridge enabling order code starts to be a bit of a mess, so it
would be great if you could list all the order variations we have
currently, and why none work for cdns-dsi.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]