Messages in this thread |  | | Date | Fri, 17 Jan 2025 18:37:00 +0530 | | From | Aradhya Bhatia <> | | Subject | Re: [PATCH v7 11/12] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable |
| |
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@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/
|  |