On 17/09/2019 16:04, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-09-17 15:51:45)
On 17/09/2019 08:43, 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@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_request.c | 30 +++++++++++++++++++++++------
1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index f12358150097..452ad7a8ff0c 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -767,16 +767,34 @@ 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;
+
+ 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))
We are getting more and more these nested ones and it will become
unmanageable to remember which ones, why and on what paths. Perhaps we
need a comment next to the member in the structure definition?
The timeline mutex is held for request construction and retire; entry
and exit of the timeline->requests. Since we have a request, it should
be holding its rq->timeline->mutex; is that what you wish documented?
Similarly that signal->timeline != rq->timeline.
GEM_BUG_ON(signal->timeline == rq->timeline);
lockdep_assert_held(&rq->timeline->mutex);
Yeah that helps.
+ return -EINTR;
+
+ if (list_is_first(&signal->link, &tl->requests)) {
+ fence = NULL;
+ } else {
+ signal = list_prev_entry(signal, link);
+ fence = dma_fence_get_rcu(&signal->fence);
+ }
+ mutex_unlock(&tl->mutex);
+ if (!fence)
Can it be no fence when it was obtained under the mutex?
Yes. The previous fence may have been retired and so removed from the
tl->requests before we acquire the mutex. It should not be freed while
it is still in the list, i.e. dma_fence_get_rcu() should never return
NULL.
I did not worry about the list_is_first check, just about
dma_fence_get_rcu. At least I'm happy that one is safe and v2 is even
better in this respect.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
