Re: [PATCH v3 2/3] drm/panthor: Add tracepoint for hardware utilisation changes

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

On Friday, 12 December 2025 10:29:21 Central European Standard Time Boris Brezillon wrote:
> On Thu, 11 Dec 2025 21:15:43 +0100
> Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> 
> > On Thursday, 11 December 2025 20:37:09 Central European Standard Time Karunika Choo wrote:
> > > On 11/12/2025 16:15, 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.
> > > > 
> > > > Expose the data as a tracepoint to userspace. This allows users to debug
> > > > various scenarios and gather interesting information, such as: knowing
> > > > how much hardware is lit up at any given time, correlating graphics
> > > > corruption with a specific powered shader core, measuring when hardware
> > > > is allowed to go to a powered off state again, and so on.
> > > > 
> > > > The registration/unregistration functions for the tracepoint go through
> > > > a wrapper in panthor_hw.c, so that v14+ can implement the same
> > > > tracepoint by adding its hardware specific IRQ on/off callbacks to the
> > > > panthor_hw.ops member.
> > > > 
> > > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > > > ---
> > > >  drivers/gpu/drm/panthor/panthor_gpu.c   | 38 ++++++++++++++++++--
> > > >  drivers/gpu/drm/panthor/panthor_gpu.h   |  2 ++
> > > >  drivers/gpu/drm/panthor/panthor_hw.c    | 62 +++++++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/panthor/panthor_hw.h    |  8 +++++
> > > >  drivers/gpu/drm/panthor/panthor_trace.h | 59 +++++++++++++++++++++++++++++++
> > > >  5 files changed, 167 insertions(+), 2 deletions(-)
> > > > 
> > > > [...]
> > > >
> > > > diff --git a/drivers/gpu/drm/panthor/panthor_trace.h b/drivers/gpu/drm/panthor/panthor_trace.h
> > > > new file mode 100644
> > > > index 000000000000..2b59d7f156b6
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/panthor/panthor_trace.h
> > > > @@ -0,0 +1,59 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 or MIT */
> > > > +/* Copyright 2025 Collabora ltd. */
> > > > +
> > > > +#undef TRACE_SYSTEM
> > > > +#define TRACE_SYSTEM panthor
> > > > +
> > > > +#if !defined(__PANTHOR_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ)
> > > > +#define __PANTHOR_TRACE_H__
> > > > +
> > > > +#include <linux/tracepoint.h>
> > > > +#include <linux/types.h>
> > > > +
> > > > +int panthor_hw_power_status_register(void);
> > > > +void panthor_hw_power_status_unregister(void);  
> > > 
> > > Hello, not sure if I'm missing something but, would doing
> > > 
> > >     #include "panthor_hw.h" 
> > > 
> > > address the need to redeclare panthor_hw_power_status_* in this file?
> > > The change looks good otherwise.  
> > 
> > It would, but only in that it now pulls in a whole bunch of header
> > definitions that the trace header does not want or need, all for
> > two function prototypes. Since the function signature of the reg/unreg
> > functions are fixed aside from the name, I don't see any harm in
> > this particular instance of duplicating it.
> > 
> > Similarly, panthor_hw.c probably doesn't want the special magic
> > tracepoint stuff from panthor_trace.h, but it does need to implement
> > those two functions, so it needs to have a prototype somewhere.
> > 
> > Maybe someone else has stronger opinions on this, I'm fine with
> > including panthor_hw.h as well (it's what I had it do originally
> > as well), but I suspect many more experienced kernel devs are
> > wary of overeager header inclusions, because it leads to really
> > slow compilation quite quickly.
> 
> In general I agree that including only headers you really need is a
> good practice (to improve compilation time), but that's also an extra
> maintenance burden when things are declared in multiple places, so I'd
> go for the panthor_hw.h inclusion here, I think.
> 

Alright, will do that in v4. Feel free to leave any more feedback
on anything in the series (e.g. the other tracepoint) so I don't
send too many tiny revisions.