Date: Thu, 16 May 2024 15:10:15 +0530
From: Aradhya Bhatia <a-bhatia1@...com>
To: Maxime Ripard <mripard@...nel.org>
CC: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
        Andrzej Hajda
	<andrzej.hajda@...el.com>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Robert
 Foss <rfoss@...nel.org>,
        Laurent Pinchart
	<Laurent.pinchart@...asonboard.com>,
        Jonas Karlman <jonas@...boo.se>,
        Jernej
 Skrabec <jernej.skrabec@...il.com>,
        Maarten Lankhorst
	<maarten.lankhorst@...ux.intel.com>,
        Jyri Sarha <jyri.sarha@....fi>,
        Thomas
 Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>, Daniel
 Vetter <daniel@...ll.ch>,
        DRI Development List
	<dri-devel@...ts.freedesktop.org>,
        Linux Kernel List
	<linux-kernel@...r.kernel.org>,
        Sam Ravnborg <sam@...nborg.org>, Thierry
 Reding <treding@...dia.com>,
        Kieran Bingham
	<kieran.bingham+renesas@...asonboard.com>,
        Boris Brezillon
	<boris.brezillon@...tlin.com>,
        Nishanth Menon <nm@...com>, Vignesh
 Raghavendra <vigneshr@...com>,
        Praneeth Bajjuri <praneeth@...com>, Udit Kumar
	<u-kumar1@...com>,
        Devarsh Thakkar <devarsht@...com>,
        Jayesh Choudhary
	<j-choudhary@...com>, Jai Luthra <j-luthra@...com>
Subject: Re: [PATCH 6/7] drm/bridge: Introduce early_enable and late disable

Hi Maxime,

Thank you for reviewing the patches!

On 16/05/24 13:52, Maxime Ripard wrote:
> 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.

Got it! I can remove these in v2.

> 
>>  	/**
>>  	 * @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.

That's where the early_enable APIs come into picture. They are being
called before the crtc is even enabled, helping cdns-dsi to be enabled
before tidss. The order then looks like -


	bridge[1]_early_enable
	---
	bridge[n]_early_enable

	crtc_enable
	---
	(the order is same from here on)


Regards
Aradhya

Powered by blists - more mailing lists