Re: [PATCH v2] drm/i915/breadcrumbs: Drop request reference for the signaler thread

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx