| Message ID | 20190917150912.11215-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915: Lock signaler timeline while navigating | expand |
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
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
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(-)