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

[Gustavo Sousa]

Quoting Matt Roper (2024-03-27 20:47:47-03:00)
>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 at intel.com>
>> >Signed-off-by: Matt Roper <matthew.d.roper at 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.

Yep. Thanks for clarifying! :-)

My bad on not reading the code carefully.

>
>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.

Alright. Makes sense.

--
Gustavo Sousa

>
>
>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