Re: [PATCH i-g-t] tests/intel/xe_exec_fault_mode: Don't return early

[Nirmoy Das <nirmoy.das@linux.intel.com>]

On 8/27/2024 4:07 PM, Andrzej Hajda wrote:
>
>
> On 27.08.2024 12:52, Nirmoy Das wrote:
>> Tests that are causing pagefaults should wait for exec queue to be ban
>> otherwise pending engine resets because of on-going pagefaults would
>> cause failure in subsequent tests to fail.
>>
>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Tejas Upadhyay <tejas.upadhyay@intel.com>
>> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1630
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   tests/intel/xe_exec_fault_mode.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/tests/intel/xe_exec_fault_mode.c 
>> b/tests/intel/xe_exec_fault_mode.c
>> index 1f1f1e50b..339c79353 100644
>> --- a/tests/intel/xe_exec_fault_mode.c
>> +++ b/tests/intel/xe_exec_fault_mode.c
>> @@ -36,6 +36,22 @@
>>   #define INVALID_VA    (0x1 << 8)
>>   #define ENABLE_SCRATCH  (0x1 << 9)
>>   +static int get_ban_property(int xe, struct 
>> drm_xe_engine_class_instance *eci,
>> +                uint32_t vm, uint32_t exec_queue)
>> +{
>> +    struct drm_xe_exec_queue_get_property args = {
>> +        .value = -1,
>> +        .reserved[0] = 0,
>> +        .property = DRM_XE_EXEC_QUEUE_GET_PROPERTY_BAN,
>> +    };
>> +
>> +    args.exec_queue_id = exec_queue;
>> +
>> +    do_ioctl(xe, DRM_IOCTL_XE_EXEC_QUEUE_GET_PROPERTY, &args);
>> +
>> +    return args.value;
>> +}
>> +
>>   /**
>>    * SUBTEST: invalid-va
>>    * Description: Access invalid va and check for EIO through user 
>> fence.
>> @@ -324,6 +340,17 @@ test_exec(int fd, struct 
>> drm_xe_engine_class_instance *eci,
>>       xe_wait_ufence(fd, &data[0].vm_sync, USER_FENCE_VALUE,
>>                  bind_exec_queues[0], NSEC_PER_SEC);
>>   +    if ((flags & INVALID_FAULT)) {
>> +        igt_set_timeout(1, "");
> You can add "waiting for ban" or sth similar here for free.

+1


>> +        for (i = 0; i < n_execs; i++) {
>> +            int e = i % n_exec_queues;
>> +                while (!get_ban_property(fd, eci, vm,
>> +                             exec_queues[e]))
>> +                    sched_yield();
>> +        }
>> +        igt_reset_timeout();
>> +    }
>> +
>
> Why not iterate over exec queues instead of execs?

Do you mean why no iterate over n_exec_queues ? Wanted to keep same flow 
like the test is doing

while doing exec jobs. n_exec_queues should work too. I can change that 
if needed.


Regards,

Nirmoy

>
> Regards
> Andrzej
>
>>       if (!(flags & INVALID_FAULT) && !(flags & INVALID_VA)) {
>>           for (i = j; i < n_execs; i++)
>>                   igt_assert_eq(data[i].data, 0xc0ffee);
>