Re: [RFC v2 1/1] drm/lima: Add optional devfreq support

From: Robin Murphy
Date: Wed Jan 01 2020 - 07:56:12 EST

On 2019-12-31 4:47 pm, Martin Blumenstingl wrote:
Hi Robin,

On Tue, Dec 31, 2019 at 5:40 PM Robin Murphy <robin.murphy@xxxxxxx> wrote:

On 2019-12-31 2:17 pm, Martin Blumenstingl wrote:
Hi Robin,

On Mon, Dec 30, 2019 at 1:47 AM Robin Murphy <robin.murphy@xxxxxxx> wrote:

On 2019-12-29 11:19 pm, Martin Blumenstingl wrote:
Hi Robin,

On Sun, Dec 29, 2019 at 11:58 PM Robin Murphy <robin.murphy@xxxxxxx> wrote:

Hi Martin,

On 2019-12-27 5:37 pm, Martin Blumenstingl wrote:
Most platforms with a Mali-400 or Mali-450 GPU also have support for
changing the GPU clock frequency. Add devfreq support so the GPU clock
rate is updated based on the actual GPU usage when the
"operating-points-v2" property is present in the board.dts.

The actual devfreq code is taken from panfrost_devfreq.c and modified so
it matches what the lima hardware needs:
- a call to dev_pm_opp_set_clkname() during initialization because there
are two clocks on Mali-4x0 IPs. "core" is the one that actually clocks
the GPU so we need to control it using devfreq.
- locking when reading or writing the devfreq statistics because (unlike
than panfrost) we have multiple PP and GP IRQs which may finish jobs

I gave this a quick try on my RK3328, and the clock scaling indeed kicks
in nicely on the glmark2 scenes that struggle, however something appears
to be missing in terms of regulator association, as the appropriate OPP
voltages aren't reflected in the GPU supply (fortunately the initial
voltage seems close enough to that of the highest OPP not to cause major
problems, on my box at least). With panfrost on RK3399 I do see the
supply voltage scaling accordingly, but I don't know my way around
devfreq well enough to know what matters in the difference :/
first of all: thank you for trying this out! :-)

does your kernel include commit 221bc77914cbcc ("drm/panfrost: Use
generic code for devfreq") for your panfrost test?
if I understand the devfreq API correct then I suspect with that
commit panfrost also won't change the voltage anymore.

Oh, you're quite right - I was already considering that change as
ancient history, but indeed it's only in 5.5-rc, while that board is
still on 5.4.y release kernels. No wonder I couldn't make sense of how
the (current) code could possibly be working :)

I'll try the latest -rc kernel tomorrow to confirm (now that PCIe is
hopefully fixed), but I'm already fairly confident you've called it
I just tested it with the lima driver (by undervolting the GPU by
0.05V) and it seems that dev_pm_opp_set_regulators is really needed.
I'll fix this in the next version of this patch and also submit a fix
for panfrost (I won't be able to test that though, so help is
appreciated in terms of testing :))

Yeah, I started hacking something up for panfrost yesterday, but at the
point of realising the core OPP code wants refactoring to actually
handle optional regulators without spewing errors, decided that was
crossing the line into "work" and thus could wait until next week :D
I'm not sure what you mean, dev_pm_opp_set_regulators uses
doesn't that mean that it is optional already?

Indeed it does call regulator_get_optional(), but it then goes on to treat the absence of a supposedly-optional regulator as a hard failure. It doesn't seem very useful having a nice abstracted interface if users still end up have to dance around and duplicate half the parsing in order to work out whether it's worth calling or not - far better IMO if it could just successfully set/put zero regulators in the cases where the OPPs are behind a firmware/mailbox DVFS interface rather than explicit in-kernel clock/regulator control.

That said, given that I think the current lima/panfrost users should all be relatively simple with either 0 or 1 regulator, you could probably just special-case -ENODEV and accept a spurious error message sometimes for the sake of an immediate fix, then we can make general improvements to the interface separately afterwards.