On 05/02/2018 13:36, Chris Wilson wrote:
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 :(
Yep bool is ugly. So maybe make the helper return status, so the caller
can kick if fence was signaled? Or you would worry about losing a little
bit of latency? That also is not ideal I agree.
Too bad, I would kind of like to consolidate if nothing to be
symmetrical wrt req->lock usage.
Otherwise patch looks safe to me. At least I failed to find any problems.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
