Message-ID: <3631152.44csPzL39Z@workhorse>
Date: Wed, 10 Dec 2025 17:27:47 +0100
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: Boris Brezillon <boris.brezillon@...labora.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>,
 Chia-I Wu <olvaffe@...il.com>, Karunika Choo <karunika.choo@....com>,
 Steven Price <steven.price@....com>
Cc: kernel@...labora.com, linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org
Subject:
 Re: [PATCH v2 3/4] drm/panthor: Add tracepoint for hardware utilisation
 changes

On Wednesday, 10 December 2025 16:57:42 Central European Standard Time Steven Price wrote:
> On 10/12/2025 14:30, Nicolas Frattaroli wrote:
> > Mali GPUs have three registers that indicate which parts of the hardware
> > are powered at any moment. These take the form of bitmaps. In the case
> > of SHADER_READY for example, a high bit indicates that the shader core
> > corresponding to that bit index is powered on. These bitmaps aren't
> > solely contiguous bits, as it's common to have holes in the sequence of
> > shader core indices, and the actual set of which cores are present is
> > defined by the "shader present" register.
> > 
> > When the GPU finishes a power state transition, it fires a
> > GPU_IRQ_POWER_CHANGED_ALL interrupt. After such an interrupt is
> > received, the _READY registers will contain new interesting data. During
> > power transitions, the GPU_IRQ_POWER_CHANGED interrupt will fire, and
> > the registers will likewise contain potentially changed data.
> > 
> > This is not to be confused with the PWR_IRQ_POWER_CHANGED_ALL interrupt,
> > which is something related to Mali v14+'s power control logic. The
> > _READY registers and corresponding interrupts are already available in
> > v9 and onwards.
> > 
> > Additionally, the SHADER_PWRFEATURES register may be of interest, which
> > contains a bit flag indicating whether raytracing functionality is
> > turned on, as the ray tracing unit's power can separately be toggled.
> > Reading this register on platforms from before it was added has no
> > unpleasant side-effects; it's officially specified to read as 0 in this
> > case.
> 
> I'm confused by this addition, SHADER_PWRFEATURES is sampled (by the
> hardware) on power up of a shader core. So the value of the register
> isn't necessarily representative of the actual hardware state. In normal
> operation it is controlled by the MCU and so probably has some
> correlation to ray tracing happening, but I don't think there's any
> gaurentee there. And on later GPUs this functionality has been moved
> into the POWER_CONTROL block and I don't think there's anything equivalent.

I was afraid that would be the case based on the hw docs wording.
In that case yeah, I'll just drop it for now. My hope was that
SHADER_PWRFEATURES would contain whatever the current status of the
hardware is, aside from just being the place to write the desired
status as well.

> Also in general I wouldn't rely on the "read as 0" parts - not
> specifically because I expect HW bugs, but because later GPUs might
> reuse those register positions for other things.

That's fun. After I remove the SHADER_PWRFEATURES register, the
register reads that will be left can only happen if the
GPU_IRQ_POWER_CHANGED/GPU_IRQ_POWER_CHANGED_ALL interrupt status
bits are set. Unless Arm has plans to reuse those bit values in
future GPUs, and someone adds them to Panthor (shadowing the old
ones), and someone then enables the tracepoint, then these reads
should never happen on hardware that doesn't have the _READY regs.

> So this seems to only work on a small number of GPUs and even then it's
> relying on a firmware behaviour which isn't gaurenteed.
>
> It would be good if we can come up with a tracepoint which is both
> useful and likely to work over a range of GPUs.

If I understand correctly, the RTU power status isn't something
that can be read like SHADER_READY on v14+. In that case, I'll just
drop the rt_on field entirely, because on v13 it doesn't necessarily
do what I want it to do, and on v14+, it doesn't exist. It was an
added "oh this is simple enough" bonus, the main goal is to have the
tracepoints to track power events.

On that note, I'd love to implement the trace event call on v14+ as
well, because I'm fairly sure it can re-use the same tracepoint
definition. I might need to generalize the register and unregister
functions though, probably by moving them somewhere else, and then
having them call into a per-hw-version function pointer. With no
v14+ hardware to test it on, I won't implement that variant for now
however.

Should I generalize the tracepoint register/unregister in this way
right now even without the v14+ implementation? Might make it more
palatable.

Kind regards,
Nicolas Frattaroli
 
> Thanks,
> Steve

Powered by blists - more mailing lists