[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240308161515.1d74fd55@collabora.com>
Date: Fri, 8 Mar 2024 16:15:15 +0100
From: Boris Brezillon <boris.brezillon@...labora.com>
To: Liviu Dudau <liviu.dudau@....com>
Cc: Adrián Larumbe <adrian.larumbe@...labora.com>, Steven
Price <steven.price@....com>, Maarten Lankhorst
<maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>, kernel@...labora.com,
linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org, Mihail
Atanassov <Mihail.Atanassov@....com>
Subject: Re: [PATCH] drm/panthor: Add support for performance counters
On Fri, 8 Mar 2024 13:52:18 +0000
Liviu Dudau <liviu.dudau@....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
Powered by blists - more mailing lists