Message-ID: <2740907.taCxCBeP46@workhorse>
Date: Thu, 11 Dec 2025 21:15:43 +0100
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: 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>,
 Chia-I Wu <olvaffe@...il.com>, Karunika Choo <karunika.choo@....com>
Cc: kernel@...labora.com, linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org
Subject:
 Re: [PATCH v3 2/3] drm/panthor: Add tracepoint for hardware utilisation
 changes

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

Kind regards,
Nicolas Frattaroli

> 
> Reviewed-by: Karunika Choo <karunika.choo@....com>
> 
> > [...]

Powered by blists - more mailing lists