Re: [igt-dev] [PATCH i-g-t 6/9] tests/perf_pmu: PMU enable race test

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

On 02/02/2018 20:52, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-02-02 18:37:51)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Test that the PMU can be safely enabled in face of interrupt-heavy load on
>> an engine.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   tests/perf_pmu.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 52 insertions(+)
>>
>> diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
>> index 27f32b8a1602..de0a1c7d936c 100644
>> --- a/tests/perf_pmu.c
>> +++ b/tests/perf_pmu.c
>> @@ -1221,6 +1221,51 @@ test_rc6(int gem_fd)
>>          assert_within_epsilon(busy - prev, 0.0, tolerance);
>>   }
>>   
>> +static void
>> +test_enable_race(int gem_fd, const struct intel_execution_engine2 *e)
>> +{
>> +       uint64_t config = I915_PMU_ENGINE_BUSY(e->class, e->instance);
>> +       struct igt_helper_process engine_load = { };
>> +       const uint32_t bbend = MI_BATCH_BUFFER_END;
>> +       struct drm_i915_gem_exec_object2 obj = { };
>> +       struct drm_i915_gem_execbuffer2 eb = { };
>> +       int fd;
>> +
>> +       igt_require(intel_gen(intel_get_drm_devid(gem_fd)) >= 8);
> 
> 	igt_require(gem_has_execlists(gem_fd));
> 
> may be clearer, it's still a proxy for has_stats. :|

Better, yes.

> 
>> +       igt_require(gem_has_engine(gem_fd, e->class, e->instance));
>> +
>> +       obj.handle = gem_create(gem_fd, 4096);
>> +       gem_write(gem_fd, obj.handle, 0, &bbend, sizeof(bbend));
>> +
>> +       eb.buffer_count = 1;
>> +       eb.buffers_ptr = to_user_pointer(&obj);
>> +       eb.flags = e2ring(gem_fd, e);
>> +
>> +       /*
>> +        * Defeat the busy stats delayed disable, we need to guarantee we are
>> +        * the first user.
>> +        */
> 
> gem_quiescent_gpu(); as well?

Can do.

>> +       sleep(2);
>> +
>> +       /* Apply interrupt-heavy load on the engine. */
> 
> s/load/nop load/ ?
> 
>> +       igt_fork_helper(&engine_load) {
>> +               for (;;)
>> +                       gem_execbuf(gem_fd, &eb);
>> +       }
>> +
>> +       /* Wait a bit to allow engine load to start. */
>> +       usleep(500e3);
> 
> .5s, I guess we aren't too concerned with paring it down to the
> minimum here as we've already spent 2s for idling.
> 
>> +
>> +       /* Enable the PMU. */
>> +       fd = open_pmu(config);
> 
> fd = -1;
> igt_until_timeout(1) {
> 	close(fd);
> 	fd = open_pmu(config);
> }
> 
> It's a race, so don't expect it to happen first time?

Definitely don't, but the delayed disable currently makes it hard to 
test (slow).

> 
>> +       /* Cleanup. */
>> +       igt_stop_helper(&engine_load);
>> +       close(fd);
> 
> Another race, right? Wrap it all in a 10x loop? Or separate out the
> close race to another lop.

Don't think so, disable is done from the delayed worker already.

> 
>> +       gem_close(gem_fd, obj.handle);
>> +       gem_quiescent_gpu(gem_fd);
>> +}
> 
> The gist of the test looks fine, so please consider exercising the race
> more than once and with that have a
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

I can wrap it in a 10x loop but until the PMU is fixed that means 10x 
longer runtime as well which I wanted to avoid, and instead count on 
execution in numbers across the shards.

Regards,

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