Re: [PATCH] drm/panthor: Add support for performance counters

Boris Brezillon
2024-03-08T15:15:18Z

On Fri, 8 Mar 2024 13:52:18 +0000
Liviu Dudau <liviu.dudau@arm.com> wrote:

> Hi Adrián,
>
> Thanks for the patch and appologies for taking a bit longer to respond,
> I was trying to gather some internal Arm feedback before replying.
>
> On Tue, Mar 05, 2024 at 04:58:16PM +0000, Adrián Larumbe wrote:
> > This brings in support for Panthor's HW performance counters and querying
> > them from UM through a specific ioctl(). The code is inspired by existing
> > functionality for the Panfrost driver, with some noteworthy differences:
> >
> > - Sample size is now reported by the firmware rather than having to reckon
> > it by hand
> > - Counter samples are chained in a ring buffer that can be accessed
> > concurrently, but only from threads within a single context (this is
> > because of a HW limitation).
> > - List of enabled counters must be explicitly told from UM
> > - Rather than allocating the BO that will contain the perfcounter values
> > in the render context's address space, the samples ring buffer is mapped
> > onto the MCU's VM.
> > - If more than one thread within the same context tries to dump a sample,
> > then the kernel will copy the same frame to every single thread that was
> > able to join the dump queue right before the FW finished processing the
> > sample request.
> > - UM must provide a BO handle for retrieval of perfcnt values rather
> > than passing a user virtual address.
> >
> > The reason multicontext access to the driver's perfcnt ioctl interface
> > isn't tolerated is because toggling a different set of counters than the
> > current one implies a counter reset, which also messes up with the ring
> > buffer's extraction and insertion pointers. This is an unfortunate
> > hardware limitation.
>
> There are a few issues that we would like to improve with this patch.
>
> I'm not sure what user space app has been used for testing this (I'm guessing
> gputop from igt?) but whatever is used it needs to understand the counters
> being exposed.

We are using perfetto to expose perfcounters.

> In your patch there is no information given to the user space
> about the layout of the counters and / or their order. Where is this being
> planned to be defined?

That's done on purpose. We want to keep the kernel side as dumb as
possible so we don't have to maintain a perfcounter database there. All
the kernel needs to know is how much data it should transfer pass to
userspace, and that's pretty much it.

> Long term, I think it would be good to have details
> about the counters available.

The perfcounter definitions are currently declared in mesa (see the G57
perfcounter definitions for instance [1]). Mesa also contains a perfetto
datasource for Panfrost [2].

>
> The other issue we can see is with the perfcnt_process_sample() and its
> handling of threshold event and overflows. If the userspace doesn't sample
> quick enough and we cross the threshold we are going to lose samples and
> there is no mechanism to communicate to user space that the values they
> are getting have gaps. I believe the default mode for the firmware is to
> give you counter values relative to the last read value, so if you drop
> samples you're not going to make any sense of the data.

If we get relative values, that's indeed a problem. I thought we were
getting absolute values though, in which case, if you miss two 32-bit
wraparounds, either your sampling rate is very slow, or events occur at
a high rate.

>
> The third topic that is worth discussing is the runtime PM. Currently your
> patch will increment the runtime PM usage count when the performance
> counter dump is enabled, which means you will not be able to instrument
> your power saving modes. It might not be a concern for the current users
> of the driver, but it is worth discussing how to enable that workflow
> for future.

I guess we could add a flags field to drm_panthor_perfcnt_config and
declare a DRM_PANTHOR_PERFCNT_CFG_ALLOW_GPU_SUSPEND flag to support this
use case.

[1]https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/panfrost/perf/G57.xml?ref_type=heads
[2]https://gitlab.freedesktop.org/mesa/mesa/-/tree/main/src/panfrost/ds?ref_type=heads