[Intel-gfx] [PATCH 3/4] drm/i915: Lock signaler timeline while navigating

[Tvrtko Ursulin]

On 18/09/2019 15:54, Chris Wilson wrote:
> As we need to take a walk back along the signaler timeline to find the
> fence before upon which we want to wait, we need to lock that timeline
> to prevent it being modified as we walk. Similarly, we also need to
> acquire a reference to the earlier fence while it still exists!
> 
> Though we lack the correct locking today, we are saved by the
> overarching struct_mutex -- but that protection is being removed.
> 
> v2: Tvrtko made me realise I was being lax and using annotations to
> ignore the AB-BA deadlock from the timeline overlap. As it would be
> possible to construct a second request that was using a semaphore from the
> same timeline as ourselves, we could quite easily end up in a situation
> where we deadlocked in our mutex waits. Avoid that by using a trylock
> and falling back to a normal dma-fence await if contended.
> 
> v3: Eek, the signal->timeline is volatile and must be carefully
> dereferenced to ensure it is valid.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com> #v2

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

Regards,

Tvrtko

> ---
>   drivers/gpu/drm/i915/i915_request.c | 68 +++++++++++++++++++----------
>   1 file changed, 46 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index fb6f21c41934..9bd8538b1907 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -768,17 +768,43 @@ i915_request_create(struct intel_context *ce)
>   static int
>   i915_request_await_start(struct i915_request *rq, struct i915_request *signal)
>   {
> -	if (list_is_first(&signal->link, &signal->timeline->requests))
> -		return 0;
> +	struct intel_timeline *tl;
> +	struct dma_fence *fence;
> +	int err;
> +
> +	GEM_BUG_ON(i915_request_timeline(rq) ==
> +		   rcu_access_pointer(signal->timeline));
>   
> -	signal = list_prev_entry(signal, link);
> -	if (intel_timeline_sync_is_later(i915_request_timeline(rq),
> -					 &signal->fence))
> +	rcu_read_lock();
> +	tl = rcu_dereference(signal->timeline);
> +	if (i915_request_started(signal) || !kref_get_unless_zero(&tl->kref))
> +		tl = NULL;
> +	rcu_read_unlock();
> +	if (!tl) /* already started or maybe even completed */
>   		return 0;
>   
> -	return i915_sw_fence_await_dma_fence(&rq->submit,
> -					     &signal->fence, 0,
> -					     I915_FENCE_GFP);
> +	fence = ERR_PTR(-EBUSY);
> +	if (mutex_trylock(&tl->mutex)) {
> +		fence = NULL;
> +		if (!i915_request_started(signal) &&
> +		    !list_is_first(&signal->link, &tl->requests)) {
> +			signal = list_prev_entry(signal, link);
> +			fence = dma_fence_get(&signal->fence);
> +		}
> +		mutex_unlock(&tl->mutex);
> +	}
> +	intel_timeline_put(tl);
> +	if (IS_ERR_OR_NULL(fence))
> +		return PTR_ERR_OR_ZERO(fence);
> +
> +	err = 0;
> +	if (intel_timeline_sync_is_later(i915_request_timeline(rq), fence))
> +		err = i915_sw_fence_await_dma_fence(&rq->submit,
> +						    fence, 0,
> +						    I915_FENCE_GFP);
> +	dma_fence_put(fence);
> +
> +	return err;
>   }
>   
>   static intel_engine_mask_t
> @@ -808,30 +834,23 @@ emit_semaphore_wait(struct i915_request *to,
>   	u32 hwsp_offset;
>   	int len;
>   	u32 *cs;
> -	int err;
>   
> -	GEM_BUG_ON(!from->timeline->has_initial_breadcrumb);
>   	GEM_BUG_ON(INTEL_GEN(to->i915) < 8);
>   
>   	/* Just emit the first semaphore we see as request space is limited. */
>   	if (already_busywaiting(to) & from->engine->mask)
> -		return i915_sw_fence_await_dma_fence(&to->submit,
> -						     &from->fence, 0,
> -						     I915_FENCE_GFP);
> +		goto await_fence;
>   
> -	err = i915_request_await_start(to, from);
> -	if (err < 0)
> -		return err;
> +	if (i915_request_await_start(to, from) < 0)
> +		goto await_fence;
>   
>   	/* Only submit our spinner after the signaler is running! */
> -	err = __i915_request_await_execution(to, from, NULL, gfp);
> -	if (err)
> -		return err;
> +	if (__i915_request_await_execution(to, from, NULL, gfp))
> +		goto await_fence;
>   
>   	/* We need to pin the signaler's HWSP until we are finished reading. */
> -	err = intel_timeline_read_hwsp(from, to, &hwsp_offset);
> -	if (err)
> -		return err;
> +	if (intel_timeline_read_hwsp(from, to, &hwsp_offset))
> +		goto await_fence;
>   
>   	len = 4;
>   	if (has_token)
> @@ -866,6 +885,11 @@ emit_semaphore_wait(struct i915_request *to,
>   	to->sched.semaphores |= from->engine->mask;
>   	to->sched.flags |= I915_SCHED_HAS_SEMAPHORE_CHAIN;
>   	return 0;
> +
> +await_fence:
> +	return i915_sw_fence_await_dma_fence(&to->submit,
> +					     &from->fence, 0,
> +					     I915_FENCE_GFP);
>   }
>   
>   static int
>