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

[Joakim Zhang]

> -----Original Message-----
> From: Joakim Zhang <qiangqing.zhang at nxp.com>
> Sent: 2020年9月22日 13:38
> To: Will Deacon <will at kernel.org>
> Cc: mark.rutland at arm.com; robin.murphy at arm.com; dl-linux-imx
> <linux-imx at nxp.com>; linux-arm-kernel at lists.infradead.org
> Subject: RE: [PATCH V2] perf/imx_ddr: Add stop event counters support for
> i.MX8MP
> 
> 
> Hi Will,
> 
> > -----Original Message-----
> > From: Will Deacon <will at kernel.org>
> > Sent: 2020年9月22日 4:57
> > To: Joakim Zhang <qiangqing.zhang at nxp.com>
> > Cc: mark.rutland at arm.com; robin.murphy at arm.com; dl-linux-imx
> > <linux-imx at nxp.com>; linux-arm-kernel at lists.infradead.org
> > Subject: Re: [PATCH V2] perf/imx_ddr: Add stop event counters support
> > for i.MX8MP
> >
> > On Mon, Sep 21, 2020 at 07:36:02PM +0100, Will Deacon wrote:
> > > On Tue, Sep 08, 2020 at 06:47:34PM +0800, Joakim Zhang wrote:
> > > > +static void ddr_perf_event_update(struct perf_event *event) {
> > > > +	struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > > > +	struct hw_perf_event *hwc = &event->hw;
> > > > +	u64 delta, prev_raw_count, new_raw_count;
> > > > +	int counter = hwc->idx;
> > > > +	int ret;
> > > > +
> > > > +	if (counter == EVENT_CYCLES_COUNTER) {
> > > > +		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);
> > > > +
> > > > +		delta = (new_raw_count - prev_raw_count) & 0xFFFFFFFF;
> > > > +
> > > > +		local64_add(delta, &event->count);
> > >
> > > Why do we treat the cycle counter so differently here?
> >
> > Ah, please can you remind me: does the cycle counter stop on overflow,
> > or does it continue to count?
> 
> For both legacy SoCs and i.MX8MP, cycle counter stop on overflow, then lock all
> counters and generate an interrupt.
> 
> 
> > > > +	} else {
> > > > +		/*
> > > > +		 * For legacy SoCs: event counters continue counting when
> > overflow,
> > > > +		 *                  no need to clear the counter.
> > > > +		 * For new SoCs: event counters stop counting when overflow,
> > need
> > > > +		 *               clear counter to let it count again.
> > > > +		 */
> > > > +		ret = ddr_perf_counter_overflow(pmu, counter);
> > > > +		if (ret)
> > > > +			dev_warn(pmu->dev, "Event Counter%d overflow
> > happened, data
> > > > +incorrect!!\n", counter);
> > >
> > > I don't understand this message: if the data is incorrect, why do we
> > > need to handle overflow at all/, rather than putting the event into
> > > an error state?
> >
> > Also, now I remember how this all works (we use the cycle counter to
> > stop overflow of the event counter), then warning here is ok, but we
> > should do something like:
> >
> > 	dev_warn_ratelimited(pmu->dev, "events lost due to counter overflow
> > (config 0x%llx)\n", event->attr.config);
> >
> > instead.

Hi Will,

+		/*
+		 * For legacy SoCs: event counters continue counting when overflow,
+		 *                  no need to clear the counter.
+		 * For new SoCs: event counters stop counting when overflow, need
+		 *               clear counter to let it count again.
+		 */
+		ret = ddr_perf_counter_overflow(pmu, counter);
+		if (ret)
+			dev_warn(pmu->dev, "Event Counter%d overflow happened, data 
+incorrect!!\n", counter);
+
+		new_raw_count = ddr_perf_read_counter(pmu, counter);
+		local64_add(new_raw_count, &event->count);
+
+		/* Clear event counter, it's fine for both legacy and new SoCs. */
+		ddr_perf_counter_enable(pmu, event->attr.config, counter, true);

I thought again, seems here also need add a spin lock, as it would be invoked from irq context and thread context. 
spin_lock_irqsave()
		new_raw_count = ddr_perf_read_counter(pmu, counter);
		local64_add(new_raw_count, &event->count);

		/* Clear event counter, it's fine for both legacy and new SoCs. */
		ddr_perf_counter_enable(pmu, event->attr.config, counter, true);
spin_unlock_irqrestore()

I'm extremely reluctant to add locking mechanism for this feature, this is also the point I want to learn from you. Thanks.

Best Regards,
Joakim Zhang
> Best Regards,
> Joakim Zhang
> > Will