| Message ID | 20200511075722.13483-4-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/20] drm/i915/gt: Mark up the racy read of execlists->context_tag | expand |
Chris Wilson <chris@chris-wilson.co.uk> writes: > The initial-breadcrumb is used to mark the end of the awaiting and the > beginning of the user payload. We verify that we do not start the user > payload before all signaler are completed, checking our semaphore setup > by looking for the initial breadcrumb being written too early. We also > want to ensure that we do not add semaphore waits after we have already > closed the semaphore section, an issue for later deferred waits. > I see that you go from timeline property to a per request property. But the last sentence in this commit message escapes me. Do we need this for proxy fence waiting of is this patch just for enhancing the assertion/gem_bug_on resolution on semaphore boundaries? -Mika > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/gt/intel_lrc.c | 5 ++++- > drivers/gpu/drm/i915/i915_request.c | 7 ++++++- > drivers/gpu/drm/i915/i915_request.h | 27 ++++++++++++++++++++------- > 3 files changed, 30 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > index c5591248dafb..f93f13d20b5a 100644 > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > @@ -1894,7 +1894,7 @@ static void defer_request(struct i915_request *rq, struct list_head * const pl) > continue; > > /* No waiter should start before its signaler */ > - GEM_BUG_ON(w->context->timeline->has_initial_breadcrumb && > + GEM_BUG_ON(i915_request_has_initial_breadcrumb(w) && > i915_request_started(w) && > !i915_request_completed(rq)); > > @@ -3501,6 +3501,7 @@ static int gen8_emit_init_breadcrumb(struct i915_request *rq) > { > u32 *cs; > > + GEM_BUG_ON(i915_request_has_initial_breadcrumb(rq)); > if (!i915_request_timeline(rq)->has_initial_breadcrumb) > return 0; > > @@ -3527,6 +3528,8 @@ static int gen8_emit_init_breadcrumb(struct i915_request *rq) > /* Record the updated position of the request's payload */ > rq->infix = intel_ring_offset(rq, cs); > > + __set_bit(I915_FENCE_FLAG_INITIAL_BREADCRUMB, &rq->fence.flags); > + > return 0; > } > > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c > index 2d5b98549ddc..00b7c4eb3f32 100644 > --- a/drivers/gpu/drm/i915/i915_request.c > +++ b/drivers/gpu/drm/i915/i915_request.c > @@ -951,6 +951,7 @@ __emit_semaphore_wait(struct i915_request *to, > u32 *cs; > > GEM_BUG_ON(INTEL_GEN(to->i915) < 8); > + GEM_BUG_ON(i915_request_has_initial_breadcrumb(to)); > > /* We need to pin the signaler's HWSP until we are finished reading. */ > err = intel_timeline_read_hwsp(from, to, &hwsp_offset); > @@ -1000,6 +1001,9 @@ emit_semaphore_wait(struct i915_request *to, > if (!intel_context_use_semaphores(to->context)) > goto await_fence; > > + if (i915_request_has_initial_breadcrumb(to)) > + goto await_fence; > + > if (!rcu_access_pointer(from->hwsp_cacheline)) > goto await_fence; > > @@ -1256,7 +1260,8 @@ __i915_request_await_execution(struct i915_request *to, > * immediate execution, and so we must wait until it reaches the > * active slot. > */ > - if (intel_engine_has_semaphores(to->engine)) { > + if (intel_engine_has_semaphores(to->engine) && > + !i915_request_has_initial_breadcrumb(to)) { > err = __emit_semaphore_wait(to, from, from->fence.seqno - 1); > if (err < 0) > return err; > diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h > index d8ce908e1346..98ae2dc82371 100644 > --- a/drivers/gpu/drm/i915/i915_request.h > +++ b/drivers/gpu/drm/i915/i915_request.h > @@ -84,19 +84,26 @@ enum { > I915_FENCE_FLAG_PQUEUE, > > /* > - * I915_FENCE_FLAG_SIGNAL - this request is currently on signal_list > + * I915_FENCE_FLAG_HOLD - this request is currently on hold > * > - * Internal bookkeeping used by the breadcrumb code to track when > - * a request is on the various signal_list. > + * This request has been suspended, pending an ongoing investigation. > */ > - I915_FENCE_FLAG_SIGNAL, > + I915_FENCE_FLAG_HOLD, > > /* > - * I915_FENCE_FLAG_HOLD - this request is currently on hold > + * I915_FENCE_FLAG_INITIAL_BREADCRUMB - this request has the initial > + * breadcrumb that marks the end of semaphore waits and start of the > + * user payload. > + */ > + I915_FENCE_FLAG_INITIAL_BREADCRUMB, > + > + /* > + * I915_FENCE_FLAG_SIGNAL - this request is currently on signal_list > * > - * This request has been suspended, pending an ongoing investigation. > + * Internal bookkeeping used by the breadcrumb code to track when > + * a request is on the various signal_list. > */ > - I915_FENCE_FLAG_HOLD, > + I915_FENCE_FLAG_SIGNAL, > > /* > * I915_FENCE_FLAG_NOPREEMPT - this request should not be preempted > @@ -390,6 +397,12 @@ static inline bool i915_request_in_priority_queue(const struct i915_request *rq) > return test_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags); > } > > +static inline bool > +i915_request_has_initial_breadcrumb(const struct i915_request *rq) > +{ > + return test_bit(I915_FENCE_FLAG_INITIAL_BREADCRUMB, &rq->fence.flags); > +} > + > /** > * Returns true if seq1 is later than seq2. > */ > -- > 2.20.1
Quoting Mika Kuoppala (2020-05-11 12:21:55) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > The initial-breadcrumb is used to mark the end of the awaiting and the > > beginning of the user payload. We verify that we do not start the user > > payload before all signaler are completed, checking our semaphore setup > > by looking for the initial breadcrumb being written too early. We also > > want to ensure that we do not add semaphore waits after we have already > > closed the semaphore section, an issue for later deferred waits. > > > > I see that you go from timeline property to a per request property. > But the last sentence in this commit message escapes me. Do we need > this for proxy fence waiting of is this patch just for enhancing > the assertion/gem_bug_on resolution on semaphore boundaries? The immediate improvement is for tidying up the assertion. It opens the way for out-of-order awaits. -Chris
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index c5591248dafb..f93f13d20b5a 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -1894,7 +1894,7 @@ static void defer_request(struct i915_request *rq, struct list_head * const pl) continue; /* No waiter should start before its signaler */ - GEM_BUG_ON(w->context->timeline->has_initial_breadcrumb && + GEM_BUG_ON(i915_request_has_initial_breadcrumb(w) && i915_request_started(w) && !i915_request_completed(rq)); @@ -3501,6 +3501,7 @@ static int gen8_emit_init_breadcrumb(struct i915_request *rq) { u32 *cs; + GEM_BUG_ON(i915_request_has_initial_breadcrumb(rq)); if (!i915_request_timeline(rq)->has_initial_breadcrumb) return 0; @@ -3527,6 +3528,8 @@ static int gen8_emit_init_breadcrumb(struct i915_request *rq) /* Record the updated position of the request's payload */ rq->infix = intel_ring_offset(rq, cs); + __set_bit(I915_FENCE_FLAG_INITIAL_BREADCRUMB, &rq->fence.flags); + return 0; } diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index 2d5b98549ddc..00b7c4eb3f32 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -951,6 +951,7 @@ __emit_semaphore_wait(struct i915_request *to, u32 *cs; GEM_BUG_ON(INTEL_GEN(to->i915) < 8); + GEM_BUG_ON(i915_request_has_initial_breadcrumb(to)); /* We need to pin the signaler's HWSP until we are finished reading. */ err = intel_timeline_read_hwsp(from, to, &hwsp_offset); @@ -1000,6 +1001,9 @@ emit_semaphore_wait(struct i915_request *to, if (!intel_context_use_semaphores(to->context)) goto await_fence; + if (i915_request_has_initial_breadcrumb(to)) + goto await_fence; + if (!rcu_access_pointer(from->hwsp_cacheline)) goto await_fence; @@ -1256,7 +1260,8 @@ __i915_request_await_execution(struct i915_request *to, * immediate execution, and so we must wait until it reaches the * active slot. */ - if (intel_engine_has_semaphores(to->engine)) { + if (intel_engine_has_semaphores(to->engine) && + !i915_request_has_initial_breadcrumb(to)) { err = __emit_semaphore_wait(to, from, from->fence.seqno - 1); if (err < 0) return err; diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h index d8ce908e1346..98ae2dc82371 100644 --- a/drivers/gpu/drm/i915/i915_request.h +++ b/drivers/gpu/drm/i915/i915_request.h @@ -84,19 +84,26 @@ enum { I915_FENCE_FLAG_PQUEUE, /* - * I915_FENCE_FLAG_SIGNAL - this request is currently on signal_list + * I915_FENCE_FLAG_HOLD - this request is currently on hold * - * Internal bookkeeping used by the breadcrumb code to track when - * a request is on the various signal_list. + * This request has been suspended, pending an ongoing investigation. */ - I915_FENCE_FLAG_SIGNAL, + I915_FENCE_FLAG_HOLD, /* - * I915_FENCE_FLAG_HOLD - this request is currently on hold + * I915_FENCE_FLAG_INITIAL_BREADCRUMB - this request has the initial + * breadcrumb that marks the end of semaphore waits and start of the + * user payload. + */ + I915_FENCE_FLAG_INITIAL_BREADCRUMB, + + /* + * I915_FENCE_FLAG_SIGNAL - this request is currently on signal_list * - * This request has been suspended, pending an ongoing investigation. + * Internal bookkeeping used by the breadcrumb code to track when + * a request is on the various signal_list. */ - I915_FENCE_FLAG_HOLD, + I915_FENCE_FLAG_SIGNAL, /* * I915_FENCE_FLAG_NOPREEMPT - this request should not be preempted @@ -390,6 +397,12 @@ static inline bool i915_request_in_priority_queue(const struct i915_request *rq) return test_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags); } +static inline bool +i915_request_has_initial_breadcrumb(const struct i915_request *rq) +{ + return test_bit(I915_FENCE_FLAG_INITIAL_BREADCRUMB, &rq->fence.flags); +} + /** * Returns true if seq1 is later than seq2. */
The initial-breadcrumb is used to mark the end of the awaiting and the beginning of the user payload. We verify that we do not start the user payload before all signaler are completed, checking our semaphore setup by looking for the initial breadcrumb being written too early. We also want to ensure that we do not add semaphore waits after we have already closed the semaphore section, an issue for later deferred waits. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/gt/intel_lrc.c | 5 ++++- drivers/gpu/drm/i915/i915_request.c | 7 ++++++- drivers/gpu/drm/i915/i915_request.h | 27 ++++++++++++++++++++------- 3 files changed, 30 insertions(+), 9 deletions(-)