[PATCH 1/1] tests/intel/xe_eu_stall: Add tests to run blocking and non-blocking read twice
Harish Chegondi
harish.chegondi at intel.com
Thu Apr 24 23:10:56 UTC 2025
On Thu, Apr 24, 2025 at 12:21:22PM +0200, Kamil Konieczny wrote:
Hi Kamil,
> Hi Dixit,,
> On 2025-04-23 at 16:14:33 -0700, Dixit, Ashutosh wrote:
> > On Wed, 23 Apr 2025 15:02:30 -0700, Harish Chegondi wrote:
> > >
> > > On Wed, Apr 23, 2025 at 10:20:06AM -0700, Dixit, Ashutosh wrote:
> > > > On Tue, 22 Apr 2025 19:57:34 -0700, Harish Chegondi wrote:
> > > > >
> > > > > Add tests that enable EU stall sampling, read all the EU stall data and
> > > > > disable EU stall sampling, two times back to back. Add tests for both
> > > > > blocking and non-blocking reads. Also, add a check for the presence of
> > > > > /proc/sys/dev/xe/observation_paranoid file which is required for the
> > > > > unprivileged-access test.
>
> imho that second change should go in separate patch.
I will separate the second change into a separate patch.
>
> > > > >
> > > > > Signed-off-by: Harish Chegondi <harish.chegondi at intel.com>
> > > > > ---
> > > > > tests/intel/xe_eu_stall.c | 44 ++++++++++++++++++++++++++++++---------
> > > > > 1 file changed, 34 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/tests/intel/xe_eu_stall.c b/tests/intel/xe_eu_stall.c
> > > > > index da9bd7843..15d4589c6 100644
> > > > > --- a/tests/intel/xe_eu_stall.c
> > > > > +++ b/tests/intel/xe_eu_stall.c
> > > > > @@ -14,9 +14,15 @@
> > > > > * SUBTEST: non-blocking-read
> > > > > * Description: Verify non-blocking read of EU stall data during a workload run
> > > > > *
> > > > > + * SUBTEST: non-blocking-read-twice
> > > > > + * Description: Run non-blocking read test twice with disable and enable between the runs
> > > > > + *
> > > > > * SUBTEST: blocking-read
> > > > > * Description: Verify blocking read of EU stall data during a workload run
> > > > > *
> > > > > + * SUBTEST: blocking-read-twice
> > > > > + * Description: Run blocking read test twice with disable and enable between the runs
> > > > > + *
> > > >
> > > > Not sure about the "read-twice" name, i.e. not sure if that name clearly
> > > > communicates the purpose of these new tests. The purpose to me seems that
> > > > reads can be done after a disable and enable, correct?
> > > >
> > > > Maybe "blocking-enable-disable" and "non-blocking-enable-disable" are
> > > > better names?
> > > Yes I will change the names. How about non-blocking-read-after-re-enable
> > > and blocking-read-after-re-enable ? This would convey that the test
> > > would enable two times and a read after the second enable ?
> >
> > "read" is sort of obvious, e.g. OA uses "blocking" and "non-blocking" as
> > the names, without "read". So, in the interest of keeping the names short,
> > I would just name them "non-blocking-re-enable" and
> > "blocking-re-enable". But anyway, the final choice of the names is yours,
> > the patch already has a R-b :)
> >
>
> I agree here, please keep names short and informative (which is hard...).
Will do.
>
> > > >
> > > > Otherwise this is:
> > > >
> > > > Reviewed-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > > >
> > > > Let me know if you are going to change the names. Thanks.
> > > >
> > > > > * SUBTEST: unprivileged-access
> > > > > * Description: Verify unprivileged open of a EU stall data stream fd
> > > > > *
> > > > > @@ -33,6 +39,7 @@
> > > > > #include <fcntl.h>
> > > > > #include <poll.h>
> > > > > #include <sys/ioctl.h>
> > > > > +#include <sys/stat.h>
> > > > > #include <sys/wait.h>
> > > > >
> > > > > #include "igt.h"
> > > > > @@ -460,13 +467,13 @@ static void print_eu_stall_data(uint32_t devid, uint8_t *buf, size_t size)
> > > > > * while the parent process reads the stall counters data, disables EU stall
> > > > > * counters once the workload completes execution.
> > > > > */
> > > > > -static void test_eustall(int drm_fd, uint32_t devid, bool blocking_read)
> > > > > +static void test_eustall(int drm_fd, uint32_t devid, bool blocking_read, int iter)
> > > > > {
> > > > > - uint32_t num_samples = 0, num_drops = 0;
> > > > > + uint32_t num_samples, num_drops;
> > > > > struct igt_helper_process work_load = {};
> > > > > struct sigaction sa = { 0 };
> > > > > int ret, flags, stream_fd;
> > > > > - uint64_t total_size = 0;
> > > > > + uint64_t total_size;
> > > > > uint8_t *buf;
> > > > >
> > > > > uint64_t properties[] = {
> > > > > @@ -520,7 +527,7 @@ static void test_eustall(int drm_fd, uint32_t devid, bool blocking_read)
> > > > > flags = O_CLOEXEC;
> > > > >
> > > > > set_fd_flags(stream_fd, flags);
> > > > > -
> > > > > +enable:
> > > > > do_ioctl(stream_fd, DRM_XE_OBSERVATION_IOCTL_ENABLE, 0);
> > > > >
> > > > > sa.sa_handler = sighandler;
> > > > > @@ -540,6 +547,9 @@ static void test_eustall(int drm_fd, uint32_t devid, bool blocking_read)
> > > > > _exit(run_gpgpu_fill(drm_fd, devid));
> > > > > }
> > > > > }
> > > > > + total_size = 0;
> > > > > + num_samples = 0;
> > > > > + num_drops = 0;
> > > > > /* Parent process reads the EU stall counters data */
> > > > > do {
> > > > > if (!blocking_read) {
> > > > > @@ -574,14 +584,16 @@ static void test_eustall(int drm_fd, uint32_t devid, bool blocking_read)
> > > > > igt_info("Number of samples: %u\n", num_samples);
> > > > > igt_info("Number of drops reported: %u\n", num_drops);
> > > > >
> > > > > + ret = wait_child(&work_load);
> > > > > + igt_assert_f(ret == 0, "waitpid() - ret: %d, errno: %d\n", ret, errno);
> > > > > + igt_assert_f(num_samples, "No EU stalls detected during the workload\n");
> > > > > +
> > > > > do_ioctl(stream_fd, DRM_XE_OBSERVATION_IOCTL_DISABLE, 0);
> > > > > + if (--iter)
> > > > > + goto enable;
> > > > >
> > > > > close(stream_fd);
> > > > > free(buf);
> > > > > -
> > > > > - ret = wait_child(&work_load);
> > > > > - igt_assert_f(ret == 0, "waitpid() - ret: %d, errno: %d\n", ret, errno);
> > > > > - igt_assert_f(num_samples, "No EU stalls detected during the workload\n");
> > > > > }
> > > > >
> > > > > static int opt_handler(int opt, int opt_index, void *data)
> > > > > @@ -634,6 +646,7 @@ igt_main_args("e:g:o:r:u:w:", long_options, help_str, opt_handler, NULL)
> > > > > {
> > > > > int drm_fd;
> > > > > uint32_t devid;
> > > > > + struct stat sb;
> > > > > bool blocking_read = true;
> > > > >
> > > > > igt_fixture {
> > > > > @@ -642,6 +655,7 @@ igt_main_args("e:g:o:r:u:w:", long_options, help_str, opt_handler, NULL)
> > > > > devid = intel_get_drm_devid(drm_fd);
> > > > > igt_require(IS_PONTEVECCHIO(devid) || intel_graphics_ver(devid) >= IP_VER(20, 0));
> > > > > igt_require_f(igt_get_gpgpu_fillfunc(devid), "no gpgpu-fill function\n");
> > > > > + igt_require(!stat(OBSERVATION_PARANOID, &sb));
>
> This looks like an unrelated fix for a separate patch?
> It is also better to use
> igt_require_f(condition, "description\n");
Will change this.
>
> > > > > if (output_file) {
> > > > > output = fopen(output_file, "w");
> > > > > igt_require(output);
> > > > > @@ -650,12 +664,22 @@ igt_main_args("e:g:o:r:u:w:", long_options, help_str, opt_handler, NULL)
> > > > >
> > > > > igt_describe("Verify non-blocking read of EU stall data during a workload run");
> > > > > igt_subtest("non-blocking-read") {
> > > > > - test_eustall(drm_fd, devid, !blocking_read);
> > > > > + test_eustall(drm_fd, devid, !blocking_read, 1);
> > > > > + }
> > > > > +
> > > > > + igt_describe("Run non-blocking read test twice with disable and enable between the runs");
> > > > > + igt_subtest("non-blocking-read-twice") {
> > > > > + test_eustall(drm_fd, devid, !blocking_read, 2);
>
> Why not just call it twice here?
> test_eustall(drm_fd, devid, !blocking_read);
> test_eustall(drm_fd, devid, !blocking_read);
With two calls to test_eustall(), it would open and close a new EU stall stream for each call.
The intention behind this test is to use the same EU stall stream but
disable and enable again between the two reads.
>
> > > > > }
> > > > >
> > > > > igt_describe("Verify blocking read of EU stall data during a workload run");
> > > > > igt_subtest("blocking-read") {
> > > > > - test_eustall(drm_fd, devid, blocking_read);
> > > > > + test_eustall(drm_fd, devid, blocking_read, 1);
> > > > > + }
> > > > > +
> > > > > + igt_describe("Run blocking read test twice with disable and enable between the runs");
> > > > > + igt_subtest("blocking-read-twice") {
> > > > > + test_eustall(drm_fd, devid, blocking_read, 2);
>
> Same here.
>
> Regards,
> Kamil
Thank You
Harish.
>
> > > > > }
> > > > >
> > > > > igt_describe("Verify that unprivileged open of a EU stall data fd fails");
> > > > > --
> > > > > 2.48.1
> > > > >
More information about the igt-dev
mailing list