Message-ID: <a2f77d1b-26aa-47cb-a2b3-9495abbe78be@collabora.com>
Date: Mon, 15 Sep 2025 15:36:23 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>,
 Boris Brezillon <boris.brezillon@...labora.com>,
 Steven Price <steven.price@....com>, Liviu Dudau <liviu.dudau@....com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Matthias Brugger
 <matthias.bgg@...il.com>, MyungJoo Ham <myungjoo.ham@...sung.com>,
 Kyungmin Park <kyungmin.park@...sung.com>,
 Chanwoo Choi <cw00.choi@...sung.com>, Jassi Brar <jassisinghbrar@...il.com>,
 Kees Cook <kees@...nel.org>, "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 Chia-I Wu <olvaffe@...il.com>, Chen-Yu Tsai <wenst@...omium.org>
Cc: kernel@...labora.com, dri-devel@...ts.freedesktop.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
 linux-pm@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2 10/10] drm/panthor: add support for MediaTek
 MFlexGraphics

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

Powered by blists - more mailing lists