Re: [PATCH 16/19] coresight: trbe: Support trigger mode

[James Clark <james.clark@linaro.org>]

On 01/12/2025 13:22, Leo Yan wrote:
> The buffer currently operates in fill mode, where tracing stops when it
> reaches the end of the buffer and a maintenance interrupt is raised.
> However, due to IRQ latency, trace data may be lost during the window in
> which tracing is halted but the program continues to run.
> 
> To mitigate the issue, this commit enables the trigger count to support
> buffer maintenance without disabling tracing.  This is fulfilled with
> two modes:
> 
> 1) Set a trigger count as a watermark and use fill mode to prevent the
>     buffer from being overwritten.  Once the count is decremented to
>     zero, an interrupt is raised for buffer maintenance, but the hardware
>     continues collecting trace data until limit.
> 
>            head            watermark tail
>       +----+---------------+---------+-------+
>       |$$$$|               |         |$$$$$$$|
>       +----+---------------+---------+-------+
>       base `---- count ----'         limit   base + nr_pages
> 
>       $$$ : Filled trace data
> 
> 2) Use wrap mode so that tracing continues when reach the top of the
>     buffer.  The trigger count is configured as "Stop on trigger" to
>     guard the trace data not to be overwritten.
> 
>                watermark   tail      head
>       +--------+-----------+---------+-------+
>       |        |           |$$$$$$$$$|       |
>       +--------+-----------+---------+-------+
>       base                                   base + nr_pages
>                                              limit
> 
>                                      `------->
>       >-- counter ---------'
> 
>       $$$ : Filled trace data
> 
> The modes are selected by comparing the limit with the trigger position.
> 
> An extra TRBE_FAULT_ACT_TRIG state is introduced for fault action, it is
> used to distinguish the trigger event from the WRAP event.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-trbe.c | 101 +++++++++++++++++++++------
>   drivers/hwtracing/coresight/coresight-trbe.h |  14 ++++
>   2 files changed, 94 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 8390d0a8fe23d35945610df15f21751279ee37ee..0551ea9b4f8286c156e3c9c7ac94e2ecd3b9dc3f 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -48,6 +48,7 @@
>   #define TRBE_TRACE_MIN_BUF_SIZE		64
>   
>   enum trbe_fault_action {
> +	TRBE_FAULT_ACT_TRIG,
>   	TRBE_FAULT_ACT_WRAP,
>   	TRBE_FAULT_ACT_SPURIOUS,
>   	TRBE_FAULT_ACT_FATAL,
> @@ -67,6 +68,7 @@ struct trbe_buf {
>   	unsigned long trbe_hw_base;
>   	unsigned long trbe_limit;
>   	unsigned long trbe_write;
> +	unsigned long trbe_count;
>   	int nr_pages;
>   	void **pages;
>   	bool snapshot;
> @@ -478,6 +480,10 @@ static unsigned long __trbe_normal_offset(struct perf_output_handle *handle)
>   	if (head < tail)
>   		limit = round_down(tail, PAGE_SIZE);
>   
> +	/* If trigger mode is enabled, no need to use limit for watermark */
> +	if (!static_branch_unlikely(&trbe_trigger_mode_bypass))
> +		goto out;
> +
>   	/*
>   	 * Wakeup may be arbitrarily far into the future. If it's not in the
>   	 * current generation, either we'll wrap before hitting it, or it's
> @@ -495,6 +501,7 @@ static unsigned long __trbe_normal_offset(struct perf_output_handle *handle)
>   	if (handle->wakeup < (handle->head + handle->size) && head <= wakeup)
>   		limit = min(limit, round_up(wakeup, PAGE_SIZE));

I kept wondering how this limit was relvant now in wrap mode. But I see 
it's skipped from the goto above, but that only jumps over one 
statement. Can we have that static branch as a regular if statement 
guarding the limit calculation instead of the goto?

Negating "trigger_mode_bypass" and renaming it into just "trigger_mode" 
might help too. The double negative of "not trigger_trigger_mode_bypass 
goto" is a bit hard to understand.

>   
> +out:
>   	/*
>