On 2023-10-06 19:43, Matthew Brost wrote:
> On Fri, Oct 06, 2023 at 03:14:04PM +0000, Matthew Brost wrote:
>> On Fri, Oct 06, 2023 at 08:59:15AM +0100, Tvrtko Ursulin wrote:
>>>
>>> On 05/10/2023 05:13, Luben Tuikov wrote:
>>>> On 2023-10-04 23:33, Matthew Brost wrote:
>>>>> On Tue, Sep 26, 2023 at 11:32:10PM -0400, Luben Tuikov wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 2023-09-19 01:01, Matthew Brost wrote:
>>>>>>> In XE, the new Intel GPU driver, a choice has made to have a 1 to 1
>>>>>>> mapping between a drm_gpu_scheduler and drm_sched_entity. At first this
>>>>>>> seems a bit odd but let us explain the reasoning below.
>>>>>>>
>>>>>>> 1. In XE the submission order from multiple drm_sched_entity is not
>>>>>>> guaranteed to be the same completion even if targeting the same hardware
>>>>>>> engine. This is because in XE we have a firmware scheduler, the GuC,
>>>>>>> which allowed to reorder, timeslice, and preempt submissions. If a using
>>>>>>> shared drm_gpu_scheduler across multiple drm_sched_entity, the TDR falls
>>>>>>> apart as the TDR expects submission order == completion order. Using a
>>>>>>> dedicated drm_gpu_scheduler per drm_sched_entity solve this problem.
>>>>>>>
>>>>>>> 2. In XE submissions are done via programming a ring buffer (circular
>>>>>>> buffer), a drm_gpu_scheduler provides a limit on number of jobs, if the
>>>>>>> limit of number jobs is set to RING_SIZE / MAX_SIZE_PER_JOB we get flow
>>>>>>> control on the ring for free.
>>>>>>>
>>>>>>> A problem with this design is currently a drm_gpu_scheduler uses a
>>>>>>> kthread for submission / job cleanup. This doesn't scale if a large
>>>>>>> number of drm_gpu_scheduler are used. To work around the scaling issue,
>>>>>>> use a worker rather than kthread for submission / job cleanup.
>>>>>>>
>>>>>>> v2:
>>>>>>> - (Rob Clark) Fix msm build
>>>>>>> - Pass in run work queue
>>>>>>> v3:
>>>>>>> - (Boris) don't have loop in worker
>>>>>>> v4:
>>>>>>> - (Tvrtko) break out submit ready, stop, start helpers into own patch
>>>>>>> v5:
>>>>>>> - (Boris) default to ordered work queue
>>>>>>>
>>>>>>> Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
>>>>>>> drivers/gpu/drm/etnaviv/etnaviv_sched.c | 2 +-
>>>>>>> drivers/gpu/drm/lima/lima_sched.c | 2 +-
>>>>>>> drivers/gpu/drm/msm/msm_ringbuffer.c | 2 +-
>>>>>>> drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +-
>>>>>>> drivers/gpu/drm/panfrost/panfrost_job.c | 2 +-
>>>>>>> drivers/gpu/drm/scheduler/sched_main.c | 118 ++++++++++-----------
>>>>>>> drivers/gpu/drm/v3d/v3d_sched.c | 10 +-
>>>>>>> include/drm/gpu_scheduler.h | 14 ++-
>>>>>>> 9 files changed, 79 insertions(+), 75 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>>> index e366f61c3aed..16f3cfe1574a 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>>> @@ -2279,7 +2279,7 @@ static int amdgpu_device_init_schedulers(struct amdgpu_device *adev)
>>>>>>> break;
>>>>>>> }
>>>>>>> - r = drm_sched_init(&ring->sched, &amdgpu_sched_ops,
>>>>>>> + r = drm_sched_init(&ring->sched, &amdgpu_sched_ops, NULL,
>>>>>>> ring->num_hw_submission, 0,
>>>>>>> timeout, adev->reset_domain->wq,
>>>>>>> ring->sched_score, ring->name,
>>>>>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>>>>>>> index 345fec6cb1a4..618a804ddc34 100644
>>>>>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>>>>>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>>>>>>> @@ -134,7 +134,7 @@ int etnaviv_sched_init(struct etnaviv_gpu *gpu)
>>>>>>> {
>>>>>>> int ret;
>>>>>>> - ret = drm_sched_init(&gpu->sched, &etnaviv_sched_ops,
>>>>>>> + ret = drm_sched_init(&gpu->sched, &etnaviv_sched_ops, NULL,
>>>>>>> etnaviv_hw_jobs_limit, etnaviv_job_hang_limit,
>>>>>>> msecs_to_jiffies(500), NULL, NULL,
>>>>>>> dev_name(gpu->dev), gpu->dev);
>>>>>>> diff --git a/drivers/gpu/drm/lima/lima_sched.c b/drivers/gpu/drm/lima/lima_sched.c
>>>>>>> index ffd91a5ee299..8d858aed0e56 100644
>>>>>>> --- a/drivers/gpu/drm/lima/lima_sched.c
>>>>>>> +++ b/drivers/gpu/drm/lima/lima_sched.c
>>>>>>> @@ -488,7 +488,7 @@ int lima_sched_pipe_init(struct lima_sched_pipe *pipe, const char *name)
>>>>>>> INIT_WORK(&pipe->recover_work, lima_sched_recover_work);
>>>>>>> - return drm_sched_init(&pipe->base, &lima_sched_ops, 1,
>>>>>>> + return drm_sched_init(&pipe->base, &lima_sched_ops, NULL, 1,
>>>>>>> lima_job_hang_limit,
>>>>>>> msecs_to_jiffies(timeout), NULL,
>>>>>>> NULL, name, pipe->ldev->dev);
>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c
>>>>>>> index 40c0bc35a44c..b8865e61b40f 100644
>>>>>>> --- a/drivers/gpu/drm/msm/msm_ringbuffer.c
>>>>>>> +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
>>>>>>> @@ -94,7 +94,7 @@ struct msm_ringbuffer *msm_ringbuffer_new(struct msm_gpu *gpu, int id,
>>>>>>> /* currently managing hangcheck ourselves: */
>>>>>>> sched_timeout = MAX_SCHEDULE_TIMEOUT;
>>>>>>> - ret = drm_sched_init(&ring->sched, &msm_sched_ops,
>>>>>>> + ret = drm_sched_init(&ring->sched, &msm_sched_ops, NULL,
>>>>>>> num_hw_submissions, 0, sched_timeout,
>>>>>>> NULL, NULL, to_msm_bo(ring->bo)->name, gpu->dev->dev);
>>>>>>
>>>>>> checkpatch.pl complains here about unmatched open parens.
>>>>>>
>>>>>
>>>>> Will fix and run checkpatch before posting next rev.
>>>>>
>>>>>>> if (ret) {
>>>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.c b/drivers/gpu/drm/nouveau/nouveau_sched.c
>>>>>>> index 88217185e0f3..d458c2227d4f 100644
>>>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_sched.c
>>>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_sched.c
>>>>>>> @@ -429,7 +429,7 @@ int nouveau_sched_init(struct nouveau_drm *drm)
>>>>>>> if (!drm->sched_wq)
>>>>>>> return -ENOMEM;
>>>>>>> - return drm_sched_init(sched, &nouveau_sched_ops,
>>>>>>> + return drm_sched_init(sched, &nouveau_sched_ops, NULL,
>>>>>>> NOUVEAU_SCHED_HW_SUBMISSIONS, 0, job_hang_limit,
>>>>>>> NULL, NULL, "nouveau_sched", drm->dev->dev);
>>>>>>> }
>>>>>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
>>>>>>> index 033f5e684707..326ca1ddf1d7 100644
>>>>>>> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
>>>>>>> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
>>>>>>> @@ -831,7 +831,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
>>>>>>> js->queue[j].fence_context = dma_fence_context_alloc(1);
>>>>>>> ret = drm_sched_init(&js->queue[j].sched,
>>>>>>> - &panfrost_sched_ops,
>>>>>>> + &panfrost_sched_ops, NULL,
>>>>>>> nentries, 0,
>>>>>>> msecs_to_jiffies(JOB_TIMEOUT_MS),
>>>>>>> pfdev->reset.wq,
>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>> index e4fa62abca41..ee6281942e36 100644
>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>> @@ -48,7 +48,6 @@
>>>>>>> * through the jobs entity pointer.
>>>>>>> */
>>>>>>> -#include <linux/kthread.h>
>>>>>>> #include <linux/wait.h>
>>>>>>> #include <linux/sched.h>
>>>>>>> #include <linux/completion.h>
>>>>>>> @@ -256,6 +255,16 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
>>>>>>> return rb ? rb_entry(rb, struct drm_sched_entity, rb_tree_node) : NULL;
>>>>>>> }
>>>>>>> +/**
>>>>>>> + * drm_sched_submit_queue - scheduler queue submission
>>>>>>
>>>>>> There is no verb in the description, and is not clear what
>>>>>> this function does unless one reads the code. Given that this
>>>>>> is DOC, this should be clearer here. Something like "queue
>>>>>> scheduler work to be executed" or something to that effect.
>>>>>>
>>>>>
>>>>> Will fix.
>>>>>> Coming back to this from reading the patch below, it was somewhat
>>>>>> unclear what "drm_sched_submit_queue()" does, since when reading
>>>>>> below, "submit" was being read by my mind as an adjective, as opposed
>>>>>> to a verb. Perhaps something like:
>>>>>>
>>>>>> drm_sched_queue_submit(), or
>>>>>> drm_sched_queue_exec(), or
>>>>>> drm_sched_queue_push(), or something to that effect. You pick. :-)
>>>>>>
>>>>>
>>>>> I prefer the name as is. In this patch we have:
>>>>>
>>>>> drm_sched_submit_queue()
>>>>> drm_sched_submit_start)
>>>>> drm_sched_submit_stop()
>>>>> drm_sched_submit_ready()
>>>>>
>>>>> I like all these functions start with 'drm_sched_submit' which allows
>>>>> for easy searching for the functions that touch the DRM scheduler
>>>>> submission state.
>>>>>
>>>>> With a little better doc are you fine with the names as is.
>>>>
>>>> Notice the following scheme in the naming,
>>>>
>>>> drm_sched_submit_queue()
>>>> drm_sched_submit_start)
>>>> drm_sched_submit_stop()
>>>> drm_sched_submit_ready()
>>>> \---+---/ \--+-/ \-+-/
>>>> | | +---> a verb
>>>> | +---------> should be a noun (something in the component)
>>>> +------------------> the kernel/software component
>>>>
>>>> And although "queue" can technically be used as a verb too, I'd rather it be "enqueue",
>>>> like this:
>>>>
>>>> drm_sched_submit_enqueue()
>>>>
>>>> And using "submit" as the noun of the component is a bit cringy,
>>>> since "submit" is really a verb, and it's cringy to make it a "state"
>>>> or an "object" we operate on in the DRM Scheduler. "Submission" is
>>>> a noun, but "submission enqueue/start/stop/ready" doesn't sound
>>>> very well thought out. "Submission" really is what the work-queue
>>>> does.
^^^^^^^^^^^^^^^^^^^ Here ^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>
>>>> I'd rather it be a real object, like for instance,
>>>>
>>>> drm_sched_wqueue_enqueue()
>>>> drm_sched_wqueue_start)
>>>> drm_sched_wqueue_stop()
>>>> drm_sched_wqueue_ready()
>>>>
>>
>> How about:
>>
>> drm_sched_submission_enqueue()
>> drm_sched_submission_start)
>> drm_sched_submission_stop()
>> drm_sched_submission_ready()
>>
>> Matt
>
> Ignore this, read Tvrtko commnt and not Luben's fully.
>
> I prefer drm_sched_wqueue over drm_sched_submit_queue as submit queue is
> a made of thing. drm_sched_submission would be my top choice but if Luben
> is opposed will go with drm_sched_wqueue in next rev.
You had me at "opposed."
I think I've explained why up there.
drm_sched_wqueue_[verb]() is clear and clean. We don't need yet another
abstraction, to the abstraction, to the naming.
If we ever implement anything different than work-queue in the future,
we may split the code and decide to keep both, maybe depending on what
a driver would like to use, etc., so it's cleanest to convey what it means.
"drm_sched_submission_[verb]()" is really cringy.
Plus, it's a good reminder to know that's it's a work-queue. Keeps driver
writers informed. There is no reason to obfuscate the code.
--
Regards,
Luben
