Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

[John Garry]

On 26/02/2020 17:46, John Garry wrote:
>>>>
>>>> However, there is an issue from metric test.
>>>
>>> Good, so I now hope that you can fix up your events and metrics JSONs 
>>> as you
>>> want them.
>> Yes, I have sent out the pull request to fix up them.
> 
> Please note that jirka also requested that I add a PMU events perf test, 
> and it looks like that will need to go before non-RFC version of this 
> series...
> 
>>
>>
>>>>
>>>> The result is incorrect if we use*more than one*  event in metric 
>>>> expression,
>>> such as:
>>>> "MetricExpr": "imx8_ddr.write/imx8_ddr.cycles", Or
>>>> "MetricExpr": "imx8_ddr.write + imx8_ddr.cycles",
>>>>
>>>> If you only set the timeout(--timeout 1000 or sleep 1), the result 
>>>> is correct.
>>>> ./perf stat -M imx8mm_ddr0_read.all sleep 1 ./perf stat --timeout 1000
>>>> -M imx8mm_ddr0_read.all
>>>>
>>>> But, if you set interval(-I parameter), only the first result is 
>>>> correct, following
>>> results are incorrect.
>>>> ./perf stat -I 1000 -M imx8mm_ddr0_read.all
>>>>
>>>> This shouldn't been introduced by your code change, this problem may 
>>>> always
>>> exist.
>>>
>>> I'm not sure if this is an issue or whether you're using the tool 
>>> properly.
>>>
>>> It might help if you can reproduce on a x86 machine or other arch 
>>> which has
>>> metric support on mainline today - like power - and query about that.
>>
>> Sorry, I only have the arm64 machine. 
> 
> Great, so you do your development on an arm64 machine as well.
> 
> So I hope you may reproduce it. You just need to add a metric for SMMU 
> to construct such case.
>> I am not familiar to perf tool, so I failed to find the cause. :-(
> 
> I wouldn't rely on me to fix this for you ATM (assuming it is actually 
> broken).
> 
> About this:

Hi Joakim,

I have updated my perf series here:

https://github.com/hisilicon/kernel-dev/commits/private-topic-perf-5.6-sys-pmu-events-v2-upstream

Please test it. It is based on identifier file in form:

/sys/bus/event_bus/devices/PMU/identifier

It includes your imx8m? support, but I am not sure if it's the latest 
JSONs there.

JFYI, I also have an RFC SMMU PMCG patch here:

https://github.com/hisilicon/kernel-dev/tree/private-topic-perf-5.6-smmu-pmcg-identifier

Since we're in the merge window, I would rather avoid posting it this 
very moment.

Here was my cover letter:

------->8--------------------------------------------

perf pmu-events: Support event aliasing for system PMUs

Currently event aliasing for only CPU and uncore PMUs is supported. In
fact, only uncore PMUs aliasing is supported for when the uncore PMUs are
fixed for a CPU is supported, which may not always be the case for certain
architectures.

This series adds support for PMU event aliasing for system and other
uncore PMUs which are not fixed to a specific CPU.

For this, we introduce system event tables in generated pmu-events.c,
which contain a per-SoC table of events of all its system PMUs. Each
per-PMU event is matched by a "COMPAT" property.

When created aliases for PMUs, we treat core/uncore* and system PMUs
differently:

- For CPU PMU, we always match for the event mapfile based on the CPUID.
   This has not changed.

- For an uncore or system PMU, we iterate through all the events in all
   the system PMU tables.

   Matches are based on the "COMPAT" property matching the PMU sysfs
   identifier contents.

* uncore PMUs may also be matched by system PMUs event support.

Initial reference support is also added for ARM SMMUv3 PMCG (Performance
Monitor Event Group) PMU for HiSilicon hip08 platform with only a single
event so far - see driver in drivers/perf/arm_smmuv3_pmu.c for that driver.

Here is a sample output with this series:

root@ubuntu:/# ./perf list
   [...]

smmu v3 pmcg:
   config_cache_miss
        [Configuration cache miss caused by transaction or(ATS or
        non-ATS)translation request. Unit: smmuv3_pmcg]
   config_struct_access
        [Configuration structure access. Unit: smmuv3_pmcg]
   cycles
        [Clock cycles. Unit: smmuv3_pmcg]
   l1_tlb
        [SMMUv3 PMCG L1 TABLE transation. Unit: smmuv3_pmcg]
   pcie_ats_trans_passed
        [PCIe ATS Translated Transaction passed through SMMU. Unit: 
smmuv3_pmcg]
   pcie_ats_trans_rq
        [PCIe ATS Translation Request received. Unit: smmuv3_pmcg]
   tlb_miss
        [TLB miss caused by incomingtransaction or (ATS or non-ATS) 
translation
         request. Unit: smmuv3_pmcg]
   trans_table_walk_access
        [Translation table walk access. Unit: smmuv3_pmcg]
   transaction
        [Transaction. Unit: smmuv3_pmcg]


root@ubuntu:/# ./perf stat -v -e smmuv3_pmcg.l1_tlb sleep 1
Using CPUID 0x00000000480fd010
Using SYSID HIP08
-> smmuv3_pmcg_200100020/event=0x8a/
-> smmuv3_pmcg_200140020/event=0x8a/
-> smmuv3_pmcg_100020/event=0x8a/
-> smmuv3_pmcg_140020/event=0x8a/
-> smmuv3_pmcg_200148020/event=0x8a/
-> smmuv3_pmcg_148020/event=0x8a/
smmuv3_pmcg.l1_tlb: 0 1001221690 1001221690
smmuv3_pmcg.l1_tlb: 0 1001220090 1001220090
smmuv3_pmcg.l1_tlb: 101 1001219660 1001219660
smmuv3_pmcg.l1_tlb: 0 1001219010 1001219010
smmuv3_pmcg.l1_tlb: 0 1001218360 1001218360
smmuv3_pmcg.l1_tlb: 134 1001217850 1001217850

Performance counter stats for 'system wide':

                235      smmuv3_pmcg.l1_tlb 


        1.001263128 seconds time elapsed

root@ubuntu:/#

Support is also added for imx8mm DDR PMU.

Series is here:
https://github.com/hisilicon/kernel-dev/commits/private-topic-perf-5.6-sys-pmu-events-v2-upstream

Differences to v1:
- Stop using SoC id and use a per-PMU identifier instead
- Add metric group sys events support
   - This is a bit hacky
- Add imx8mm DDR Perf support


Thanks,
John

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

RE: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

[Joakim Zhang]

> -----Original Message-----
> From: John Garry <john.garry@huawei.com>
> Sent: 2020年3月30日 19:04
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: will@kernel.org; mark.rutland@arm.com; acme@kernel.org;
> linux-perf-users@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Frank Li
> <frank.li@nxp.com>; Jiri Olsa <jolsa@redhat.com>; Zhangshaokun
> <zhangshaokun@hisilicon.com>; Linuxarm <linuxarm@huawei.com>;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf
> 
> On 26/02/2020 17:46, John Garry wrote:
> Hi Joakim,
Hi John,

It can't work on my platform:

root@imx8mmevk:~# cat /sys/bus/event_source/devices/imx8_ddr0/identifier
i.mx8mm


root@imx8mmevk:~# ./perf list metric
List of pre-defined events (to be used in -e):

Metrics:

  imx8mm_ddr0_read.all
       [Bytes of all masters read from ddr0. Unit: uncore_i.mx8,ddrc ]


root@imx8mmevk:~# ./perf stat -a -I 1000 -M imx8mm_ddr0_read.all
event syntax error: ''
                      \___ parser error

 Usage: perf stat [<options>] [<command>]

    -M, --metrics <metric/metric group list>
                          monitor specified metrics or metric groups (separated by ,)

However, it can work on branch: private-topic-perf-5.6-pmu-events-upstream-v2. What changes have you made compared to last?

Could you help do below change? Since some Socs may have two ddr controller(ddr0/ddr1) with the same event code.
Delete file: tools/perf/pmu-events/arch/arm64/freescale/imx8mm/sys/ddrc.json
Replace file: tools/perf/pmu-events/arch/arm64/freescale/imx8mm/sys/metrics.json, put all events into metric.
[
   {
	"PublicDescription": "ddr0 cycles event",
	"MetricName": "imx8mm_ddr0.cycles",
	"MetricExpr": "imx8_ddr0\\/cycles\\/",
	"Unit": "imx8_ddr",
	"Compat": "i.mx8mm"
   },
   {
	"PublicDescription": "ddr0 read-cycles event",
	"MetricName": "imx8mm_ddr0.read_cycles",
	"MetricExpr": "imx8_ddr0\\/read\\-cycles\\/",
	"Unit": "imx8_ddr",
	"Compat": "i.mx8mm"
   },
   {
	"PublicDescription": "ddr0 write-cycles event",
	"MetricName": "imx8mm_ddr0.write_cycles",
	"MetricExpr": "imx8_ddr0\\/write\\-cycles\\/",
	"Unit": "imx8_ddr",
	"Compat": "i.mx8mm"
   },
   {
	"PublicDescription": "ddr0 read event",
	"MetricName": "imx8mm_ddr0.read",
	"MetricExpr": "imx8_ddr0\\/read\\/",
	"Unit": "imx8_ddr",
	"Compat": "i.mx8mm"
   },
   {
	"PublicDescription": "ddr0 write event",
	"MetricName": "imx8mm_ddr0.write",
	"MetricExpr": "imx8_ddr0\\/write\\/",
	"Unit": "imx8_ddr",
	"Compat": "i.mx8mm"
   },
   {
	"PublicDescription": "bytes all masters read from ddr0 based on read-cycles event.",
	"MetricName": "imx8mm_ddr0_read.all",
	"MetricExpr": "( imx8_ddr0\\/read\\-cycles\\/ ) * 4 * 4",
	"ScaleUnit": "9.765625e-4MB",
	"Unit": "imx8_ddr",
	"Compat": "i.mx8mm"
    },
   {
	"PublicDescription": "bytes all masters write to ddr0 based on read-cycles event.",
	"MetricName": "imx8mm_ddr0_write.all",
	"MetricExpr": "( imx8_ddr0\\/write\\-cycles\\/ ) * 4 * 4",
	"ScaleUnit": "9.765625e-4MB",
	"Unit": "imx8_ddr",
	"Compat": "i.mx8mm"
    }
]

Thanks.

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

Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

[John Garry]

Hi Joakim,

> 
> It can't work on my platform:
> 
> root@imx8mmevk:~# cat /sys/bus/event_source/devices/imx8_ddr0/identifier
> i.mx8mm
> 
> 
> root@imx8mmevk:~# ./perf list metric
> List of pre-defined events (to be used in -e):
> 
> Metrics:
> 
>    imx8mm_ddr0_read.all
>         [Bytes of all masters read from ddr0. Unit: uncore_i.mx8,ddrc ]

I think I simply missed something in jevents.c . Can you add this, 
regenerate, and retry:

diff --git a/tools/perf/pmu-events/jevents.c 
b/tools/perf/pmu-events/jevents.c
index 675aec9881ce..16dcf00c792a 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -258,6 +258,7 @@ static struct map {
        { "hisi_sccl,l3c", "hisi_sccl,l3c" },
        /* it's not realistic to keep adding these, we need something 
more scalable ... */
        { "smmuv3_pmcg", "smmuv3_pmcg" },
+       { "imx8_ddr", "imx8_ddr" },
        { "L3PMC", "amd_l3" },
        {}


> 
> 
> root@imx8mmevk:~# ./perf stat -a -I 1000 -M imx8mm_ddr0_read.all
> event syntax error: ''
>                        \___ parser error
> 
>   Usage: perf stat [<options>] [<command>]
> 
>      -M, --metrics <metric/metric group list>
>                            monitor specified metrics or metric groups (separated by ,)
> 
> However, it can work on branch: private-topic-perf-5.6-pmu-events-upstream-v2. What changes have you made compared to last?
> 
> Could you help do below change? Since some Socs may have two ddr controller(ddr0/ddr1) with the same event code.

The perf tool can handle that. So just run perf with -v option, and it 
will show event count breakdown per PMU, like this:

root@ubuntu:/# ./perf stat -v -e smmuv3_pmcg.l1_tlb sleep 1
Using CPUID 0x00000000480fd010
Using SYSID HIP08
-> smmuv3_pmcg_200100020/event=0x8a/
-> smmuv3_pmcg_200140020/event=0x8a/
-> smmuv3_pmcg_100020/event=0x8a/
-> smmuv3_pmcg_140020/event=0x8a/
-> smmuv3_pmcg_200148020/event=0x8a/
-> smmuv3_pmcg_148020/event=0x8a/
smmuv3_pmcg.l1_tlb: 0 1001221690 1001221690
smmuv3_pmcg.l1_tlb: 0 1001220090 1001220090
smmuv3_pmcg.l1_tlb: 101 1001219660 1001219660
smmuv3_pmcg.l1_tlb: 0 1001219010 1001219010
smmuv3_pmcg.l1_tlb: 0 1001218360 1001218360
smmuv3_pmcg.l1_tlb: 134 1001217850 1001217850


Here smmuv3_pmcg_200100020, smmuv3_pmcg_200140020, etc are all instances 
of the same PMU, like your DDRC.

And if you want to run just for a single PMU, try like:

./perf stat -e smmuv3_pmcg_200148020/smmuv3_pmcg.l1_tlb/

[thanks for jirka for that hint :) ]

Thanks,
John

> Delete file: tools/perf/pmu-events/arch/arm64/freescale/imx8mm/sys/ddrc.json
> Replace file: tools/perf/pmu-events/arch/arm64/freescale/imx8mm/sys/metrics.json, put all events into metric.
> [
>     {
> 	"PublicDescription": "ddr0 cycles event",
> 	"MetricName": "imx8mm_ddr0.cycles",
> 	"MetricExpr": "imx8_ddr0\\/cycles\\/",
> 	"Unit": "imx8_ddr",
> 	"Compat": "i.mx8mm"
>     },
>     {
> 	"PublicDescription": "ddr0 read-cycles event",
> 	"MetricName": "imx8mm_ddr0.read_cycles",
> 	"MetricExpr": "imx8_ddr0\\/read\\-cycles\\/",
> 	"Unit": "imx8_ddr",
> 	"Compat": "i.mx8mm"
>     },
>     {
> 	"PublicDescription": "ddr0 write-cycles event",
> 	"MetricName": "imx8mm_ddr0.write_cycles",
> 	"MetricExpr": "imx8_ddr0\\/write\\-cycles\\/",
> 	"Unit": "imx8_ddr",
> 	"Compat": "i.mx8mm"
>     },
>     {
> 	"PublicDescription": "ddr0 read event",
> 	"MetricName": "imx8mm_ddr0.read",
> 	"MetricExpr": "imx8_ddr0\\/read\\/",
> 	"Unit": "imx8_ddr",
> 	"Compat": "i.mx8mm"
>     },
>     {
> 	"PublicDescription": "ddr0 write event",
> 	"MetricName": "imx8mm_ddr0.write",
> 	"MetricExpr": "imx8_ddr0\\/write\\/",
> 	"Unit": "imx8_ddr",
> 	"Compat": "i.mx8mm"
>     },
>     {
> 	"PublicDescription": "bytes all masters read from ddr0 based on read-cycles event.",
> 	"MetricName": "imx8mm_ddr0_read.all",
> 	"MetricExpr": "( imx8_ddr0\\/read\\-cycles\\/ ) * 4 * 4",
> 	"ScaleUnit": "9.765625e-4MB",
> 	"Unit": "imx8_ddr",
> 	"Compat": "i.mx8mm"
>      },
>     {
> 	"PublicDescription": "bytes all masters write to ddr0 based on read-cycles event.",
> 	"MetricName": "imx8mm_ddr0_write.all",
> 	"MetricExpr": "( imx8_ddr0\\/write\\-cycles\\/ ) * 4 * 4",
> 	"ScaleUnit": "9.765625e-4MB",
> 	"Unit": "imx8_ddr",
> 	"Compat": "i.mx8mm"
>      }
> ]
> 
> Thanks.
> 
> Best Regards,
> Joakim Zhang
> 

Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

[John Garry]

Hi Joakim,

> 
> It can't work on my platform:
> 
> root@imx8mmevk:~# cat /sys/bus/event_source/devices/imx8_ddr0/identifier
> i.mx8mm
> 
> 
> root@imx8mmevk:~# ./perf list metric
> List of pre-defined events (to be used in -e):
> 
> Metrics:
> 
>    imx8mm_ddr0_read.all
>         [Bytes of all masters read from ddr0. Unit: uncore_i.mx8,ddrc ]

I think I simply missed something in jevents.c . Can you add this, 
regenerate, and retry:

diff --git a/tools/perf/pmu-events/jevents.c 
b/tools/perf/pmu-events/jevents.c
index 675aec9881ce..16dcf00c792a 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -258,6 +258,7 @@ static struct map {
        { "hisi_sccl,l3c", "hisi_sccl,l3c" },
        /* it's not realistic to keep adding these, we need something 
more scalable ... */
        { "smmuv3_pmcg", "smmuv3_pmcg" },
+       { "imx8_ddr", "imx8_ddr" },
        { "L3PMC", "amd_l3" },
        {}


> 
> 
> root@imx8mmevk:~# ./perf stat -a -I 1000 -M imx8mm_ddr0_read.all
> event syntax error: ''
>                        \___ parser error
> 
>   Usage: perf stat [<options>] [<command>]
> 
>      -M, --metrics <metric/metric group list>
>                            monitor specified metrics or metric groups (separated by ,)
> 
> However, it can work on branch: private-topic-perf-5.6-pmu-events-upstream-v2. What changes have you made compared to last?
> 
> Could you help do below change? Since some Socs may have two ddr controller(ddr0/ddr1) with the same event code.

The perf tool can handle that. So just run perf with -v option, and it 
will show event count breakdown per PMU, like this:

root@ubuntu:/# ./perf stat -v -e smmuv3_pmcg.l1_tlb sleep 1
Using CPUID 0x00000000480fd010
Using SYSID HIP08
-> smmuv3_pmcg_200100020/event=0x8a/
-> smmuv3_pmcg_200140020/event=0x8a/
-> smmuv3_pmcg_100020/event=0x8a/
-> smmuv3_pmcg_140020/event=0x8a/
-> smmuv3_pmcg_200148020/event=0x8a/
-> smmuv3_pmcg_148020/event=0x8a/
smmuv3_pmcg.l1_tlb: 0 1001221690 1001221690
smmuv3_pmcg.l1_tlb: 0 1001220090 1001220090
smmuv3_pmcg.l1_tlb: 101 1001219660 1001219660
smmuv3_pmcg.l1_tlb: 0 1001219010 1001219010
smmuv3_pmcg.l1_tlb: 0 1001218360 1001218360
smmuv3_pmcg.l1_tlb: 134 1001217850 1001217850


Here smmuv3_pmcg_200100020, smmuv3_pmcg_200140020, etc are all instances 
of the same PMU, like your DDRC.

And if you want to run just for a single PMU, try like:

./perf stat -e smmuv3_pmcg_200148020/smmuv3_pmcg.l1_tlb/

[thanks for jirka for that hint :) ]

Thanks,
John

> Delete file: tools/perf/pmu-events/arch/arm64/freescale/imx8mm/sys/ddrc.json
> Replace file: tools/perf/pmu-events/arch/arm64/freescale/imx8mm/sys/metrics.json, put all events into metric.
> [
>     {
> 	"PublicDescription": "ddr0 cycles event",
> 	"MetricName": "imx8mm_ddr0.cycles",
> 	"MetricExpr": "imx8_ddr0\\/cycles\\/",
> 	"Unit": "imx8_ddr",
> 	"Compat": "i.mx8mm"
>     },
>     {
> 	"PublicDescription": "ddr0 read-cycles event",
> 	"MetricName": "imx8mm_ddr0.read_cycles",
> 	"MetricExpr": "imx8_ddr0\\/read\\-cycles\\/",
> 	"Unit": "imx8_ddr",
> 	"Compat": "i.mx8mm"
>     },
>     {
> 	"PublicDescription": "ddr0 write-cycles event",
> 	"MetricName": "imx8mm_ddr0.write_cycles",
> 	"MetricExpr": "imx8_ddr0\\/write\\-cycles\\/",
> 	"Unit": "imx8_ddr",
> 	"Compat": "i.mx8mm"
>     },
>     {
> 	"PublicDescription": "ddr0 read event",
> 	"MetricName": "imx8mm_ddr0.read",
> 	"MetricExpr": "imx8_ddr0\\/read\\/",
> 	"Unit": "imx8_ddr",
> 	"Compat": "i.mx8mm"
>     },
>     {
> 	"PublicDescription": "ddr0 write event",
> 	"MetricName": "imx8mm_ddr0.write",
> 	"MetricExpr": "imx8_ddr0\\/write\\/",
> 	"Unit": "imx8_ddr",
> 	"Compat": "i.mx8mm"
>     },
>     {
> 	"PublicDescription": "bytes all masters read from ddr0 based on read-cycles event.",
> 	"MetricName": "imx8mm_ddr0_read.all",
> 	"MetricExpr": "( imx8_ddr0\\/read\\-cycles\\/ ) * 4 * 4",
> 	"ScaleUnit": "9.765625e-4MB",
> 	"Unit": "imx8_ddr",
> 	"Compat": "i.mx8mm"
>      },
>     {
> 	"PublicDescription": "bytes all masters write to ddr0 based on read-cycles event.",
> 	"MetricName": "imx8mm_ddr0_write.all",
> 	"MetricExpr": "( imx8_ddr0\\/write\\-cycles\\/ ) * 4 * 4",
> 	"ScaleUnit": "9.765625e-4MB",
> 	"Unit": "imx8_ddr",
> 	"Compat": "i.mx8mm"
>      }
> ]
> 
> Thanks.
> 
> Best Regards,
> Joakim Zhang
> 


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

RE: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

[Joakim Zhang]

Hi John,

> -----Original Message-----
> From: John Garry <john.garry@huawei.com>
> Sent: 2020年4月7日 23:00
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: will@kernel.org; mark.rutland@arm.com; acme@kernel.org;
> linux-perf-users@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Frank Li
> <frank.li@nxp.com>; Jiri Olsa <jolsa@redhat.com>; Zhangshaokun
> <zhangshaokun@hisilicon.com>; Linuxarm <linuxarm@huawei.com>;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf
> 
> Hi Joakim,
> 
> >
> > It can't work on my platform:
> >
> > root@imx8mmevk:~# cat
> > /sys/bus/event_source/devices/imx8_ddr0/identifier
> > i.mx8mm
> >
> >
> > root@imx8mmevk:~# ./perf list metric
> > List of pre-defined events (to be used in -e):
> >
> > Metrics:
> >
> >    imx8mm_ddr0_read.all
> >         [Bytes of all masters read from ddr0. Unit: uncore_i.mx8,ddrc
> > ]
> 
> I think I simply missed something in jevents.c . Can you add this, regenerate,
> and retry:
> 
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index 675aec9881ce..16dcf00c792a 100644
> --- a/tools/perf/pmu-events/jevents.c
> +++ b/tools/perf/pmu-events/jevents.c
> @@ -258,6 +258,7 @@ static struct map {
>         { "hisi_sccl,l3c", "hisi_sccl,l3c" },
>         /* it's not realistic to keep adding these, we need something more
> scalable ... */
>         { "smmuv3_pmcg", "smmuv3_pmcg" },
> +       { "imx8_ddr", "imx8_ddr" },
>         { "L3PMC", "amd_l3" },
>         {}
Yes, events can work after adding this code, however, metrics still can't work, it seems that metricgroup fails to parse the metric expression. You may change something from metricgroup.c causing this issue.

> > root@imx8mmevk:~# ./perf stat -a -I 1000 -M imx8mm_ddr0_read.all event
> > syntax error: ''
> >                        \___ parser error
> >
> >   Usage: perf stat [<options>] [<command>]
> >
> >      -M, --metrics <metric/metric group list>
> >                            monitor specified metrics or metric groups
> > (separated by ,)
> >
> > However, it can work on branch:
> private-topic-perf-5.6-pmu-events-upstream-v2. What changes have you made
> compared to last?
> >
> > Could you help do below change? Since some Socs may have two ddr
> controller(ddr0/ddr1) with the same event code.
> 
> The perf tool can handle that. So just run perf with -v option, and it will show
> event count breakdown per PMU, like this:
> 
> root@ubuntu:/# ./perf stat -v -e smmuv3_pmcg.l1_tlb sleep 1 Using CPUID
> 0x00000000480fd010 Using SYSID HIP08
> -> smmuv3_pmcg_200100020/event=0x8a/
> -> smmuv3_pmcg_200140020/event=0x8a/
> -> smmuv3_pmcg_100020/event=0x8a/
> -> smmuv3_pmcg_140020/event=0x8a/
> -> smmuv3_pmcg_200148020/event=0x8a/
> -> smmuv3_pmcg_148020/event=0x8a/
> smmuv3_pmcg.l1_tlb: 0 1001221690 1001221690
> smmuv3_pmcg.l1_tlb: 0 1001220090 1001220090
> smmuv3_pmcg.l1_tlb: 101 1001219660 1001219660
> smmuv3_pmcg.l1_tlb: 0 1001219010 1001219010
> smmuv3_pmcg.l1_tlb: 0 1001218360 1001218360
> smmuv3_pmcg.l1_tlb: 134 1001217850 1001217850
>
> Here smmuv3_pmcg_200100020, smmuv3_pmcg_200140020, etc are all
> instances of the same PMU, like your DDRC.
> 
> And if you want to run just for a single PMU, try like:
> 
> ./perf stat -e smmuv3_pmcg_200148020/smmuv3_pmcg.l1_tlb/
Thank you for your hint, it is really useful.

Then please help update the ddrc.json and metric.json:
ddrc.json:
[
   {
           "BriefDescription": "ddr cycles event",
           "EventCode": "0x00",
           "EventName": "imx8_ddr.cycles",
           "Unit": "imx8_ddr",
           "Compat": "i.mx8mm"
   },
   {
           "BriefDescription": "ddr read-cycles event",
           "EventCode": "0x2a",
           "EventName": "imx8_ddr.read_cycles",
           "Unit": "imx8_ddr",
           "Compat": "i.mx8mm"
   },
   {
           "BriefDescription": "ddr write-cycles event",
           "EventCode": "0x2b",
           "EventName": "imx8_ddr.write_cycles",
           "Unit": "imx8_ddr",
           "Compat": "i.mx8mm"
   },
   {
           "BriefDescription": "ddr read event",
           "EventCode": "0x35",
           "EventName": "imx8_ddr.read",
           "Unit": "imx8_ddr",
           "Compat": "i.mx8mm"
   },
   {
           "BriefDescription": "ddr write event",
           "EventCode": "0x38",
           "EventName": "imx8_ddr.write",
           "Unit": "imx8_ddr",
           "Compat": "i.mx8mm"
   }
]

metric.json:
[
   {
	    "BriefDescription": "bytes all masters read from ddr based on read-cycles event",
	    "MetricName": "imx8mm_ddr_read.all",
	    "MetricExpr": "imx8_ddr.read_cycles * 4 * 4",
	    "ScaleUnit": "9.765625e-4MB",
	    "Unit": "imx8_ddr",
	    "Compat": "i.mx8mm"
    },
   {
	    "BriefDescription": "bytes all masters write to ddr based on write-cycles event",
	    "MetricName": "imx8mm_ddr_write.all",
	    "MetricExpr": "imx8_ddr.write_cycles * 4 * 4",
	    "ScaleUnit": "9.765625e-4MB",
	    "Unit": "imx8_ddr",
	    "Compat": "i.mx8mm"
    }
]

Best Regards,
Joakim Zhang
> [thanks for jirka for that hint :) ]
> 
> Thanks,
> John

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

Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

[John Garry]

On 08/04/2020 06:04, Joakim Zhang wrote:
>>   . Can you add this, regenerate,
>> and retry:
>>
>> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
>> index 675aec9881ce..16dcf00c792a 100644
>> --- a/tools/perf/pmu-events/jevents.c
>> +++ b/tools/perf/pmu-events/jevents.c
>> @@ -258,6 +258,7 @@ static struct map {
>>          { "hisi_sccl,l3c", "hisi_sccl,l3c" },
>>          /* it's not realistic to keep adding these, we need something more
>> scalable ... */
>>          { "smmuv3_pmcg", "smmuv3_pmcg" },
>> +       { "imx8_ddr", "imx8_ddr" },
>>          { "L3PMC", "amd_l3" },
>>          {}
> Yes, events can work after adding this code, however, metrics still can't work, it seems that metricgroup fails to parse the metric expression. You may change something from metricgroup.c causing this issue.

That code should be effectively the same.

What does perf list show for the metrics now?

> 
>>> root@imx8mmevk:~# ./perf stat -a -I 1000 -M imx8mm_ddr0_read.all event

Can you run with -v option and share the result?

>>> syntax error: ''
>>>                         \___ parser error
>>>
>>>    Usage: perf stat [<options>] [<command>]
>>>
>>>       -M, --metrics <metric/metric group list>
>>>                             monitor specified metrics or metric groups
>>> (separated by ,)
>>>
>>> However, it can work on branch:
>> private-topic-perf-5.6-pmu-events-upstream-v2. What changes have you made
>> compared to last?
>>> Could you help do below change? Since some Socs may have two ddr
>> controller(ddr0/ddr1) with the same event code.
>>
>> The perf tool can handle that. So just run perf with -v option, and it will show
>> event count breakdown per PMU, like this:
>>
>> root@ubuntu:/# ./perf stat -v -e smmuv3_pmcg.l1_tlb sleep 1 Using CPUID
>> 0x00000000480fd010 Using SYSID HIP08
>> -> smmuv3_pmcg_200100020/event=0x8a/
>> -> smmuv3_pmcg_200140020/event=0x8a/
>> -> smmuv3_pmcg_100020/event=0x8a/
>> -> smmuv3_pmcg_140020/event=0x8a/
>> -> smmuv3_pmcg_200148020/event=0x8a/
>> -> smmuv3_pmcg_148020/event=0x8a/
>> smmuv3_pmcg.l1_tlb: 0 1001221690 1001221690
>> smmuv3_pmcg.l1_tlb: 0 1001220090 1001220090
>> smmuv3_pmcg.l1_tlb: 101 1001219660 1001219660
>> smmuv3_pmcg.l1_tlb: 0 1001219010 1001219010
>> smmuv3_pmcg.l1_tlb: 0 1001218360 1001218360
>> smmuv3_pmcg.l1_tlb: 134 1001217850 1001217850
>>
>> Here smmuv3_pmcg_200100020, smmuv3_pmcg_200140020, etc are all
>> instances of the same PMU, like your DDRC.
>>
>> And if you want to run just for a single PMU, try like:
>>
>> ./perf stat -e smmuv3_pmcg_200148020/smmuv3_pmcg.l1_tlb/
> Thank you for your hint, it is really useful.
> 
> Then please help update the ddrc.json and metric.json:

ok, fine

> ddrc.json:
> [
>     {
>             "BriefDescription": "ddr cycles event",
>             "EventCode": "0x00",
>             "EventName": "imx8_ddr.cycles",

note: if you have just "cycles" here, instead of "imx8_ddr.cycles", then 
the HW PMU event will not show in perf list and you only get the alias 
under “imx8_ddr” group. This can be good if your PMU has so many events 
and you don't want to show them all. For hisilicon PMU, we have over 600 
HW PMU events.

However, as I see, a downside is that many PMUs have "cycles" events, 
and if you just use "cycles" as perf event, then it may just use the 
incorrect PMU by accident. I need to check this more.

thanks,
John

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

RE: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

[Joakim Zhang]

Hi John,

> -----Original Message-----
> From: John Garry <john.garry@huawei.com>
> Sent: 2020年4月8日 18:26
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: will@kernel.org; mark.rutland@arm.com; acme@kernel.org;
> linux-perf-users@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Frank Li
> <frank.li@nxp.com>; Jiri Olsa <jolsa@redhat.com>; Zhangshaokun
> <zhangshaokun@hisilicon.com>; Linuxarm <linuxarm@huawei.com>;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf
> 
> On 08/04/2020 06:04, Joakim Zhang wrote:
> >>   . Can you add this, regenerate,
> >> and retry:
> >>
> >> diff --git a/tools/perf/pmu-events/jevents.c
> >> b/tools/perf/pmu-events/jevents.c index 675aec9881ce..16dcf00c792a
> >> 100644
> >> --- a/tools/perf/pmu-events/jevents.c
> >> +++ b/tools/perf/pmu-events/jevents.c
> >> @@ -258,6 +258,7 @@ static struct map {
> >>          { "hisi_sccl,l3c", "hisi_sccl,l3c" },
> >>          /* it's not realistic to keep adding these, we need
> >> something more scalable ... */
> >>          { "smmuv3_pmcg", "smmuv3_pmcg" },
> >> +       { "imx8_ddr", "imx8_ddr" },
> >>          { "L3PMC", "amd_l3" },
> >>          {}
> > Yes, events can work after adding this code, however, metrics still can't work,
> it seems that metricgroup fails to parse the metric expression. You may change
> something from metricgroup.c causing this issue.
> 
> That code should be effectively the same.
> 
> What does perf list show for the metrics now?
root@imx8mmevk:~# ./perf list ddrc

List of pre-defined events (to be used in -e):


ddrc:
  imx8_ddr.cycles
       [ddr cycles event. Unit: imx8_ddr]
  imx8_ddr.read
       [ddr read event. Unit: imx8_ddr]
  imx8_ddr.read_cycles
       [ddr read-cycles event. Unit: imx8_ddr]
  imx8_ddr.write
       [ddr write event. Unit: imx8_ddr]
  imx8_ddr.write_cycles
       [ddr write-cycles event. Unit: imx8_ddr]

root@imx8mmevk:~# ./perf list metric

List of pre-defined events (to be used in -e):


Metrics:

  imx8mm_ddr_read.all
       [bytes all masters read from ddr based on read-cycles event. Unit: imx8_d
dr ]
  imx8mm_ddr_write.all
       [bytes all masters write to ddr based on read-cycles event. Unit: imx8_dd
r ]

root@imx8mmevk:~# ./perf stat -a -I 1000 -v -M imx8mm_ddr_read.all
Using CPUID 0x00000000410fd040
adding
event syntax error: ''
                      \___ parser error

 Usage: perf stat [<options>] [<command>]

    -M, --metrics <metric/metric group list>
                          monitor specified metrics or metric groups (separated by ,)

It seems that metricgroup fails to parse the metric expression. 
metric.json:
[
   {
	    "BriefDescription": "bytes all masters read from ddr based on read-cycles event",
	    "MetricName": "imx8mm_ddr_read.all",
	    "MetricExpr": "imx8_ddr.read_cycles * 4 * 4",
	    "ScaleUnit": "9.765625e-4MB",
	    "Unit": "imx8_ddr",
	    "Compat": "i.mx8mm"
    },
   {
	    "BriefDescription": "bytes all masters write to ddr based on write-cycles event",
	    "MetricName": "imx8mm_ddr_write.all",
	    "MetricExpr": "imx8_ddr.write_cycles * 4 * 4",
	    "ScaleUnit": "9.765625e-4MB",
	    "Unit": "imx8_ddr",
	    "Compat": "i.mx8mm"
    }
]

> >
> >>> root@imx8mmevk:~# ./perf stat -a -I 1000 -M imx8mm_ddr0_read.all
> >>> event
> 
> Can you run with -v option and share the result?
> 
> >>> syntax error: ''
> >>>                         \___ parser error
> >>>
> >>>    Usage: perf stat [<options>] [<command>]
> >>>
> >>>       -M, --metrics <metric/metric group list>
> >>>                             monitor specified metrics or metric
> >>> groups (separated by ,)
> >>>
> >>> However, it can work on branch:
> >> private-topic-perf-5.6-pmu-events-upstream-v2. What changes have you
> >> made compared to last?
> >>> Could you help do below change? Since some Socs may have two ddr
> >> controller(ddr0/ddr1) with the same event code.
> >>
> >> The perf tool can handle that. So just run perf with -v option, and
> >> it will show event count breakdown per PMU, like this:
> >>
> >> root@ubuntu:/# ./perf stat -v -e smmuv3_pmcg.l1_tlb sleep 1 Using
> >> CPUID
> >> 0x00000000480fd010 Using SYSID HIP08
> >> -> smmuv3_pmcg_200100020/event=0x8a/
> >> -> smmuv3_pmcg_200140020/event=0x8a/
> >> -> smmuv3_pmcg_100020/event=0x8a/
> >> -> smmuv3_pmcg_140020/event=0x8a/
> >> -> smmuv3_pmcg_200148020/event=0x8a/
> >> -> smmuv3_pmcg_148020/event=0x8a/
> >> smmuv3_pmcg.l1_tlb: 0 1001221690 1001221690
> >> smmuv3_pmcg.l1_tlb: 0 1001220090 1001220090
> >> smmuv3_pmcg.l1_tlb: 101 1001219660 1001219660
> >> smmuv3_pmcg.l1_tlb: 0 1001219010 1001219010
> >> smmuv3_pmcg.l1_tlb: 0 1001218360 1001218360
> >> smmuv3_pmcg.l1_tlb: 134 1001217850 1001217850
> >>
> >> Here smmuv3_pmcg_200100020, smmuv3_pmcg_200140020, etc are all
> >> instances of the same PMU, like your DDRC.
> >>
> >> And if you want to run just for a single PMU, try like:
> >>
> >> ./perf stat -e smmuv3_pmcg_200148020/smmuv3_pmcg.l1_tlb/
> > Thank you for your hint, it is really useful.
> >
> > Then please help update the ddrc.json and metric.json:
> 
> ok, fine
> 
> > ddrc.json:
> > [
> >     {
> >             "BriefDescription": "ddr cycles event",
> >             "EventCode": "0x00",
> >             "EventName": "imx8_ddr.cycles",
> 
> note: if you have just "cycles" here, instead of "imx8_ddr.cycles", then the HW
> PMU event will not show in perf list and you only get the alias under “imx8_ddr”
> group. This can be good if your PMU has so many events and you don't want to
> show them all. For hisilicon PMU, we have over 600 HW PMU events.
We can perf list like: ./perf list ddrc, to show what we want to list.

> However, as I see, a downside is that many PMUs have "cycles" events, and if
> you just use "cycles" as perf event, then it may just use the incorrect PMU by
> accident. I need to check this more.
Not quite understand☹ Any improvement should I made?

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

Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

[John Garry]

On 08/04/2020 12:41, Joakim Zhang wrote:
>> That code should be effectively the same.
>>
>> What does perf list show for the metrics now?
> root@imx8mmevk:~# ./perf list ddrc
> 
> List of pre-defined events (to be used in -e):
> 
> 
> ddrc:
>    imx8_ddr.cycles
>         [ddr cycles event. Unit: imx8_ddr]
>    imx8_ddr.read
>         [ddr read event. Unit: imx8_ddr]
>    imx8_ddr.read_cycles
>         [ddr read-cycles event. Unit: imx8_ddr]
>    imx8_ddr.write
>         [ddr write event. Unit: imx8_ddr]
>    imx8_ddr.write_cycles
>         [ddr write-cycles event. Unit: imx8_ddr]
> 
> root@imx8mmevk:~# ./perf list metric
> 
> List of pre-defined events (to be used in -e):
> 
> 
> Metrics:
> 
>    imx8mm_ddr_read.all
>         [bytes all masters read from ddr based on read-cycles event. Unit: imx8_d
> dr ]
>    imx8mm_ddr_write.all
>         [bytes all masters write to ddr based on read-cycles event. Unit: imx8_dd
> r ]
> 
> root@imx8mmevk:~# ./perf stat -a -I 1000 -v -M imx8mm_ddr_read.all
> Using CPUID 0x00000000410fd040
> adding
> event syntax error: ''
>                        \___ parser error
> 
>   Usage: perf stat [<options>] [<command>]
> 
>      -M, --metrics <metric/metric group list>
>                            monitor specified metrics or metric groups (separated by ,)
> 
> It seems that metricgroup fails to parse the metric expression.
> metric.json:
> [
>     {
> 	    "BriefDescription": "bytes all masters read from ddr based on read-cycles event",
> 	    "MetricName": "imx8mm_ddr_read.all",
> 	    "MetricExpr": "imx8_ddr.read_cycles * 4 * 4",
> 	    "ScaleUnit": "9.765625e-4MB",
> 	    "Unit": "imx8_ddr",

I noticed that previously we used imx8,ddr, which I think was wrong, so 
can you try to remove this here?

If that doesn't work, then I'll try this myself.

Thanks,
John

> 	    "Compat": "i.mx8mm"
>      },
>     {
> 	    "BriefDescription": "bytes all masters write to ddr based on write-cycles event",
> 	    "MetricName": "imx8mm_ddr_write.all",
> 	    "MetricExpr": "imx8_ddr.write_cycles * 4 * 4",
> 	    "ScaleUnit": "9.765625e-4MB",
> 	    "Unit": "imx8_ddr",
> 	    "Compat": "i.mx8mm"
>      }
> ]
> 
>>>>> root@imx8mmevk:~# ./perf stat

Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

[John Garry]

On 08/04/2020 12:41, Joakim Zhang wrote:
>> That code should be effectively the same.
>>
>> What does perf list show for the metrics now?
> root@imx8mmevk:~# ./perf list ddrc
> 
> List of pre-defined events (to be used in -e):
> 
> 
> ddrc:
>    imx8_ddr.cycles
>         [ddr cycles event. Unit: imx8_ddr]
>    imx8_ddr.read
>         [ddr read event. Unit: imx8_ddr]
>    imx8_ddr.read_cycles
>         [ddr read-cycles event. Unit: imx8_ddr]
>    imx8_ddr.write
>         [ddr write event. Unit: imx8_ddr]
>    imx8_ddr.write_cycles
>         [ddr write-cycles event. Unit: imx8_ddr]
> 
> root@imx8mmevk:~# ./perf list metric
> 
> List of pre-defined events (to be used in -e):
> 
> 
> Metrics:
> 
>    imx8mm_ddr_read.all
>         [bytes all masters read from ddr based on read-cycles event. Unit: imx8_d
> dr ]
>    imx8mm_ddr_write.all
>         [bytes all masters write to ddr based on read-cycles event. Unit: imx8_dd
> r ]
> 
> root@imx8mmevk:~# ./perf stat -a -I 1000 -v -M imx8mm_ddr_read.all
> Using CPUID 0x00000000410fd040
> adding
> event syntax error: ''
>                        \___ parser error
> 
>   Usage: perf stat [<options>] [<command>]
> 
>      -M, --metrics <metric/metric group list>
>                            monitor specified metrics or metric groups (separated by ,)
> 
> It seems that metricgroup fails to parse the metric expression.
> metric.json:
> [
>     {
> 	    "BriefDescription": "bytes all masters read from ddr based on read-cycles event",
> 	    "MetricName": "imx8mm_ddr_read.all",
> 	    "MetricExpr": "imx8_ddr.read_cycles * 4 * 4",
> 	    "ScaleUnit": "9.765625e-4MB",
> 	    "Unit": "imx8_ddr",

I noticed that previously we used imx8,ddr, which I think was wrong, so 
can you try to remove this here?

If that doesn't work, then I'll try this myself.

Thanks,
John

> 	    "Compat": "i.mx8mm"
>      },
>     {
> 	    "BriefDescription": "bytes all masters write to ddr based on write-cycles event",
> 	    "MetricName": "imx8mm_ddr_write.all",
> 	    "MetricExpr": "imx8_ddr.write_cycles * 4 * 4",
> 	    "ScaleUnit": "9.765625e-4MB",
> 	    "Unit": "imx8_ddr",
> 	    "Compat": "i.mx8mm"
>      }
> ]
> 
>>>>> root@imx8mmevk:~# ./perf stat


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

RE: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

[Joakim Zhang]

> -----Original Message-----
> From: John Garry <john.garry@huawei.com>
> Sent: 2020年4月15日 17:28
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: will@kernel.org; mark.rutland@arm.com; acme@kernel.org;
> linux-perf-users@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Jiri Olsa
> <jolsa@redhat.com>; Zhangshaokun <zhangshaokun@hisilicon.com>;
> Linuxarm <linuxarm@huawei.com>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf
> 
> On 08/04/2020 12:41, Joakim Zhang wrote:
> >> That code should be effectively the same.
> >>
> >> What does perf list show for the metrics now?
> > root@imx8mmevk:~# ./perf list ddrc
> >
> > List of pre-defined events (to be used in -e):
> >
> >
> > ddrc:
> >    imx8_ddr.cycles
> >         [ddr cycles event. Unit: imx8_ddr]
> >    imx8_ddr.read
> >         [ddr read event. Unit: imx8_ddr]
> >    imx8_ddr.read_cycles
> >         [ddr read-cycles event. Unit: imx8_ddr]
> >    imx8_ddr.write
> >         [ddr write event. Unit: imx8_ddr]
> >    imx8_ddr.write_cycles
> >         [ddr write-cycles event. Unit: imx8_ddr]
> >
> > root@imx8mmevk:~# ./perf list metric
> >
> > List of pre-defined events (to be used in -e):
> >
> >
> > Metrics:
> >
> >    imx8mm_ddr_read.all
> >         [bytes all masters read from ddr based on read-cycles event.
> > Unit: imx8_d dr ]
> >    imx8mm_ddr_write.all
> >         [bytes all masters write to ddr based on read-cycles event.
> > Unit: imx8_dd r ]
> >
> > root@imx8mmevk:~# ./perf stat -a -I 1000 -v -M imx8mm_ddr_read.all
> > Using CPUID 0x00000000410fd040 adding event syntax error: ''
> >                        \___ parser error
> >
> >   Usage: perf stat [<options>] [<command>]
> >
> >      -M, --metrics <metric/metric group list>
> >                            monitor specified metrics or metric groups
> > (separated by ,)
> >
> > It seems that metricgroup fails to parse the metric expression.
> > metric.json:
> > [
> >     {
> > 	    "BriefDescription": "bytes all masters read from ddr based on
> read-cycles event",
> > 	    "MetricName": "imx8mm_ddr_read.all",
> > 	    "MetricExpr": "imx8_ddr.read_cycles * 4 * 4",
> > 	    "ScaleUnit": "9.765625e-4MB",
> > 	    "Unit": "imx8_ddr",
> 
> I noticed that previously we used imx8,ddr, which I think was wrong, so can you
> try to remove this here?
> 
> If that doesn't work, then I'll try this myself.

Hi John,

It still cannot work with this code change.

Best Regards,
Joakim Zhang
> Thanks,
> John
> 
> > 	    "Compat": "i.mx8mm"
> >      },
> >     {
> > 	    "BriefDescription": "bytes all masters write to ddr based on
> write-cycles event",
> > 	    "MetricName": "imx8mm_ddr_write.all",
> > 	    "MetricExpr": "imx8_ddr.write_cycles * 4 * 4",
> > 	    "ScaleUnit": "9.765625e-4MB",
> > 	    "Unit": "imx8_ddr",
> > 	    "Compat": "i.mx8mm"
> >      }
> > ]
> >
> >>>>> root@imx8mmevk:~# ./perf stat

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

Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

[John Garry]

+ Ian (please check the patch at the bottom, thanks)

On 16/04/2020 08:15, Joakim Zhang wrote:
>> ?
>>
>> If that doesn't work, then I'll try this myself.
> Hi John,
> 
> It still cannot work with this code change.
> 

So I got a SMMUv3 PMCG test metric working here:

https://github.com/hisilicon/kernel-dev/commits/private-topic-perf-5.7-sys-pmu-events-v1-debug

as follows:

john@ubuntu:~/perf list metric

List of pre-defined events (to be used in -e):

Metrics:

   smmuv3_pmcg.fake
        [SMMUv3 PMCG fake metric]

john@ubuntu:~/sudo ./perf stat -a -M smmuv3_pmcg.fake sleep 1

  Performance counter stats for 'system wide':

    531510      smmuv3_pmcg.transaction  (49.90%)
124914193      smmuv3_pmcg.cycles       (49.90%)
    531331      smmuv3_pmcg.transaction  (50.29%)
124879887      smmuv3_pmcg.cycles       (50.29%)
    531269      smmuv3_pmcg.transaction  (50.45%)
124867388      smmuv3_pmcg.cycles       (50.45%)
    531372      smmuv3_pmcg.transaction  (50.45%)
124896723      smmuv3_pmcg.cycles       (50.45%)
    531431      smmuv3_pmcg.transaction  (50.12%)
124901700      smmuv3_pmcg.cycles       (50.12%)
    531574      smmuv3_pmcg.transaction  (49.72%)
124924231      smmuv3_pmcg.cycles       (49.72%)
    531598      smmuv3_pmcg.transaction  (49.56%)
124916914      smmuv3_pmcg.cycles       (49.56%)
    531440      smmuv3_pmcg.transaction  (49.56%)
124902473      smmuv3_pmcg.cycles       (49.56%)

        1.001167246 seconds time elapsed

So you can compare this to yours.

I did have to fix what looks like a bug in mainline on that branch:

--->8-----

perf parse-events: Fix comparison of evsel and leader pmu name

Since we now strdup() the pmu name for the event selector, use strcmp()
instead of pointer equality for comparison.

Fixes: d4953f7 ("perf parse-events: Fix 3 use after frees found with 
clang ASANutil/parse-events.c")
Signed-off-by: John Garry <john.garry@huawei.com>
---
tools/perf/util/parse-events.c
@@ -1629,7 +1629,7 @@ parse_events__set_leader_for_uncore_aliase(char 
*name, struct list_head *list,
		 * event. That can be used to distinguish the leader from
		 * other members, even they have the same event name.
		 */
		if ((leader != evsel) && (leader->pmu_name == evsel->pmu_name)) {
		if ((leader != evsel) && !strcmp(leader->pmu_name, evsel->pmu_name)) {
			is_leader = false;
			continue;
		}


---8<

Without it, I would get a spew of these:

assertion failed at util/parse-events.c:1637

@Ian, does this change look right? I am not intimately familiar with 
that code.

Thanks,
John

Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

[John Garry]

+ Ian (please check the patch at the bottom, thanks)

On 16/04/2020 08:15, Joakim Zhang wrote:
>> ?
>>
>> If that doesn't work, then I'll try this myself.
> Hi John,
> 
> It still cannot work with this code change.
> 

So I got a SMMUv3 PMCG test metric working here:

https://github.com/hisilicon/kernel-dev/commits/private-topic-perf-5.7-sys-pmu-events-v1-debug

as follows:

john@ubuntu:~/perf list metric

List of pre-defined events (to be used in -e):

Metrics:

   smmuv3_pmcg.fake
        [SMMUv3 PMCG fake metric]

john@ubuntu:~/sudo ./perf stat -a -M smmuv3_pmcg.fake sleep 1

  Performance counter stats for 'system wide':

    531510      smmuv3_pmcg.transaction  (49.90%)
124914193      smmuv3_pmcg.cycles       (49.90%)
    531331      smmuv3_pmcg.transaction  (50.29%)
124879887      smmuv3_pmcg.cycles       (50.29%)
    531269      smmuv3_pmcg.transaction  (50.45%)
124867388      smmuv3_pmcg.cycles       (50.45%)
    531372      smmuv3_pmcg.transaction  (50.45%)
124896723      smmuv3_pmcg.cycles       (50.45%)
    531431      smmuv3_pmcg.transaction  (50.12%)
124901700      smmuv3_pmcg.cycles       (50.12%)
    531574      smmuv3_pmcg.transaction  (49.72%)
124924231      smmuv3_pmcg.cycles       (49.72%)
    531598      smmuv3_pmcg.transaction  (49.56%)
124916914      smmuv3_pmcg.cycles       (49.56%)
    531440      smmuv3_pmcg.transaction  (49.56%)
124902473      smmuv3_pmcg.cycles       (49.56%)

        1.001167246 seconds time elapsed

So you can compare this to yours.

I did have to fix what looks like a bug in mainline on that branch:

--->8-----

perf parse-events: Fix comparison of evsel and leader pmu name

Since we now strdup() the pmu name for the event selector, use strcmp()
instead of pointer equality for comparison.

Fixes: d4953f7 ("perf parse-events: Fix 3 use after frees found with 
clang ASANutil/parse-events.c")
Signed-off-by: John Garry <john.garry@huawei.com>
---
tools/perf/util/parse-events.c
@@ -1629,7 +1629,7 @@ parse_events__set_leader_for_uncore_aliase(char 
*name, struct list_head *list,
		 * event. That can be used to distinguish the leader from
		 * other members, even they have the same event name.
		 */
		if ((leader != evsel) && (leader->pmu_name == evsel->pmu_name)) {
		if ((leader != evsel) && !strcmp(leader->pmu_name, evsel->pmu_name)) {
			is_leader = false;
			continue;
		}


---8<

Without it, I would get a spew of these:

assertion failed at util/parse-events.c:1637

@Ian, does this change look right? I am not intimately familiar with 
that code.

Thanks,
John



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

RE: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

[Joakim Zhang]

> -----Original Message-----
> From: John Garry <john.garry@huawei.com>
> Sent: 2020年4月17日 0:10
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; irogers@google.com
> Cc: will@kernel.org; mark.rutland@arm.com; acme@kernel.org;
> linux-perf-users@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Jiri Olsa
> <jolsa@redhat.com>; Zhangshaokun <zhangshaokun@hisilicon.com>;
> Linuxarm <linuxarm@huawei.com>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf
> 
> + Ian (please check the patch at the bottom, thanks)
> 
> On 16/04/2020 08:15, Joakim Zhang wrote:
> >> ?
> >>
> >> If that doesn't work, then I'll try this myself.
> > Hi John,
> >
> > It still cannot work with this code change.
> >
> 
> So I got a SMMUv3 PMCG test metric working here:
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
> om%2Fhisilicon%2Fkernel-dev%2Fcommits%2Fprivate-topic-perf-5.7-sys-pmu-e
> vents-v1-debug&amp;data=02%7C01%7Cqiangqing.zhang%40nxp.com%7Ce57
> 56ab5ec574a73f6c808d7e220bcc1%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C637226502540401438&amp;sdata=j2XIoVGv9kvdDIbX9USyInMI
> mar%2F5vB7db5uE3U1Jy0%3D&amp;reserved=0
> 
> as follows:
> 
> john@ubuntu:~/perf list metric
> 
> List of pre-defined events (to be used in -e):
> 
> Metrics:
> 
>    smmuv3_pmcg.fake
>         [SMMUv3 PMCG fake metric]
> 
> john@ubuntu:~/sudo ./perf stat -a -M smmuv3_pmcg.fake sleep 1
> 
>   Performance counter stats for 'system wide':
> 
>     531510      smmuv3_pmcg.transaction  (49.90%)
> 124914193      smmuv3_pmcg.cycles       (49.90%)
>     531331      smmuv3_pmcg.transaction  (50.29%)
> 124879887      smmuv3_pmcg.cycles       (50.29%)
>     531269      smmuv3_pmcg.transaction  (50.45%)
> 124867388      smmuv3_pmcg.cycles       (50.45%)
>     531372      smmuv3_pmcg.transaction  (50.45%)
> 124896723      smmuv3_pmcg.cycles       (50.45%)
>     531431      smmuv3_pmcg.transaction  (50.12%)
> 124901700      smmuv3_pmcg.cycles       (50.12%)
>     531574      smmuv3_pmcg.transaction  (49.72%)
> 124924231      smmuv3_pmcg.cycles       (49.72%)
>     531598      smmuv3_pmcg.transaction  (49.56%)
> 124916914      smmuv3_pmcg.cycles       (49.56%)
>     531440      smmuv3_pmcg.transaction  (49.56%)
> 124902473      smmuv3_pmcg.cycles       (49.56%)
> 
>         1.001167246 seconds time elapsed
> 
> So you can compare this to yours.
> 
> I did have to fix what looks like a bug in mainline on that branch:

Tested on private-topic-perf-5.7-sys-pmu-events-v1-debug branch, DDR Perf can work now, thanks.
Please remember to update JSON files which notified before.

Best Regards,
Joakim Zhang
> --->8-----
> 
> perf parse-events: Fix comparison of evsel and leader pmu name
> 
> Since we now strdup() the pmu name for the event selector, use strcmp()
> instead of pointer equality for comparison.
> 
> Fixes: d4953f7 ("perf parse-events: Fix 3 use after frees found with clang
> ASANutil/parse-events.c")
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
> tools/perf/util/parse-events.c
> @@ -1629,7 +1629,7 @@ parse_events__set_leader_for_uncore_aliase(char
> *name, struct list_head *list,
> 		 * event. That can be used to distinguish the leader from
> 		 * other members, even they have the same event name.
> 		 */
> 		if ((leader != evsel) && (leader->pmu_name == evsel->pmu_name)) {
> 		if ((leader != evsel) && !strcmp(leader->pmu_name,
> evsel->pmu_name)) {
> 			is_leader = false;
> 			continue;
> 		}
> 
> 
> ---8<
> 
> Without it, I would get a spew of these:
> 
> assertion failed at util/parse-events.c:1637
> 
> @Ian, does this change look right? I am not intimately familiar with that code.
> 
> Thanks,
> John
> 

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

Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

[John Garry]

On 17/04/2020 06:49, Joakim Zhang wrote:
>>   smmuv3_pmcg.transaction  (49.56%)
>> 124902473      smmuv3_pmcg.cycles       (49.56%)
>>
>>          1.001167246 seconds time elapsed
>>
>> So you can compare this to yours.
>>
>> I did have to fix what looks like a bug in mainline on that branch:
> Tested on private-topic-perf-5.7-sys-pmu-events-v1-debug branch, DDR Perf can work now, thanks.
> Please remember to update JSON files which notified before.
> 
> Best Regards,
> Joakim Zhang
>> --->8-----


ok, good. So without this fix I supplied, it did still look to work for 
me, but will lots of complaining from perf.

Anyway, I'll look to send this series soon, and I'll update the imx 
JSONs as requested. You will need to then add a fresh signed-off-by to 
what I send.

thanks,
john

Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

[John Garry]

On 17/04/2020 06:49, Joakim Zhang wrote:
>>   smmuv3_pmcg.transaction  (49.56%)
>> 124902473      smmuv3_pmcg.cycles       (49.56%)
>>
>>          1.001167246 seconds time elapsed
>>
>> So you can compare this to yours.
>>
>> I did have to fix what looks like a bug in mainline on that branch:
> Tested on private-topic-perf-5.7-sys-pmu-events-v1-debug branch, DDR Perf can work now, thanks.
> Please remember to update JSON files which notified before.
> 
> Best Regards,
> Joakim Zhang
>> --->8-----


ok, good. So without this fix I supplied, it did still look to work for 
me, but will lots of complaining from perf.

Anyway, I'll look to send this series soon, and I'll update the imx 
JSONs as requested. You will need to then add a fresh signed-off-by to 
what I send.

thanks,
john


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

RE: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf

[Joakim Zhang]

> -----Original Message-----
> From: John Garry <john.garry@huawei.com>
> Sent: 2020年4月17日 16:15
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; irogers@google.com
> Cc: will@kernel.org; mark.rutland@arm.com; acme@kernel.org;
> linux-perf-users@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Jiri Olsa
> <jolsa@redhat.com>; Zhangshaokun <zhangshaokun@hisilicon.com>;
> Linuxarm <linuxarm@huawei.com>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 0/6] perf/imx_ddr: Add namespace for i.MX8 DDR Perf
> 
> On 17/04/2020 06:49, Joakim Zhang wrote:
> >>   smmuv3_pmcg.transaction  (49.56%)
> >> 124902473      smmuv3_pmcg.cycles       (49.56%)
> >>
> >>          1.001167246 seconds time elapsed
> >>
> >> So you can compare this to yours.
> >>
> >> I did have to fix what looks like a bug in mainline on that branch:
> > Tested on private-topic-perf-5.7-sys-pmu-events-v1-debug branch, DDR Perf
> can work now, thanks.
> > Please remember to update JSON files which notified before.
> >
> > Best Regards,
> > Joakim Zhang
> >> --->8-----
> 
> 
> ok, good. So without this fix I supplied, it did still look to work for me, but will
> lots of complaining from perf.

Agree!

> Anyway, I'll look to send this series soon, and I'll update the imx JSONs as
> requested. You will need to then add a fresh signed-off-by to what I send.

Feel free add my signed-off-by tag directly:
	Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>

After test, I will give my Tested-by tag.

Thanks.

Best Regards,
Joakim Zhang
> thanks,
> john

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