Message-ID: <20260122154013.06f6b655@fedora>
Date: Thu, 22 Jan 2026 15:40:13 +0100
From: Boris Brezillon <boris.brezillon@...labora.com>
To: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
Cc: 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>,
 kernel@...labora.com, linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v10 0/4] Add a few tracepoints to panthor

On Fri, 16 Jan 2026 13:57:29 +0100
Nicolas Frattaroli <nicolas.frattaroli@...labora.com> wrote:

> This series adds two tracepoints to panthor.
> 
> The first tracepoint allows for inspecting the power status of the
> hardware subdivisions, e.g. how many shader cores are powered on. This
> is done by reading three hardware registers when a certain IRQ fires.
> 
> The second tracepoint instruments panthor's job IRQ handler. This is
> more useful than the generic interrupt tracing functionality, as the
> tracepoint has the events bit mask included, which indicates which
> command stream group interfaces triggered the interrupt.
> 
> To test the tracepoints, the following can be used:
> 
>   :~# echo 1 > /sys/kernel/tracing/events/panthor/gpu_power_status/enable
>   :~# echo 1 > /sys/kernel/tracing/events/panthor/gpu_job_irq/enable
>   :~# echo 1 > /sys/kernel/tracing/tracing_on
>   :~# cat /sys/kernel/tracing/trace_pipe
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>

Queued to drm-misc-next.

> ---
> Changes in v10:
> - Start processing phase in raw_irq_handler (Boris)
> - enable/disable_events: Only write mask contents in ACTIVE state
>   (Boris)
> - Elaborate on what mask_lock protects in comment (Boris)
> - nb: checkpatch reports unnecessary line continuations in the macros
>   at the end of the comment blocks. These warnings are false-positives,
>   the line continuations are necessary.
> - Link to v9: https://lore.kernel.org/r/20260115-panthor-tracepoints-v9-0-e13e4f7d01dc@collabora.com
> 
> Changes in v9:
> - Rebase onto drm-tip 2026y-01m-14d-17h-09m-04s
> - Move the mask to pirq->mask change in the suspended refactor patch to
>   the follow-up patch
> - Move the INT_MASK restoration changes in the suspended refactor patch
>   to the follow-up patch
> - Move panthor_irq::mask_lock member right below the mask member
> - panthor/mmu: Remove the enable_events calls before resume, as they're
>   redundant
> - panthor/mmu: Rework the AS fault clearing logic. Drop the spurious
>   disable_events in panthor_vm_active, but use
>   disable_events/enable_events in as_disable/as_enable respectively. This
>   requires doing a forward declaration of the panthor_mmu_irq_handler to
>   get a definition of the helpers before it itself is defined. This
>   works out great, because it means I also no longer have to move the
>   entire panthor_vm_active function down some.
> - Drop an accidentally added redundant empty line
> - Link to v8: https://lore.kernel.org/r/20260112-panthor-tracepoints-v8-0-63efcb421d22@collabora.com
> 
> Changes in v8:
> - Reorder panthor_irq::state patch to be before the new mask
>   modification helpers. The full set of states was kept (rather than
>   just active/suspended) as they don't hurt and make the follow-up patch
>   more concise in scope.
> - Also bail out on STATE_SUSPENDING in irq_raw_handler
> - Job irq: initialize start to 0 and don't emit a tracepoint if it is 0,
>   to fix passing an uninitialised stack variable to userspace if the
>   tracepoint was enabled while the handler was running
> - Link to v7: https://lore.kernel.org/r/20260108-panthor-tracepoints-v7-0-afeae181f74a@collabora.com
> 
> Changes in v7:
> - Get rid of old resume IRQ helper by reworking code throughout panthor,
>   and make what used to be resume_restore in v6 the new resume.
> - Rename mask_enable/mask_disable to enable_events/disable_events.
> - Turn panthor_irq::suspended into a multi-state value, and utilise it
>   in the IRQ helpers as appropriate.
> - Link to v6: https://lore.kernel.org/r/20251223-panthor-tracepoints-v6-0-d3c998ee9efc@collabora.com
> 
> Changes in v6:
> - Read the mask member into a local while holding the lock in
>   irq_threaded_handler.
> - Drop the lock before entering the while loop, letting the threaded
>   handler function run without holding a spinlock
> - Re-acquire the spinlock at the end of irq_threaded_handler, OR'ing the
>   mask register's contents with the mask local ANDed by the member. This
>   avoids stomping over any other modified bits, or restoring ones that
>   have been disabled in the meantime.
> - Link to v5: https://lore.kernel.org/r/20251221-panthor-tracepoints-v5-0-889ef78165d8@collabora.com
> 
> Changes in v5:
> - Change the panthor IRQ helpers to guard the mask member and register
>   with a spinlock. The rationale behind using a spinlock, rather than
>   some constellation of atomics, is that we have to guarantee mutual
>   exclusion for state beyond just a single value, namely both the register
>   write, and writes to/reads from the mask member, including
>   reads-from-member-writes-to-register. Making the mask atomic does not do
>   anything to avoid concurrency issues in such a case.
> - Change the IRQ mask member to not get zeroed when suspended. It's
>   possible something outside of the IRQ helpers depends on this
>   behaviour, but I'd argue the code should not access the mask outside
>   of the IRQ helpers, as it'll do so with no lock taken.
> - Drop the mask_set function, but add mask_enable/mask_disable helpers
>   to enable/disable individual parts of the IRQ mask.
> - Add a resume_restore IRQ helper that does the same thing as resume,
>   but does not overwrite the mask member. This avoids me having to
>   refactor whatever panthor_mmu.c is doing with that poor mask member.
> - Link to v4: https://lore.kernel.org/r/20251217-panthor-tracepoints-v4-0-916186cb8d03@collabora.com
> 
> Changes in v4:
> - Include "panthor_hw.h" in panthor_trace.h instead of duplicating the
>   reg/unreg function prototypes.
> - Link to v3: https://lore.kernel.org/r/20251211-panthor-tracepoints-v3-0-924c9d356a5c@collabora.com
> 
> Changes in v3:
> - Drop PWRFEATURES patch, as this register is no longer needed by this
>   series.
> - Eliminate the rt_on field from the gpu_power_status register, as per
>   Steven Price's feedback.
> - Make gpu_power_status tracepoint reg/unreg functions generic across
>   hardware generations by wrapping a hw op in panthor_hw.c.
> - Reimplement the <= v13 IRQ mask modification functions as the new hw
>   ops functions. v14 can add its own ops in due time.
> - Link to v2: https://lore.kernel.org/r/20251210-panthor-tracepoints-v2-0-ace2e29bad0f@collabora.com
> 
> Changes in v2:
> - Only enable the GPU_IRQ_POWER_CHANGED_* IRQ mask bits when the
>   tracepoint is enabled. Necessitates the new irq helper patch.
> - Only enable the GPU_IRQ_POWER_CHANGED_* IRQ mask bits if the hardware
>   architecture is <= v13, as v14 changes things.
> - Use _READY instead of _PWRACTIVE registers, and rename the tracepoint
>   accordingly.
> - Also read the status of the ray tracing unit's power. This is a global
>   flag for all shader cores, it seems. Necessitates the new register
>   definition patch.
> - Move the POWER_CHANGED_* check to earlier in the interrupt handler.
> - Also listen to POWER_CHANGED, not just POWER_CHANGED_ALL, as this
>   provides useful information with the _READY registers.
> - Print the device name in both tracepoints, to disambiguate things on
>   systems with multiple Mali GPUs.
> - Document the gpu_power_status tracepoint, so the meaning of the fields
>   is made clear.
> - Link to v1: https://lore.kernel.org/r/20251203-panthor-tracepoints-v1-0-871c8917e084@collabora.com
> 
> ---
> Nicolas Frattaroli (4):
>       drm/panthor: Rework panthor_irq::suspended into panthor_irq::state
>       drm/panthor: Extend IRQ helpers for mask modification/restoration
>       drm/panthor: Add tracepoint for hardware utilisation changes
>       drm/panthor: Add gpu_job_irq tracepoint
> 
>  drivers/gpu/drm/panthor/panthor_device.h | 107 ++++++++++++++++++++++++++-----
>  drivers/gpu/drm/panthor/panthor_fw.c     |  16 ++++-
>  drivers/gpu/drm/panthor/panthor_gpu.c    |  30 ++++++++-
>  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_mmu.c    |  47 +++++++-------
>  drivers/gpu/drm/panthor/panthor_pwr.c    |   2 +-
>  drivers/gpu/drm/panthor/panthor_trace.h  |  86 +++++++++++++++++++++++++
>  9 files changed, 318 insertions(+), 42 deletions(-)
> ---
> base-commit: 733664f1edf3c01cc68e6dd0bbdb135158a98a1d
> change-id: 20251203-panthor-tracepoints-488af09d46e7
> 
> Best regards,

Powered by blists - more mailing lists