Message-ID: <srbgele3uejml6c6dmkojc77jspuajlee5kzp5fhnpsn2drc3d@mdcbqucbgdte>
Date: Tue, 21 Jan 2025 12:50:47 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Aradhya Bhatia <aradhya.bhatia@...ux.dev>
Cc: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>, 
	Laurent Pinchart <laurent.pinchart@...asonboard.com>, Andrzej Hajda <andrzej.hajda@...el.com>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>, Jonas Karlman <jonas@...boo.se>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Nishanth Menon <nm@...com>, 
	Vignesh Raghavendra <vigneshr@...com>, Devarsh Thakkar <devarsht@...com>, 
	Praneeth Bajjuri <praneeth@...com>, Udit Kumar <u-kumar1@...com>, 
	Jayesh Choudhary <j-choudhary@...com>, DRI Development List <dri-devel@...ts.freedesktop.org>, 
	Linux Kernel List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v7 11/12] drm/atomic-helper: Re-order bridge chain
 pre-enable and post-disable

On Mon, Jan 20, 2025 at 11:18:22PM +0530, Aradhya Bhatia wrote:
> Hi Dmitry,
> 
> On 20/01/25 14:08, Dmitry Baryshkov wrote:
> > On Fri, Jan 17, 2025 at 06:37:00PM +0530, Aradhya Bhatia wrote:
> >> Hi Dmitry,
> >>
> >> On 14/01/25 16:54, Dmitry Baryshkov wrote:
> >>> On Tue, Jan 14, 2025 at 11:26:25AM +0530, Aradhya Bhatia wrote:
> >>>> Move the bridge pre_enable call before crtc enable, and the bridge
> >>>> post_disable call after the crtc disable.
> >>>>
> >>>> The sequence of enable after this patch will look like:
> >>>>
> >>>> 	bridge[n]_pre_enable
> >>>> 	...
> >>>> 	bridge[1]_pre_enable
> >>>>
> >>>> 	crtc_enable
> >>>> 	encoder_enable
> >>>>
> >>>> 	bridge[1]_enable
> >>>> 	...
> >>>> 	bridge[n]_enable
> >>>>
> >>>> And, the disable sequence for the display pipeline will look like:
> >>>>
> >>>> 	bridge[n]_disable
> >>>> 	...
> >>>> 	bridge[1]_disable
> >>>>
> >>>> 	encoder_disable
> >>>> 	crtc_disable
> >>>>
> >>>> 	bridge[1]_post_disable
> >>>> 	...
> >>>> 	bridge[n]_post_disable
> >>>>
> >>>> The definition of bridge pre_enable hook says that,
> >>>> "The display pipe (i.e. clocks and timing signals) feeding this bridge
> >>>> will not yet be running when this callback is called".
> >>>>
> >>>> Since CRTC is also a source feeding the bridge, it should not be enabled
> >>>> before the bridges in the pipeline are pre_enabled. Fix that by
> >>>> re-ordering the sequence of bridge pre_enable and bridge post_disable.
> >>>
> >>> The patch contains both refactoring of the corresponding functions and
> >>> changing of the order. Please split it into two separate commits.
> >>>
> >>
> >> I assume that you already understand this, so this is just for the
> >> record -
> >>
> >> There is no trivial way to do this. Initially, this is what I had in my
> >> mind - about what the split patches would look like.
> >>
> >> #1
> >>  * refactoring of entire loops into separate functions.
> >>  * Separate out the pre_enable and enable operations inside the
> >>    encoder_bridge_enable().
> >>  * call them in their (seemingly) original sequences
> >> 	- crtc_enable
> >> 	- encoder_bridge_enable(pre_enable)
> >> 	- encoder_bridge_enable(!pre_enable)
> >>
> >> #2
> >>  * rearrange the calls to re-order the operation
> >> 	- encoder_bridge_enable(pre_enable)
> >> 	- crtc_enable
> >> 	- encoder_bridge_enable(!pre_enable)
> >>
> >> This would have made the patch#2 seem quite trivial, as patch#1 would
> >> take the brunt of changes, while keeping the functionality intact.
> >>
> >>
> >> What I have now realized is that, the above isn't possible,
> >> unfortunately. The moment we separate out pre_enable and enable into 2
> >> different calls, the overall sequence gets even changed when there are
> >> multiple pipelines in the system.
> >>
> >> So to implement the split properly, the first patch would look like this
> >>
> >> #1
> >>  * refactoring of entire loops into separate functions.
> >>  * The calling sequences will be as follows -
> >>  	- crtc_enable()
> >> 	- encoder_bridge_enable()
> >> 		- this will do both pre_enable and enable
> >> 		  unconditionally.
> >>
> >> The pre_enable and enable operations wouldn't be separated in patch 1,
> >> just that the crtc enable and encoder bridge pre_enable/enable loops
> >> would be put into individual functions.
> >>
> >> The next patch will then introduce booleans, and separate out pre_enable
> >> and enable, and implement the new order -
> >>
> >> #2
> >>  * pre_enable and enable operations will be conditionally segregated
> >>    inside encoder_bridge_enable(), based on the pre_enable boolean.
> >>  * The calling sequences will then be updated to -
> >> 	- encoder_bridge_enable(pre_enable)
> >> 	- crtc_enable()
> >> 	- encoder_bridge_enable(!pre_enable)
> > 
> > 
> > I'd say slightly differently:
> > 
> > #1 Refactor loops into separate functions:
> >   - crtc_enable()
> >   - encoder_bridge_enable(), loops over encoders and calls
> >     pre_enable(bridges), enable(encoder), enable(bridges)
> > 
> > #2 Split loops into separate functions:
> >   - crtc_enable()
> >   - encoder_bridge_pre_enable(), loops over encoders and calls
> >     pre_enable(bridges),
> >   - encoder_bridge_enable(), loops over encoders and calls
> >     enable(encoder), enable(bridges)
> > 
> 
> When we consider setups with multiple independent displays, there are
> often multiple crtcs in the system, each with their own encoder-bridge
> chain.
> 
> In such a scenario, the sequence of crtc/encoder/bridge enable calls
> after the #2 that you suggested, will not the remain same as that after
> #1 (which is the _original_ sequence of calls).

Yes. The order of ops between display has changed, but each display is
still programmed in exactly the same way as before.

> 
> Do we still require #2 as an intermediate step between the original
> sequence, and the intended new sequence? Wouldn't having the sequence
> change in 2 half-steps add to the confusion (should some driver break
> due to either of the refactorings)?

That's the point. Having two refactorings in one commit makes it harder
to understand, which one broke the driver. Having two separate commits
allows users to revert the latter commit and check what caused the
issue.

> 
> > #3 Move crtc_enable() calls:
> >   - encoder_bridge_pre_enable(), loops over encoders and calls
> >     pre_enable(bridges),
> >   - crtc_enable()
> >   - encoder_bridge_enable(), loops over encoders and calls
> >     enable(encoder), enable(bridges)
> > 
> > You might use enum or booleans to implement encoder_bridge_pre_enable(),
> > or just keep it as a completely separate function (might be a better
> > option).
> 
> Yeah, I suppose a separate function may be better. There will, however,
> be some code duplication when we loop over the encoder twice, once for
> pre_enable(bridge) and the other for enable(encoder) and enable(bridge).
> 
> I hope that will be acceptable?

I'd prefer two separate functions, but I won't insist on that.

> 
> > 
> > The reason why I'm suggesting it is pretty easy: your patch performs two
> > refactorings at the same time. If one of the drivers breaks, we have no
> > way to identify, which of the refactorings caused the break.>
> >>
> >> This wouldn't be all that much trivial as I had imagined it to be earlier.
> >>
> >> It would also *kind of* like these patches in a previous revision,
> >> v5:11/13 [0], and v5:12/13 [1]. The only differences being,
> >>
> >> 1) these older patches separated out only the bridge/encoder operations
> >> into a function, and not the crtc operations, and
> >>
> >> 2) An enum is being used instead of the booleans.
> >>
> >>
> >> I believe this is what you are looking for? If I have misunderstood
> >> something, do let me know.
> >>
> >>
> >> Regards
> >> Aradhya
> >>
> >>
> >> [0]: v5:11/13
> >> drm/atomic-helper: Separate out Encoder-Bridge enable and disable
> >> https://lore.kernel.org/all/20241019200530.270738-4-aradhya.bhatia@linux.dev/
> >>
> >> [1]: v5:12/13
> >> drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
> >> https://lore.kernel.org/all/20241019200530.270738-5-aradhya.bhatia@linux.dev/
> >>
> > 
> 
> 
> Regards
> Aradhya
> 

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists