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

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6207 bytes --]

On Wed, Jun 24, 2026 at 11:06:49AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 24-Jun-26 10:53, Maxime Ripard wrote:
> > On Thu, May 28, 2026 at 03:02:09PM +0200, Hans de Goede wrote:
> >> On 28-May-26 2:08 PM, Maxime Ripard wrote:
> >>> On Thu, May 28, 2026 at 12:12:10PM +0200, Hans de Goede wrote:
> >>>> Hi Brian,
> >>>>
> >>>> On 28-May-26 1:03 AM, Brian Masney wrote:
> >>>>> Hi Hans,
> >>>>>
> >>>>> On Wed, May 27, 2026 at 11:48:08AM +0200, Hans de Goede wrote:
> >>>>>> 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.
> >>>>>
> >>>>> Why won't that work in the case where different parent clocks are selected?
> >>>>> I'll describe a scenario below.
> >>>>>
> >>>>>>
> >>>>>>    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.
> >>>>>
> >>>>> I think the deferred unbinding could have some potential here where
> >>>>> there is some kind of notification mechanism between simple-framebuffer
> >>>>> and the real drm driver. So:
> >>>>>
> >>>>> - simple-framebuffer driver takes reference(s) to the clk(s).
> >>>>>
> >>>>> - real drm driver eventually loads, takes reference(s) to the necessary
> >>>>>   clk(s).
> >>>>>
> >>>>> - real drm driver sends a notification to simple-framebuffer that it's
> >>>>>   done, and has control.
> >>>>>
> >>>>> - simple-framebuffer can unbind and release its references to the clks.
> >>>>>
> >>>>> No clks will be shutdown prematurely in this scenario.
> >>>>>
> >>>>> If the real drm driver needs a different parent, then presumably things
> >>>>> should be setup correctly, and simple-framebuffer can have the clocks
> >>>>> shut down when it calls clk_disable_unprepare().
> >>>>
> >>>> If the real drm driver needs a different parent, then how does it
> >>>> do the reparenting while the simple-framebuffer driver is holding
> >>>> a reference to the clock ?  In that case the clock might have
> >>>> a protected_count of non 0 (depends on the core-clk flags) and
> >>>> reparenting won't work.
> >>>
> >>> The only case where it should reparent you listed was that you might
> >>> need to pick up a different resolution. However, that can only be
> >>> enforced by an ioctl or a client.
> >>>
> >>> simplefb/drm is removed in msm_drm_kms_init. The device is published
> >>> drm_dev_register called right after msm_drm_kms_init, and the clients
> >>> are registered in msm_drm_kms_post_init, called after drm_dev_register.
> >>>
> >>> There's no way in the current msm architecture to have a modeset happen
> >>> while the simpledrm driver is still active.
> >>
> >> Ok, new plan, please let me know what you think about this:
> >>
> >> 1. Add a new "disable" callback argument to
> >> devm_aperture_acquire_for_platform_device() and store this in
> >> struct aperture_range
> >>
> >> 2. Add a new aperture_disable_conflicting_devices() which
> >> calls the disable callback for matching devices.
> >>
> >> 3. Have the simple[fd|drm] drivers implement a disable callback
> >> which unregisters the drm dev and releases any claims on the
> >> aperture mem-region, while keeping clks, regulators, etc.
> >> enabled. And have them check if disable was called on remove()
> >> and if not do the disable() things on remove(0
> >>
> >> 4. Have msm call aperture_disable_conflicting_devices() where it
> >> now call aperture_remove_conflicting_devices() and call
> >> aperture_remove_conflicting_devices() at the point where it has
> >> claimed any clks it needs.
> >>
> >> Does this sound like something which would be acceptable ?
> > 
> > Generally speaking, yes, but I'd also like to understand what you're
> > trying to fix exactly, because I don't see how it can be what you hinted
> > at before.
> 
> The basic problem is that on handover from simpledrm -> msm
> the clocks get turned off, without fully shutting down the
> display pipeline first (since simpledrm does not know how).
> 
> Then when msm later tries to re-enable things, the hw is
> in a confused state and we hit this error:
> 
> [    2.980181] disp_cc_mdss_dptx3_pixel0_clk_src: rcg didn't update its configuration.
> [    2.980272] WARNING: drivers/clk/qcom/clk-rcg2.c:136 at update_config+0xdc/0x100
> 
> So we need a way to handover things to the msm driver
> without anything getting disabled by simpledrm in an
> uncontrolled (not following proper pwr-down seq) manner.
> 
> The idea behind the 2 step simpledrm removal process is
> that on the first step drm_dev_unplug() gets called for
> the simpledrm device and any struct resource claim on
> the aperture gets released. While keeping all the clocks,
> regulators, etc. claims.
>
> Then msm can take things over and call the second step
> when it has taken everything over. On the second step
> simpeldrm will release its enable count ref on the clocks,
> etc. in question.

So I'm not really sure you need a hand-over, and you kind of rejected
doing a proper one anyway.

I see two issues there:
 - simpledrm doesn't safely disable its resources on QC SoCs

 - there's no way to claim the resources from simpledrm after the main
   drm driver is active.

And you're trying to work around the first one with the second one. The
second one is generic to me, so it shouldn't be an msm (or simpledrm)
specific solution.

The first one is SoC-specific, and fixing the second won't prevent
completely the first. If you unbind simpledrm while it's active, you'll
get the same issue, and you can't rely on the msm driver being always
there to cover for you.

So we need to fix both. We should discuss the second part with Thomas
really, but for the first one, simple-framebuffer having its own
compatible will allow you to trigger any coordinated poweroff sequence.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]