From: "Adrián Larumbe" <adrian.larumbe@collabora.com>
To: "Zbigniew Kempczyński" <zbigniew.kempczynski@intel.com>
Cc: tvrtko.ursulin@intel.com, robdclark@chromium.org,
kamil.konieczny@linux.intel.com, lucas.demarchi@intel.com,
igt-dev@lists.freedesktop.org, healych@amazon.com,
kernel@collabora.com, Tvrtko Ursulin <tursulin@ursulin.net>,
Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [PATCH v3 1/2] lib: Add DRM driver sysfs profiling knob toggling functions
Date: Thu, 16 May 2024 20:28:07 +0100 [thread overview]
Message-ID: <lgboobv5bugrlbxlkhm5juyvxayfcyknlm4alqmojkjx2eskku@wdq34qhymnbx> (raw)
In-Reply-To: <20240513071233.lhefjndjurne2pt7@zkempczy-mobl2>
Hi, Zbigniew
On 13.05.2024 09:12, Zbigniew Kempczyński wrote:
> On Fri, May 10, 2024 at 08:15:20PM +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@ursulin.net>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@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@collabora.com>
> > + * Copyright © 2024 Amazon.com, Inc. or its affiliates.
> > + *
> > + */
> > +
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > +#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
> > +
> > +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) {
>
> goto end;
I'll also change the other functions to do the error handling after a goto label.
> > + 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);
>
> According to realloc above - if it will fail we'll get segfault here.
Sorry, completely forgot to do the due error checking here, will go into the next version.
> > + profiled_devices[i++].original_state = orig_state;
> > +
> > + close(sysfs_fd);
> > + }
> > +
>
> end:
>
> > + if (i == 0) {
> > + free(profiled_devices);
> > + profiled_devices = NULL;
> > + } else
> > + profiled_devices[i].syspath = NULL; /* Array terminator */
> > +
> > + return profiled_devices;
> > +}
> > +
> > +
> > +void igt_devices_toggle_profiling(struct igt_profiled_device *devices, bool enable)
>
> I think you should document this. Function sets profiling if enable == 1,
> for enable == 0 it restores original state. For me 'toggle' is a little
> bit confusing - I would rather expect 0 -> 1 and 1 -> 0 state change but
> you're restoring original state. Maybe 'set_profiling' would be better?
You're right about this, the naming might be a bit confusing. Maybe rename it to
igt_devices_enable_restore_profiling?
> > +{
> > + 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);
> > + }
> > +}
> > +
> > +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);
> > +}
> > +
> > +void igt_devices_update_original_profiling_state(struct igt_profiled_device *devices)
>
> You may use igt_devices_toggle_profiling(devices, true); to achieve
> the same effect (adding read() which checks on which devices profiling
> is still set to 0). I assume igt_devices_profiled() collects original
> state which should be restored so it's state should be const.
I think besides the iteration of the device list and opening the sysfs file, there isn't much
else that I can refactor between both functions. Maybe turning the actual loop and opening
the file into a macro? But even like this, I don't see much of a point in trying to save some
lines for the sake of only two functions.
> > +{
> > + 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;
> > + }
>
> Only reason I see new_state could be changed if external process
> would change it. Otherwise it is same to state from igt_devices_profiled().
That's indeed the case, and it was discussed in v2 of the patch series. Now I realised
I forgot to include patchwork links in the cover letter, but some people were afraid
the sysfs knob might be left in an inconsistent state if more tha once instance of
gputop was run at the same time. Tvrtko suggested checking the sysfs knob state at the
end of every gputop client display iteration to check for potential changes.
> --
> Zbigniew
>
> > +
> > + 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@collabora.com>
> > + * Copyright © 2024 Amazon.com, Inc. or its affiliates.
> > + */
> > +
> > +#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
> >
next prev parent reply other threads:[~2024-05-16 19:28 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-10 19:15 [PATCH v3 0/2] Add gputop support for sysfs profiling knob Adrián Larumbe
2024-05-10 19:15 ` [PATCH v3 1/2] lib: Add DRM driver sysfs profiling knob toggling functions Adrián Larumbe
2024-05-13 7:12 ` Zbigniew Kempczyński
2024-05-16 19:28 ` Adrián Larumbe [this message]
2024-05-20 7:23 ` Zbigniew Kempczyński
-- strict thread matches above, loose matches on Subject: below --
2024-05-10 19:22 [PATCH v3 0/2] Add gputop support for sysfs profiling knob Adrián Larumbe
2024-05-10 19:22 ` [PATCH v3 1/2] lib: Add DRM driver sysfs profiling knob toggling functions Adrián Larumbe
2024-05-15 17:50 ` Kamil Konieczny
2024-05-16 19:52 ` Adrián Larumbe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=lgboobv5bugrlbxlkhm5juyvxayfcyknlm4alqmojkjx2eskku@wdq34qhymnbx \
--to=adrian.larumbe@collabora.com \
--cc=daniel@ffwll.ch \
--cc=healych@amazon.com \
--cc=igt-dev@lists.freedesktop.org \
--cc=kamil.konieczny@linux.intel.com \
--cc=kernel@collabora.com \
--cc=lucas.demarchi@intel.com \
--cc=robdclark@chromium.org \
--cc=tursulin@ursulin.net \
--cc=tvrtko.ursulin@intel.com \
--cc=zbigniew.kempczynski@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.