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

[Joakim Zhang]

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.

OK.

Any other improvements should be done? If you agree with this in principle, I will send out a V3 soon.

Best Regards,
Joakim Zhang
> Will