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@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/ >> > Regards Aradhya
