linux-kernel - Re: [PATCH 6/7] drm/bridge: Introduce early

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1a6b8a83-b378-4869-b536-0fca76e428bf@ti.com>
Date: Fri, 24 May 2024 16:38:13 +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,

On 21/05/24 18:45, Maxime Ripard wrote:
> 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.

Hmm, if I understand you right, the newer sequence of calls will look
like this,

	bridge[n]_pre_enable
	---
	bridge[1]_pre_enable

	crtc_enable
	encoder_enable

	bridge[1]_enable
	---
	bridge[n]_enable

I do agree with this. This makes sense. CRTC is indeed such a source,
and should ideally be enabled after the bridges are pre_enabled.

> 
> Would that work?
> 

So, this could potentially work, yes. The cdns-dsi would get pre_enabled
after all bridges after cdns-dsi are pre_enabled. But over a quick test
with BBAI64 + RPi Panel, I don't see any issue.

However, the one concern that I have right now, is about breaking any
existing (albeit faulty) implementation which relies on CRTC being
enabled before the bridges are pre_enabled. =)


Regards
Aradhya