On Wed, May 27, 2026 at 05:09:39PM +0200, Hans de Goede wrote: > Hi Maxime, > > On 27-May-26 14:48, Maxime Ripard wrote: > > On Wed, May 27, 2026 at 02:21:50PM +0200, Hans de Goede wrote: > >>>> clk_prepare_enable() is fine, the clocks are already on so no > >>>> ordering worries and it ensures that the clocks and their parents cannot > >>>> get turned off by incrementing their enable, prepare and protect counts. > >>>> > >>>> clk_disable_unprepare() is a problem though since it actually turns the > >>>> clocks off. Instead something is needed which only decrements those > >>>> counts. > >>>> > >>>> This series introduces a new clk-core function called > >>>> __clk_disable_unprepare_counts_only() (1) which does just that. Prefixed > >>>> with '__' to indicate that normally drivers should not use this. > >>>> > >>>> Michael, Stephen sorry for needing to add a new clk-core function for > >>>> this, > >>>> but I see no other way of solving this (2)(3). The changes are not that > >>>> big / not too bad. > >>>> > >>>> I've also considered making __clk_disable_unprepare_counts_only() > >>>> implement > >>>> all the logic itself instead of adding the extra parameter to > >>>> clk_core_unprepare() and clk_core_disable() but that leads in duplicating > >>>> quite a bit of logic (4) so this seems better. > >>>> > >>>> The other 2 patches just replace the clk_disable_unprepare() calls in > >>>> the simple[fb|drm] driver with the new helper. > >>>> > >>>> This series fixes the display not coming up after switching to the msm > >>>> driver when a simple-framebuffer node with clocks listed is used. > >>>> > >>>> 1) I'm open to changing the name, this is the best I could come up with. > >>>> > >>>> 2) One option considered was detaching the simple-framebuffer driver > >>>> later, > >>>> after the real display driver has had a chance to claim the clocks. > >>>> But > >>>> this won't work in cases where the real display driver picks different > >>>> parent clocks then the boot firmware did and needs to reparent clocks. > >>>> > >>>> Basically the goal is for things to behave as if the > >>>> simple-framebuffer > >>>> driver was not there at all, because that leaves the hw in the state > >>>> the real display driver expects. > >>> > >>> So I know it's not really what you had in mind, but if you are only > >>> concerned about the transition from the bootloader to the DRM driver, > >>> then I think supporting the following work would help: > >>> > >>> https://lore.kernel.org/r/[email protected] > >>> > >>> With that series, we can build the initial KMS state from the hardware > >>> state, which means that if you were to change the resolution at boot, it > >>> would be executed just like any mode change in KMS. > >> > >> Hmm, so your suggestion would be to have the initial KMS driver probe() > >> only do a read back without touching anything. Then claim clks matching > >> the read back config and then only release the simple* driver and thus > >> the clocks after this? > > > > Almost. Part of the call to drm_mode_config_create_initial_state() if > > needed to make it grab its resources. See atomic_install_state in that > > series. So probe wouldn't have anything more to do. > > > >> That is certainly an interesting proposal but IMHO almost orthogonal > >> to this one (1). > > > > Why do you think it's orthogonal? > > It would completely fix your problem. > > AFAICT in its current version it does not contain a mechanism to > delay the remove conflicting framebuffer call, so no it does not > fix my problem.
The conflicting framebuffer is removed by drivers through an explicit call to aperture_remove_all_conflicting_devices(). Just move that call after the call to drm_mode_config_create_initial_state, there's nothing to integrate. > >> Even if it may fix the issue in the end, it seems that that work is > >> still quite a way from going upstream > > > > It's likely to be merged in the next few weeks. > > > >> and even then initially it only potentially fixes this for the TIDSS > >> driver since that solution needs a lot of per driver work. > > > > I mean, yeah, but the good thing is that you only have one driver to > > care about, right? > > I would prefer my proposed KISS solution, which works everywhere, over > your quite complex solution which requires complex per driver changes. > > When I was still working on: > > https://fedoraproject.org/wiki/Changes/FlickerFreeBoot > > I needed to constantly fix i915 fastset support and when I stopped > fixing it due to -ENOTIME it pretty much was broken most of the time. > > Last time I check modern (Alderlake +) laptops never had working > fastset support and there always is a visible most set on newer > laptop models because of this. > > This state-readback stuff is quite complicated, as the list of > known issues in your cover-letter shows. It's kind of a bad faith argument. This list is 4 issues. One is getting fixed in the next version, the other two are specific to tidss which you don't care about. > I wish you all the best with it, but I really do not see this as a > good alternative for fixing the issues *this* series tries to address > given the huge disparity in complexity between the 2 fixes. > > I'm also worried that the readback stuff is likely going to be fragile > so might end up being disabled or automatically detect some issue and > disable itself at which point we're back to having the original > problem this series tries to fix. > > Also atm AFAIK there are no plans to add state readback support to > msm and there are a lot of other higher priority items to work on. > > ### > > Anyways since your patch-series will not work for msm in its > current state, I would still very much like to hear from the clock > maintainers what they think of the approach suggested here. I really don't get this argument either. Sure, it doesn't work for msm. But the clock framework didn't have that unsafe function call either, and yet you did the work to add it. Maxime
signature.asc
Description: PGP signature
