Quoting Chris Wilson (2018-02-05 13:36:36)
> 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 :(
Also bear in mind this is just an intermediate step in my plans. :)
static int intel_breadcrumbs_signaler(void *arg)
{
struct intel_engine_cs *engine = arg;
struct intel_breadcrumbs *b = &engine->breadcrumbs;
struct drm_i915_gem_request *rq, *n;
/* Install ourselves with high priority to reduce signalling latency */
signaler_set_rtpriority();
do {
bool do_schedule = true;
LIST_HEAD(list);
u32 seqno;
set_current_state(TASK_INTERRUPTIBLE);
if (list_empty(&b->signals))
goto sleep;
/*
* We are either woken up by the interrupt bottom-half,
* or by a client adding a new signaller. In both cases,
* the GPU seqno may have advanced beyond our oldest signal.
* If it has, propagate the signal, remove the waiter and
* check again with the next oldest signal. Otherwise we
* need to wait for a new interrupt from the GPU or for
* a new client.
*/
seqno = intel_engine_get_seqno(engine);
spin_lock_irq(&b->rb_lock);
list_for_each_entry_safe(rq, n, &b->signals, signaling.link) {
u32 this = rq->signaling.wait.seqno;
GEM_BUG_ON(!rq->signaling.wait.seqno);
if (!i915_seqno_passed(seqno, this))
break;
if (this == i915_gem_request_global_seqno(rq)) {
__intel_engine_remove_wait(engine,
&rq->signaling.wait);
rq->signaling.wait.seqno = 0;
__list_del_entry(&rq->signaling.link);
if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
&rq->fence.flags)) {
list_add_tail(&rq->signaling.link,
&list);
i915_gem_request_get(rq);
}
}
}
spin_unlock_irq(&b->rb_lock);
if (!list_empty(&list)) {
local_bh_disable();
list_for_each_entry_safe(rq, n, &list, signaling.link) {
dma_fence_signal(&rq->fence);
i915_gem_request_put(rq);
}
local_bh_enable(); /* kick start the tasklets */
/*
* If the engine is saturated we may be continually
* processing completed requests. This angers the
* NMI watchdog if we never let anything else
* have access to the CPU. Let's pretend to be nice
* and relinquish the CPU if we burn through the
* entire RT timeslice!
*/
do_schedule = need_resched();
}
if (unlikely(do_schedule)) {
if (current->state & TASK_NORMAL &&
!list_empty(&b->signals) &&
engine->irq_seqno_barrier &&
test_and_clear_bit(ENGINE_IRQ_BREADCRUMB,
&engine->irq_posted)) {
engine->irq_seqno_barrier(engine);
intel_engine_wakeup(engine);
}
sleep:
if (kthread_should_park())
kthread_parkme();
if (unlikely(kthread_should_stop()))
break;
schedule();
}
} while (1);
__set_current_state(TASK_RUNNING);
return 0;
}
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
