| Message ID | 20240512222655.2792754-1-mcanal@igalia.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/v3d: Improve Performance Counters handling | expand |
On 5/12/24 19:23, Maíra Canal wrote:> > Maíra Canal (6): > drm/v3d: Add Performance Counters descriptions for V3D 4.2 and 7.1 > drm/v3d: Different V3D versions can have different number of perfcnt > drm/v3d: Create a new V3D parameter for the maximum number of perfcnt > drm/v3d: Create new IOCTL to expose performance counters information > drm/v3d: Use V3D_MAX_COUNTERS instead of V3D_PERFCNT_NUM > drm/v3d: Deprecate the use of the Performance Counters enum > > drivers/gpu/drm/v3d/v3d_drv.c | 11 + > drivers/gpu/drm/v3d/v3d_drv.h | 14 +- > drivers/gpu/drm/v3d/v3d_perfmon.c | 36 ++- > .../gpu/drm/v3d/v3d_performance_counters.h | 208 ++++++++++++++++++ > drivers/gpu/drm/v3d/v3d_sched.c | 2 +- > include/uapi/drm/v3d_drm.h | 48 ++++ > 6 files changed, 316 insertions(+), 3 deletions(-) > create mode 100644 drivers/gpu/drm/v3d/v3d_performance_counters.h > Applied to drm-misc/drm-misc-next! Best Regards, - Maíra
On Mon, 20 May 2024, Maíra Canal <mcanal@igalia.com> wrote: > On 5/12/24 19:23, Maíra Canal wrote:> >> Maíra Canal (6): >> drm/v3d: Add Performance Counters descriptions for V3D 4.2 and 7.1 >> drm/v3d: Different V3D versions can have different number of perfcnt >> drm/v3d: Create a new V3D parameter for the maximum number of perfcnt >> drm/v3d: Create new IOCTL to expose performance counters information >> drm/v3d: Use V3D_MAX_COUNTERS instead of V3D_PERFCNT_NUM >> drm/v3d: Deprecate the use of the Performance Counters enum > >> drivers/gpu/drm/v3d/v3d_drv.c | 11 + >> drivers/gpu/drm/v3d/v3d_drv.h | 14 +- >> drivers/gpu/drm/v3d/v3d_perfmon.c | 36 ++- >> .../gpu/drm/v3d/v3d_performance_counters.h | 208 ++++++++++++++++++ >> drivers/gpu/drm/v3d/v3d_sched.c | 2 +- >> include/uapi/drm/v3d_drm.h | 48 ++++ >> 6 files changed, 316 insertions(+), 3 deletions(-) >> create mode 100644 drivers/gpu/drm/v3d/v3d_performance_counters.h >> > > Applied to drm-misc/drm-misc-next! What compiler do you use? I'm hitting the same as kernel test robot [1] with arm-linux-gnueabihf-gcc 12.2.0. In general, I don't think it's a great idea to put arrays in headers, and then include it everywhere via v3d_drv.h. You're not just relying on the compiler to optimize it away in compilation units where its not referenced (likely to happen), but also for the linker to deduplicate rodata (possible, but I'm not sure that it will happen). I think you need to move the arrays to a .c file, and then either a) add interfaces to access the arrays, or b) declare the arrays and make them global. For the latter you also need to figure out how to expose the size. BR, Jani. [1] https://lore.kernel.org/r/202405211137.hueFkLKG-lkp@intel.com
Hi Jani, On 5/21/24 08:07, Jani Nikula wrote: > On Mon, 20 May 2024, Maíra Canal <mcanal@igalia.com> wrote: >> On 5/12/24 19:23, Maíra Canal wrote:> >>> Maíra Canal (6): >>> drm/v3d: Add Performance Counters descriptions for V3D 4.2 and 7.1 >>> drm/v3d: Different V3D versions can have different number of perfcnt >>> drm/v3d: Create a new V3D parameter for the maximum number of perfcnt >>> drm/v3d: Create new IOCTL to expose performance counters information >>> drm/v3d: Use V3D_MAX_COUNTERS instead of V3D_PERFCNT_NUM >>> drm/v3d: Deprecate the use of the Performance Counters enum > >>> drivers/gpu/drm/v3d/v3d_drv.c | 11 + >>> drivers/gpu/drm/v3d/v3d_drv.h | 14 +- >>> drivers/gpu/drm/v3d/v3d_perfmon.c | 36 ++- >>> .../gpu/drm/v3d/v3d_performance_counters.h | 208 ++++++++++++++++++ >>> drivers/gpu/drm/v3d/v3d_sched.c | 2 +- >>> include/uapi/drm/v3d_drm.h | 48 ++++ >>> 6 files changed, 316 insertions(+), 3 deletions(-) >>> create mode 100644 drivers/gpu/drm/v3d/v3d_performance_counters.h >>> >> >> Applied to drm-misc/drm-misc-next! > > What compiler do you use? I'm hitting the same as kernel test robot [1] > with arm-linux-gnueabihf-gcc 12.2.0. I use clang version 17.0.6. > > In general, I don't think it's a great idea to put arrays in headers, > and then include it everywhere via v3d_drv.h. You're not just relying on > the compiler to optimize it away in compilation units where its not > referenced (likely to happen), but also for the linker to deduplicate > rodata (possible, but I'm not sure that it will happen). > > I think you need to move the arrays to a .c file, and then either a) add > interfaces to access the arrays, or b) declare the arrays and make them > global. For the latter you also need to figure out how to expose the > size. I'll write a patch to fix it. Sorry for the disturbance, I didn't notice it with clang. Best Regards, - Maíra > > BR, > Jani. > > > [1] https://lore.kernel.org/r/202405211137.hueFkLKG-lkp@intel.com > >
On Tue, 21 May 2024, Maíra Canal <mcanal@igalia.com> wrote: > Hi Jani, > > On 5/21/24 08:07, Jani Nikula wrote: >> On Mon, 20 May 2024, Maíra Canal <mcanal@igalia.com> wrote: >>> On 5/12/24 19:23, Maíra Canal wrote:> >>>> Maíra Canal (6): >>>> drm/v3d: Add Performance Counters descriptions for V3D 4.2 and 7.1 >>>> drm/v3d: Different V3D versions can have different number of perfcnt >>>> drm/v3d: Create a new V3D parameter for the maximum number of perfcnt >>>> drm/v3d: Create new IOCTL to expose performance counters information >>>> drm/v3d: Use V3D_MAX_COUNTERS instead of V3D_PERFCNT_NUM >>>> drm/v3d: Deprecate the use of the Performance Counters enum > >>>> drivers/gpu/drm/v3d/v3d_drv.c | 11 + >>>> drivers/gpu/drm/v3d/v3d_drv.h | 14 +- >>>> drivers/gpu/drm/v3d/v3d_perfmon.c | 36 ++- >>>> .../gpu/drm/v3d/v3d_performance_counters.h | 208 ++++++++++++++++++ >>>> drivers/gpu/drm/v3d/v3d_sched.c | 2 +- >>>> include/uapi/drm/v3d_drm.h | 48 ++++ >>>> 6 files changed, 316 insertions(+), 3 deletions(-) >>>> create mode 100644 drivers/gpu/drm/v3d/v3d_performance_counters.h >>>> >>> >>> Applied to drm-misc/drm-misc-next! >> >> What compiler do you use? I'm hitting the same as kernel test robot [1] >> with arm-linux-gnueabihf-gcc 12.2.0. > > I use clang version 17.0.6. > >> >> In general, I don't think it's a great idea to put arrays in headers, >> and then include it everywhere via v3d_drv.h. You're not just relying on >> the compiler to optimize it away in compilation units where its not >> referenced (likely to happen), but also for the linker to deduplicate >> rodata (possible, but I'm not sure that it will happen). >> >> I think you need to move the arrays to a .c file, and then either a) add >> interfaces to access the arrays, or b) declare the arrays and make them >> global. For the latter you also need to figure out how to expose the >> size. > > I'll write a patch to fix it. Sorry for the disturbance, I didn't notice > it with clang. Another report [1]. BR, Jani. [1] https://lore.kernel.org/all/20240529122955.4cc16889@canb.auug.org.au
This series has the intention to address two issues with Performance Counters on V3D: 1. Update the number of Performance Counters for V3D 7.1 V3D 7.1 has 93 performance counters, while V3D 4.2 has only 87. Although the series [1] enabled support for V3D 7.1, it didn’t replace the maximum number of performance counters. This led to errors in user space as the Vulkan driver updated the maximum number of performance counters, but the kernel didn’t. Currently, the user space can request values for performance counters that are greater than 87 and the kernel will return an error instead of the values. That’s why `dEQP-VK.query_pool.performance_query.*` currently fails on Mesa CI [2]. This series intends to fix the `dEQP-VK.query_pool.performance_query.*` fail. 2. Make the kernel able to provide the Performance Counter descriptions Although all the management of the Performance Monitors is done through IOCTLs, which means that the code is in the kernel, the performance counter descriptions are in Mesa. This means two things: (#1) only Mesa has access to the descriptions and (#2) we can have inconsistencies between the information provided by Mesa and the kernel, as seen in the first issue addressed by this series. To minimize the risk of inconsistencies, this series proposes to use the kernel as a “single source of truth”. Therefore, if there are any changes to the performance monitors, all the changes must be done only in the kernel. This means that all information about the maximum number of performance counters and all the descriptions will now be retrieved from the kernel. This series is coupled with a Mesa series [3] that enabled the use of the new IOCTL. I appreciate any feedback from both the kernel and Mesa implementations. [1] https://lore.kernel.org/dri-devel/20231031073859.25298-1-itoral@igalia.com/ [2] https://gitlab.freedesktop.org/mesa/mesa/-/commit/ea1f09a5f21839f4f3b93610b58507c4bd9b9b81 [3] https://gitlab.freedesktop.org/mairacanal/mesa/-/tree/v3dv/fix-perfcnt Best Regards, - Maíra Canal --- v1 -> v2: https://lore.kernel.org/dri-devel/20240508143306.2435304-2-mcanal@igalia.com/T/ * [5/6] s/DRM_V3D_PARAM_V3D_MAX_PERF_COUNTERS/DRM_V3D_PARAM_MAX_PERF_COUNTERS (Iago Toral) * [6/6] Include a reference to the new DRM_V3D_PARAM_MAX_PERF_COUNTERS param (Iago Toral) * Add Iago's R-b (Iago Toral) Maíra Canal (6): drm/v3d: Add Performance Counters descriptions for V3D 4.2 and 7.1 drm/v3d: Different V3D versions can have different number of perfcnt drm/v3d: Create a new V3D parameter for the maximum number of perfcnt drm/v3d: Create new IOCTL to expose performance counters information drm/v3d: Use V3D_MAX_COUNTERS instead of V3D_PERFCNT_NUM drm/v3d: Deprecate the use of the Performance Counters enum drivers/gpu/drm/v3d/v3d_drv.c | 11 + drivers/gpu/drm/v3d/v3d_drv.h | 14 +- drivers/gpu/drm/v3d/v3d_perfmon.c | 36 ++- .../gpu/drm/v3d/v3d_performance_counters.h | 208 ++++++++++++++++++ drivers/gpu/drm/v3d/v3d_sched.c | 2 +- include/uapi/drm/v3d_drm.h | 48 ++++ 6 files changed, 316 insertions(+), 3 deletions(-) create mode 100644 drivers/gpu/drm/v3d/v3d_performance_counters.h