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.
>
> > + 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.
> [... 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?
> [... 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.
Kind regards,
Nicolas Frattaroli
