Re: [PATCH V3 1/4] drivers/perf: imx_ddr: Add ddr performance counter support

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

 



On Fri, Feb 15, 2019 at 11:02 AM Mark Rutland <mark.rutland@xxxxxxx> wrote:
>
> On Fri, Feb 15, 2019 at 10:04:10AM -0600, Zhi Li wrote:
> > On Fri, Feb 15, 2019 at 8:22 AM Mark Rutland <mark.rutland@xxxxxxx> wrote:
> > >
> > > On Wed, Feb 13, 2019 at 06:12:27PM +0000, Frank Li wrote:
> > > > +struct fsl_ddr_devtype_data {
> > > > +     unsigned int flags;
> > > > +};
> > > > +
> > > > +static const struct fsl_ddr_devtype_data imx8_data;
> > > > +static const struct fsl_ddr_devtype_data imx8m_data = {
> > > > +     .flags = DDR_CAP_AXI_ID,
> > > > +};
> > > > +
> > > > +static const struct of_device_id imx_ddr_pmu_dt_ids[] = {
> > > > +     { .compatible = "fsl,imx8-ddr-pmu", .data = (void *)&imx8_data},
> > > > +     { .compatible = "fsl,imx8m-ddr-pmu", .data = (void *)&imx8m_data},
> > > > +     { /* sentinel */ }
> > > > +};
> > >
> > > You could encode the flags directly in of_device_id::data, and avoid the
> > > need for this structure entirely.
> >
> > I plan use fsl_ddr_devtype to save some friendly AXI ID name in future, such as
> > GPU  0x111122222
> > VPU  0x333344444
> >
> > so user can use bus master name instead of to look ref manual to get hex number.
> >
> > perf stat -e ddr0/axid_read, axid=GPU
> >
> > But perf application can't pass string by config.
> >
> > Any suggestion here ?
>
> Remove it for now, and we can consider it if/when you add that
> functionality.
>
> However, I will say that in general I am not a fan of coding SoC-level
> details into individual drivers, and I would prefer to avoid hard coding
> the AXI IDs here regardless.
>
> > > > +static u32 ddr_perf_alloc_counter(struct ddr_pmu *pmu, int event)
> > > > +{
> > > > +     int i;
> > > > +
> > > > +     /* Always map cycle event to counter 0 */
> > > > +     if (event == EVENT_CYCLES_ID)
> > > > +             return EVENT_CYCLES_COUNTER;
> > >
> > > What if the cycle counter is already in use?
> >
> > cycles counter is dedicated to cycles events.
> > Can't use for other event.
>
> What I meant was if you have two cycles events, they'll both try to
> configure the cycle counter (e.g. when it overflows), which may not
> behave as you expect.
>
> I think that you should only allow one event to use the cycle counter at
> a time.

Yes, one \only one cycle event can use cycle counter.

best regards
Frank Li

>
> > > > +static irqreturn_t ddr_perf_irq_handler(int irq, void *p)
> > > > +{
> > > > +     int i;
> > > > +     u8 reg;
> > > > +     int val;
> > > > +     int counter;
> > > > +     struct ddr_pmu *pmu = (struct ddr_pmu *) p;
> > > > +     struct perf_event *event;
> > > > +
> > > > +     /* Only cycles counter overflowed can issue irq. all counters will
> > > > +      * be stopped when cycles counter overflow. but other counter don't stop
> > > > +      * when overflow happen. Update all of the local counter values,
> > > > +      * then reset the cycles counter, so the others can continue
> > > > +      * counting.
> > > > +      */
> > >
> > > That is a rather unfortunate design decision.
> > >
> > > Can any of the counters count multiple events per cycle?
> >
> > No,  other events always much less than cycle counters.
>
> Ok. Please describe that in the comment.
>
> Thanks,
> Mark.



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux