| Message ID | 20191104070616.29834-2-qiangqing.zhang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] docs/perf: Add AXI ID filter capabilities information | expand |
On Mon, Nov 04, 2019 at 07:09:24AM +0000, Joakim Zhang wrote: > caps/filter indicates whether HW supports AXI ID filter or not. > caps/enhanced_filter indicates whether HW supports enhanced AXI ID filter > or not. > > Users can check filter features from userspace with these attributions. > > Suggested-by: Will Deacon <will@kernel.org> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > --- > drivers/perf/fsl_imx8_ddr_perf.c | 59 ++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c > index 3bbf806209a6..6db484251950 100644 > --- a/drivers/perf/fsl_imx8_ddr_perf.c > +++ b/drivers/perf/fsl_imx8_ddr_perf.c > @@ -84,6 +84,64 @@ struct ddr_pmu { > int id; > }; > > +enum ddr_perf_filter_capabilities { > + PERF_CAP_AXI_ID_FILTER = 0, > + PERF_CAP_AXI_ID_FILTER_ENHANCED, > + PERF_CAP_AXI_ID_FEAT_MAX, > +}; > + > +static int ddr_perf_filter_feat_caps[PERF_CAP_AXI_ID_FEAT_MAX] = { > + [PERF_CAP_AXI_ID_FILTER] = DDR_CAP_AXI_ID_FILTER, > + [PERF_CAP_AXI_ID_FILTER_ENHANCED] = DDR_CAP_AXI_ID_FILTER_ENHANCED, > +}; > + > +static u32 ddr_perf_filter_cap_get(struct ddr_pmu *pmu, int cap) > +{ > + switch (cap) { > + case PERF_CAP_AXI_ID_FILTER: > + return !!(pmu->devtype_data->quirks); > + case PERF_CAP_AXI_ID_FILTER_ENHANCED: > + return (pmu->devtype_data->quirks == > + ddr_perf_filter_feat_caps[cap]); > + } I think this is a bit error-prone if you add additional caps in future, since you'll need to remember to go back and update this case. I rewrote it as follows to try to prevent this: -static int ddr_perf_filter_feat_caps[PERF_CAP_AXI_ID_FEAT_MAX] = { - [PERF_CAP_AXI_ID_FILTER] = DDR_CAP_AXI_ID_FILTER, - [PERF_CAP_AXI_ID_FILTER_ENHANCED] = DDR_CAP_AXI_ID_FILTER_ENHANCED, -}; - static u32 ddr_perf_filter_cap_get(struct ddr_pmu *pmu, int cap) { + u32 quirks = pmu->devtype_data->quirks; + switch (cap) { case PERF_CAP_AXI_ID_FILTER: - return !!(pmu->devtype_data->quirks); + return quirks & DDR_CAP_AXI_ID_FILTER; case PERF_CAP_AXI_ID_FILTER_ENHANCED: - return (pmu->devtype_data->quirks == - ddr_perf_filter_feat_caps[cap]); + quirks &= DDR_CAP_AXI_ID_FILTER_ENHANCED; + return quirks == DDR_CAP_AXI_ID_FILTER_ENHANCED; + default: + WARN(1, "unknown filter cap %d\n", cap); } - WARN(1, "unknown filter cap %d\n", cap); - return 0; } which also means we can drop ddr_perf_filter_feat_caps[] altogether. > +#define PERF_EXT_ATTR_ENTRY(_name, _func, _var) \ > + (&((struct dev_ext_attribute[]) { \ > + { __ATTR(_name, 0444, _func, NULL), (void *)_var } \ > + })[0].attr.attr) In another thread, we recently realised that the array syntax is not needed here, so I've updated this to be: #define PERF_EXT_ATTR_ENTRY(_name, _func, _var) \ - (&((struct dev_ext_attribute[]) { \ - { __ATTR(_name, 0444, _func, NULL), (void *)_var } \ - })[0].attr.attr) + (&((struct dev_ext_attribute) { \ + __ATTR(_name, 0444, _func, NULL), (void *)_var \ + }).attr.attr) I've made those two changes, so I'll queue this up for 5.5. Thanks. Will
> -----Original Message----- > From: Will Deacon <will@kernel.org> > Sent: 2019年11月5日 0:54 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: mark.rutland@arm.com; robin.murphy@arm.com; dl-linux-imx > <linux-imx@nxp.com>; Frank Li <frank.li@nxp.com>; > linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH 2/2] perf/imx_ddr: Dump AXI ID filter info to userspace > > On Mon, Nov 04, 2019 at 07:09:24AM +0000, Joakim Zhang wrote: > > caps/filter indicates whether HW supports AXI ID filter or not. > > caps/enhanced_filter indicates whether HW supports enhanced AXI ID > > filter or not. > > > > Users can check filter features from userspace with these attributions. > > > > Suggested-by: Will Deacon <will@kernel.org> > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > > --- > > drivers/perf/fsl_imx8_ddr_perf.c | 59 > > ++++++++++++++++++++++++++++++++ > > 1 file changed, 59 insertions(+) > > > > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c > > b/drivers/perf/fsl_imx8_ddr_perf.c > > index 3bbf806209a6..6db484251950 100644 > > --- a/drivers/perf/fsl_imx8_ddr_perf.c > > +++ b/drivers/perf/fsl_imx8_ddr_perf.c > > @@ -84,6 +84,64 @@ struct ddr_pmu { > > int id; > > }; > > > > +enum ddr_perf_filter_capabilities { > > + PERF_CAP_AXI_ID_FILTER = 0, > > + PERF_CAP_AXI_ID_FILTER_ENHANCED, > > + PERF_CAP_AXI_ID_FEAT_MAX, > > +}; > > + > > +static int ddr_perf_filter_feat_caps[PERF_CAP_AXI_ID_FEAT_MAX] = { > > + [PERF_CAP_AXI_ID_FILTER] = DDR_CAP_AXI_ID_FILTER, > > + [PERF_CAP_AXI_ID_FILTER_ENHANCED] = > DDR_CAP_AXI_ID_FILTER_ENHANCED, > > +}; > > + > > +static u32 ddr_perf_filter_cap_get(struct ddr_pmu *pmu, int cap) { > > + switch (cap) { > > + case PERF_CAP_AXI_ID_FILTER: > > + return !!(pmu->devtype_data->quirks); > > + case PERF_CAP_AXI_ID_FILTER_ENHANCED: > > + return (pmu->devtype_data->quirks == > > + ddr_perf_filter_feat_caps[cap]); > > + } > > I think this is a bit error-prone if you add additional caps in future, since you'll > need to remember to go back and update this case. I rewrote it as follows to > try to prevent this: > > > -static int ddr_perf_filter_feat_caps[PERF_CAP_AXI_ID_FEAT_MAX] = { > - [PERF_CAP_AXI_ID_FILTER] = DDR_CAP_AXI_ID_FILTER, > - [PERF_CAP_AXI_ID_FILTER_ENHANCED] = > DDR_CAP_AXI_ID_FILTER_ENHANCED, > -}; > - > static u32 ddr_perf_filter_cap_get(struct ddr_pmu *pmu, int cap) { > + u32 quirks = pmu->devtype_data->quirks; > + > switch (cap) { > case PERF_CAP_AXI_ID_FILTER: > - return !!(pmu->devtype_data->quirks); > + return quirks & DDR_CAP_AXI_ID_FILTER; Hi Will, The reason return !!(pmu->devtype_data->quirks) is for backward compatibility, I will extend the driver for another type of filter, like below: #define DDR_CAP_AXI_ID_CHANNEL_PORT 0x4 i.MX8mscale serials DDR PMU all counters share one filter, so we cannot enter different IDs at the same time. But in i.MX8 serials, each counter has their own filter, so it supports different IDs. If return quirks & DDR_CAP_AXI_ID_FILTER, it will show there is no filter. Thanks for your improvement, this is no problem, keep it. I can modify it when enable this type of filter, just explaining. > case PERF_CAP_AXI_ID_FILTER_ENHANCED: > - return (pmu->devtype_data->quirks == > - ddr_perf_filter_feat_caps[cap]); > + quirks &= DDR_CAP_AXI_ID_FILTER_ENHANCED; > + return quirks == DDR_CAP_AXI_ID_FILTER_ENHANCED; > + default: > + WARN(1, "unknown filter cap %d\n", cap); > } > > - WARN(1, "unknown filter cap %d\n", cap); > - > return 0; > } > > > which also means we can drop ddr_perf_filter_feat_caps[] altogether. > > > +#define PERF_EXT_ATTR_ENTRY(_name, _func, _var) \ > > + (&((struct dev_ext_attribute[]) { \ > > + { __ATTR(_name, 0444, _func, NULL), (void *)_var } \ > > + })[0].attr.attr) > > In another thread, we recently realised that the array syntax is not needed > here, so I've updated this to be: > > > #define PERF_EXT_ATTR_ENTRY(_name, _func, _var) \ > - (&((struct dev_ext_attribute[]) { \ > - { __ATTR(_name, 0444, _func, NULL), (void *)_var } \ > - })[0].attr.attr) > + (&((struct dev_ext_attribute) { \ > + __ATTR(_name, 0444, _func, NULL), (void *)_var \ > + }).attr.attr) > > > I've made those two changes, so I'll queue this up for 5.5. Thanks. Will, need me send a patch to remove this array syntax under the directory drivers/perf/... together? Best Regards, Joakim Zhang > Will
On Tue, Nov 05, 2019 at 01:48:44AM +0000, Joakim Zhang wrote: > > I've made those two changes, so I'll queue this up for 5.5. Thanks. > > Will, need me send a patch to remove this array syntax under the directory drivers/perf/... together? No need, but please check: https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=for-next/perf Will
> -----Original Message----- > From: Will Deacon <will@kernel.org> > Sent: 2019年11月5日 17:47 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: mark.rutland@arm.com; robin.murphy@arm.com; dl-linux-imx > <linux-imx@nxp.com>; Frank Li <frank.li@nxp.com>; > linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH 2/2] perf/imx_ddr: Dump AXI ID filter info to userspace > > On Tue, Nov 05, 2019 at 01:48:44AM +0000, Joakim Zhang wrote: > > > I've made those two changes, so I'll queue this up for 5.5. Thanks. > > > > Will, need me send a patch to remove this array syntax under the directory > drivers/perf/... together? > > No need, but please check: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern > el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fwill%2Flinux.git%2Flog%2F > %3Fh%3Dfor-next%2Fperf&data=02%7C01%7Cqiangqing.zhang%40nxp.c > om%7Cf724d214bf7d446d9adc08d761d51632%7C686ea1d3bc2b4c6fa92cd99c > 5c301635%7C0%7C1%7C637085440135712168&sdata=VDXdIzPUl9kkTu > mRr4bVi7mD6eHr1Wl%2FfmTbfrLpnF8%3D&reserved=0 Will, I have already checked, no problem, thank you! Best Regards, Joakim Zhang > Will
diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c index 3bbf806209a6..6db484251950 100644 --- a/drivers/perf/fsl_imx8_ddr_perf.c +++ b/drivers/perf/fsl_imx8_ddr_perf.c @@ -84,6 +84,64 @@ struct ddr_pmu { int id; }; +enum ddr_perf_filter_capabilities { + PERF_CAP_AXI_ID_FILTER = 0, + PERF_CAP_AXI_ID_FILTER_ENHANCED, + PERF_CAP_AXI_ID_FEAT_MAX, +}; + +static int ddr_perf_filter_feat_caps[PERF_CAP_AXI_ID_FEAT_MAX] = { + [PERF_CAP_AXI_ID_FILTER] = DDR_CAP_AXI_ID_FILTER, + [PERF_CAP_AXI_ID_FILTER_ENHANCED] = DDR_CAP_AXI_ID_FILTER_ENHANCED, +}; + +static u32 ddr_perf_filter_cap_get(struct ddr_pmu *pmu, int cap) +{ + switch (cap) { + case PERF_CAP_AXI_ID_FILTER: + return !!(pmu->devtype_data->quirks); + case PERF_CAP_AXI_ID_FILTER_ENHANCED: + return (pmu->devtype_data->quirks == + ddr_perf_filter_feat_caps[cap]); + } + + WARN(1, "unknown filter cap %d\n", cap); + + return 0; +} + +static ssize_t ddr_perf_filter_cap_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct ddr_pmu *pmu = dev_get_drvdata(dev); + struct dev_ext_attribute *ea = + container_of(attr, struct dev_ext_attribute, attr); + int cap = (long)ea->var; + + return snprintf(buf, PAGE_SIZE, "%u\n", + ddr_perf_filter_cap_get(pmu, cap)); +} + +#define PERF_EXT_ATTR_ENTRY(_name, _func, _var) \ + (&((struct dev_ext_attribute[]) { \ + { __ATTR(_name, 0444, _func, NULL), (void *)_var } \ + })[0].attr.attr) + +#define PERF_FILTER_EXT_ATTR_ENTRY(_name, _var) \ + PERF_EXT_ATTR_ENTRY(_name, ddr_perf_filter_cap_show, _var) + +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), + NULL, +}; + +static struct attribute_group ddr_perf_filter_cap_attr_group = { + .name = "caps", + .attrs = ddr_perf_filter_cap_attr, +}; + static ssize_t ddr_perf_cpumask_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -181,6 +239,7 @@ static const struct attribute_group *attr_groups[] = { &ddr_perf_events_attr_group, &ddr_perf_format_attr_group, &ddr_perf_cpumask_attr_group, + &ddr_perf_filter_cap_attr_group, NULL, };
caps/filter indicates whether HW supports AXI ID filter or not. caps/enhanced_filter indicates whether HW supports enhanced AXI ID filter or not. Users can check filter features from userspace with these attributions. Suggested-by: Will Deacon <will@kernel.org> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> --- drivers/perf/fsl_imx8_ddr_perf.c | 59 ++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)