Il 15/09/25 15:32, Nicolas Frattaroli ha scritto:
On Monday, 15 September 2025 12:28:09 Central European Summer Time
AngeloGioacchino Del Regno wrote:
Il 12/09/25 20:37, Nicolas Frattaroli ha scritto:
MediaTek uses some glue logic to control frequency and power on some of
their GPUs. This is best exposed as a devfreq driver, as it saves us
from having to hardcode OPPs into the device tree, and can be extended
with additional devfreq-y logic like more clever governors that use the
hardware's GPUEB MCU to set frame time targets and power limits.
Add this driver to the panthor subdirectory. It needs to live here as it
needs to call into panthor's devfreq layer, and panthor for its part
also needs to call into this driver during probe to get a devfreq device
registered. Solving the cyclical dependency by having mediatek_mfg live
without knowledge of what a panthor is would require moving the devfreq
provider stuff into a generic devfreq subsystem solution, which I didn't
want to do.
Signed-off-by: Nicolas Frattaroli <nicolas.frattar...@collabora.com>
---
drivers/gpu/drm/panthor/Kconfig | 13 +
drivers/gpu/drm/panthor/Makefile | 2 +
drivers/gpu/drm/panthor/mediatek_mfg.c | 1053
++++++++++++++++++++++++++++++++
3 files changed, 1068 insertions(+)
[ ... snip ...]
+static int mtk_mfg_eb_on(struct mtk_mfg *mfg)
+{
+ struct device *dev = &mfg->pdev->dev;
+ u32 val;
+ int ret;
+
+ /*
+ * If MFG is already on from e.g. the bootloader, we should skip doing
+ * the power-on sequence, as it wouldn't work without powering it off
+ * first.
+ */
+ if ((readl(mfg->rpc + RPC_PWR_CON) & PWR_ACK_M) == PWR_ACK_M)
+ return 0;
+
+ ret = readl_poll_timeout(mfg->rpc + RPC_GHPM_RO0_CON, val,
+ !(val & (GHPM_PWR_STATE_M | GHPM_STATE_M)),
+ GPUEB_POLL_US, GPUEB_TIMEOUT_US);
+ if (ret) {
+ dev_err(dev, "timed out waiting for EB to power on\n");
+ return ret;
+ }
+
+ mtk_mfg_update_reg_bits(mfg->rpc + mfg->ghpm_en_reg, GHPM_ENABLE_M,
+ GHPM_ENABLE_M);
+
+ mtk_mfg_update_reg_bits(mfg->rpc + RPC_GHPM_CFG0_CON, GHPM_ON_SEQ_M, 0);
+ mtk_mfg_update_reg_bits(mfg->rpc + RPC_GHPM_CFG0_CON, GHPM_ON_SEQ_M,
+ GHPM_ON_SEQ_M);
+
+ mtk_mfg_update_reg_bits(mfg->rpc + mfg->ghpm_en_reg, GHPM_ENABLE_M, 0);
+
+
+ ret = readl_poll_timeout(mfg->rpc + RPC_PWR_CON, val,
+ (val & PWR_ACK_M) == PWR_ACK_M,
+ GPUEB_POLL_US, GPUEB_TIMEOUT_US);
I wonder if you can check how much time does the GPUEB really take to poweron,
just so that we might be able to reduce delay_us here.
I already did, that's where the 50us value is from as far as I remember.
Ah, perfect.
+ if (ret) {
+ dev_err(dev, "timed out waiting for EB power ack, val = 0x%X\n",
+ val);
+ return ret;
+ }
+
+ ret = readl_poll_timeout(mfg->gpr + GPR_LP_STATE, val,
+ (val == EB_ON_RESUME),
+ GPUEB_POLL_US, GPUEB_TIMEOUT_US);
Same here - and I think this one is more critical, as I can see this
suspend/resume
control being used more extensively in the future.
Specifically, I'm wondering if we could add runtime PM ops that will request EB
suspend/resume - and also if doing so would make any sense.
I am guessing that the "suspend" LP_STATE stops the internal state machine,
making
the EB MCU to either go in a low-power state or to anyway lower its power usage
by
at least suspending the iterations.
I think I briefly fiddled with this but then it did nothing other than
break everything. Is the current time it takes to resume a problem?
Of course - here I mean that we could have
1. System suspend ops that powers off the EB completely like you're doing here
and
2. Runtime PM op that may be called (very) aggressively
...this would obviously not be feasible if the EB suspend/resume (without
complete
poweron/off) takes too much time to actually happen.
We probably don't want to aggressively suspend the thing doing DVFS
while a workload is running, and if no workload is running, it
already suspends. I can't really say how normal desktop usage will
play out yet, but generally speaking I think it's a bit early to
find a comfortable place on the transition latency vs power draw
curve at this point.
Okay let's leave it for now and revisit that after everything is upstreamed.
It's only an improvement anyway, not critical for functionality, and maybe
not even feasible.
[... snip ...]
+static int mtk_mfg_init_shared_mem(struct mtk_mfg *mfg)
+{
[... snip ...]
+
+ dev_info(dev, "initialised mem at phys 0x%016llX\n", mfg->sram_phys);
I don't like exposing addresses in kmsg. Please just don't.
It's a physical address. This is not a kernel pointer, but something
that can be read from the DTS. But sure, I'll remove it I guess?
Yeah, please.
[... snip ...]
Cheers,
Angelo
You can assume me not responding to a part of the feedback in this
e-mail means I'll address it in the next revision of the patch
series.
Alright!
Cheers,
Angelo
Kind regards,
Nicolas Frattaroli
