| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f34a0c76-cb61-4c5a-b16c-e6f715f1d048@linux.dev> Date: Fri, 17 Jan 2025 18:37:00 +0530 From: Aradhya Bhatia <aradhya.bhatia@...ux.dev> To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org> Cc: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>, Laurent Pinchart <laurent.pinchart@...asonboard.com>, Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>, Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Nishanth Menon <nm@...com>, Vignesh Raghavendra <vigneshr@...com>, Devarsh Thakkar <devarsht@...com>, Praneeth Bajjuri <praneeth@...com>, Udit Kumar <u-kumar1@...com>, Jayesh Choudhary <j-choudhary@...com>, DRI Development List <dri-devel@...ts.freedesktop.org>, Linux Kernel List <linux-kernel@...r.kernel.org> 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/
Powered by blists - more mailing lists