[PATCH V2 4/4] perf vendor events: Add JSON metrics for imx8mp DDR Perf

[Joakim Zhang]

Hi John,

> -----Original Message-----
> From: John Garry <john.garry at huawei.com>
> Sent: 2021年1月25日 21:45
> To: Joakim Zhang <qiangqing.zhang at nxp.com>; acme at kernel.org;
> jolsa at redhat.com
> Cc: will at kernel.org; linux-perf-users at vger.kernel.org; dl-linux-imx
> <linux-imx at nxp.com>; mark.rutland at arm.com;
> linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH V2 4/4] perf vendor events: Add JSON metrics for imx8mp
> DDR Perf
> 
> On 25/01/2021 08:59, Joakim Zhang wrote:
> > +   },
> > +   {
> > +	   "BriefDescription": "bytes of display isp1 read from ddr",
> 
> is it proper to have "bytes", while scale unit has KB?

"KB" would shown in metric result, I think users should understand it well.


> > +	   "MetricName": "imx8mp_ddr_read.isp1",
> > +	   "MetricExpr":
> > +"imx8_ddr0 at axid\\-read\\,axi_mask\\=0x0000\\,axi_id\\=0x006d@",
> 
> eh, does this really even qualify as a "metric"? Seems only a single event...

Yes, actually it needs a single metric. I consult to Will before, there is no way to program it in the kernel. I also don't want users to enter a long
command line, what worst is that users need find out the AXI ID for each master. If we put it in the metric, it is more friendly. Is it reasonable that
a single metric could be regarded as a special metric? Such as "( imx8_ddr0 at axid\\-read\\,axi_mask\\=0x0000\\,axi_id\\=0x006d@ ) * 1 * 1".


> > +	   "ScaleUnit": "9.765625e-4KB",
> > +	   "Unit": "imx8_ddr",
> > +	   "Compat": "i.MX8MP"
> > +   },
> > +   {
> > +	   "BriefDescription": "bytes of display isp1 write to ddr",
> > +	   "MetricName": "imx8mp_ddr_write.isp1",
> > +	   "MetricExpr":
> "imx8_ddr0 at axid\\-write\\,axi_mask\\=0x0000\\,axi_id\\=0x006d@",
> > +	   "MetricGroup": "imx8_ddr_DDR_MON",
> > +	   "Unit": "imx8_ddr",
> 
> why no scale unit here?

I will add it. Thanks.


> > +	   "Compat": "i.MX8MP"
> 
> btw, not sure if Arnaldo is ok with you not cc'ing other guys mentioned in
> "performance events subsystem" or "performance events tooling arm64"..

OK, I will cc them next version. Thanks for your kindly review.


Best Regards,
Joakim Zhang
> Thanks,
> john