[Intel-gfx] [PATCH 2/2] drm/i915: Only emit one semaphore per request

[Tvrtko Ursulin]

On 01/04/2019 16:57, Chris Wilson wrote:
> Ideally we only need one semaphore per ring to accommodate waiting on
> multiple engines in parallel. However, since we do not know which fences
> we will finally be waiting on, we emit a semaphore for every fence. It
> turns out to be quite easy to trick ourselves into exhausting our
> ringbuffer causing an error, just by feeding in a batch that depends on
> several thousand contexts.
> 
> Since we never can be waiting on more than one semaphore in parallel
> (other than perhaps the desire to busywait on multiple engines), just
> pick the first fence for our semaphore. If we pick the wrong fence to
> busywait on, we just miss an opportunity to reduce latency.
> 
> An adaption might be to use sched.flags as either a semaphore counter,
> or to track the first busywait on each engine, converting it back to a
> single use bit prior to closing the request.
> 
> v2: Track first semaphore used per-engine (this caters for our basic
> igt that semaphores are working).
> 
> Testcase: igt/gem_exec_fence/long-history
> Reported-by: Mika Kuoppala <mika.kuoppala at intel.com>
> Fixes: e88619646971 ("drm/i915: Use HW semaphores for inter-engine synchronisation on gen8+")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_request.c         | 13 +++++++++++--
>   drivers/gpu/drm/i915/i915_scheduler.c       |  5 +++--
>   drivers/gpu/drm/i915/i915_scheduler_types.h |  3 ++-
>   3 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index e9c2094ab8ea..88a6d9968c15 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -783,6 +783,14 @@ emit_semaphore_wait(struct i915_request *to,
>   	GEM_BUG_ON(!from->timeline->has_initial_breadcrumb);
>   	GEM_BUG_ON(INTEL_GEN(to->i915) < 8);
>   
> +	BUILD_BUG_ON(sizeof(from->engine->mask) > sizeof(to->sched.semaphores));
> +
> +	/* Just emit the first semaphore we see as request space is limited. */
> +	if (to->sched.semaphores & from->engine->mask)
> +		return i915_sw_fence_await_dma_fence(&to->submit,
> +						     &from->fence, 0,
> +						     I915_FENCE_GFP);
> +
>   	/* We need to pin the signaler's HWSP until we are finished reading. */
>   	err = i915_timeline_read_hwsp(from, to, &hwsp_offset);
>   	if (err)
> @@ -814,7 +822,8 @@ emit_semaphore_wait(struct i915_request *to,
>   	*cs++ = 0;
>   
>   	intel_ring_advance(to, cs);
> -	to->sched.flags |= I915_SCHED_HAS_SEMAPHORE;
> +	to->sched.semaphores |= from->engine->mask;
> +	to->sched.flags |= I915_SCHED_HAS_SEMAPHORE_CHAIN;
>   	return 0;
>   }
>   
> @@ -1126,7 +1135,7 @@ void i915_request_add(struct i915_request *request)
>   		 * far in the distance past over useful work, we keep a history
>   		 * of any semaphore use along our dependency chain.
>   		 */
> -		if (!(request->sched.flags & I915_SCHED_HAS_SEMAPHORE))
> +		if (!(request->sched.flags & I915_SCHED_HAS_SEMAPHORE_CHAIN))
>   			attr.priority |= I915_PRIORITY_NOSEMAPHORE;
>   
>   		/*
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> index e0f609d01564..77dbf7d74e12 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> @@ -41,6 +41,7 @@ void i915_sched_node_init(struct i915_sched_node *node)
>   	INIT_LIST_HEAD(&node->waiters_list);
>   	INIT_LIST_HEAD(&node->link);
>   	node->attr.priority = I915_PRIORITY_INVALID;
> +	node->semaphores = 0;
>   	node->flags = 0;
>   }
>   
> @@ -73,9 +74,9 @@ bool __i915_sched_node_add_dependency(struct i915_sched_node *node,
>   		dep->flags = flags;
>   
>   		/* Keep track of whether anyone on this chain has a semaphore */
> -		if (signal->flags & I915_SCHED_HAS_SEMAPHORE &&
> +		if (signal->flags & I915_SCHED_HAS_SEMAPHORE_CHAIN &&
>   		    !node_started(signal))
> -			node->flags |= I915_SCHED_HAS_SEMAPHORE;
> +			node->flags |= I915_SCHED_HAS_SEMAPHORE_CHAIN;
>   
>   		ret = true;
>   	}
> diff --git a/drivers/gpu/drm/i915/i915_scheduler_types.h b/drivers/gpu/drm/i915/i915_scheduler_types.h
> index 5c94b3eb5c81..aa3aeae5404a 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler_types.h
> +++ b/drivers/gpu/drm/i915/i915_scheduler_types.h
> @@ -76,7 +76,8 @@ struct i915_sched_node {
>   	struct list_head link;
>   	struct i915_sched_attr attr;
>   	unsigned int flags;
> -#define I915_SCHED_HAS_SEMAPHORE	BIT(0)
> +#define I915_SCHED_HAS_SEMAPHORE_CHAIN	BIT(0)
> +	unsigned int semaphores; /* intel_engine_mask_t */

But can't use the type yet?

>   };
>   
>   struct i915_dependency {
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko