Re: [PATCH v10 3/8] perf: imx_perf: let the driver manage the counter usage rather the user

[Xu Yang <xu.yang_2@nxp.com>]

Hi Will,

On Mon, Apr 22, 2024 at 05:51:53PM +0100, Will Deacon wrote:
> On Fri, Apr 19, 2024 at 02:38:19PM -0400, Frank Li wrote:
> > On Fri, Apr 19, 2024 at 04:49:13PM +0100, Will Deacon wrote:
> > > On Mon, Apr 15, 2024 at 02:13:15PM +0800, Xu Yang wrote:
> > > > In current design, the user of perf app needs to input counter ID to count
> > > > events. However, this is not user-friendly since the user needs to lookup
> > > > the map table to find the counter. Instead of letting the user to input
> > > > the counter, let this driver to manage the counters in this patch.
> > > > 
> > > > This will be implemented by:
> > > >  1. allocate counter 0 for cycle event.
> > > >  2. find unused counter from 1-10 for reference events.
> > > >  3. allocate specific counter for counter-specific events.
> > > > 
> > > > In this patch, counter attr will be kept for back-compatible but all the
> > > > value passed down by counter=<n> will be ignored. To mark counter-specific
> > > > events, counter ID will be encoded into perf_pmu_events_attr.id.
> > > > 
> > > > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > > 
> > > > ---
> > > > Changes in v6:
> > > >  - new patch
> > > > Changes in v7:
> > > >  - no changes
> > > > Changes in v8:
> > > >  - add Rb tag
> > > > Changes in v9:
> > > >  - keep 'counter' attr for back-compatible
> > > > Changes in v10:
> > > >  - add some explanation about 'counter' attr in commit message
> > > > ---
> > > >  drivers/perf/fsl_imx9_ddr_perf.c | 168 ++++++++++++++++++-------------
> > > >  1 file changed, 100 insertions(+), 68 deletions(-)
> > > 
> > > [...]
> > > 
> > > > @@ -245,8 +249,8 @@ static const struct attribute_group ddr_perf_events_attr_group = {
> > > >  	.attrs = ddr_perf_events_attrs,
> > > >  };
> > > >  
> > > > -PMU_FORMAT_ATTR(event, "config:0-7");
> > > > -PMU_FORMAT_ATTR(counter, "config:8-15");
> > > > +PMU_FORMAT_ATTR(event, "config:0-15");
> > > > +PMU_FORMAT_ATTR(counter, "config:16-23");
> > > 
> > > Although these mappings are advertised in sysfs, I don't think we can
> > > change them because userspace could be relying on them. I also can't
> > > find any examples of other PMU drivers in the kernel changing these
> > > mappings after being merged, so please keep tthem the same.
> > > 
> > > If you need to expand the properties to be 16-bit, then you'll need to
> > > split them into 2x8-bit fields.
> > 
> > I just see tools/perf/tests/pmu.c: { "krava01", "config:0-1,62-63\n", }
> > So I supposed "config:" supported below format
> > 
> > PMU_FORMAT_ATTR(event, "config:0-7,16-19");
> > 
> > I just want to confim with you because I have not found other perf driver
> > using such format yet.
> 
> Oh, wow, that's new syntax for me too! Does the perf tool parse it
> properly? (and what happens if an older tool sees the new syntax?)

I've tried the new syntax with different perf version (kernel 6.x, 5.x
and 4.x), all the perf tool can parse it properly. Then I'll use this
syntax for my patchset in next version.

Thanks,
Xu Yang

>
> Will