On 27/10/2022 01:18, Stephen Boyd wrote:
Reviving this old thread because this commit has lead to a couple bugs
now.
Quoting Ulf Hansson (2022-06-22 03:26:52)
On Fri, 17 Jun 2022 at 21:58, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
Hi Bjorn and Dmitry,
Yu reported a lockdep warning coming from the gdsc driver. It looks like
the runtime PM usage in gdsc.c is causing lockdep to see an AA deadlock
possibility with 'genpd->mlock'. I suspect this is because we have
commit 1b771839de05 ("clk: qcom: gdsc: enable optional power domain
support"), and that is now calling runtime PM code from within the genpd
code.
This commit has caused a deadlock at boot for Doug[1] and I see that the
camera driver on Google CoachZ and Wormdingler devices doesn't work
after resuming from suspend once this commit is applied. I'm leaning
towards sending a revert, because it seems to cause 3 issues while
removing the regulator hack that was in place to enable MMCX. This patch
is cleaning up the hack, but trading the hack for three more problems.
I think genpd already has nested lock support, so the only
solution is to not use runtime PM from within genpd code and start
expressing genpd parent relationships in genpd itself?
Not sure exactly what you mean here, but yes, expressing the
parent/child domain relationship is always needed.
Having gdsc_disable() to do runtime PM calls (gdsc_pm_runtime_put())
seems awkward to me. Why is that needed, more exactly?
It seems like this is needed so that the gdsc_enable() and
gdsc_disable() calls can read/write the registers for the genpd, while
those registers live in some clk controller that needs either a
different clk (probably some AHB clk) or another genpd to be enabled. It
looks like for qcom,sm8250-dispcc it relies on MMCX gdsc (which is a
genpd). From a hardware view, the MDSS_GDSC provided by the display clk
controller is probably not a sub-domain of MMCX. Instead, we need to
have MMCX enabled so that we can access the registers for the MDSS GDSC.
Yes, exactly.
My question is if it makes sense to simply describe that the GDSCs
provided by a device are sub-domains of whatever power domains are
listed in DT for that device? I think if we did that here for sm8250
dispcc, we wouldn't need to use runtime PM within the genpd code
assuming that the MMCX parent genpd is enabled before we attempt to
read/write the dispcc gdsc registers. Hopefully that is also done, i.e.
enabling parent domains before enabling child domains if the parent is
disabled.
I will check this tomorrow. It should be possible to handle the
MMCX/MDSS_GDSC relationship in this way.
Is this already being done with pm_genpd_add_subdomain() in
gdsc_register()? I see that we're attaching that to dispcc's struct
device::pm_domain, but I assume that is different from the MMCX genpd.
No, I think the only domain there is the MMCX domain, so this call s
Maybe that is the problem here. Dmitry can you further describe the
problem being solved?
I must admit, I don't remember what caused me to add this call. May be
it was added before me getting the pm_genpd_add_subdomain() call in place.
Or maybe genpd
needs to drop locks while calling down into gdsc_disable() and reacquire
them after that?
Nope, that doesn't work. This internals of genpd relies on this
behaviour, as it allows it to properly deal with power-on|off for
parent/child domains, for example.
Ok.
[1] https://lore.kernel.org/r/20220922154354.2486595-1-dianders@xxxxxxxxxxxx
--
With best wishes
Dmitry
