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

[Maxime Ripard <mripard@kernel.org>]

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

Hi,

On Thu, May 16, 2024 at 03:10:15PM GMT, Aradhya Bhatia wrote:
> >>  	/**
> >>  	 * @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?
> > 
> 
> That's right. I cannot use bridge_atomic_pre_enable /
> bridge_atomic_enable here. But that's because my source is CRTC, which
> gets enabled via crtc_atomic_enable.
> 
> 
> > 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.
> > 
> 
> Of course! I can elaborate on the order.
> 
> Without my patches (and given there isn't any bridge setting the
> "pre_enable_prev_first" flag) the order of enable for any single display
> chain, looks like this -
> 
> 	crtc_enable
> 	
> 	bridge[n]_pre_enable
> 	---
> 	bridge[1]_pre_enable
> 
> 	encoder_enable
> 
> 	bridge[1]_enable
> 	---
> 	bridge[n]_enable
> 
> The tidss enables at the crtc_enable level, and hence is the first
> entity with stream on. cdns-dsi doesn't stand a chance with
> bridge_atmoic_pre_enable / bridge_atmoic_enable hooks. And there is no
> bridge call happening before crtc currently.

Thanks for filling the blanks :)

I assume that since cdns-dsi is a bridge, and it only has a simple
encoder implementation, for it to receive some video signal we need to
enable the CRTC before the bridge.

If so, I think that's the original intent between the bridge pre_enable.
The original documentation had:

  pre_enable: this contains things needed to be done for the bridge
  before this contains things needed to be done for the bridge before
  this contains things needed to be done for the bridge before.

and the current one has:

  The display pipe (i.e. clocks and timing signals) feeding this bridge
  will not yet be running when this callback is called. The bridge must
  not enable the display link feeding the next bridge in the chain (if
  there is one) when this callback is called.

I would say the CRTC is such a source, even more so now that the encoder
is usually transparent, so I think we should instead move the crtc
enable call after the bridge pre_enable.

Would that work?

Maxime

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