On Fri, 8 Mar 2024 13:52:18 +0000 Liviu Dudau <liviu.dudau@xxxxxxx> 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
