[Intel-gfx] [PATCH 1/2] drm/i915/guc: Advance over port[0] if set and not preempting

[Mika Kuoppala]

Chris Wilson <chris at chris-wilson.co.uk> writes:

> Quoting Chris Wilson (2017-11-24 13:37:44)
>> Our execlist emulation is intended to only use a maximum of 2 ports per
>> engine, so as to not overflow the wq. (By knowing the limits, we can
>> avoid having to handle the wq exhaustion.) However, upon adding
>> preemption, we lost the skip over the first port if set for the
>> non-preemption path. Restore it.
>> 
>> Reported-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
>> Fixes: c41937fd994a ("drm/i915/guc: Preemption! With GuC")
>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Michał Winiarski <michal.winiarski at intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_guc_submission.c | 29 ++++++++++++++++-------------
>>  1 file changed, 16 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
>> index cbf5a96f5806..70e64bdb73dd 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
>> @@ -743,23 +743,26 @@ static void guc_dequeue(struct intel_engine_cs *engine)
>>         if (!rb)
>>                 goto unlock;
>>  
>> -       if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) && port_isset(port)) {
>> -               struct guc_preempt_work *preempt_work =
>> -                       &engine->i915->guc.preempt_work[engine->id];
>> -
>> -               if (rb_entry(rb, struct i915_priolist, node)->priority >
>> -                   max(port_request(port)->priotree.priority, 0)) {
>> -                       execlists_set_active(execlists,
>> -                                            EXECLISTS_ACTIVE_PREEMPT);
>> -                       queue_work(engine->i915->guc.preempt_wq,
>> -                                  &preempt_work->work);
>> -                       goto unlock;
>> -               } else if (port_isset(last_port)) {
>> -                       goto unlock;
>> +       if (port_isset(port)) {
>> +               if (HAS_LOGICAL_RING_PREEMPTION(engine->i915)) {
>> +                       struct guc_preempt_work *preempt_work =
>> +                               &engine->i915->guc.preempt_work[engine->id];
>> +
>> +                       if (rb_entry(rb, struct i915_priolist, node)->priority >
>> +                           max(port_request(port)->priotree.priority, 0)) {
>> +                               execlists_set_active(execlists,
>> +                                                    EXECLISTS_ACTIVE_PREEMPT);
>> +                               queue_work(engine->i915->guc.preempt_wq,
>> +                                          &preempt_work->work);
>> +                               goto unlock;
>> +                       }
>>                 }
>>  
>>                 port++;
>> +               if (port_isset(port))
>
> You probably want to stick with last_port, or at least Mika will want to
> make it last_port again after he expands ELSP[] and propagates all the
> changes.

This looks cleaner this way. We can always bring it back if it truely
helps readability. There has been too much last_port == port[1]
assumptions to tackle with.

-Mika