Re: [PATCH v2 10/10] drm/panthor: add support for MediaTek MFlexGraphics

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

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.frattaroli@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