Message ID | 20170222114610.5819-9-chris@chris-wilson.co.uk (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 22/02/2017 11:46, Chris Wilson wrote: > The plan in the near-future is to allow requests to be removed from the > signaler. We can no longer then rely on holding a reference to the > request for the duration it is in the signaling tree, and instead must > obtain a reference to the request for the current operation using RCU. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_breadcrumbs.c | 24 ++++++++++++++++-------- > drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +- > 2 files changed, 17 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c > index d5bf4c0b2deb..62e6b8181200 100644 > --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c > +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c > @@ -496,7 +496,11 @@ static int intel_breadcrumbs_signaler(void *arg) > * need to wait for a new interrupt from the GPU or for > * a new client. > */ > - request = READ_ONCE(b->first_signal); > + rcu_read_lock(); > + request = rcu_dereference(b->first_signal); > + if (request) > + request = i915_gem_request_get_rcu(request); > + rcu_read_unlock(); > if (signal_complete(request)) { > local_bh_disable(); > dma_fence_signal(&request->fence); > @@ -515,24 +519,28 @@ static int intel_breadcrumbs_signaler(void *arg) > * the oldest before picking the next one. > */ > spin_lock_irq(&b->lock); > - if (request == b->first_signal) { > + if (request == rcu_access_pointer(b->first_signal)) { > struct rb_node *rb = > rb_next(&request->signaling.node); > - b->first_signal = rb ? to_signaler(rb) : NULL; > + rcu_assign_pointer(b->first_signal, > + rb ? to_signaler(rb) : NULL); > } > rb_erase(&request->signaling.node, &b->signals); > spin_unlock_irq(&b->lock); > > i915_gem_request_put(request); > } else { > - if (kthread_should_stop()) > + if (kthread_should_stop()) { > + GEM_BUG_ON(request); > break; > + } > > schedule(); > > if (kthread_should_park()) > kthread_parkme(); > } > + i915_gem_request_put(request); > } while (1); > __set_current_state(TASK_RUNNING); > > @@ -597,7 +605,7 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request) > rb_link_node(&request->signaling.node, parent, p); > rb_insert_color(&request->signaling.node, &b->signals); > if (first) > - smp_store_mb(b->first_signal, request); > + rcu_assign_pointer(b->first_signal, request); > > spin_unlock(&b->lock); > > @@ -670,7 +678,7 @@ void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine) > /* The engines should be idle and all requests accounted for! */ > WARN_ON(READ_ONCE(b->first_wait)); > WARN_ON(!RB_EMPTY_ROOT(&b->waiters)); > - WARN_ON(READ_ONCE(b->first_signal)); > + WARN_ON(rcu_access_pointer(b->first_signal)); > WARN_ON(!RB_EMPTY_ROOT(&b->signals)); > > if (!IS_ERR_OR_NULL(b->signaler)) > @@ -686,8 +694,8 @@ bool intel_breadcrumbs_busy(struct intel_engine_cs *engine) > > spin_lock_irq(&b->lock); > > - if (b->first_wait) { > - wake_up_process(b->first_wait->tsk); > + if (rcu_access_pointer(b->first_signal)) { > + wake_up_process(b->signaler); > busy |= intel_engine_flag(engine); > } > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index b91c2c7ef5a6..3fcb9dd19b07 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -242,7 +242,7 @@ struct intel_engine_cs { > struct rb_root signals; /* sorted by retirement */ > struct intel_wait *first_wait; /* oldest waiter by retirement */ > struct task_struct *signaler; /* used for fence signalling */ > - struct drm_i915_gem_request *first_signal; > + struct drm_i915_gem_request __rcu *first_signal; > struct timer_list fake_irq; /* used after a missed interrupt */ > struct timer_list hangcheck; /* detect missed interrupts */ > > Looks OK. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c index d5bf4c0b2deb..62e6b8181200 100644 --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c @@ -496,7 +496,11 @@ static int intel_breadcrumbs_signaler(void *arg) * need to wait for a new interrupt from the GPU or for * a new client. */ - request = READ_ONCE(b->first_signal); + rcu_read_lock(); + request = rcu_dereference(b->first_signal); + if (request) + request = i915_gem_request_get_rcu(request); + rcu_read_unlock(); if (signal_complete(request)) { local_bh_disable(); dma_fence_signal(&request->fence); @@ -515,24 +519,28 @@ static int intel_breadcrumbs_signaler(void *arg) * the oldest before picking the next one. */ spin_lock_irq(&b->lock); - if (request == b->first_signal) { + if (request == rcu_access_pointer(b->first_signal)) { struct rb_node *rb = rb_next(&request->signaling.node); - b->first_signal = rb ? to_signaler(rb) : NULL; + rcu_assign_pointer(b->first_signal, + rb ? to_signaler(rb) : NULL); } rb_erase(&request->signaling.node, &b->signals); spin_unlock_irq(&b->lock); i915_gem_request_put(request); } else { - if (kthread_should_stop()) + if (kthread_should_stop()) { + GEM_BUG_ON(request); break; + } schedule(); if (kthread_should_park()) kthread_parkme(); } + i915_gem_request_put(request); } while (1); __set_current_state(TASK_RUNNING); @@ -597,7 +605,7 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request) rb_link_node(&request->signaling.node, parent, p); rb_insert_color(&request->signaling.node, &b->signals); if (first) - smp_store_mb(b->first_signal, request); + rcu_assign_pointer(b->first_signal, request); spin_unlock(&b->lock); @@ -670,7 +678,7 @@ void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine) /* The engines should be idle and all requests accounted for! */ WARN_ON(READ_ONCE(b->first_wait)); WARN_ON(!RB_EMPTY_ROOT(&b->waiters)); - WARN_ON(READ_ONCE(b->first_signal)); + WARN_ON(rcu_access_pointer(b->first_signal)); WARN_ON(!RB_EMPTY_ROOT(&b->signals)); if (!IS_ERR_OR_NULL(b->signaler)) @@ -686,8 +694,8 @@ bool intel_breadcrumbs_busy(struct intel_engine_cs *engine) spin_lock_irq(&b->lock); - if (b->first_wait) { - wake_up_process(b->first_wait->tsk); + if (rcu_access_pointer(b->first_signal)) { + wake_up_process(b->signaler); busy |= intel_engine_flag(engine); } diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index b91c2c7ef5a6..3fcb9dd19b07 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -242,7 +242,7 @@ struct intel_engine_cs { struct rb_root signals; /* sorted by retirement */ struct intel_wait *first_wait; /* oldest waiter by retirement */ struct task_struct *signaler; /* used for fence signalling */ - struct drm_i915_gem_request *first_signal; + struct drm_i915_gem_request __rcu *first_signal; struct timer_list fake_irq; /* used after a missed interrupt */ struct timer_list hangcheck; /* detect missed interrupts */
The plan in the near-future is to allow requests to be removed from the signaler. We can no longer then rely on holding a reference to the request for the duration it is in the signaling tree, and instead must obtain a reference to the request for the current operation using RCU. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_breadcrumbs.c | 24 ++++++++++++++++-------- drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +- 2 files changed, 17 insertions(+), 9 deletions(-)