Re: [PATCH v2] perf: imx9_perf: Introduce AXI filter version to refactor the driver and better extension

[Will Deacon <will@kernel.org>]

On Wed, Dec 11, 2024 at 01:35:16PM +0800, Xu Yang wrote:
> On Tue, Dec 10, 2024 at 01:37:32PM +0000, Will Deacon wrote:
> > On Tue, Dec 10, 2024 at 10:02:12AM +0800, Xu Yang wrote:
> > > On Mon, Dec 09, 2024 at 03:44:20PM +0000, Will Deacon wrote:
> > > > On Mon, Nov 25, 2024 at 06:43:38PM +0800, Xu Yang wrote:
> > > > > The imx93 is the first supported DDR PMU that supports read transaction,
> > > > > write transaction and read beats events which corresponding respecitively
> > > > > to counter 2, 3 and 4.
> > > > > 
> > > > > However, transaction-based AXI match has low accuracy when get total bits
> > > > > compared to beats-based. And imx93 doesn't assign AXI_ID to each master.
> > > > > So axi filter is not used widely on imx93. This could be regards as AXI
> > > > > filter version 1.
> > > > > 
> > > > > To improve the AXI filter capability, imx95 supports 1 read beats and 3
> > > > > write beats event which corresponding respecitively to counter 2-5. imx95
> > > > > also detailed AXI_ID allocation so that most of the master could be count
> > > > > individually. This could be regards as AXI filter version 2.
> > > > > 
> > > > > This will introduce AXI filter version to refactor the driver and support
> > > > > better extension, such as coming imx943.
> > > > > 
> > > > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > > > 
> > > > > ---
> > > > > Changes in v2:
> > > > >  - modify subject
> > > > >  - add comments for AXI_FILTER version
> > > > >  - type -> filter_ver
> > > > > ---
> > > > >  drivers/perf/fsl_imx9_ddr_perf.c | 33 ++++++++++++++++++++++++--------
> > > > >  1 file changed, 25 insertions(+), 8 deletions(-)
> > 
> > [...]
> > 
> > > > > @@ -624,11 +641,11 @@ static int ddr_perf_event_add(struct perf_event *event, int flags)
> > > > >  	hwc->idx = counter;
> > > > >  	hwc->state |= PERF_HES_STOPPED;
> > > > >  
> > > > > -	if (is_imx93(pmu))
> > > > > +	if (axi_filter_v1(pmu))
> > > > >  		/* read trans, write trans, read beat */
> > > > >  		imx93_ddr_perf_monitor_config(pmu, event_id, counter, cfg1, cfg2);
> > > > 
> > > > Hmm, doesn't this change mean we now enable this for imx91 as well? My
> > > > reading of the commit message is that imx93 was the first chip which
> > > > supports this.
> > > 
> > > Yes, it's enabled for imx91 too. In fact, imx91 is compatible with imx93.
> > > They use same configuration for axi filter.
> > 
> > Ok, but my worry is that the above code looks like userspace now _must_
> > provide valid values for the config1 (axi_id) and config2 (axi_mask)
> > fields on imx91, whereas before I think they were ignored by the driver.
> > 
> > In fact, without this change, how were the PMCFGn registers configured
> > on imx91? It looks to me like they were left uninitialised...
> 
> Before this change, PMCFGn registers are indeed not configured on imx91.
> However, they should be configured as imx93. I notice this thing when
> make this patch. First thing I tried is to add is_imx91(), then check it
> and is_imx93() by "||" operator. However, this way seems not scalable as
> more imx9x Soc comes out. Basically, AXI filter version will keep at V2
> unless big changes due to new features. However, perf tool need export
> correct MetricName via identifier in sysfs. So I made this patch, then
> PMCFGn will be configured based on axi filter version rather than pmu
> name.

Gotcha. But that means this is a fix, right? The commit message doesn't
really indicate that and we probably want a Fixes: tag to indicate how
far it should be backported.

Please can you send a v3 with that so I can apply it?

Thanks,

Will