On Wed, Apr 10, 2024 at 04:45:47PM +0100, Will Deacon wrote: > On Wed, Apr 10, 2024 at 07:39:46AM +0000, Xu Yang wrote: > > > On Fri, Mar 22, 2024 at 02:39:25PM +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. > > > > > > I think we still have to support the old interface so that we don't break > > > those existing users (even if the driver just ignores whatever counter ID > > > is provided in a backwards-compatible way). > > > > > > > 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 attribute is removed too. To mark counter-specific > > > > events, counter ID will be encoded into perf_pmu_events_attr.id. > > > > > > > > Reviewed-by: Frank Li <Frank.Li@xxxxxxx> > > > > Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx> > > > > > > > > --- > > > > Changes in v6: > > > > - new patch > > > > Changes in v7: > > > > - no changes > > > > Changes in v8: > > > > - add Rb tag > > > > --- > > > > drivers/perf/fsl_imx9_ddr_perf.c | 168 ++++++++++++++++++------------- > > > > 1 file changed, 99 insertions(+), 69 deletions(-) > > > > > > > > diff --git a/drivers/perf/fsl_imx9_ddr_perf.c b/drivers/perf/fsl_imx9_ddr_perf.c > > > > index 0017f2c9ef91..b728719b494c 100644 > > > > --- a/drivers/perf/fsl_imx9_ddr_perf.c > > > > +++ b/drivers/perf/fsl_imx9_ddr_perf.c > > > > @@ -245,14 +249,12 @@ 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"); > > > > > > Sadly, this is a user-visible change so I think it will break old tools, > > > won't it? > > > > For imx ddr pmu, most of the people will use metrics rather event itself, > > and we have speficy the format of event parameters in metrics table. > > The parameters is also updated in this patchset. > > > > And to easy use for user, the counter should be hidden (transparent) to > > user after I had talk with Frank. Then, the user need't to look up the event > > table to find which counter to use. > > > > So this patchset will basically not break the usage of perf tools and will > > improve practicality. > > Sorry, but I don't agree. The original commit adding this driver > (55691f99d417) gives the following examples in the commit message: > > For example: > perf stat -a -I 1000 -e imx9_ddr0/eddrtq_pm_rd_trans_filt,counter=2,axi_mask=ID_MASK,axi_id=ID/ > perf stat -a -I 1000 -e imx9_ddr0/eddrtq_pm_wr_trans_filt,counter=3,axi_mask=ID_MASK,axi_id=ID/ > perf stat -a -I 1000 -e imx9_ddr0/eddrtq_pm_rd_beat_filt,counter=4,axi_mask=ID_MASK,axi_id=ID/ > > I don't think these will work any more if we apply this patch. Yang: keep compatible is important. Please try well's suggestion "if the driver just ignores whatever counter ID is provided in a backwards-compatible way)." If you have further question, you can ping directly. Please avoid use outlook to reply community email. It always append annoised "EXT". Frank > > Will