Re: [PATCH RFC] perf:Add driver for HiSilicon PCIe PMU

[Robin Murphy]

On 2020-03-13 2:14 pm, Jonathan Cameron wrote:
> On Fri, 13 Mar 2020 13:23:53 +0000
> Robin Murphy <robin.murphy@xxxxxxx> wrote:
>
> > On 2020-03-12 12:06 pm, Qi Liu wrote:
> > > From: Qi liu <liuqi115@xxxxxxxxxx>
> > >
> > > PCIe PMU Root Complex Integrate End Point(IEP) device is
> > > supported to sample bandwidth, latency, buffer occupation,
> > > bandwidth utilization etc.
> > > Each PMU IEP device monitors multiple root ports, and each
> > > IEP is registered as a pmu in /sys/bus/event_source/devices,
> > > so users can select the target IEP, and use filter to select
> > > root port, function and event.
> > > Filtering options are:
> > > event:    - select the event.
> > > subevent: - select the subevent.
> > > port:     - select target root port.
> > > func:     - select target EP device under the port.
> > >
> > > Example: hisi_pcie_00_14_00/event=0x08,subevent=0x04,   \
> > > port=0x05,func=0x00/ -I 1000
> > >
> > > PMU IEP device is described by its bus, device and function,
> > > target root port is 0x05 and target EP under it is function
> > > 0x00. Subevent 0x04 of event 0x08 is sampled.
> > >
> > > Note that in this RFC:
> > > 1. PCIe PMU IEP hardware is still in development.
> > > 2. Perf common event list is undetermined, and name of these
> > > events still need to be discussed.
> > > 3. port filter could only select one port each time.
> > > 4. There are two possible schemes of pmu registration, one is
> > > register each root port as a pmu, it is easier for users to
> > > select target port. The other one is register each IEP as pmu,
> > > for counters are per IEP, not per root port, the second scheme
> > > describes hardware PMC much more reasonable, but need to add
> > > "port" filter option to select port. We use the second one in
> > > this RFC.
>
> Whilst it's great to have detailed feedback, just to make it clear...
>
> This is an RFC for the reasons above.  They include that the hardware
> is still in development - i.e. we can't test it because they've not finished
> implementation yet.

That's OK - TBH I'd be more sympathetic if I wasn't still in the middle 
of developing a massively more complicated PMU driver for hardware that 
*does* exist, but that I only have sporadic access to one trivial 
configuration of :)

> The intention of posting so early is to get some feedback on the general
> approach and the specific points in 2,3 and 4 above.

It's probably hard to comment on point 2 until it's nailed down what 
actual events exist and what the values represent.

For point 3, there's a relatively intuitive way to support the case 
where all counters are affected by a single global filter - essentially, 
the first scheduled event 'claims' the global filter settings, then any 
further events have to have the same settings to be allowed in - we 
implement this in arm_smmuv3_pmu.c, for one example.

> The key fiddly point with this is that it is a shared PMU across a set
> of PCIe Root Ports (there are several of these devices on each SoC in
> the system covering a set of ports each).
> That makes for a somewhat fiddly interface, hence the RFC.
The general rule of thumb I've been told is that the granularity of "a 
PMU" usually wants to be the granularity at which you can start/stop 
multiple counters atomically (consider when you want to accurately 
measure a metric which requires relative values of two or more events). 
In that context, the structure implemented here looks like the right choice.

Robin.

> Thanks to everyone who has reviewed though as definitely some stuff for liuqi to
> fix!
>
> Jonathan