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

AngeloGioacchino Del Regno
2025-09-15T13:36:26Z

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

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
>
>