Re: [PATCH drm-misc-next v2] drm/sched: implement dynamic job-flow control
Hi Danilo, On Tue, 24 Oct 2023 00:57:47 +0200 Danilo Krummrich wrote: > > > + > > > + /** > > > + * @update_job_credits: Called once the scheduler is considering this > > > + * job for execution. > > > + * > > > + * Drivers may use this to update the job's submission credits, which is > > > + * useful to e.g. deduct the number of native fences which have been > > > + * signaled meanwhile. > > > + * > > > + * The callback must either return the new number of submission credits > > > + * for the given job, or zero if no update is required. > > > > Any reason for having this special zero-means-no-update case? I mean, > > drivers could just return sched_job->submission_credits if nothing > > changed, and that would simplify the semantics IMHO. Another option, if > > I think I just did this because I thought it's a clever way to get rid of the > need to deal with zero-sized jobs, which do not make much sense. In > drm_sched_job_init() passing a zero job size defaults to one, which I think is > reasonable. Doing the same thing here is more likely to hide a bug. However, > the > same is probably true for 'zero means no update' though. Maybe we should just > WARN() in such a case. > > > we want to avoid the sched_job->submission_credits assignment when > > nothing changes would be to make it a void function and let it update > > the sched_job->submission_credits directly. > > Sure, that's an option as well. However, I'd probably prefer the new job size > to > be the return value. Having to sanity check job->submission_credits afterwards > isn't that nice either. Uh, sorry for the late reply, I see you've sent a v3 already :-/. I keep thinking it'd be simpler to make this a void function that updates s_job->submission_credits directly. I also don't see the problem with doing a sanity check on job->submission_credits. I mean, if the driver is doing something silly, you can't do much to prevent it anyway, except warn the user that something wrong has happened. If you want to WARN_ON(job->submission_credits == 0 || job->submission_credits > job_old_submission_credits); that's fine. But none of this sanity checking has to do with the function prototype/semantics, and I'm still not comfortable with this 0 => no-change. If there's no change, we should just leave job->submission_credits unchanged (or return job->submission_credits) instead of inventing a new special case. Regards, Boris
Re: [PATCH drm-misc-next v2] drm/sched: implement dynamic job-flow control
On 2023-10-23 18:35, Danilo Krummrich wrote:
> On Wed, Oct 11, 2023 at 09:52:36PM -0400, Luben Tuikov wrote:
>> Hi,
>>
>> Thanks for fixing the title and submitting a v2 of this patch. Comments
>> inlined below.
>>
>> On 2023-10-09 18:35, Danilo Krummrich wrote:
>>> Currently, job flow control is implemented simply by limiting the number
>>> of jobs in flight. Therefore, a scheduler is initialized with a
>>> submission limit that corresponds to the number of jobs which can be
>>> sent to the hardware.
>>>
>>> This implies that for each job, drivers need to account for the maximum
>>> job size possible in order to not overflow the ring buffer.
>>>
>>> However, there are drivers, such as Nouveau, where the job size has a
>>> rather large range. For such drivers it can easily happen that job
>>> submissions not even filling the ring by 1% can block subsequent
>>> submissions, which, in the worst case, can lead to the ring run dry.
>>>
>>> In order to overcome this issue, allow for tracking the actual job size
>>> instead of the number of jobs. Therefore, add a field to track a job's
>>> submission credits, which represents the number of credits a job
>>> contributes to the scheduler's submission limit.
>>>
>>> Signed-off-by: Danilo Krummrich
>>> ---
>>> Changes in V2:
>>> ==
>>> - fixed up influence on scheduling fairness due to consideration of a
>>> job's
>>> size
>>> - If we reach a ready entity in drm_sched_select_entity() but can't
>>> actually
>>> queue a job from it due to size limitations, just give up and go to
>>> sleep
>>> until woken up due to a pending job finishing, rather than continue
>>> to try
>>> other entities.
>>> - added a callback to dynamically update a job's credits (Boris)
>>> - renamed 'units' to 'credits'
>>> - fixed commit message and comments as requested by Luben
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +-
>>> drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +-
>>> drivers/gpu/drm/lima/lima_sched.c | 2 +-
>>> drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
>>> drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +-
>>> drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
>>> .../gpu/drm/scheduler/gpu_scheduler_trace.h | 2 +-
>>> drivers/gpu/drm/scheduler/sched_entity.c | 5 +-
>>> drivers/gpu/drm/scheduler/sched_main.c| 101 +-
>>> drivers/gpu/drm/v3d/v3d_gem.c | 2 +-
>>> include/drm/gpu_scheduler.h | 33 --
>>> 11 files changed, 115 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> index 78476bc75b4e..d54daaf64bf1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> @@ -115,7 +115,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct
>>> amdgpu_vm *vm,
>>> if (!entity)
>>> return 0;
>>>
>>> - return drm_sched_job_init(&(*job)->base, entity, owner);
>>> + return drm_sched_job_init(&(*job)->base, entity, 1, owner);
>>> }
>>>
>>> int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev,
>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
>>> b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
>>> index 45403ea38906..74a446711207 100644
>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
>>> @@ -538,7 +538,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev,
>>> void *data,
>>>
>>> ret = drm_sched_job_init(>sched_job,
>>> >sched_entity[args->pipe],
>>> -submit->ctx);
>>> +1, submit->ctx);
>>> if (ret)
>>> goto err_submit_put;
>>>
>>> diff --git a/drivers/gpu/drm/lima/lima_sched.c
>>> b/drivers/gpu/drm/lima/lima_sched.c
>>> index 50c2075228aa..5dc6678e1eb9 100644
>>> --- a/drivers/gpu/drm/lima/lima_sched.c
>>> +++ b/drivers/gpu/drm/lima/lima_sched.c
>>> @@ -123,7 +123,7 @@ int lima_sched_task_init(struct lima_sched_task *task,
>>> for (i = 0; i < num_bos; i++)
>>> drm_gem_object_get([i]->base.base);
>>>
>>> - err = drm_sched_job_init(>base, >base, vm);
>>> + err = drm_sched_job_init(>base, >base, 1, vm);
>>> if (err) {
>>> kfree(task->bos);
>>> return err;
>>> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
>>> b/drivers/gpu/drm/msm/msm_gem_submit.c
>>> index 3f1aa4de3b87..6d230c38e4f5 100644
>>> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
>>> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
>>> @@ -48,7 +48,7 @@ static struct msm_gem_submit *submit_create(struct
>>> drm_device *dev,
>>> return ERR_PTR(ret);
>>> }
>>>
>>> - ret = drm_sched_job_init(>base, queue->entity, queue);
>>> + ret = drm_sched_job_init(>base, queue->entity, 1, queue);
>>> if (ret) {
>>>
Re: [PATCH drm-misc-next v2] drm/sched: implement dynamic job-flow control
On 2023-10-23 18:57, Danilo Krummrich wrote:
> On Tue, Oct 10, 2023 at 09:41:51AM +0200, Boris Brezillon wrote:
>> On Tue, 10 Oct 2023 00:35:53 +0200
>> Danilo Krummrich wrote:
>>
>>> Currently, job flow control is implemented simply by limiting the number
>>> of jobs in flight. Therefore, a scheduler is initialized with a
>>> submission limit that corresponds to the number of jobs which can be
>>> sent to the hardware.
>>>
>>> This implies that for each job, drivers need to account for the maximum
>>> job size possible in order to not overflow the ring buffer.
>>>
>>> However, there are drivers, such as Nouveau, where the job size has a
>>> rather large range. For such drivers it can easily happen that job
>>> submissions not even filling the ring by 1% can block subsequent
>>> submissions, which, in the worst case, can lead to the ring run dry.
>>>
>>> In order to overcome this issue, allow for tracking the actual job size
>>> instead of the number of jobs. Therefore, add a field to track a job's
>>> submission credits, which represents the number of credits a job
>>> contributes to the scheduler's submission limit.
>>>
>>> Signed-off-by: Danilo Krummrich
>>> ---
>>> Changes in V2:
>>> ==
>>> - fixed up influence on scheduling fairness due to consideration of a
>>> job's
>>> size
>>> - If we reach a ready entity in drm_sched_select_entity() but can't
>>> actually
>>> queue a job from it due to size limitations, just give up and go to
>>> sleep
>>> until woken up due to a pending job finishing, rather than continue
>>> to try
>>> other entities.
>>> - added a callback to dynamically update a job's credits (Boris)
>>> - renamed 'units' to 'credits'
>>> - fixed commit message and comments as requested by Luben
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +-
>>> drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +-
>>> drivers/gpu/drm/lima/lima_sched.c | 2 +-
>>> drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
>>> drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +-
>>> drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
>>> .../gpu/drm/scheduler/gpu_scheduler_trace.h | 2 +-
>>> drivers/gpu/drm/scheduler/sched_entity.c | 5 +-
>>> drivers/gpu/drm/scheduler/sched_main.c| 101 +-
>>> drivers/gpu/drm/v3d/v3d_gem.c | 2 +-
>>> include/drm/gpu_scheduler.h | 33 --
>>> 11 files changed, 115 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> index 78476bc75b4e..d54daaf64bf1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> @@ -115,7 +115,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct
>>> amdgpu_vm *vm,
>>> if (!entity)
>>> return 0;
>>>
>>> - return drm_sched_job_init(&(*job)->base, entity, owner);
>>> + return drm_sched_job_init(&(*job)->base, entity, 1, owner);
>>> }
>>>
>>> int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev,
>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
>>> b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
>>> index 45403ea38906..74a446711207 100644
>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
>>> @@ -538,7 +538,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev,
>>> void *data,
>>>
>>> ret = drm_sched_job_init(>sched_job,
>>> >sched_entity[args->pipe],
>>> -submit->ctx);
>>> +1, submit->ctx);
>>> if (ret)
>>> goto err_submit_put;
>>>
>>> diff --git a/drivers/gpu/drm/lima/lima_sched.c
>>> b/drivers/gpu/drm/lima/lima_sched.c
>>> index 50c2075228aa..5dc6678e1eb9 100644
>>> --- a/drivers/gpu/drm/lima/lima_sched.c
>>> +++ b/drivers/gpu/drm/lima/lima_sched.c
>>> @@ -123,7 +123,7 @@ int lima_sched_task_init(struct lima_sched_task *task,
>>> for (i = 0; i < num_bos; i++)
>>> drm_gem_object_get([i]->base.base);
>>>
>>> - err = drm_sched_job_init(>base, >base, vm);
>>> + err = drm_sched_job_init(>base, >base, 1, vm);
>>> if (err) {
>>> kfree(task->bos);
>>> return err;
>>> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
>>> b/drivers/gpu/drm/msm/msm_gem_submit.c
>>> index 3f1aa4de3b87..6d230c38e4f5 100644
>>> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
>>> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
>>> @@ -48,7 +48,7 @@ static struct msm_gem_submit *submit_create(struct
>>> drm_device *dev,
>>> return ERR_PTR(ret);
>>> }
>>>
>>> - ret = drm_sched_job_init(>base, queue->entity, queue);
>>> + ret = drm_sched_job_init(>base, queue->entity, 1, queue);
>>> if (ret) {
>>> kfree(submit->hw_fence);
>>> kfree(submit);
>>> diff --git
Re: [PATCH drm-misc-next v2] drm/sched: implement dynamic job-flow control
On Tue, Oct 10, 2023 at 09:41:51AM +0200, Boris Brezillon wrote:
> On Tue, 10 Oct 2023 00:35:53 +0200
> Danilo Krummrich wrote:
>
> > Currently, job flow control is implemented simply by limiting the number
> > of jobs in flight. Therefore, a scheduler is initialized with a
> > submission limit that corresponds to the number of jobs which can be
> > sent to the hardware.
> >
> > This implies that for each job, drivers need to account for the maximum
> > job size possible in order to not overflow the ring buffer.
> >
> > However, there are drivers, such as Nouveau, where the job size has a
> > rather large range. For such drivers it can easily happen that job
> > submissions not even filling the ring by 1% can block subsequent
> > submissions, which, in the worst case, can lead to the ring run dry.
> >
> > In order to overcome this issue, allow for tracking the actual job size
> > instead of the number of jobs. Therefore, add a field to track a job's
> > submission credits, which represents the number of credits a job
> > contributes to the scheduler's submission limit.
> >
> > Signed-off-by: Danilo Krummrich
> > ---
> > Changes in V2:
> > ==
> > - fixed up influence on scheduling fairness due to consideration of a
> > job's
> > size
> > - If we reach a ready entity in drm_sched_select_entity() but can't
> > actually
> > queue a job from it due to size limitations, just give up and go to
> > sleep
> > until woken up due to a pending job finishing, rather than continue
> > to try
> > other entities.
> > - added a callback to dynamically update a job's credits (Boris)
> > - renamed 'units' to 'credits'
> > - fixed commit message and comments as requested by Luben
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +-
> > drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +-
> > drivers/gpu/drm/lima/lima_sched.c | 2 +-
> > drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
> > drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +-
> > drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
> > .../gpu/drm/scheduler/gpu_scheduler_trace.h | 2 +-
> > drivers/gpu/drm/scheduler/sched_entity.c | 5 +-
> > drivers/gpu/drm/scheduler/sched_main.c| 101 +-
> > drivers/gpu/drm/v3d/v3d_gem.c | 2 +-
> > include/drm/gpu_scheduler.h | 33 --
> > 11 files changed, 115 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > index 78476bc75b4e..d54daaf64bf1 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > @@ -115,7 +115,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct
> > amdgpu_vm *vm,
> > if (!entity)
> > return 0;
> >
> > - return drm_sched_job_init(&(*job)->base, entity, owner);
> > + return drm_sched_job_init(&(*job)->base, entity, 1, owner);
> > }
> >
> > int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev,
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> > b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> > index 45403ea38906..74a446711207 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> > @@ -538,7 +538,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev,
> > void *data,
> >
> > ret = drm_sched_job_init(>sched_job,
> > >sched_entity[args->pipe],
> > -submit->ctx);
> > +1, submit->ctx);
> > if (ret)
> > goto err_submit_put;
> >
> > diff --git a/drivers/gpu/drm/lima/lima_sched.c
> > b/drivers/gpu/drm/lima/lima_sched.c
> > index 50c2075228aa..5dc6678e1eb9 100644
> > --- a/drivers/gpu/drm/lima/lima_sched.c
> > +++ b/drivers/gpu/drm/lima/lima_sched.c
> > @@ -123,7 +123,7 @@ int lima_sched_task_init(struct lima_sched_task *task,
> > for (i = 0; i < num_bos; i++)
> > drm_gem_object_get([i]->base.base);
> >
> > - err = drm_sched_job_init(>base, >base, vm);
> > + err = drm_sched_job_init(>base, >base, 1, vm);
> > if (err) {
> > kfree(task->bos);
> > return err;
> > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> > b/drivers/gpu/drm/msm/msm_gem_submit.c
> > index 3f1aa4de3b87..6d230c38e4f5 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > @@ -48,7 +48,7 @@ static struct msm_gem_submit *submit_create(struct
> > drm_device *dev,
> > return ERR_PTR(ret);
> > }
> >
> > - ret = drm_sched_job_init(>base, queue->entity, queue);
> > + ret = drm_sched_job_init(>base, queue->entity, 1, queue);
> > if (ret) {
> > kfree(submit->hw_fence);
> > kfree(submit);
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.c
> >
Re: [PATCH drm-misc-next v2] drm/sched: implement dynamic job-flow control
On Wed, Oct 11, 2023 at 09:52:36PM -0400, Luben Tuikov wrote:
> Hi,
>
> Thanks for fixing the title and submitting a v2 of this patch. Comments
> inlined below.
>
> On 2023-10-09 18:35, Danilo Krummrich wrote:
> > Currently, job flow control is implemented simply by limiting the number
> > of jobs in flight. Therefore, a scheduler is initialized with a
> > submission limit that corresponds to the number of jobs which can be
> > sent to the hardware.
> >
> > This implies that for each job, drivers need to account for the maximum
> > job size possible in order to not overflow the ring buffer.
> >
> > However, there are drivers, such as Nouveau, where the job size has a
> > rather large range. For such drivers it can easily happen that job
> > submissions not even filling the ring by 1% can block subsequent
> > submissions, which, in the worst case, can lead to the ring run dry.
> >
> > In order to overcome this issue, allow for tracking the actual job size
> > instead of the number of jobs. Therefore, add a field to track a job's
> > submission credits, which represents the number of credits a job
> > contributes to the scheduler's submission limit.
> >
> > Signed-off-by: Danilo Krummrich
> > ---
> > Changes in V2:
> > ==
> > - fixed up influence on scheduling fairness due to consideration of a
> > job's
> > size
> > - If we reach a ready entity in drm_sched_select_entity() but can't
> > actually
> > queue a job from it due to size limitations, just give up and go to
> > sleep
> > until woken up due to a pending job finishing, rather than continue
> > to try
> > other entities.
> > - added a callback to dynamically update a job's credits (Boris)
> > - renamed 'units' to 'credits'
> > - fixed commit message and comments as requested by Luben
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +-
> > drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +-
> > drivers/gpu/drm/lima/lima_sched.c | 2 +-
> > drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
> > drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +-
> > drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
> > .../gpu/drm/scheduler/gpu_scheduler_trace.h | 2 +-
> > drivers/gpu/drm/scheduler/sched_entity.c | 5 +-
> > drivers/gpu/drm/scheduler/sched_main.c| 101 +-
> > drivers/gpu/drm/v3d/v3d_gem.c | 2 +-
> > include/drm/gpu_scheduler.h | 33 --
> > 11 files changed, 115 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > index 78476bc75b4e..d54daaf64bf1 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > @@ -115,7 +115,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct
> > amdgpu_vm *vm,
> > if (!entity)
> > return 0;
> >
> > - return drm_sched_job_init(&(*job)->base, entity, owner);
> > + return drm_sched_job_init(&(*job)->base, entity, 1, owner);
> > }
> >
> > int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev,
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> > b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> > index 45403ea38906..74a446711207 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> > @@ -538,7 +538,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev,
> > void *data,
> >
> > ret = drm_sched_job_init(>sched_job,
> > >sched_entity[args->pipe],
> > -submit->ctx);
> > +1, submit->ctx);
> > if (ret)
> > goto err_submit_put;
> >
> > diff --git a/drivers/gpu/drm/lima/lima_sched.c
> > b/drivers/gpu/drm/lima/lima_sched.c
> > index 50c2075228aa..5dc6678e1eb9 100644
> > --- a/drivers/gpu/drm/lima/lima_sched.c
> > +++ b/drivers/gpu/drm/lima/lima_sched.c
> > @@ -123,7 +123,7 @@ int lima_sched_task_init(struct lima_sched_task *task,
> > for (i = 0; i < num_bos; i++)
> > drm_gem_object_get([i]->base.base);
> >
> > - err = drm_sched_job_init(>base, >base, vm);
> > + err = drm_sched_job_init(>base, >base, 1, vm);
> > if (err) {
> > kfree(task->bos);
> > return err;
> > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> > b/drivers/gpu/drm/msm/msm_gem_submit.c
> > index 3f1aa4de3b87..6d230c38e4f5 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > @@ -48,7 +48,7 @@ static struct msm_gem_submit *submit_create(struct
> > drm_device *dev,
> > return ERR_PTR(ret);
> > }
> >
> > - ret = drm_sched_job_init(>base, queue->entity, queue);
> > + ret = drm_sched_job_init(>base, queue->entity, 1, queue);
> > if (ret) {
> > kfree(submit->hw_fence);
> >
Re: [PATCH drm-misc-next v2] drm/sched: implement dynamic job-flow control
On 2023-10-11 22:10, Danilo Krummrich wrote:
>
>
> On 10/12/23 03:52, Luben Tuikov wrote:
>> Hi,
>>
>> Thanks for fixing the title and submitting a v2 of this patch. Comments
>> inlined below.
>>
>> On 2023-10-09 18:35, Danilo Krummrich wrote:
>>> Currently, job flow control is implemented simply by limiting the number
>>> of jobs in flight. Therefore, a scheduler is initialized with a
>>> submission limit that corresponds to the number of jobs which can be
>>> sent to the hardware.
>>>
>>> This implies that for each job, drivers need to account for the maximum
>>> job size possible in order to not overflow the ring buffer.
>>>
>>> However, there are drivers, such as Nouveau, where the job size has a
>>> rather large range. For such drivers it can easily happen that job
>>> submissions not even filling the ring by 1% can block subsequent
>>> submissions, which, in the worst case, can lead to the ring run dry.
>>>
>>> In order to overcome this issue, allow for tracking the actual job size
>>> instead of the number of jobs. Therefore, add a field to track a job's
>>> submission credits, which represents the number of credits a job
>>> contributes to the scheduler's submission limit.
>>>
>>> Signed-off-by: Danilo Krummrich
>>> ---
>>> Changes in V2:
>>> ==
>>>- fixed up influence on scheduling fairness due to consideration of a
>>> job's
>>> size
>>> - If we reach a ready entity in drm_sched_select_entity() but can't
>>> actually
>>>queue a job from it due to size limitations, just give up and go to
>>> sleep
>>>until woken up due to a pending job finishing, rather than continue
>>> to try
>>>other entities.
>>>- added a callback to dynamically update a job's credits (Boris)
>>>- renamed 'units' to 'credits'
>>>- fixed commit message and comments as requested by Luben
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +-
>>> drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +-
>>> drivers/gpu/drm/lima/lima_sched.c | 2 +-
>>> drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
>>> drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +-
>>> drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
>>> .../gpu/drm/scheduler/gpu_scheduler_trace.h | 2 +-
>>> drivers/gpu/drm/scheduler/sched_entity.c | 5 +-
>>> drivers/gpu/drm/scheduler/sched_main.c| 101 +-
>>> drivers/gpu/drm/v3d/v3d_gem.c | 2 +-
>>> include/drm/gpu_scheduler.h | 33 --
>>> 11 files changed, 115 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> index 78476bc75b4e..d54daaf64bf1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> @@ -115,7 +115,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct
>>> amdgpu_vm *vm,
>>> if (!entity)
>>> return 0;
>>>
>>> - return drm_sched_job_init(&(*job)->base, entity, owner);
>>> + return drm_sched_job_init(&(*job)->base, entity, 1, owner);
>>> }
>>>
>>> int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev,
>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
>>> b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
>>> index 45403ea38906..74a446711207 100644
>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
>>> @@ -538,7 +538,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev,
>>> void *data,
>>>
>>> ret = drm_sched_job_init(>sched_job,
>>> >sched_entity[args->pipe],
>>> -submit->ctx);
>>> +1, submit->ctx);
>>> if (ret)
>>> goto err_submit_put;
>>>
>>> diff --git a/drivers/gpu/drm/lima/lima_sched.c
>>> b/drivers/gpu/drm/lima/lima_sched.c
>>> index 50c2075228aa..5dc6678e1eb9 100644
>>> --- a/drivers/gpu/drm/lima/lima_sched.c
>>> +++ b/drivers/gpu/drm/lima/lima_sched.c
>>> @@ -123,7 +123,7 @@ int lima_sched_task_init(struct lima_sched_task *task,
>>> for (i = 0; i < num_bos; i++)
>>> drm_gem_object_get([i]->base.base);
>>>
>>> - err = drm_sched_job_init(>base, >base, vm);
>>> + err = drm_sched_job_init(>base, >base, 1, vm);
>>> if (err) {
>>> kfree(task->bos);
>>> return err;
>>> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
>>> b/drivers/gpu/drm/msm/msm_gem_submit.c
>>> index 3f1aa4de3b87..6d230c38e4f5 100644
>>> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
>>> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
>>> @@ -48,7 +48,7 @@ static struct msm_gem_submit *submit_create(struct
>>> drm_device *dev,
>>> return ERR_PTR(ret);
>>> }
>>>
>>> - ret = drm_sched_job_init(>base, queue->entity, queue);
>>> + ret = drm_sched_job_init(>base, queue->entity, 1, queue);
>>> if (ret) {
>>>
Re: [PATCH drm-misc-next v2] drm/sched: implement dynamic job-flow control
On 10/12/23 03:52, Luben Tuikov wrote:
Hi,
Thanks for fixing the title and submitting a v2 of this patch. Comments inlined
below.
On 2023-10-09 18:35, Danilo Krummrich wrote:
Currently, job flow control is implemented simply by limiting the number
of jobs in flight. Therefore, a scheduler is initialized with a
submission limit that corresponds to the number of jobs which can be
sent to the hardware.
This implies that for each job, drivers need to account for the maximum
job size possible in order to not overflow the ring buffer.
However, there are drivers, such as Nouveau, where the job size has a
rather large range. For such drivers it can easily happen that job
submissions not even filling the ring by 1% can block subsequent
submissions, which, in the worst case, can lead to the ring run dry.
In order to overcome this issue, allow for tracking the actual job size
instead of the number of jobs. Therefore, add a field to track a job's
submission credits, which represents the number of credits a job
contributes to the scheduler's submission limit.
Signed-off-by: Danilo Krummrich
---
Changes in V2:
==
- fixed up influence on scheduling fairness due to consideration of a job's
size
- If we reach a ready entity in drm_sched_select_entity() but can't
actually
queue a job from it due to size limitations, just give up and go to sleep
until woken up due to a pending job finishing, rather than continue to
try
other entities.
- added a callback to dynamically update a job's credits (Boris)
- renamed 'units' to 'credits'
- fixed commit message and comments as requested by Luben
---
drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +-
drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +-
drivers/gpu/drm/lima/lima_sched.c | 2 +-
drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +-
drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
.../gpu/drm/scheduler/gpu_scheduler_trace.h | 2 +-
drivers/gpu/drm/scheduler/sched_entity.c | 5 +-
drivers/gpu/drm/scheduler/sched_main.c| 101 +-
drivers/gpu/drm/v3d/v3d_gem.c | 2 +-
include/drm/gpu_scheduler.h | 33 --
11 files changed, 115 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 78476bc75b4e..d54daaf64bf1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -115,7 +115,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct
amdgpu_vm *vm,
if (!entity)
return 0;
- return drm_sched_job_init(&(*job)->base, entity, owner);
+ return drm_sched_job_init(&(*job)->base, entity, 1, owner);
}
int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev,
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 45403ea38906..74a446711207 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -538,7 +538,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void
*data,
ret = drm_sched_job_init(>sched_job,
>sched_entity[args->pipe],
-submit->ctx);
+1, submit->ctx);
if (ret)
goto err_submit_put;
diff --git a/drivers/gpu/drm/lima/lima_sched.c b/drivers/gpu/drm/lima/lima_sched.c
index 50c2075228aa..5dc6678e1eb9 100644
--- a/drivers/gpu/drm/lima/lima_sched.c
+++ b/drivers/gpu/drm/lima/lima_sched.c
@@ -123,7 +123,7 @@ int lima_sched_task_init(struct lima_sched_task *task,
for (i = 0; i < num_bos; i++)
drm_gem_object_get([i]->base.base);
- err = drm_sched_job_init(>base, >base, vm);
+ err = drm_sched_job_init(>base, >base, 1, vm);
if (err) {
kfree(task->bos);
return err;
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
b/drivers/gpu/drm/msm/msm_gem_submit.c
index 3f1aa4de3b87..6d230c38e4f5 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -48,7 +48,7 @@ static struct msm_gem_submit *submit_create(struct drm_device
*dev,
return ERR_PTR(ret);
}
- ret = drm_sched_job_init(>base, queue->entity, queue);
+ ret = drm_sched_job_init(>base, queue->entity, 1, queue);
if (ret) {
kfree(submit->hw_fence);
kfree(submit);
diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.c
b/drivers/gpu/drm/nouveau/nouveau_sched.c
index f26a814a9920..e991426d86e4 100644
--- a/drivers/gpu/drm/nouveau/nouveau_sched.c
+++ b/drivers/gpu/drm/nouveau/nouveau_sched.c
@@ -89,7 +89,7 @@ nouveau_job_init(struct nouveau_job *job,
}
- ret = drm_sched_job_init(>base, >base, NULL);
Re: [PATCH drm-misc-next v2] drm/sched: implement dynamic job-flow control
Hi,
Thanks for fixing the title and submitting a v2 of this patch. Comments inlined
below.
On 2023-10-09 18:35, Danilo Krummrich wrote:
> Currently, job flow control is implemented simply by limiting the number
> of jobs in flight. Therefore, a scheduler is initialized with a
> submission limit that corresponds to the number of jobs which can be
> sent to the hardware.
>
> This implies that for each job, drivers need to account for the maximum
> job size possible in order to not overflow the ring buffer.
>
> However, there are drivers, such as Nouveau, where the job size has a
> rather large range. For such drivers it can easily happen that job
> submissions not even filling the ring by 1% can block subsequent
> submissions, which, in the worst case, can lead to the ring run dry.
>
> In order to overcome this issue, allow for tracking the actual job size
> instead of the number of jobs. Therefore, add a field to track a job's
> submission credits, which represents the number of credits a job
> contributes to the scheduler's submission limit.
>
> Signed-off-by: Danilo Krummrich
> ---
> Changes in V2:
> ==
> - fixed up influence on scheduling fairness due to consideration of a job's
> size
> - If we reach a ready entity in drm_sched_select_entity() but can't
> actually
> queue a job from it due to size limitations, just give up and go to
> sleep
> until woken up due to a pending job finishing, rather than continue to
> try
> other entities.
> - added a callback to dynamically update a job's credits (Boris)
> - renamed 'units' to 'credits'
> - fixed commit message and comments as requested by Luben
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +-
> drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +-
> drivers/gpu/drm/lima/lima_sched.c | 2 +-
> drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
> drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +-
> drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
> .../gpu/drm/scheduler/gpu_scheduler_trace.h | 2 +-
> drivers/gpu/drm/scheduler/sched_entity.c | 5 +-
> drivers/gpu/drm/scheduler/sched_main.c| 101 +-
> drivers/gpu/drm/v3d/v3d_gem.c | 2 +-
> include/drm/gpu_scheduler.h | 33 --
> 11 files changed, 115 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 78476bc75b4e..d54daaf64bf1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -115,7 +115,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct
> amdgpu_vm *vm,
> if (!entity)
> return 0;
>
> - return drm_sched_job_init(&(*job)->base, entity, owner);
> + return drm_sched_job_init(&(*job)->base, entity, 1, owner);
> }
>
> int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> index 45403ea38906..74a446711207 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> @@ -538,7 +538,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void
> *data,
>
> ret = drm_sched_job_init(>sched_job,
>>sched_entity[args->pipe],
> - submit->ctx);
> + 1, submit->ctx);
> if (ret)
> goto err_submit_put;
>
> diff --git a/drivers/gpu/drm/lima/lima_sched.c
> b/drivers/gpu/drm/lima/lima_sched.c
> index 50c2075228aa..5dc6678e1eb9 100644
> --- a/drivers/gpu/drm/lima/lima_sched.c
> +++ b/drivers/gpu/drm/lima/lima_sched.c
> @@ -123,7 +123,7 @@ int lima_sched_task_init(struct lima_sched_task *task,
> for (i = 0; i < num_bos; i++)
> drm_gem_object_get([i]->base.base);
>
> - err = drm_sched_job_init(>base, >base, vm);
> + err = drm_sched_job_init(>base, >base, 1, vm);
> if (err) {
> kfree(task->bos);
> return err;
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 3f1aa4de3b87..6d230c38e4f5 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -48,7 +48,7 @@ static struct msm_gem_submit *submit_create(struct
> drm_device *dev,
> return ERR_PTR(ret);
> }
>
> - ret = drm_sched_job_init(>base, queue->entity, queue);
> + ret = drm_sched_job_init(>base, queue->entity, 1, queue);
> if (ret) {
> kfree(submit->hw_fence);
> kfree(submit);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.c
> b/drivers/gpu/drm/nouveau/nouveau_sched.c
> index f26a814a9920..e991426d86e4 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_sched.c
> +++
Re: [PATCH drm-misc-next v2] drm/sched: implement dynamic job-flow control
On Tue, 10 Oct 2023 00:35:53 +0200
Danilo Krummrich wrote:
> Currently, job flow control is implemented simply by limiting the number
> of jobs in flight. Therefore, a scheduler is initialized with a
> submission limit that corresponds to the number of jobs which can be
> sent to the hardware.
>
> This implies that for each job, drivers need to account for the maximum
> job size possible in order to not overflow the ring buffer.
>
> However, there are drivers, such as Nouveau, where the job size has a
> rather large range. For such drivers it can easily happen that job
> submissions not even filling the ring by 1% can block subsequent
> submissions, which, in the worst case, can lead to the ring run dry.
>
> In order to overcome this issue, allow for tracking the actual job size
> instead of the number of jobs. Therefore, add a field to track a job's
> submission credits, which represents the number of credits a job
> contributes to the scheduler's submission limit.
>
> Signed-off-by: Danilo Krummrich
> ---
> Changes in V2:
> ==
> - fixed up influence on scheduling fairness due to consideration of a job's
> size
> - If we reach a ready entity in drm_sched_select_entity() but can't
> actually
> queue a job from it due to size limitations, just give up and go to
> sleep
> until woken up due to a pending job finishing, rather than continue to
> try
> other entities.
> - added a callback to dynamically update a job's credits (Boris)
> - renamed 'units' to 'credits'
> - fixed commit message and comments as requested by Luben
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +-
> drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +-
> drivers/gpu/drm/lima/lima_sched.c | 2 +-
> drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
> drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +-
> drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
> .../gpu/drm/scheduler/gpu_scheduler_trace.h | 2 +-
> drivers/gpu/drm/scheduler/sched_entity.c | 5 +-
> drivers/gpu/drm/scheduler/sched_main.c| 101 +-
> drivers/gpu/drm/v3d/v3d_gem.c | 2 +-
> include/drm/gpu_scheduler.h | 33 --
> 11 files changed, 115 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 78476bc75b4e..d54daaf64bf1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -115,7 +115,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct
> amdgpu_vm *vm,
> if (!entity)
> return 0;
>
> - return drm_sched_job_init(&(*job)->base, entity, owner);
> + return drm_sched_job_init(&(*job)->base, entity, 1, owner);
> }
>
> int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> index 45403ea38906..74a446711207 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> @@ -538,7 +538,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void
> *data,
>
> ret = drm_sched_job_init(>sched_job,
>>sched_entity[args->pipe],
> - submit->ctx);
> + 1, submit->ctx);
> if (ret)
> goto err_submit_put;
>
> diff --git a/drivers/gpu/drm/lima/lima_sched.c
> b/drivers/gpu/drm/lima/lima_sched.c
> index 50c2075228aa..5dc6678e1eb9 100644
> --- a/drivers/gpu/drm/lima/lima_sched.c
> +++ b/drivers/gpu/drm/lima/lima_sched.c
> @@ -123,7 +123,7 @@ int lima_sched_task_init(struct lima_sched_task *task,
> for (i = 0; i < num_bos; i++)
> drm_gem_object_get([i]->base.base);
>
> - err = drm_sched_job_init(>base, >base, vm);
> + err = drm_sched_job_init(>base, >base, 1, vm);
> if (err) {
> kfree(task->bos);
> return err;
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 3f1aa4de3b87..6d230c38e4f5 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -48,7 +48,7 @@ static struct msm_gem_submit *submit_create(struct
> drm_device *dev,
> return ERR_PTR(ret);
> }
>
> - ret = drm_sched_job_init(>base, queue->entity, queue);
> + ret = drm_sched_job_init(>base, queue->entity, 1, queue);
> if (ret) {
> kfree(submit->hw_fence);
> kfree(submit);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.c
> b/drivers/gpu/drm/nouveau/nouveau_sched.c
> index f26a814a9920..e991426d86e4 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_sched.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_sched.c
> @@ -89,7 +89,7 @@ nouveau_job_init(struct nouveau_job *job,
>
> }
