[PATCH 1/2] docs/perf: Add AXI ID filter capabilities information

[PATCH 1/2] docs/perf: Add AXI ID filter capabilities information

[Joakim Zhang]

Add capabilities information for AXI ID filter.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 Documentation/admin-guide/perf/imx-ddr.rst | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/perf/imx-ddr.rst b/Documentation/admin-guide/perf/imx-ddr.rst
index d1347928ac39..9c5742e44b28 100644
--- a/Documentation/admin-guide/perf/imx-ddr.rst
+++ b/Documentation/admin-guide/perf/imx-ddr.rst
@@ -17,7 +17,8 @@ The "format" directory describes format of the config (event ID) and config1
 (AXI filtering) fields of the perf_event_attr structure, see /sys/bus/event_source/
 devices/imx8_ddr0/format/. The "events" directory describes the events types
 hardware supported that can be used with perf tool, see /sys/bus/event_source/
-devices/imx8_ddr0/events/.
+devices/imx8_ddr0/events/. The "caps" directory describes filter features implemented
+in DDR PMU, see /sys/bus/events_source/devices/imx8_ddr0/caps/.
   e.g.::
         perf stat -a -e imx8_ddr0/cycles/ cmd
         perf stat -a -e imx8_ddr0/read/,imx8_ddr0/write/ cmd
@@ -25,9 +26,12 @@ devices/imx8_ddr0/events/.
 AXI filtering is only used by CSV modes 0x41 (axid-read) and 0x42 (axid-write)
 to count reading or writing matches filter setting. Filter setting is various
 from different DRAM controller implementations, which is distinguished by quirks
-in the driver.
+in the driver. You also can dump info from userspace, filter in "caps" directory
+indicates whether PMU supports AXI ID filter or not; enhanced_filter indicates
+whether PMU supports enhanced AXI ID filter or not. Value 0 for un-supported, and
+value 1 for supported.
 
-* With DDR_CAP_AXI_ID_FILTER quirk.
+* With DDR_CAP_AXI_ID_FILTER quirk(filter: 1, enhanced_filter: 0).
   Filter is defined with two configuration parts:
   --AXI_ID defines AxID matching value.
   --AXI_MASKING defines which bits of AxID are meaningful for the matching.
@@ -51,7 +55,7 @@ in the driver.
   e.g.::
         perf stat -a -e imx8_ddr0/axid-read,axi_id=0x12/ cmd, which will monitor ARID=0x12
 
-* With DDR_CAP_AXI_ID_FILTER_ENHANCED quirk.
+* With DDR_CAP_AXI_ID_FILTER_ENHANCED quirk(filter: 1, enhanced_filter: 1).
   This is the extension of DDR_CAP_AXI_ID_FILTER quirk which is only support getting
   bursts from DDR transaction, i.e. only can get DDR read or write requests. You
   can select DDR_CAP_AXI_ID_FILTER_ENHANCED quirk if HW supports enhanced filter.
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] perf/imx_ddr: Dump AXI ID filter info to userspace

[Joakim Zhang]

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]);
+	}
+
+	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,
 };
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] perf/imx_ddr: Dump AXI ID filter info to userspace

[Will Deacon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 2/2] perf/imx_ddr: Dump AXI ID filter info to userspace

[Joakim Zhang]

> -----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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] perf/imx_ddr: Dump AXI ID filter info to userspace

[Will Deacon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 2/2] perf/imx_ddr: Dump AXI ID filter info to userspace

[Joakim Zhang]

> -----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&amp;data=02%7C01%7Cqiangqing.zhang%40nxp.c
> om%7Cf724d214bf7d446d9adc08d761d51632%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C1%7C637085440135712168&amp;sdata=VDXdIzPUl9kkTu
> mRr4bVi7mD6eHr1Wl%2FfmTbfrLpnF8%3D&amp;reserved=0

Will, I have already checked, no problem, thank you!

Best Regards,
Joakim Zhang
> Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel