Message-ID: <aed91df5-16ab-409a-aa3e-1bfa8910d83d@arm.com>
Date: Mon, 12 Jan 2026 09:15:41 +0000
From: Steven Price <steven.price@....com>
To: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>,
 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>
Cc: kernel@...labora.com, linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v7 4/4] drm/panthor: Add gpu_job_irq tracepoint

On 11/01/2026 11:49, Nicolas Frattaroli wrote:
> On Friday, 9 January 2026 17:23:32 Central European Standard Time Steven Price wrote:
>> On 08/01/2026 14:19, Nicolas Frattaroli wrote:
>>> Mali's CSF firmware triggers the job IRQ whenever there's new firmware
>>> events for processing. While this can be a global event (BIT(31) of the
>>> status register), it's usually an event relating to a command stream
>>> group (the other bit indices).
>>>
>>> Panthor throws these events onto a workqueue for processing outside the
>>> IRQ handler. It's therefore useful to have an instrumented tracepoint
>>> that goes beyond the generic IRQ tracepoint for this specific case, as
>>> it can be augmented with additional data, namely the events bit mask.
>>>
>>> This can then be used to debug problems relating to GPU jobs events not
>>> being processed quickly enough. The duration_ns field can be used to
>>> work backwards from when the tracepoint fires (at the end of the IRQ
>>> handler) to figure out when the interrupt itself landed, providing not
>>> just information on how long the work queueing took, but also when the
>>> actual interrupt itself arrived.
>>>
>>> With this information in hand, the IRQ handler itself being slow can be
>>> excluded as a possible source of problems, and attention can be directed
>>> to the workqueue processing instead.
>>>
>>> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
>>> ---
>>>  drivers/gpu/drm/panthor/panthor_fw.c    | 13 +++++++++++++
>>>  drivers/gpu/drm/panthor/panthor_trace.h | 28 ++++++++++++++++++++++++++++
>>>  2 files changed, 41 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
>>> index 0e46625f7621..b3b48c1b049c 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_fw.c
>>> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
>>> @@ -26,6 +26,7 @@
>>>  #include "panthor_mmu.h"
>>>  #include "panthor_regs.h"
>>>  #include "panthor_sched.h"
>>> +#include "panthor_trace.h"
>>>  
>>>  #define CSF_FW_NAME "mali_csffw.bin"
>>>  
>>> @@ -1060,6 +1061,12 @@ static void panthor_fw_init_global_iface(struct panthor_device *ptdev)
>>>  
>>>  static void panthor_job_irq_handler(struct panthor_device *ptdev, u32 status)
>>>  {
>>> +	u32 duration;
>>> +	u64 start;
>>> +
>>> +	if (tracepoint_enabled(gpu_job_irq))
>>> +		start = ktime_get_ns();
>>> +
>>>  	gpu_write(ptdev, JOB_INT_CLEAR, status);
>>>  
>>>  	if (!ptdev->fw->booted && (status & JOB_INT_GLOBAL_IF))
>>> @@ -1072,6 +1079,12 @@ static void panthor_job_irq_handler(struct panthor_device *ptdev, u32 status)
>>>  		return;
>>>  
>>>  	panthor_sched_report_fw_events(ptdev, status);
>>> +
>>> +	if (tracepoint_enabled(gpu_job_irq)) {
>>> +		if (check_sub_overflow(ktime_get_ns(), start, &duration))
>>
>> It's minor but if the tracepoint was enabled during the handler, the
>> duration will use start uninitialised. It's probably best to initialise
>> start just to avoid a potential stack leak.
> 
> Good catch.
> 
> Should I unconditionally initialize it to ktime_get_ns(), or do we want
> to avoid a call into that and initialize it to something that will result
> in a nonsense duration? Alternatively we initialize it to 0 and skip the
> tracepoint if !start.
> 
> My gut tells me reading the monotonic clock shouldn't be considered
> expensive, though having the tracepoint overhead with an inactive
> tracepoint be within a Planck time of "free" would be preferable,
> so I'm leaning towards
> 
>     u64 start = 0;
> 
>     if (tracepoint_enabled(gpu_job_irq))
>             start = ktime_get_ns();
> 
>     ...
> 
>     if (start && tracepoint_enabled(gpu_job_irq)) {
>             ...

Yeah I'd go with this option. There is quite a bit of effort to keep
reading the clock cheap, but it's still going to be much more expensive
than not reading it. And we really don't care if we drop the first
tracepoint when it's enabled.

Note that reordering the condition to

     if (tracepoint_enabled(gpu_job_irq) && start) {

would be slightly preferable, so that the static key avoids the check of
'start' when the tracepoint is disabled. Although with compiler
optimisations and CPU out-of-order execution, I'm not sure whether the
difference is actually measurable ;)

Thanks,
Steve

> Kind regards,
> Nicolas Frattaroli
> 
>>
>> Thanks,
>> Steve
>>
>>> +			duration = U32_MAX;
>>> +		trace_gpu_job_irq(ptdev->base.dev, status, duration);
>>> +	}
>>>  }
>>>  PANTHOR_IRQ_HANDLER(job, JOB, panthor_job_irq_handler);
>>>  
>>> diff --git a/drivers/gpu/drm/panthor/panthor_trace.h b/drivers/gpu/drm/panthor/panthor_trace.h
>>> index 5bd420894745..6ffeb4fe6599 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_trace.h
>>> +++ b/drivers/gpu/drm/panthor/panthor_trace.h
>>> @@ -48,6 +48,34 @@ TRACE_EVENT_FN(gpu_power_status,
>>>  	panthor_hw_power_status_register, panthor_hw_power_status_unregister
>>>  );
>>>  
>>> +/**
>>> + * gpu_job_irq - called after a job interrupt from firmware completes
>>> + * @dev: pointer to the &struct device, for printing the device name
>>> + * @events: bitmask of BIT(CSG id) | BIT(31) for a global event
>>> + * @duration_ns: Nanoseconds between job IRQ handler entry and exit
>>> + *
>>> + * The panthor_job_irq_handler() function instrumented by this tracepoint exits
>>> + * once it has queued the firmware interrupts for processing, not when the
>>> + * firmware interrupts are fully processed. This tracepoint allows for debugging
>>> + * issues with delays in the workqueue's processing of events.
>>> + */
>>> +TRACE_EVENT(gpu_job_irq,
>>> +	TP_PROTO(const struct device *dev, u32 events, u32 duration_ns),
>>> +	TP_ARGS(dev, events, duration_ns),
>>> +	TP_STRUCT__entry(
>>> +		__string(dev_name, dev_name(dev))
>>> +		__field(u32, events)
>>> +		__field(u32, duration_ns)
>>> +	),
>>> +	TP_fast_assign(
>>> +		__assign_str(dev_name);
>>> +		__entry->events		= events;
>>> +		__entry->duration_ns	= duration_ns;
>>> +	),
>>> +	TP_printk("%s: events=0x%x duration_ns=%d", __get_str(dev_name),
>>> +		  __entry->events, __entry->duration_ns)
>>> +);
>>> +
>>>  #endif /* __PANTHOR_TRACE_H__ */
>>>  
>>>  #undef TRACE_INCLUDE_PATH
>>>
>>
>>
> 
> 
> 
> 

Powered by blists - more mailing lists