Re: [igt-dev] [PATCH i-g-t 5/9] tests/perf_pmu: Add trailing edge idle test variants

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 02/02/2018 20:45, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-02-02 18:37:50)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Additional set of tests which stops the batch and sleeps for a bit
>> before sampling the counter in order to test that the busyness stop
>> being recorded correctly.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   tests/perf_pmu.c | 81 +++++++++++++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 66 insertions(+), 15 deletions(-)
>>
>> diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
>> index 3faa04a4f68a..27f32b8a1602 100644
>> --- a/tests/perf_pmu.c
>> +++ b/tests/perf_pmu.c
>> @@ -131,6 +131,21 @@ static unsigned int e2ring(int gem_fd, const struct intel_execution_engine2 *e)
>>   }
>>   
>>   #define TEST_BUSY (1)
>> +#define FLAG_SYNC (2)
>> +#define TEST_TRAILING_IDLE (4)
>> +
>> +static void end_spin(int fd, igt_spin_t *spin, unsigned int flags)
>> +{
>> +       if (!spin)
>> +               return;
>> +
>> +       igt_spin_batch_end(spin);
>> +
>> +       if (flags & FLAG_SYNC)
>> +               gem_sync(fd, spin->handle);
>> +       else
>> +               usleep(batch_duration_ns / 5000);
> 
> Split this to
> 
> 	if (flags & FLAG_SYNC)
> 		gem_sync();
> 	
> 	if (flags & TEST_TRAILING_IDLE)
> 		usleep();
> 
> That keeps it descriptive.

I wanted to only do one per invocation. For tests that have flags = 
TEST_TRAILING_IDLE | FLAG_SYNC, but I think I can also change those to 
not or the flags on the second invocation but only pass in FLAG_SYNC. I 
think it works for now.

>> +}
>>   
>>   static void
>>   single(int gem_fd, const struct intel_execution_engine2 *e, unsigned int flags)
>> @@ -149,9 +164,11 @@ single(int gem_fd, const struct intel_execution_engine2 *e, unsigned int flags)
>>   
>>          val = pmu_read_single(fd);
>>          slept = measured_usleep(batch_duration_ns / 1000);
>> +       if (spin && (flags & TEST_TRAILING_IDLE))
>> +               end_spin(gem_fd, spin, flags);
> 
> Can simplify and miss the first if (spin) here as end_spin does it for
> you. And later again in no_sema()

True.

> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks, I'll sent the updated version.

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev