Re: [PATCH i-g-t] tests/xe_compute: Don't require ccs_mode on every GT

[Matt Roper <matthew.d.roper@intel.com>]

On Wed, Mar 27, 2024 at 11:14:27AM -0300, Gustavo Sousa wrote:
> Quoting Matt Roper (2024-03-22 19:19:08-03:00)
> >On platforms with standalone media, we expect some GTs to have CCS
> >engines (meaning ccs_mode can be tested if there's more than one CCS),
> >but the media GT will never have any CCS engines and will never expose
> >ccs_mode.  Move the igt_require() out of the loop so that we don't
> >declare the whole test a skip upon encountering the media GT when the
> >test already executed successfully on the primary GT.
> >
> >Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> >Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >---
> > tests/intel/xe_compute.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> >diff --git a/tests/intel/xe_compute.c b/tests/intel/xe_compute.c
> >index 42f42ca0c..fffcfa08e 100644
> >--- a/tests/intel/xe_compute.c
> >+++ b/tests/intel/xe_compute.c
> >@@ -51,13 +51,15 @@ test_ccs_mode(int num_gt)
> 
> Looks like we also need to fix test_compute_kernel_with_ccs_mode()?
> 
> > {
> >         struct drm_xe_engine_class_instance *hwe;
> >         u32 gt, m, ccs_mode, vm, q, num_slices;
> >-        int fd, gt_fd;
> >+        int fd, gt_fd, num_gt_with_ccs_mode = 0;
> > 
> >         for (gt = 0; gt < num_gt; gt++) {
> >-                igt_require(get_num_cslices(gt, &num_slices));
> >+                if (get_num_cslices(gt, &num_slices) == 0)
> 
> This next comment is not really related to this specific patch, but I
> wonder if `igt_require(get_num_cslices(gt, &num_slices))` should have
> been `igt_require(get_num_cslices(gt, &num_slices) > 0)`, since ccs_mode
> is only exposed in that case.

get_num_cslices() actually returns a boolean success/fail for whether it
was able to read a value at all rather than the number of cslices it
discovered (the number of cslices is instead returned via the num_slices
parameter that gets passed by reference).  So I think the check should
be correct (fail means we couldn't read the sysfs, usually because it
didn't exist on this GT).  In v2 I'll clarify by checking for
!get_num_cslices() rather than "get_num_cslices() == 0" since that's
more appropriate for testing a bool value anyway.

The two sysfs nodes this test uses (num_cslices and ccs_mode) only get
created if the GT has two or more cslices.  Arguably we could improve
the test a bit by adding some assertions that we never read a value of 0
or 1 from the num_cslices sysfs (since the sysfs shouldn't have existed
in that case), but that's getting a bit away from the goal of this
specific patch.


Matt

> 
> If so, then maybe add a follow up patch replacing `get_num_cslices(gt,
> &num_slices) == 0` with `get_num_cslices(gt, &num_slices) <= 1` in the
> condition above?
> 
> --
> Gustavo Sousa
> 
> >+                        continue;
> > 
> >                 gt_fd = gt_sysfs_open(gt);
> >                 igt_assert(igt_sysfs_printf(gt_fd, "ccs_mode", "%u", 0) < 0);
> >+                num_gt_with_ccs_mode++;
> >                 for (m = 1; m <= num_slices; m++) {
> >                         /* compute slices are to be equally distributed among enabled engines */
> >                         if (num_slices % m) {
> >@@ -105,6 +107,8 @@ test_ccs_mode(int num_gt)
> > 
> >                 close(gt_fd);
> >         }
> >+
> >+        igt_require(num_gt_with_ccs_mode > 0);
> > }
> > 
> > /**
> >-- 
> >2.43.0
> >

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation