Am 17.03.2017 um 03:35 schrieb Zhou, David(ChunMing): > Hi Christian, Andres, > > I understand your mean, especially high priority queue from Andres is > for VR, which is liking an engine to himself, but our scheduler is a > generic thing, which is used by multiple case. > From scheduler perspective, scheduling policy is popular, e.g. CPU > scheduler, it provides different time piece for different priority > queue, high priority has long time to execute, low has short time, but > everyone has the chance to run, just sooner or later. > As Michel pointed out even the CPU scheduler allows for a process to dominate everyone else when you have the right privileges and is useful for the use case. > The proposal I send is same goal for gpu scheduler, we don't desire to > one engine is occupied by one person, high priority can have more > chance to run, low priority has less, but low priority must to have. > The ratio is power of 2 in current implementation, we can adjust it > for more different situations if necessary. Even we can put a > parameter to be adusted convenience. > For Andres case, he can pass a big time piece to it so that the > efficiency is same with occupied previous. We perfectly understand what your patch is doing, it's just not that we want this behavior. When there is high priority work available it should completely dominate everything else. Regards, Christian. > > Regards, > David Zhou > > > -----Original Message----- > From: Andres Rodriguez [mailto:andresx7 at gmail.com] > Sent: Friday, March 17, 2017 12:32 AM > To: Christian König <deathsimple at vodafone.de>; Zhou, David(ChunMing) > <David1.Zhou at amd.com>; Koenig, Christian <Christian.Koenig at amd.com>; > amd-gfx at lists.freedesktop.org > Subject: Re: [PATCH 2/2] drm/amd/sched: add schuduling policy > > > > On 2017-03-16 12:01 PM, Christian König wrote: > > Am 16.03.2017 um 16:31 schrieb Andres Rodriguez: > >> > >> > >> On 2017-03-16 11:13 AM, Andres Rodriguez wrote: > >>> > >>> > >>> 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. > > > > Thanks for the quick response, and yes that's what I was expecting as > > requirement for that feature as well. > > > >>> > >>> 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. > >> > >> Wanted to correct myself here. The LRU policy doesn't actually > >> provide a guarantee. It approximates it. > > > > What David is perfectly right with is that this has the potential for > > undesired side effects. > > > > But I completely agree that deadline or fair queue scheduling is > > tricky to implement even when it is desired. > > > > So letting a submission dominate all other is perfectly ok for me as > > long as it is limited to a certain set of processes somehow. > > > > Christian. > > I also wanted to comment that I think policies for the gpu_scheduler > are something interesting to explore. Not just at single ring level, > but also policies that tie all the gpu_schedulers for a single ASIC > together. Currently they all operate independently, but technically > they share the underlying HW so there is some resource aliasing. > > There has been a lot of experimentation on this area in the VR world > by other OS vendors. Enhancing the policies provided by the SPI > through SW can have some pretty great benefits. Although because of > restrictions these kind of changes are only activated for certain HMDs. > > I think we have a good opportunity here to make the linux version of > VR better for all HMDs, and not just the ones associated with the OS > vendor. > > Regards, > Andres > > > > >> > >> Regards, > >> Andres > >> > >>> 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 { > >>>>> > >>>>> > >>>> > >> _______________________________________________ > >> amd-gfx mailing list > >> amd-gfx at lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > > > -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20170317/9f435446/attachment-0001.html>
