Re: [PATCH 0/3] clk: Add __clk_disable_unprepare_counts_only() and use this in simple[fb|drm]

[Hans de Goede <johannes.goede@oss.qualcomm.com>]

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/20260423-drm-state-readout-v2-0-8549f87cb978@kernel.org
>>>
>>> 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.

>> 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. 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.

Regards,

Hans