RE: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@xxxxxxx]
> Sent: 02 October 2018 17:35
> To: Jean-Philippe Brucker <jean-philippe.brucker@xxxxxxx>; Shameerali
> Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>;
> lorenzo.pieralisi@xxxxxxx
> Cc: mark.rutland@xxxxxxx; vkilari@xxxxxxxxxxxxxx;
> neil.m.leeder@xxxxxxxxx; pabba@xxxxxxxxxxxxxx; John Garry
> <john.garry@xxxxxxxxxx>; will.deacon@xxxxxxx; rruigrok@xxxxxxxxxxxxxx;
> Linuxarm <linuxarm@xxxxxxxxxx>; linux-acpi@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; Guohanjun (Hanjun Guo)
> <guohanjun@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver
> 
> On 02/10/18 17:19, Jean-Philippe Brucker wrote:
> > On 02/10/2018 15:11, Jean-Philippe Brucker wrote:
> >>> +	cfgr = readl_relaxed(smmu_pmu->reg_base + SMMU_PMCG_CFGR);
> >
> > Something I missed previously: when SMMU_PMCG_CFGR.SID_FILTER_TYPE
> is 1,
> > filtering for all counters is configured by SMMU_PMCG_SMR0 and
> > SMMU_PMCG_EVTYPER0 (instead of having one separate filter per counter).
> 
> Oh, I hadn't even noticed it had that mode as well...

Thanks Jean. Missed that completely. 
 
> > In that mode with your patch, if the user applies a filter to the first
> > event in the list passed to perf, it will be applied to all events.
> > Filter applied on any subsequent event will be ignored. Could we make
> > this more explicit? Maybe in the probe print that the PMCG is
> > global-filtering, and when attempting to apply a filter to something
> > else than EVCNTR0, return an error?
> 
> FWIW filtering is always per-counter-group on the SMMUv2 PMU, and it's
> actually pretty straightforward to cope with - pmu->add() just needs to
> reject the event if one with an incompatible configuration is already
> scheduled, so perf core handles it much like having more events than
> counters, by rotating the mutually-exclusive sets.

I will take a look and address this in next version.

Thanks,
Shameer