[Intel-gfx] [PATCH v7] drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd)

[Tvrtko Ursulin]

On 28/06/2018 14:28, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-06-28 14:21:06)
>>
>> On 28/06/2018 14:11, Chris Wilson wrote:
>>> +/*
>>> + * Check the unread Context Status Buffers and manage the submission of new
>>> + * contexts to the ELSP accordingly.
>>> + */
>>> +static void execlists_submission_tasklet(unsigned long data)
>>> +{
>>> +     struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
>>> +     unsigned long flags;
>>> +
>>> +     GEM_TRACE("%s awake?=%d, active=%x\n",
>>> +               engine->name,
>>> +               engine->i915->gt.awake,
>>> +               engine->execlists.active);
>>> +
>>> +     spin_lock_irqsave(&engine->timeline.lock, flags);
>>> +
>>> +     if (engine->i915->gt.awake) /* we may be delayed until after we idle! */
>>
>> No races between the check and tasklet call? In other words the code
>> path which you were describing that can race is taking the timeline lock?
> 
> intel_engine_is_idle() is unserialised.

Okay, think I understand. Ship it!

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko

>>> +             __execlists_submission_tasklet(engine);
>>> +
>>> +     spin_unlock_irqrestore(&engine->timeline.lock, flags);
>>> +}
>>> +
>>>    static void queue_request(struct intel_engine_cs *engine,
>>>                          struct i915_sched_node *node,
>>>                          int prio)
>>> @@ -1144,16 +1155,30 @@ static void queue_request(struct intel_engine_cs *engine,
>>>                      &lookup_priolist(engine, prio)->requests);
>>>    }
>>>    
>>> -static void __submit_queue(struct intel_engine_cs *engine, int prio)
>>> +static void __update_queue(struct intel_engine_cs *engine, int prio)
>>>    {
>>>        engine->execlists.queue_priority = prio;
>>> -     tasklet_hi_schedule(&engine->execlists.tasklet);
>>> +}
>>> +
>>> +static void __submit_queue_imm(struct intel_engine_cs *engine)
>>> +{
>>> +     struct intel_engine_execlists * const execlists = &engine->execlists;
>>> +
>>> +     if (reset_in_progress(execlists))
>>> +             return; /* defer until we restart the engine following reset */
>>
>> We have a tasklet kick somewhere in that path?
> 
> In execlists_reset_finish()
> 
>>> +     if (execlists->tasklet.func == execlists_submission_tasklet)
>>> +             __execlists_submission_tasklet(engine);
>>> +     else
>>> +             tasklet_hi_schedule(&execlists->tasklet);
>>>    }
>>>    
>>>    static void submit_queue(struct intel_engine_cs *engine, int prio)
>>>    {
>>> -     if (prio > engine->execlists.queue_priority)
>>> -             __submit_queue(engine, prio);
>>> +     if (prio > engine->execlists.queue_priority) {
>>> +             __update_queue(engine, prio);
>>> +             __submit_queue_imm(engine);
>>> +     }
>>>    }
>>>    
>>>    static void execlists_submit_request(struct i915_request *request)
>>> @@ -1165,11 +1190,12 @@ static void execlists_submit_request(struct i915_request *request)
>>>        spin_lock_irqsave(&engine->timeline.lock, flags);
>>>    
>>>        queue_request(engine, &request->sched, rq_prio(request));
>>> -     submit_queue(engine, rq_prio(request));
>>>    
>>>        GEM_BUG_ON(!engine->execlists.first);
>>>        GEM_BUG_ON(list_empty(&request->sched.link));
>>>    
>>> +     submit_queue(engine, rq_prio(request));
>>> +
>>>        spin_unlock_irqrestore(&engine->timeline.lock, flags);
>>>    }
>>>    
>>> @@ -1296,8 +1322,11 @@ static void execlists_schedule(struct i915_request *request,
>>>                }
>>>    
>>>                if (prio > engine->execlists.queue_priority &&
>>> -                 i915_sw_fence_done(&sched_to_request(node)->submit))
>>> -                     __submit_queue(engine, prio);
>>> +                 i915_sw_fence_done(&sched_to_request(node)->submit)) {
>>> +                     /* defer submission until after all of our updates */
>>> +                     __update_queue(engine, prio);
>>> +                     tasklet_hi_schedule(&engine->execlists.tasklet);
>>
>> _imm suffix on __submit_queue_imm sounds unused if there is no plain
>> __submit_queue, which could have been used here. But meh.
> 
> I thought of trying to emphasis the immediate nature of this path. It's
> not a huge deal, but I didn't particularly like calling it
> direct_submit_queue() (or just direct_submit(), too many submits!)
> __ prefix to indicate that it's an inner function to submit_queue,
> _imm suffix to indicate what's special.
> -Chris
>