[PATCH 2/2] drm/amd/sched: add schuduling policy

[Andres Rodriguez]

On 2017-03-16 05:15 AM, zhoucm1 wrote:
>
>
> On 2017年03月16日 17:10, Christian König wrote:
>> Am 16.03.2017 um 10:00 schrieb Chunming Zhou:
>>> if high priority rq is full, then process with low priority could be
>>> starve.
>>> Add policy for this problem, the high proiority can ahead of next
>>> priority queue,
>>> the ratio is 2 : 1.
>>>
>>> Change-Id: I58f4a6b9cdce8689b18dd8e83dd6e2cf5f99d5fb
>>> Signed-off-by: Chunming Zhou <David1.Zhou at amd.com>
>>
>> Well, the idea behind the high priority queues is to actually starve
>> the low priority queues to a certain amount.
>>
>> At least for the kernel queue that is really desired.
> Yes, agree, but we certainly don't want  low priority queue is totally
> dead, which doesn't have chance to run if high priority queue is always
> full and busy.
> If without Andres changes, it doesn't matter. But after Andres changes
> upstream, we need scheduling policy.
>
> Regards,
> David Zhou

Hey David,

The desired behavior is actually starvation. This is why allocating a 
high priority context is locked behind root privileges at the moment.

High priority work includes many different features intended for the 
safety of the user wearing the headset. These include:
     - stopping the user from tripping over furniture
     - preventing motion sickness

We cannot compromise these safety features in order to run non-critical 
tasks.

The current approach also has the disadvantage of heavily interleaving 
work. This is going to have two undesired side effects. First, there 
will be a performance overhead from having the queue context switch so 
often.

Second, this approach improves concurrency of work in a ring with mixed 
priority work, but actually decreases overall system concurrency. When a 
ring's HW queue has any high priority work committed, the whole ring 
will be executing at high priority. So interleaving the work will result 
in extending the time a ring spends in high priority mode. This 
effectively extends time that a ring might spend with CU's reserved 
which will have a performance impact on other rings.

The best approach here is to make sure we don't map high priority work 
and regular priority work to the same ring. This is why the LRU policy 
for userspace ring ids to kernel ring ids was introduced. This policy 
provides the guarantee as a side effect. But further work there could be 
useful to actually check context priorities when doing the ring mapping.

By placing work on different rings, we let the hardware actually 
dispatch work according to wave parameters like VGPR usage, etc. Trying 
to deduce all this information in the GPU scheduler will get very 
complicated.

This is NAK'd by me.

Regards,
Andres


>>
>> Christian.
>>
>>> ---
>>>   drivers/gpu/drm/amd/scheduler/gpu_scheduler.c | 26
>>> +++++++++++++++++++++++---
>>>   drivers/gpu/drm/amd/scheduler/gpu_scheduler.h |  2 ++
>>>   2 files changed, 25 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>>> b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>>> index 0f439dd..4637b6f 100644
>>> --- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>>> +++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>>> @@ -35,11 +35,16 @@
>>>   static void amd_sched_process_job(struct fence *f, struct fence_cb
>>> *cb);
>>>     /* Initialize a given run queue struct */
>>> -static void amd_sched_rq_init(struct amd_sched_rq *rq)
>>> +static void amd_sched_rq_init(struct amd_gpu_scheduler *sched, enum
>>> +                  amd_sched_priority pri)
>>>   {
>>> +    struct amd_sched_rq *rq = &sched->sched_rq[pri];
>>> +
>>>       spin_lock_init(&rq->lock);
>>>       INIT_LIST_HEAD(&rq->entities);
>>>       rq->current_entity = NULL;
>>> +    rq->wait_base = pri * 2;
>>> +    rq->wait = rq->wait_base;
>>>   }
>>>     static void amd_sched_rq_add_entity(struct amd_sched_rq *rq,
>>> @@ -494,17 +499,32 @@ static void amd_sched_wakeup(struct
>>> amd_gpu_scheduler *sched)
>>>   {
>>>       struct amd_sched_entity *entity;
>>>       int i;
>>> +    bool skip;
>>>         if (!amd_sched_ready(sched))
>>>           return NULL;
>>>   +retry:
>>> +    skip = false;
>>>       /* Kernel run queue has higher priority than normal run queue*/
>>>       for (i = AMD_SCHED_PRIORITY_MAX - 1; i >=
>>> AMD_SCHED_PRIORITY_MIN; i--) {
>>> +        if ((i > AMD_SCHED_PRIORITY_MIN) &&
>>> +            (sched->sched_rq[i - 1].wait >=
>>> sched->sched_rq[i].wait_base)) {
>>> +            sched->sched_rq[i - 1].wait = sched->sched_rq[i -
>>> 1].wait_base;
>>> +            skip = true;
>>> +            continue;
>>> +        }
>>>           entity = amd_sched_rq_select_entity(&sched->sched_rq[i]);
>>> -        if (entity)
>>> +        if (entity) {
>>> +            if (i > AMD_SCHED_PRIORITY_MIN)
>>> +                sched->sched_rq[i - 1].wait++;
>>>               break;
>>> +        }
>>>       }
>>>   +    if (!entity && skip)
>>> +        goto retry;
>>> +
>>>       return entity;
>>>   }
>>>   @@ -608,7 +628,7 @@ int amd_sched_init(struct amd_gpu_scheduler
>>> *sched,
>>>       sched->name = name;
>>>       sched->timeout = timeout;
>>>       for (i = AMD_SCHED_PRIORITY_MIN; i < AMD_SCHED_PRIORITY_MAX; i++)
>>> -        amd_sched_rq_init(&sched->sched_rq[i]);
>>> +        amd_sched_rq_init(sched, i);
>>>         init_waitqueue_head(&sched->wake_up_worker);
>>>       init_waitqueue_head(&sched->job_scheduled);
>>> diff --git a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>>> b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>>> index 99f0240..4caed30 100644
>>> --- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>>> +++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>>> @@ -64,6 +64,8 @@ struct amd_sched_rq {
>>>       spinlock_t        lock;
>>>       struct list_head    entities;
>>>       struct amd_sched_entity    *current_entity;
>>> +    int wait_base;
>>> +    int wait;
>>>   };
>>>     struct amd_sched_fence {
>>
>>
>