Quoting Tvrtko Ursulin (2018-02-05 13:23:54)
>
> On 03/02/2018 10:19, Chris Wilson wrote:
> > @@ -656,41 +705,21 @@ static int intel_breadcrumbs_signaler(void *arg)
> > * a new client.
> > */
> > rcu_read_lock();
> > - request = rcu_dereference(b->first_signal);
> > - if (request)
> > - request = i915_gem_request_get_rcu(request);
> > + request = get_first_signal_rcu(b);
> > rcu_read_unlock();
> > if (signal_complete(request)) {
> > - local_bh_disable();
> > - dma_fence_signal(&request->fence);
> > - local_bh_enable(); /* kick start the tasklets */
> > -
> > - spin_lock_irq(&b->rb_lock);
> > -
> > - /* Wake up all other completed waiters and select the
> > - * next bottom-half for the next user interrupt.
> > - */
> > - __intel_engine_remove_wait(engine,
> > - &request->signaling.wait);
> > -
> > - /* Find the next oldest signal. Note that as we have
> > - * not been holding the lock, another client may
> > - * have installed an even older signal than the one
> > - * we just completed - so double check we are still
> > - * the oldest before picking the next one.
> > - */
> > - if (request == rcu_access_pointer(b->first_signal)) {
> > - struct rb_node *rb =
> > - rb_next(&request->signaling.node);
> > - rcu_assign_pointer(b->first_signal,
> > - rb ? to_signaler(rb) : NULL);
> > + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> > + &request->fence.flags)) {
> > + local_bh_disable();
> > + dma_fence_signal(&request->fence);
> > + local_bh_enable(); /* kick start the tasklets */
> > }
> > - rb_erase(&request->signaling.node, &b->signals);
> > - RB_CLEAR_NODE(&request->signaling.node);
> > -
> > - spin_unlock_irq(&b->rb_lock);
> >
> > - i915_gem_request_put(request);
> > + if (READ_ONCE(request->signaling.wait.seqno)) {
> > + spin_lock_irq(&b->rb_lock);
> > + __intel_engine_remove_signal(engine, request);
> > + spin_unlock_irq(&b->rb_lock);
> > + }
>
> How would you view taking the request->lock over this block in the
> signaler and then being able to call simply
> intel_engine_cancel_signaling - ending up with very similar code as in
> i915_gem_request_retire?
I am not keen on the conflation here (maybe it's just a hatred of bool
parameters). But at first glance it's just the commonality of
remove_signal, which is already a common routine?
> Only difference would be the tasklet kicking, but maybe still it would
> be possible to consolidate the two in one helper.
>
> __i915_gem_request_retire_signaling(req, bool kick_tasklets)
> {
> if (!DMA_FENCE_FLAG_SIGNALED_BIT) {
> dma_fence_signal_locked(...);
> if (kick_tasklets) {
> local_bh_disable();
> local_bh_enable();
> }
We can't kick the tasklet inside a spinlock. Especially not a lockclass
as nested as request.lock :(
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
