Hi Dmitry, On 21/01/25 16:20, Dmitry Baryshkov wrote: > 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. Yes, that's right. Sequence for each display will remain the same 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. Right. It's easier to debug each display independently in multi-display setups, and I can now understand how #2 will be able to help. This explanation helped a lot. Thank you! >> >>> #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. Alright! I have my work cut out for me for the next revision. > >> >>> >>> 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 >> > -- Regards Aradhya
