[v2] drm/i915: Lock signaler timeline while navigating
diff mbox series

Message ID 20190917150912.11215-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [v2] drm/i915: Lock signaler timeline while navigating
Related show

Commit Message

Chris Wilson Sept. 17, 2019, 3:09 p.m. UTC
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.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
Add the lockdep assert and GEM_BUG_ON to note the overlapping timelines.
---
 drivers/gpu/drm/i915/i915_request.c | 31 +++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

Comments

Tvrtko Ursulin Sept. 17, 2019, 3:20 p.m. UTC | #1
On 17/09/2019 16:09, 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.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
> Add the lockdep assert and GEM_BUG_ON to note the overlapping timelines.
> ---
>   drivers/gpu/drm/i915/i915_request.c | 31 +++++++++++++++++++++++------
>   1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index f12358150097..3966b330b5de 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -767,16 +767,35 @@ 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))
> +	struct intel_timeline *tl = signal->timeline;
> +	struct dma_fence *fence;
> +	int err;
> +
> +	lockdep_assert_held(&rq->timeline->mutex);
> +	GEM_BUG_ON(rq->timeline == signal->timeline);
> +
> +	if (list_is_first(&signal->link, &tl->requests))
>   		return 0;
>   
> -	signal = list_prev_entry(signal, link);
> -	if (intel_timeline_sync_is_later(rq->timeline, &signal->fence))
> +	if (mutex_lock_interruptible_nested(&tl->mutex, SINGLE_DEPTH_NESTING))
> +		return -EINTR;
> +
> +	fence = NULL;
> +	if (!list_is_first(&signal->link, &tl->requests))
> +		fence = dma_fence_get(&list_prev_entry(signal, link)->fence);
> +
> +	mutex_unlock(&tl->mutex);
> +	if (!fence)
>   		return 0;
>   
> -	return i915_sw_fence_await_dma_fence(&rq->submit,
> -					     &signal->fence, 0,
> -					     I915_FENCE_GFP);
> +	err = 0;
> +	if (!intel_timeline_sync_is_later(rq->timeline, 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
> 

Yes the asserts make it obvious now why the nested annotation is needed.

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

Regards,

Tvrtko

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index f12358150097..3966b330b5de 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -767,16 +767,35 @@  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))
+	struct intel_timeline *tl = signal->timeline;
+	struct dma_fence *fence;
+	int err;
+
+	lockdep_assert_held(&rq->timeline->mutex);
+	GEM_BUG_ON(rq->timeline == signal->timeline);
+
+	if (list_is_first(&signal->link, &tl->requests))
 		return 0;
 
-	signal = list_prev_entry(signal, link);
-	if (intel_timeline_sync_is_later(rq->timeline, &signal->fence))
+	if (mutex_lock_interruptible_nested(&tl->mutex, SINGLE_DEPTH_NESTING))
+		return -EINTR;
+
+	fence = NULL;
+	if (!list_is_first(&signal->link, &tl->requests))
+		fence = dma_fence_get(&list_prev_entry(signal, link)->fence);
+
+	mutex_unlock(&tl->mutex);
+	if (!fence)
 		return 0;
 
-	return i915_sw_fence_await_dma_fence(&rq->submit,
-					     &signal->fence, 0,
-					     I915_FENCE_GFP);
+	err = 0;
+	if (!intel_timeline_sync_is_later(rq->timeline, 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