[PATCH v3 1/2] lib: Add DRM driver sysfs profiling knob toggling functions
Adrián Larumbe
adrian.larumbe at collabora.com
Thu May 16 19:52:13 UTC 2024
Hi Kamil, thanks for your comments.
On 15.05.2024 19:50, Kamil Konieczny wrote:
> Hi Adrián,
> On 2024-05-10 at 20:22:37 +0100, Adrián Larumbe wrote:
> > Some DRM drivers need to have their accounting HW toggled on demand from
> > user space so that fdinfo's drm-engine and drm-cycles tags can be updated
> > upon job completion.
> >
> > A profiler such as gputop should be able to check which DRM devices have
> > such a sysfs knob, record its original state, toggle-enable it and revert
> > this operation right before exiting.
> >
> > Also create a new static library dependency for this family of functions so
> > that it can later be linked against gputop.
> >
> > Cc: Tvrtko Ursulin <tursulin at ursulin.net>
> > Cc: Daniel Vetter <daniel at ffwll.ch>
> > Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > Signed-off-by: Adrián Larumbe <adrian.larumbe at collabora.com>
> > ---
> > lib/igt_profiling.c | 141 ++++++++++++++++++++++++++++++++++++++++++++
> > lib/igt_profiling.h | 21 +++++++
> > lib/meson.build | 8 +++
> > 3 files changed, 170 insertions(+)
> > create mode 100644 lib/igt_profiling.c
> > create mode 100644 lib/igt_profiling.h
> >
> > diff --git a/lib/igt_profiling.c b/lib/igt_profiling.c
> > new file mode 100644
> > index 000000000000..f5ac40d072ac
> > --- /dev/null
> > +++ b/lib/igt_profiling.c
> > @@ -0,0 +1,141 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2024 Collabora Ltd.
> > + * Copyright © 2024 Adrian Larumbe <adrian.larumbe at collabora.com>
> > + * Copyright © 2024 Amazon.com, Inc. or its affiliates.
>
> This is unexpected, why not:
>
> * Copyright © 2024 Collabora Ltd.
> *
> * Author: Adrian Larumbe <adrian.larumbe at collabora.com>
>
> Or:
>
> * Copyright © 2024 Collabora Ltd.
> * Copyright © 2024 Amazon.com, Inc. or its affiliates.
> *
> * Author: Adrian Larumbe <adrian.larumbe at collabora.com>
>
> Amazon would be ok but I am not so sure about 'affiliates'.
> Did you recive some funds from Amazon? Or reused its code?
Yep, this effort was sponsored by Amazon and they asked me to include
this copyright notice in the affected files.
> Adding Petri to cc.
>
> > + *
> > + */
> > +
> > +#include <fcntl.h>
> > +#include <unistd.h>
> -------------^
> Move this to begin, rest system header after it and sort them.
>
> > +#include <dirent.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <assert.h>
> > +#include <stdbool.h>
> > +
> > +#include "igt_profiling.h"
> > +
> > +#define SYSFS_DRM "/sys/class/drm"
> > +#define NUM_DEVICES 10
> > +
>
> Add description to each new public function.
>
> > +struct igt_profiled_device *igt_devices_profiled(void)
> > +{
> > + struct igt_profiled_device *profiled_devices;
> > + unsigned int devlist_len = NUM_DEVICES;
> > + unsigned int i = 0;
> > + struct dirent *entry;
> > + DIR *dev_dir;
> > +
> > + /* The return array will be resized in case there are too many devices */
> > + profiled_devices = malloc(devlist_len * sizeof(struct igt_profiled_device));
> > + if (!profiled_devices)
> > + return NULL;
> > +
> > + dev_dir = opendir(SYSFS_DRM);
> > + if (!dev_dir) {
> > + free(profiled_devices);
> > + return NULL;
> > + }
> > +
> > + while ((entry = readdir(dev_dir)) != NULL) {
> > + char path[PATH_MAX];
> > + char orig_state;
> > + int sysfs_fd;
> > +
> > + /* All DRM device entries are symlinks to other paths within sysfs */
> > + if (entry->d_type != DT_LNK)
> > + continue;
> > +
> > + /* We're only interested in render nodes */
> > + if (strstr(entry->d_name, "render") != entry->d_name)
> > + continue;
> > +
> > + snprintf(path, sizeof(path), "%s/%s/device/%s",
> > + SYSFS_DRM, entry->d_name, "profiling");
> > +
> > + if (access(path, F_OK))
> > + continue;
> > +
> > + sysfs_fd = open(path, O_RDONLY);
> > + if (sysfs_fd == -1)
> > + continue;
> > +
> > + if (read(sysfs_fd, &orig_state, 1) <= 0) {
> > + close(sysfs_fd);
> > + continue;
> > + }
> > +
> > + if (i == (devlist_len - 1)) {
> > + devlist_len += NUM_DEVICES;
> > + profiled_devices = realloc(profiled_devices, devlist_len);
> > + }
> > +
> > + profiled_devices[i].syspath = strdup(path);
> > + profiled_devices[i++].original_state = orig_state;
> > +
> > + close(sysfs_fd);
> > + }
> > +
> > + if (i == 0) {
> > + free(profiled_devices);
> > + profiled_devices = NULL;
> > + } else
> > + profiled_devices[i].syspath = NULL; /* Array terminator */
> > +
> > + return profiled_devices;
> > +}
> > +
> > +
>
> Add description.
>
> > +void igt_devices_toggle_profiling(struct igt_profiled_device *devices, bool enable)
> > +{
> > + assert(devices);
> > +
> > + for (unsigned int i = 0; devices[i].syspath; i++) {
> > +
> > + int sysfs_fd = open(devices[i].syspath, O_WRONLY);
> > +
> > + if (sysfs_fd < 0)
> > + continue;
> > +
> > + write(sysfs_fd, enable ? "1" : &devices[i].original_state, 1);
> > + close(sysfs_fd);
> > + }
> > +}
> > +
>
> Add description.
>
> > +void igt_devices_free_profiling(struct igt_profiled_device *devices)
> > +{
> > + assert(devices);
> > +
> > + for (unsigned int i = 0; devices[i].syspath; i++)
> > + free(devices[i].syspath);
> > +
> > + free(devices);
> > +}
> > +
>
> Add description.
>
> > +void igt_devices_update_original_profiling_state(struct igt_profiled_device *devices)
> > +{
> > + assert(devices);
> > +
> > + for (unsigned int i = 0; devices[i].syspath; i++) {
> > + char new_state;
> > + int sysfs_fd;
> > +
> > + sysfs_fd = open(devices[i].syspath, O_RDWR);
> > + if (sysfs_fd == -1)
> > + continue;
> > +
> > + if (!read(sysfs_fd, &new_state, 1)) {
> > + close(sysfs_fd);
> > + continue;
> > + }
> > +
> > + if (new_state == '0') {
> > + write(sysfs_fd, "1", 1);
> > + devices[i].original_state = new_state;
> > + }
> > +
> > + close(sysfs_fd);
> > + }
> > +}
> > diff --git a/lib/igt_profiling.h b/lib/igt_profiling.h
> > new file mode 100644
> > index 000000000000..fd0835d9a699
> > --- /dev/null
> > +++ b/lib/igt_profiling.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2024 Collabora Ltd.
> > + * Copyright © 2024 Adrian Larumbe <adrian.larumbe at collabora.com>
> ------^
> > + * Copyright © 2024 Amazon.com, Inc. or its affiliates.
> ------^
> Same here.
I'll describe all the new public functions in the next patch series iteration.
> Regards,
> Kamil
>
> > + */
> > +
> > +#ifndef IGT_PROFILING_H
> > +#define IGT_PROFILING_H
> > +
> > +struct igt_profiled_device {
> > + char *syspath;
> > + char original_state;
> > +};
> > +
> > +void igt_devices_toggle_profiling(struct igt_profiled_device *devices, bool enable);
> > +struct igt_profiled_device *igt_devices_profiled(void);
> > +void igt_devices_free_profiling(struct igt_profiled_device *devices);
> > +void igt_devices_update_original_profiling_state(struct igt_profiled_device *devices);
> > +
> > +#endif /* IGT_PROFILING_H */
> > diff --git a/lib/meson.build b/lib/meson.build
> > index e2f740c116f8..6f61ed5c558b 100644
> > --- a/lib/meson.build
> > +++ b/lib/meson.build
> > @@ -291,6 +291,14 @@ lib_igt_drm_fdinfo_build = static_library('igt_drm_fdinfo',
> >
> > lib_igt_drm_fdinfo = declare_dependency(link_with : lib_igt_drm_fdinfo_build,
> > include_directories : inc)
> > +
> > +lib_igt_profiling_build = static_library('igt_profiling',
> > + ['igt_profiling.c'],
> > + include_directories : inc)
> > +
> > +lib_igt_profiling = declare_dependency(link_with : lib_igt_profiling_build,
> > + include_directories : inc)
> > +
> > i915_perf_files = [
> > 'igt_list.c',
> > 'i915/perf.c',
> > --
> > 2.44.0
> >
More information about the igt-dev
mailing list