Re: [PATCH i-g-t] tests/xe_eudebug_online: correct calculate the time diff

[Maciej Patelczyk <maciej.patelczyk@intel.com>]

On 13.01.2025 09:26, Hajda, Andrzej wrote:
>
> W dniu 10.01.2025 o 16:34, Maciej Patelczyk pisze:
>> When calculating time difference between two timespec values use
>> library function which does it right.
>>
>> Previous way wasn't working well on slower machines. It was only
>> taking the tv_nsec field from struct timespec.
>>
>> Fixes: bbf1730baf ("tests/xe_eudebug_online: Debug client which
>>                      runs workloads on EU")
>> CC: Dominik Grzegorzek <dominik.grzegorzek@intel.com>
>> Signed-off-by: Maciej Patelczyk <maciej.patelczyk@intel.com>
>> ---
>>   tests/intel/xe_eudebug_online.c | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/intel/xe_eudebug_online.c 
>> b/tests/intel/xe_eudebug_online.c
>> index 052774a3b..f532387af 100644
>> --- a/tests/intel/xe_eudebug_online.c
>> +++ b/tests/intel/xe_eudebug_online.c
>> @@ -2235,6 +2235,11 @@ static int find_suitable_engines(struct 
>> drm_xe_engine_class_instance **hwes,
>>       return engine_count;
>>   }
>>   +static uint64_t timespecs_diff_us(struct timespec *ts1, struct 
>> timespec *ts2)
>> +{
>> +    return (uint64_t)(fabs(igt_time_elapsed(ts1, ts2)) * USEC_PER_SEC);
>
>
> It seems igt_time_elapsed handles correctly case ts1 > ts2, so the 
> conversion looks correct.
>
> Something to bikeshed is the conversion from int type to double, then 
> int again, up to you.
>
> I think there is no need to explicit conversion to uint64_t, no big deal.
>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
>
> Regards
> Andrzej
>
>
Thanks Andrzej.

I wanted encapsulate all in that function so that no further changes are 
needed.

I could change the diff variable to double but since WORKLOAD_DELAY_US 
is long int what's the point?

Some conversion is unavoidable anyway.

Regards,

Maciej

>> +}
>> +
>>   /**
>>    * SUBTEST: breakpoint-many-sessions-single-tile
>>    * Description:
>> @@ -2255,7 +2260,7 @@ static void test_many_sessions_on_tiles(int fd, 
>> bool multi_tile)
>>       struct online_debug_data **data;
>>       struct drm_xe_engine_class_instance **hwe;
>>       struct drm_xe_eudebug_event_eu_attention *eus;
>> -    uint64_t current_t, next_t, diff;
>> +    uint64_t diff;
>>       int attempt_mask = 0, final_mask, should_break;
>>       int i;
>>   @@ -2318,10 +2323,8 @@ static void test_many_sessions_on_tiles(int 
>> fd, bool multi_tile)
>>       igt_assert_eq(attempt_mask, final_mask);
>>         for (i = 0; i < n - 1; i++) {
>> -        /* Convert timestamps to microseconds */
>> -        current_t = data[i]->exception_arrived.tv_nsec * 1000;
>> -        next_t = data[i + 1]->exception_arrived.tv_nsec * 1000;
>> -        diff = current_t < next_t ? next_t - current_t : current_t - 
>> next_t;
>> +        diff = timespecs_diff_us(&data[i]->exception_arrived,
>> +                     &data[i + 1]->exception_arrived);
>>             if (multi_tile)
>>               igt_assert_f(diff < WORKLOAD_DELAY_US,