[PATCH v7 11/12] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable

[Dmitry Baryshkov]

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)

#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).

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/
> 

-- 
With best wishes
Dmitry