On Fri, 27 Oct 2023 09:44:13 +0200 Christian König <christian.koenig@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx> 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). > 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?
