RE: [EXT] Re: [PATCH v2 1/5] perf: fsl_imx8_ddr: Add AXI ID PORT CHANNEL filter support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Will,

> 
> Hi Will,
> 
> > On Wed, Oct 11, 2023 at 02:08:34PM +0800, Xu Yang wrote:
> > > This is the extension of AXI ID filter.
> > >
> > > Filter is defined with 2 configuration registers per counter 1-3 (counter
> > > 0 is not used for filtering and lacks these registers).
> > > * Counter N MASK COMP register - AXI_ID and AXI_MASKING.
> > > * Counter N MUX CNTL register - AXI CHANNEL and AXI PORT.
> > >   -- 0: address channel
> > >   -- 1: data channel
> > >
> > > This filter is exposed to userspace as an additional (channel, port) pair.
> > > The definition of axi_channel is inverted in userspace, and it will be
> > > reverted in driver automatically.
> > >
> > > AXI filter of Perf Monitor in DDR Subsystem, only a single port0 exist, so
> > > axi_port is reserved which should be 0.
> > >
> > > e.g.
> > > perf stat -a -e imx8_ddr0/axid-read,axi_mask=0xMMMM,axi_id=0xDDDD,axi_channel=0xH/ cmd
> > > perf stat -a -e imx8_ddr0/axid-write,axi_mask=0xMMMM,axi_id=0xDDDD,axi_channel=0xH/ cmd
> > >
> > > Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> > >
> > > ---
> > > Changes since v2:
> > >  - no changes
> > > ---
> > >  drivers/perf/fsl_imx8_ddr_perf.c | 39 ++++++++++++++++++++++++++++++++
> > >  1 file changed, 39 insertions(+)
> > >
> > > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
> > > index 92611c98120f..d0eae2d7e64b 100644
> > > --- a/drivers/perf/fsl_imx8_ddr_perf.c
> > > +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> > > @@ -19,6 +19,8 @@
> > >  #define COUNTER_READ         0x20
> > >
> > >  #define COUNTER_DPCR1                0x30
> > > +#define COUNTER_MUX_CNTL     0x50
> > > +#define COUNTER_MASK_COMP    0x54
> > >
> > >  #define CNTL_OVER            0x1
> > >  #define CNTL_CLEAR           0x2
> > > @@ -32,6 +34,13 @@
> > >  #define CNTL_CSV_SHIFT               24
> > >  #define CNTL_CSV_MASK                (0xFFU << CNTL_CSV_SHIFT)
> > >
> > > +#define READ_PORT_SHIFT              0
> > > +#define READ_PORT_MASK               (0x7 << READ_PORT_SHIFT)
> > > +#define READ_CHANNEL_REVERT  0x00000008      /* bit 3 for read channel select */
> > > +#define WRITE_PORT_SHIFT     8
> > > +#define WRITE_PORT_MASK              (0x7 << WRITE_PORT_SHIFT)
> > > +#define WRITE_CHANNEL_REVERT 0x00000800      /* bit 11 for write channel select */
> > > +
> > >  #define EVENT_CYCLES_ID              0
> > >  #define EVENT_CYCLES_COUNTER 0
> > >  #define NUM_COUNTERS         4
> > > @@ -50,6 +59,7 @@ static DEFINE_IDA(ddr_ida);
> > >  /* DDR Perf hardware feature */
> > >  #define DDR_CAP_AXI_ID_FILTER                        0x1     /* support AXI ID filter */
> > >  #define DDR_CAP_AXI_ID_FILTER_ENHANCED               0x3     /* support enhanced AXI ID filter */
> > > +#define DDR_CAP_AXI_ID_PORT_CHANNEL_FILTER   0x4     /* support AXI ID PORT CHANNEL filter */
> > >
> > >  struct fsl_ddr_devtype_data {
> > >       unsigned int quirks;    /* quirks needed for different DDR Perf core */
> > > @@ -144,6 +154,7 @@ static const struct attribute_group ddr_perf_identifier_attr_group = {
> > >  enum ddr_perf_filter_capabilities {
> > >       PERF_CAP_AXI_ID_FILTER = 0,
> > >       PERF_CAP_AXI_ID_FILTER_ENHANCED,
> > > +     PERF_CAP_AXI_ID_PORT_CHANNEL_FILTER,
> > >       PERF_CAP_AXI_ID_FEAT_MAX,
> > >  };
> > >
> > > @@ -157,6 +168,8 @@ static u32 ddr_perf_filter_cap_get(struct ddr_pmu *pmu, int cap)
> > >       case PERF_CAP_AXI_ID_FILTER_ENHANCED:
> > >               quirks &= DDR_CAP_AXI_ID_FILTER_ENHANCED;
> > >               return quirks == DDR_CAP_AXI_ID_FILTER_ENHANCED;
> > > +     case PERF_CAP_AXI_ID_PORT_CHANNEL_FILTER:
> > > +             return !!(quirks & DDR_CAP_AXI_ID_PORT_CHANNEL_FILTER);
> > >       default:
> > >               WARN(1, "unknown filter cap %d\n", cap);
> > >       }
> > > @@ -187,6 +200,7 @@ static ssize_t ddr_perf_filter_cap_show(struct device *dev,
> > >  static struct attribute *ddr_perf_filter_cap_attr[] = {
> > >       PERF_FILTER_EXT_ATTR_ENTRY(filter, PERF_CAP_AXI_ID_FILTER),
> > >       PERF_FILTER_EXT_ATTR_ENTRY(enhanced_filter, PERF_CAP_AXI_ID_FILTER_ENHANCED),
> > > +     PERF_FILTER_EXT_ATTR_ENTRY(super_filter, PERF_CAP_AXI_ID_PORT_CHANNEL_FILTER),
> > >       NULL,
> > >  };
> > >
> > > @@ -272,11 +286,15 @@ static const struct attribute_group ddr_perf_events_attr_group = {
> > >  PMU_FORMAT_ATTR(event, "config:0-7");
> > >  PMU_FORMAT_ATTR(axi_id, "config1:0-15");
> > >  PMU_FORMAT_ATTR(axi_mask, "config1:16-31");
> > > +PMU_FORMAT_ATTR(axi_port, "config2:0-2");
> > > +PMU_FORMAT_ATTR(axi_channel, "config2:3-3");
> >
> > Any specific reason to allocate from config2, rather than the upper 32
> > bits of config1?
> 
> No. Either way is ok for me.
> 
> >
> > > @@ -553,6 +572,26 @@ static int ddr_perf_event_add(struct perf_event *event, int flags)
> > >               return -EOPNOTSUPP;
> > >       }
> > >
> > > +     if (pmu->devtype_data->quirks & DDR_CAP_AXI_ID_PORT_CHANNEL_FILTER) {
> > > +             if (ddr_perf_is_filtered(event)) {
> > > +                     /* revert axi id masking(axi_mask) value */
> > > +                     cfg1 ^= AXI_MASKING_REVERT;
> > > +                     writel(cfg1, pmu->base + COUNTER_MASK_COMP + ((counter - 1) << 4));
> >
> > Please can you explain what this "reverting" is doing? It looks like a
> > user-visible change in behaviour to me, or are you saying that the driver
> > currently does the wrong thing on hardware that supports the new filter?
> 
> In sys/metrics.json table as below,
> 
> "MetricExpr": "imx8_ddr0@axid\\-read\\,axi_mask\\=0x0000\\,axi_id\\=0x0065@",
> 
> We have set axi_mask to 0x0000 for most of the metics and assume that
> bit 0 is used for masking. In hardware register, bit 1 is used for masking
> axi id. So there exists a reverting operation. It also works for me to
> set actual axi mask value in sys/metrics.json. But, because this patch
> is just a supplement for filter, and previous axi filter already use a
> reverting operation for filter, so I did the same thing there.
> 
> >
> > > +
> > > +                     if (cfg == 0x41) {
> >
> > What is this 0x41 for?
> 
> IMX8_DDR_PMU_EVENT_ATTR(axid-read, 0x41),
> This 0x41 means axi read filter event.
> 
> Thanks,
> Xu Yang
> 
> >
> > Will

Do these patches still need optimization?

Thanks,
Xu Yang





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux