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