Re: [igt-dev] [PATCH i-g-t 05/24] i915/gem_exec_schedule: Verify that using HW semaphores doesn't block

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

On 22/03/2019 09:21, Chris Wilson wrote:
> We may use HW semaphores to schedule nearly-ready work such that they
> are already spinning on the GPU waiting for the completion on another
> engine. However, we don't want for that spinning task to actually block
> any real work should it be scheduled.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   tests/i915/gem_exec_schedule.c | 87 ++++++++++++++++++++++++++++++++++
>   1 file changed, 87 insertions(+)
> 
> diff --git a/tests/i915/gem_exec_schedule.c b/tests/i915/gem_exec_schedule.c
> index 4f0577b4e..ae850c4a3 100644
> --- a/tests/i915/gem_exec_schedule.c
> +++ b/tests/i915/gem_exec_schedule.c
> @@ -48,6 +48,10 @@
>   
>   #define MAX_CONTEXTS 1024
>   
> +#define LOCAL_I915_EXEC_BSD_SHIFT      (13)
> +#define LOCAL_I915_EXEC_BSD_MASK       (3 << LOCAL_I915_EXEC_BSD_SHIFT)
> +#define ENGINE_MASK  (I915_EXEC_RING_MASK | LOCAL_I915_EXEC_BSD_MASK)
> +
>   IGT_TEST_DESCRIPTION("Check that we can control the order of execution");
>   
>   static inline
> @@ -320,6 +324,86 @@ static void smoketest(int fd, unsigned ring, unsigned timeout)
>   	}
>   }
>   
> +static uint32_t __batch_create(int i915, uint32_t offset)
> +{
> +	const uint32_t bbe = MI_BATCH_BUFFER_END;
> +	uint32_t handle;
> +
> +	handle = gem_create(i915, ALIGN(offset + 4, 4096));
> +	gem_write(i915, handle, offset, &bbe, sizeof(bbe));
> +
> +	return handle;
> +}
> +
> +static uint32_t batch_create(int i915)
> +{
> +	return __batch_create(i915, 0);
> +}
> +
> +static void semaphore_userlock(int i915)
> +{
> +	struct drm_i915_gem_exec_object2 obj = {
> +		.handle = batch_create(i915),
> +	};
> +	igt_spin_t *spin = NULL;
> +	unsigned int engine;
> +	uint32_t scratch;
> +
> +	igt_require(gem_scheduler_has_preemption(i915));
> +
> +	/*
> +	 * Given the use of semaphores to govern parallel submission
> +	 * of nearly-ready work to HW, we still want to run actually
> +	 * ready work immediately. Without semaphores, the dependent
> +	 * work wouldn't be submitted so our ready work will run.
> +	 */
> +
> +	scratch = gem_create(i915, 4096);
> +	for_each_physical_engine(i915, engine) {
> +		if (!spin) {
> +			spin = igt_spin_batch_new(i915,
> +						  .dependency = scratch,
> +						  .engine = engine);
> +		} else {
> +			typeof(spin->execbuf.flags) saved = spin->execbuf.flags;

u64 reads better and struct eb won't change anyway.

> +
> +			spin->execbuf.flags &= ~ENGINE_MASK;
> +			spin->execbuf.flags |= engine;
> +
> +			gem_execbuf(i915, &spin->execbuf);

Do you need to wait for spinner to be running before submitting these 
ones, to make sure the logic emits a semaphore poll for them and submits 
them straight away?

> +
> +			spin->execbuf.flags = saved;
> +		}
> +	}
> +	igt_require(spin);
> +	gem_close(i915, scratch);
> +
> +	/*
> +	 * On all dependent engines, the request may be executing (busywaiting
> +	 * on a HW semaphore) but it should not prevent any real work from
> +	 * taking precedence.
> +	 */
> +	scratch = gem_context_create(i915);
> +	for_each_physical_engine(i915, engine) {
> +		struct drm_i915_gem_execbuffer2 execbuf = {
> +			.buffers_ptr = to_user_pointer(&obj),
> +			.buffer_count = 1,
> +			.flags = engine,
> +			.rsvd1 = scratch,
> +		};
> +
> +		if (engine == (spin->execbuf.flags & ENGINE_MASK))
> +			continue;

Ugh saving and restoring eb flags to find the spinning engine here I 
feel will be a land mine for the upcoming for_each_physical_engine 
conversion but what can we do.

> +
> +		gem_execbuf(i915, &execbuf);
> +	}
> +	gem_context_destroy(i915, scratch);
> +	gem_sync(i915, obj.handle); /* to hang unless we can preempt */

I got lost - how does this work if the spinner is still keeping the 
obj.handle busy?

> +	gem_close(i915, obj.handle);
> +
> +	igt_spin_batch_free(i915, spin);
> +}
> +
>   static void reorder(int fd, unsigned ring, unsigned flags)
>   #define EQUAL 1
>   {
> @@ -1307,6 +1391,9 @@ igt_main
>   			igt_require(gem_scheduler_has_ctx_priority(fd));
>   		}
>   
> +		igt_subtest("semaphore-user")
> +			semaphore_userlock(fd);
> +
>   		igt_subtest("smoketest-all")
>   			smoketest(fd, ALL_ENGINES, 30);
>   
> 

Regards,

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