Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control

["Christian König" <christian.koenig@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 4208 bytes --]

Am 27.10.23 um 10:22 schrieb Boris Brezillon:
> On Fri, 27 Oct 2023 09:44:13 +0200
> Christian König<christian.koenig@amd.com>  wrote:
>
>> Am 27.10.23 um 09:39 schrieb Boris Brezillon:
>>> On Fri, 27 Oct 2023 09:35:01 +0200
>>> Christian König<christian.koenig@amd.com>   wrote:
>>>   
>>>> Am 27.10.23 um 09:32 schrieb Boris Brezillon:
>>>>> On Fri, 27 Oct 2023 09:22:12 +0200
>>>>> Christian König<christian.koenig@amd.com>   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.
>>>>>>> +	 *
>>>>>>> +	 * This callback is optional.
>>>>>>> +	 */
>>>>>>> +	u32 (*update_job_credits)(struct drm_sched_job *sched_job);
>>>>>> Why do we need an extra callback for this?
>>>>>>
>>>>>> Just document that prepare_job() is allowed to reduce the number of
>>>>>> credits the job might need.
>>>>> ->prepare_job() is called only once if the returned fence is NULL, but
>>>>> we need this credit-update to happen every time a job is considered for
>>>>> execution by the scheduler.
>>>> But the job is only considered for execution once. How do you see that
>>>> this is called multiple times?
>>> Nope, it's not. If drm_sched_can_queue() returns false, the scheduler
>>> will go look for another entity that has a job ready for execution, and
>>> get back to this entity later, and test drm_sched_can_queue() again.
>>> Basically, any time drm_sched_can_queue() is called, the job credits
>>> update should happen, so we have an accurate view of how many credits
>>> this job needs.
>> Well, that is the handling which I already rejected because it creates
>> unfairness between processes. When you consider the credits needed
>> *before* scheduling jobs with a lower credit count are always preferred
>> over jobs with a higher credit count.
> My bad, it doesn't pick another entity when an entity with a
> ready job that doesn't fit the queue is found, it just bails out from
> drm_sched_rq_select_entity_rr() and returns NULL (AKA: no ready entity
> found). But we still want to update the job credits before checking if
> the job fits or not (next time this entity is tested).

Why? I only see a few possibility here:

1. You need to wait for submissions to the same scheduler to finish 
before the one you want to push to the ring.
     This case can be avoided by trying to cast the dependency fences to 
drm_sched_fences and looking if they are already scheduled.

2. You need to wait for submissions to a different scheduler instance 
and in this case you should probably return that as dependency instead.

So to me it looks like when prepare_job() is called because it is 
selected as next job for submission you should already know how many 
credits it needs.

>> What you can do is to look at the credits of a job *after* it was picked
>> up for scheduling so that you can scheduler more jobs.
> Sure, but then you might further delay your job if something made it
> smaller (ie. native fences got signaled) between ->prepare_job() and
> drm_sched_can_queue(). And any new drm_sched_can_queue() test would
> just see the old credits value.
>
> Out of curiosity, what are you worried about with this optional
> ->update_job_credits() call in the drm_sched_can_queue() path? Is the
> if (sched->update_job_credits) overhead considered too high for drivers
> that don't need it?

Since the dma_fences are also used for resource management the scheduler 
is vital for correct system operation.

We had massively problems because people tried to over-optimize the 
dma_fence handling which lead to very hard to narrow down memory 
corruptions.

So for every change you come up here you need to have a very very good 
justification. And just saving a bit size of your ring buffer is 
certainly not one of them.

Regards,
Christian.

[-- Attachment #2: Type: text/html, Size: 5925 bytes --]