Re: [PATCH] perf/imx_ddr: Add stop event counters support for i.MX8MP

[Will Deacon <will@kernel.org>]

On Mon, Sep 07, 2020 at 05:53:59PM +0800, Joakim Zhang wrote:
> DDR Perf driver only supports free-running event counters(counter1/2/3)
> now, this patch adds support for stop event counters.
> 
> Legacy SoCs:
> Cycle counter(counter0) is a special counter, only count cycles. When
> cycle counter overflow, it will lock all counters and generate an
> interrupt. In ddr_perf_irq_handler, disable cycle counter then all
> counters would stop at the same time, update all counters' count, then
> enable cycle counter that all counters count again. During this process,
> only clear cycle counter, no need to clear event counters since they are
> free-running counters. They would continue counting after overflow and
> do/while loop from ddr_perf_event_update can handle event counters
> overflow case.
> 
> i.MX8MP:
> Almost all is the same as legacy SoCs, the only difference is that, event
> counters are not free-running any more. Like cycle counter, when event
> counters overflow, they would stop counting unless clear the counter,
> and no interrupt generate for event counters. So we should clear event
> counters that let them re-count when cycle counter overflow, which ensure
> event counters will not lose data.

Was this supposed to be an improvement over the "Legacy SoCs"
implementation? It seems even worse...

Do you _have_ to write zeroes back to the event counters to get them going
again, or will any value do?

> diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
> index 90884d14f95f..057e361eb391 100644
> --- a/drivers/perf/fsl_imx8_ddr_perf.c
> +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> @@ -14,6 +14,7 @@
>  #include <linux/of_device.h>
>  #include <linux/of_irq.h>
>  #include <linux/perf_event.h>
> +#include <linux/spinlock.h>
>  #include <linux/slab.h>
>  
>  #define COUNTER_CNTL		0x0
> @@ -82,6 +83,7 @@ struct ddr_pmu {
>  	const struct fsl_ddr_devtype_data *devtype_data;
>  	int irq;
>  	int id;
> +	spinlock_t lock;
>  };
>  
>  enum ddr_perf_filter_capabilities {
> @@ -368,16 +370,19 @@ static void ddr_perf_event_update(struct perf_event *event)
>  	struct hw_perf_event *hwc = &event->hw;
>  	u64 delta, prev_raw_count, new_raw_count;
>  	int counter = hwc->idx;
> +	unsigned long flags;
>  
> -	do {
> -		prev_raw_count = local64_read(&hwc->prev_count);
> -		new_raw_count = ddr_perf_read_counter(pmu, counter);
> -	} while (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
> -			new_raw_count) != prev_raw_count);
> +	spin_lock_irqsave(&pmu->lock, flags);
> +
> +	prev_raw_count = local64_read(&hwc->prev_count);
> +	new_raw_count = ddr_perf_read_counter(pmu, counter);
>  
>  	delta = (new_raw_count - prev_raw_count) & 0xFFFFFFFF;
>  
>  	local64_add(delta, &event->count);
> +	local64_set(&hwc->prev_count, new_raw_count);

Hmm, assuming that the event counters never overflow, why do we care about
the prev count at all? In other words, why don't we just add the counter
value to event->count and reset the hardware to zero every time?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel