Re: [PATCH i-g-t] tests/intel/xe_gt_freq: re-introduce exec tests of xe_gt_freq

[Riana Tauro <riana.tauro@intel.com>]

Hi Suja

On 2/26/2024 4:38 PM, Sundaresan, Sujaritha wrote:
> 
> On 2/22/2024 11:15 AM, Riana Tauro wrote:
>> The tests freq_range_exec, freq_fixed_exec were removed due
>> to a design flaw which caused failure in tests. The workload execution
>> finished earlier before the frequency validation when gt was set to
>> run at a higher frequency.
>>
>> These tests are being reintroduced with a spinner based approach
>> in this patch to avoid such failures.
>>
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>> ---
>>   tests/intel/xe_gt_freq.c | 106 ++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 105 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/intel/xe_gt_freq.c b/tests/intel/xe_gt_freq.c
>> index bf63e4298..f0de341cb 100644
>> --- a/tests/intel/xe_gt_freq.c
>> +++ b/tests/intel/xe_gt_freq.c
>> @@ -17,6 +17,7 @@
>>   #include "xe_drm.h"
>>   #include "xe/xe_ioctl.h"
>> +#include "xe/xe_spin.h"
>>   #include "xe/xe_query.h"
>>   #include "xe/xe_util.h"
>> @@ -153,6 +154,9 @@ static void test_freq_basic_api(int fd, int gt_id)
>>   /**
>>    * SUBTEST: freq_fixed_idle
>>    * Description: Test fixed frequency request with exec_queue in idle 
>> state
>> + *
>> + * SUBTEST: freq_fixed_exec
>> + * Description: Test fixed frequency request when spinner is run
>>    */
>>   static void test_freq_fixed(int fd, int gt_id, bool gt_idle)
>> @@ -217,8 +221,10 @@ static void test_freq_fixed(int fd, int gt_id, 
>> bool gt_idle)
>>   /**
>>    * SUBTEST: freq_range_idle
>>    * Description: Test range frequency request with exec_queue in idle 
>> state
>> + *
>> + * SUBTEST: freq_range_exec
>> + * Description: Test range frequency request when spinner is run
>>    */
>> -
>>   static void test_freq_range(int fd, int gt_id, bool gt_idle)
>>   {
>>       uint32_t rpn = get_freq(fd, gt_id, "rpn");
>> @@ -319,10 +325,88 @@ static void test_reset(int fd, int gt_id, int 
>> cycles)
>>       }
>>   }
>> +static void test_spin(int fd, struct drm_xe_engine_class_instance 
>> *eci, bool fixed)
>> +{
>> +    struct drm_xe_sync sync[2] = {
>> +        { .type = DRM_XE_SYNC_TYPE_SYNCOBJ, .flags = 
>> DRM_XE_SYNC_FLAG_SIGNAL, },
>> +        { .type = DRM_XE_SYNC_TYPE_SYNCOBJ, .flags = 
>> DRM_XE_SYNC_FLAG_SIGNAL, },
>> +    };
>> +    struct drm_xe_exec exec = {
>> +        .num_batch_buffer = 1,
>> +        .num_syncs = 2,
>> +        .syncs = to_user_pointer(sync),
>> +    };
>> +    uint64_t addr = 0x1a0000;
>> +    struct xe_spin_opts spin_opts = {
>> +        .addr = addr,
>> +        .preempt = false
>> +    };
>> +    struct xe_spin *spin;
>> +    uint32_t exec_queue;
>> +    uint32_t syncobj;
>> +    size_t bo_size;
>> +    uint32_t bo;
>> +    uint32_t vm;
>> +
>> +    vm = xe_vm_create(fd, 0, 0);
>> +    bo_size = sizeof(*spin);
>> +    bo_size = xe_bb_size(fd, bo_size);
>> +
>> +    bo = xe_bo_create(fd, vm, bo_size,
>> +              vram_if_possible(fd, eci->gt_id), 0);
>> +    spin = xe_bo_map(fd, bo, bo_size);
>> +
>> +    exec_queue = xe_exec_queue_create(fd, vm, eci, 0);
>> +    syncobj = syncobj_create(fd, 0);
>> +
>> +    sync[0].handle = syncobj_create(fd, 0);
>> +    xe_vm_bind_async(fd, vm, 0, bo, 0, addr, bo_size, sync, 1);
>> +
>> +    xe_spin_init(spin, &spin_opts);
>> +
>> +    sync[0].flags &= ~DRM_XE_SYNC_FLAG_SIGNAL;
>> +    sync[1].flags |= DRM_XE_SYNC_FLAG_SIGNAL;
>> +    sync[1].handle = syncobj;
>> +
>> +    exec.exec_queue_id = exec_queue;
>> +    exec.address = addr;
>> +    xe_exec(fd, &exec);
>> +
>> +    xe_spin_wait_started(spin);
>> +    usleep(50000);
>> +    igt_assert(!syncobj_wait(fd, &syncobj, 1, 1, 0, NULL));
>> +
>> +    igt_info("Running on GT %d Engine %s:%d\n", eci->gt_id,
>> +         xe_engine_class_string(eci->engine_class), 
>> eci->engine_instance);
>> +
>> +    if (fixed)
>> +        test_freq_fixed(fd, eci->gt_id, false);
>> +    else
>> +        test_freq_range(fd, eci->gt_id, false);
>> +
>> +    xe_spin_end(spin);
>> +
>> +    igt_assert(syncobj_wait(fd, &syncobj, 1, INT64_MAX, 0, NULL));
>> +    igt_assert(syncobj_wait(fd, &sync[0].handle, 1, INT64_MAX, 0, 
>> NULL));
>> +
>> +    sync[0].flags |= DRM_XE_SYNC_FLAG_SIGNAL;
>> +    xe_vm_unbind_async(fd, vm, 0, 0, addr, bo_size, sync, 1);
>> +    igt_assert(syncobj_wait(fd, &sync[0].handle, 1, INT64_MAX, 0, 
>> NULL));
>> +
>> +    syncobj_destroy(fd, sync[0].handle);
>> +    syncobj_destroy(fd, syncobj);
>> +    xe_exec_queue_destroy(fd, exec_queue);
>> +
>> +    munmap(spin, bo_size);
>> +    gem_close(fd, bo);
>> +    xe_vm_destroy(fd, vm);
>> +}
>> +
>>   igt_main
>>   {
>>       int fd;
>>       int gt;
>> +    struct drm_xe_engine_class_instance *hwe;
>>       uint32_t stash_min;
>>       uint32_t stash_max;
>> @@ -352,6 +436,16 @@ igt_main
>>           }
>>       }
>> +    igt_subtest("freq_fixed_exec") {
>> +        xe_for_each_gt(fd, gt) {
>> +            xe_for_each_engine(fd, hwe) {
>> +                if (hwe->gt_id != gt)
>> +                    continue;
>> +                test_spin(fd, hwe, true);
>> +            }
>> +        }
>> +    }
>> +
>>       igt_subtest("freq_range_idle") {
>>           xe_for_each_gt(fd, gt) {
>>               igt_require_f(igt_wait(xe_is_gt_in_c6(fd, gt), 1000, 10),
>> @@ -360,6 +454,16 @@ igt_main
>>           }
>>       }
>> +    igt_subtest("freq_range_exec") {
>> +        xe_for_each_gt(fd, gt) {
>> +            xe_for_each_engine(fd, hwe) {
>> +                if (hwe->gt_id != gt)
>> +                    continue;
>> +                test_spin(fd, hwe, false);
>> +            }
>> +        }
>> +    }
>> +
>>       igt_subtest("freq_low_max") {
>>           xe_for_each_gt(fd, gt) {
>>               test_freq_low_max(fd, gt);
> 
> Functionally lgtm. This test had to be revised due to existing design 
> flaws.
> 
> I'm not sure but there were some naming suggestions for the subtests 
> during the initial discussions.

spinner uses exec too. So the names should be okay.
If there are any other suggestions, i will change it.
> 
> Do we have the tests passing in pre-merge ?

Will add it to fast-feedback list and resend to check results.

Thanks,
Riana Tauro
> 
> Thanks,
> 
> Suja
>