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@xxxxxxxxx/
>
> [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@xxxxxxxxx/
>
--
With best wishes
Dmitry
