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 {
>>
>>
>
