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

[Aradhya Bhatia <aradhya.bhatia@linux.dev>]

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

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)?

> #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?

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