Re: [Intel-gfx] [PATCH 04/17] drm/i915: Tweak scheduler's kick_submission()

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

On 10/03/2020 11:00, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-03-10 10:07:33)
>>
>> On 06/03/2020 13:38, Chris Wilson wrote:
>>> Skip useless priority bumping on adding a new dependency, but otherwise
>>> prevent tasklet scheduling until we have completed all the potential
>>> rescheduling.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>>    drivers/gpu/drm/i915/i915_scheduler.c | 7 ++++++-
>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
>>> index 52f71e83e088..603cba36d6a4 100644
>>> --- a/drivers/gpu/drm/i915/i915_scheduler.c
>>> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
>>> @@ -209,6 +209,8 @@ static void kick_submission(struct intel_engine_cs *engine,
>>>        if (!inflight)
>>>                goto unlock;
>>>    
>>> +     engine->execlists.queue_priority_hint = prio;
>>> +
>>
>> What is the significance of moving this up? I couldn't correlate it to
>> the commit message.
> 
> It's correcting the priority bumping. If we have the same context as
> active, we should not be skipping the hint update and so avoid the useless
> bump on a later dependency.
> 
>>>        /*
>>>         * If we are already the currently executing context, don't
>>>         * bother evaluating if we should preempt ourselves.
>>> @@ -216,7 +218,6 @@ static void kick_submission(struct intel_engine_cs *engine,
>>>        if (inflight->context == rq->context)
>>>                goto unlock;
>>>    
>>> -     engine->execlists.queue_priority_hint = prio;
>>>        if (need_preempt(prio, rq_prio(inflight)))
>>>                tasklet_hi_schedule(&engine->execlists.tasklet);
>>>    
>>> @@ -463,11 +464,15 @@ int i915_sched_node_add_dependency(struct i915_sched_node *node,
>>>        if (!dep)
>>>                return -ENOMEM;
>>>    
>>> +     local_bh_disable();
>>> +
>>>        if (!__i915_sched_node_add_dependency(node, signal, dep,
>>>                                              I915_DEPENDENCY_EXTERNAL |
>>>                                              I915_DEPENDENCY_ALLOC))
>>>                i915_dependency_free(dep);
>>>    
>>> +     local_bh_enable(); /* kick submission tasklet */
>>> +
>>
>> And this presumably postpones the tasklet until __bump_priority ->
>> __i915_schedule is finished. But then why the request submission path
>> which calls __i915_sched_node_add_dependency directly does not need this
>> treatment?
> 
> Because we haven't completed the updates by that point, and upon actual
> request submission the tasklet is flushed. Plus on not all request
> submission paths would it be legal.

Okay,

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