On Thu, Sep 28, 2023 at 12:14:12PM -0400, Luben Tuikov wrote:
> On 2023-09-19 01:01, Matthew Brost wrote:
> > Rather than call free_job and run_job in same work item have a dedicated
> > work item for each. This aligns with the design and intended use of work
> > queues.
> >
> > v2:
> > - Test for DMA_FENCE_FLAG_TIMESTAMP_BIT before setting
> > timestamp in free_job() work item (Danilo)
> > v3:
> > - Drop forward dec of drm_sched_select_entity (Boris)
> > - Return in drm_sched_run_job_work if entity NULL (Boris)
> >
> > Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/scheduler/sched_main.c | 290 +++++++++++++++----------
> > include/drm/gpu_scheduler.h | 8 +-
> > 2 files changed, 182 insertions(+), 116 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> > index 588c735f7498..1e21d234fb5c 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -213,11 +213,12 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
> > * drm_sched_rq_select_entity_rr - Select an entity which could provide a job to run
> > *
> > * @rq: scheduler run queue to check.
> > + * @dequeue: dequeue selected entity
>
> Change this to "peek" as indicated below.
>
> > *
> > * Try to find a ready entity, returns NULL if none found.
> > */
> > static struct drm_sched_entity *
> > -drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> > +drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq, bool dequeue)
> > {
> > struct drm_sched_entity *entity;
> >
> > @@ -227,8 +228,10 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> > if (entity) {
> > list_for_each_entry_continue(entity, &rq->entities, list) {
> > if (drm_sched_entity_is_ready(entity)) {
> > - rq->current_entity = entity;
> > - reinit_completion(&entity->entity_idle);
> > + if (dequeue) {
> > + rq->current_entity = entity;
> > + reinit_completion(&entity->entity_idle);
> > + }
>
> Please rename "dequeue" or invert its logic, as from this patch it seems that
> it is hiding (gating out) current behaviour.
>
> Ideally, I'd prefer it be inverted, so that current behaviour, i.e. what people
> are used to the rq_select_entity_*() to do, is default--preserved.
>
> Perhaps use "peek" as the name of this new variable, to indicate that
> we're not setting it to be the current entity.
>
> I prefer "peek" to others, as the former tells me "Hey, I'm only
> peeking at the rq and not really doing the default behaviour I've been
> doing which you're used to." So, probably use "peek". ("Peek" also has historical
> significance...).
>
Peek it is. Will change.
> > spin_unlock(&rq->lock);
> > return entity;
> > }
> > @@ -238,8 +241,10 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> > list_for_each_entry(entity, &rq->entities, list) {
> >
> > if (drm_sched_entity_is_ready(entity)) {
> > - rq->current_entity = entity;
> > - reinit_completion(&entity->entity_idle);
> > + if (dequeue) {
>
> if (!peek) {
>
+1
> > + rq->current_entity = entity;
> > + reinit_completion(&entity->entity_idle);
> > + }
> > spin_unlock(&rq->lock);
> > return entity;
> > }
> > @@ -257,11 +262,12 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> > * drm_sched_rq_select_entity_fifo - Select an entity which provides a job to run
> > *
> > * @rq: scheduler run queue to check.
> > + * @dequeue: dequeue selected entity
>
> * @peek: Just find, don't set to current.
>
+1
> > *
> > * Find oldest waiting ready entity, returns NULL if none found.> */
> > static struct drm_sched_entity *
> > -drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
> > +drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq, bool dequeue)
> > {
> > struct rb_node *rb;
> >
> > @@ -271,8 +277,10 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
> >
> > entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
> > if (drm_sched_entity_is_ready(entity)) {
> > - rq->current_entity = entity;
> > - reinit_completion(&entity->entity_idle);
> > + if (dequeue) {
>
> if (!peek) {
>
> > + rq->current_entity = entity;
> > + reinit_completion(&entity->entity_idle);
> > + }
> > break;
> > }
> > }
> > @@ -282,13 +290,102 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
> > }
> >
> > /**
> > - * drm_sched_submit_queue - scheduler queue submission
> > + * drm_sched_run_job_queue - queue job submission
> > + * @sched: scheduler instance
> > + */
>
> Perhaps it would be clearer to a DOC reader if there were verbs
> in this function comment? I feel this was mentioned in the review
> to patch 2...
>
> > +static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
> > +{
> > + if (!READ_ONCE(sched->pause_submit))
> > + queue_work(sched->submit_wq, &sched->work_run_job);
> > +}
> > +
> > +/**
> > + * drm_sched_can_queue -- Can we queue more to the hardware?
> > + * @sched: scheduler instance
> > + *
> > + * Return true if we can push more jobs to the hw, otherwise false.
> > + */
> > +static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched)
> > +{
> > + return atomic_read(&sched->hw_rq_count) <
> > + sched->hw_submission_limit;
> > +}
> > +
> > +/**
> > + * drm_sched_select_entity - Select next entity to process
> > + *
> > + * @sched: scheduler instance
> > + * @dequeue: dequeue selected entity
>
> When I see "dequeue" I'm thinking "list_del()". Let's
> use "peek" here as mentioned above.
>
> > + *
> > + * Returns the entity to process or NULL if none are found.
> > + */
> > +static struct drm_sched_entity *
> > +drm_sched_select_entity(struct drm_gpu_scheduler *sched, bool dequeue)
>
> drm_sched_select_entity(struct drm_gpu_scheduler *sched, bool peek)
>
+1
> > +{
> > + struct drm_sched_entity *entity;
> > + int i;
> > +
> > + if (!drm_sched_can_queue(sched))
> > + return NULL;
> > +
> > + if (sched->single_entity) {
> > + if (!READ_ONCE(sched->single_entity->stopped) &&
> > + drm_sched_entity_is_ready(sched->single_entity))
> > + return sched->single_entity;
> > +
> > + return NULL;
> > + }
> > +
> > + /* Kernel run queue has higher priority than normal run queue*/
> > + for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= DRM_SCHED_PRIORITY_MIN; i--) {
> > + entity = sched->sched_policy == DRM_SCHED_POLICY_FIFO ?
> > + drm_sched_rq_select_entity_fifo(&sched->sched_rq[i],
> > + dequeue) :
> > + drm_sched_rq_select_entity_rr(&sched->sched_rq[i],
> > + dequeue);
> > + if (entity)
> > + break;
> > + }
> > +
> > + return entity;
> > +}
> > +
> > +/**
> > + * drm_sched_run_job_queue_if_ready - queue job submission if ready
> > * @sched: scheduler instance
> > */
> > -static void drm_sched_submit_queue(struct drm_gpu_scheduler *sched)
> > +static void drm_sched_run_job_queue_if_ready(struct drm_gpu_scheduler *sched)
> > +{
> > + if (drm_sched_select_entity(sched, false))
> > + drm_sched_run_job_queue(sched);
> > +}
> > +
> > +/**
> > + * drm_sched_free_job_queue - queue free job
>
> * drm_sched_free_job_queue - enqueue free-job work
>
> > + *
> > + * @sched: scheduler instance to queue free job
>
> * @sched: scheduler instance to queue free job work for
>
>
Will change both.
> > + */
> > +static void drm_sched_free_job_queue(struct drm_gpu_scheduler *sched)
> > {
> > if (!READ_ONCE(sched->pause_submit))
> > - queue_work(sched->submit_wq, &sched->work_submit);
> > + queue_work(sched->submit_wq, &sched->work_free_job);
> > +}
> > +
> > +/**
> > + * drm_sched_free_job_queue_if_ready - queue free job if ready
>
> * drm_sched_free_job_queue_if_ready - enqueue free-job work if ready
>
Will change this too.
> > + *
> > + * @sched: scheduler instance to queue free job
> > + */
> > +static void drm_sched_free_job_queue_if_ready(struct drm_gpu_scheduler *sched)
> > +{
> > + struct drm_sched_job *job;
> > +
> > + spin_lock(&sched->job_list_lock);
> > + job = list_first_entry_or_null(&sched->pending_list,
> > + struct drm_sched_job, list);
> > + if (job && dma_fence_is_signaled(&job->s_fence->finished))
> > + drm_sched_free_job_queue(sched);
> > + spin_unlock(&sched->job_list_lock);
> > }
> >
> > /**
> > @@ -310,7 +407,7 @@ static void drm_sched_job_done(struct drm_sched_job *s_job, int result)
> > dma_fence_get(&s_fence->finished);
> > drm_sched_fence_finished(s_fence, result);
> > dma_fence_put(&s_fence->finished);
> > - drm_sched_submit_queue(sched);
> > + drm_sched_free_job_queue(sched);
> > }
> >
> > /**
> > @@ -885,18 +982,6 @@ void drm_sched_job_cleanup(struct drm_sched_job *job)
> > }
> > EXPORT_SYMBOL(drm_sched_job_cleanup);
> >
> > -/**
> > - * drm_sched_can_queue -- Can we queue more to the hardware?
> > - * @sched: scheduler instance
> > - *
> > - * Return true if we can push more jobs to the hw, otherwise false.
> > - */
> > -static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched)
> > -{
> > - return atomic_read(&sched->hw_rq_count) <
> > - sched->hw_submission_limit;
> > -}
> > -
> > /**
> > * drm_sched_wakeup_if_can_queue - Wake up the scheduler
> > * @sched: scheduler instance
> > @@ -906,43 +991,7 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched)
> > void drm_sched_wakeup_if_can_queue(struct drm_gpu_scheduler *sched)
> > {
> > if (drm_sched_can_queue(sched))
> > - drm_sched_submit_queue(sched);
> > -}
> > -
> > -/**
> > - * drm_sched_select_entity - Select next entity to process
> > - *
> > - * @sched: scheduler instance
> > - *
> > - * Returns the entity to process or NULL if none are found.
> > - */
> > -static struct drm_sched_entity *
> > -drm_sched_select_entity(struct drm_gpu_scheduler *sched)
> > -{
> > - struct drm_sched_entity *entity;
> > - int i;
> > -
> > - if (!drm_sched_can_queue(sched))
> > - return NULL;
> > -
> > - if (sched->single_entity) {
> > - if (!READ_ONCE(sched->single_entity->stopped) &&
> > - drm_sched_entity_is_ready(sched->single_entity))
> > - return sched->single_entity;
> > -
> > - return NULL;
> > - }
> > -
> > - /* Kernel run queue has higher priority than normal run queue*/
> > - for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= DRM_SCHED_PRIORITY_MIN; i--) {
> > - entity = sched->sched_policy == DRM_SCHED_POLICY_FIFO ?
> > - drm_sched_rq_select_entity_fifo(&sched->sched_rq[i]) :
> > - drm_sched_rq_select_entity_rr(&sched->sched_rq[i]);
> > - if (entity)
> > - break;
> > - }
> > -
> > - return entity;
> > + drm_sched_run_job_queue(sched);
> > }
> >
> > /**
> > @@ -974,8 +1023,10 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
> > typeof(*next), list);
> >
> > if (next) {
> > - next->s_fence->scheduled.timestamp =
> > - job->s_fence->finished.timestamp;
> > + if (test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT,
> > + &next->s_fence->scheduled.flags))
> > + next->s_fence->scheduled.timestamp =
> > + job->s_fence->finished.timestamp;
> > /* start TO timer for next job */
> > drm_sched_start_timeout(sched);
> > }
> > @@ -1025,74 +1076,84 @@ drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
> > EXPORT_SYMBOL(drm_sched_pick_best);
> >
> > /**
> > - * drm_sched_main - main scheduler thread
> > + * drm_sched_free_job_work - worker to call free_job
> > *
> > - * @param: scheduler instance
> > + * @w: free job work
> > */
> > -static void drm_sched_main(struct work_struct *w)
> > +static void drm_sched_free_job_work(struct work_struct *w)
> > {
> > struct drm_gpu_scheduler *sched =
> > - container_of(w, struct drm_gpu_scheduler, work_submit);
> > - struct drm_sched_entity *entity;
> > + container_of(w, struct drm_gpu_scheduler, work_free_job);
> > struct drm_sched_job *cleanup_job;
> > - int r;
> >
> > if (READ_ONCE(sched->pause_submit))
> > return;
> >
> > cleanup_job = drm_sched_get_cleanup_job(sched);
> > - entity = drm_sched_select_entity(sched);
> > -
> > - if (!entity && !cleanup_job)
> > - return; /* No more work */
> > -
> > - if (cleanup_job)
> > + if (cleanup_job) {
> > sched->ops->free_job(cleanup_job);
> >
> > - if (entity) {
> > - struct dma_fence *fence;
> > - struct drm_sched_fence *s_fence;
> > - struct drm_sched_job *sched_job;
> > -
> > - sched_job = drm_sched_entity_pop_job(entity);
> > - if (!sched_job) {
> > - complete_all(&entity->entity_idle);
> > - if (!cleanup_job)
> > - return; /* No more work */
> > - goto again;
> > - }
> > + drm_sched_free_job_queue_if_ready(sched);
> > + drm_sched_run_job_queue_if_ready(sched);
> > + }
> > +}
> > +
> > +/**
> > + * drm_sched_run_job_work - worker to call run_job
> > + *
> > + * @w: run job work
> > + */
> > +static void drm_sched_run_job_work(struct work_struct *w)
> > +{
> > + struct drm_gpu_scheduler *sched =
> > + container_of(w, struct drm_gpu_scheduler, work_run_job);
> > + struct drm_sched_entity *entity;
> > + struct dma_fence *fence;
> > + struct drm_sched_fence *s_fence;
> > + struct drm_sched_job *sched_job;
> > + int r;
> >
> > - s_fence = sched_job->s_fence;
> > + if (READ_ONCE(sched->pause_submit))
> > + return;
> >
> > - atomic_inc(&sched->hw_rq_count);
> > - drm_sched_job_begin(sched_job);
> > + entity = drm_sched_select_entity(sched, true);
> > + if (!entity)
> > + return;
> >
> > - trace_drm_run_job(sched_job, entity);
> > - fence = sched->ops->run_job(sched_job);
> > + sched_job = drm_sched_entity_pop_job(entity);
> > + if (!sched_job) {
> > complete_all(&entity->entity_idle);
> > - drm_sched_fence_scheduled(s_fence, fence);
> > + return; /* No more work */
> > + }
> >
> > - if (!IS_ERR_OR_NULL(fence)) {
> > - /* Drop for original kref_init of the fence */
> > - dma_fence_put(fence);
> > + s_fence = sched_job->s_fence;
> >
> > - r = dma_fence_add_callback(fence, &sched_job->cb,
> > - drm_sched_job_done_cb);
> > - if (r == -ENOENT)
> > - drm_sched_job_done(sched_job, fence->error);
> > - else if (r)
> > - DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
> > - r);
> > - } else {
> > - drm_sched_job_done(sched_job, IS_ERR(fence) ?
> > - PTR_ERR(fence) : 0);
> > - }
> > + atomic_inc(&sched->hw_rq_count);
> > + drm_sched_job_begin(sched_job);
> > +
> > + trace_drm_run_job(sched_job, entity);
> > + fence = sched->ops->run_job(sched_job);
> > + complete_all(&entity->entity_idle);
> > + drm_sched_fence_scheduled(s_fence, fence);
> >
> > - wake_up(&sched->job_scheduled);
> > + if (!IS_ERR_OR_NULL(fence)) {
> > + /* Drop for original kref_init of the fence */
> > + dma_fence_put(fence);
> > +
> > + r = dma_fence_add_callback(fence, &sched_job->cb,
> > + drm_sched_job_done_cb);
> > + if (r == -ENOENT)
> > + drm_sched_job_done(sched_job, fence->error);
> > + else if (r)
> > + DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
> > + r);
>
> Please align "r);" to the open brace on the previous line. If you're using Emacs
> with sane Linux settings, press the "Tab" key anywhere on the line to indent it.
> (It should run c-indent-line-or-region, usually using leading-tabs-only mode. Pressing
> it again, over and over, on an already indented line, does nothing. Column indenting--say
> for columns in 2D/3D/etc., array, usually happens using spaces, which is portable.
> Also please take an overview with "scrips/checkpatch.pl --strict".)
>
Will run checkpatch.
> Wrap-around was bumped to 100 in the Linux kernel so you can put the 'r' on
> the same line without style problems.
>
Using Vi with wrap around of 80 but know 100 is allowed. Will fix.
> > + } else {
> > + drm_sched_job_done(sched_job, IS_ERR(fence) ?
> > + PTR_ERR(fence) : 0);
> > }
> >
> > -again:
> > - drm_sched_submit_queue(sched);
> > + wake_up(&sched->job_scheduled);
> > + drm_sched_run_job_queue_if_ready(sched);
> > }
> >
> > /**
> > @@ -1159,7 +1220,8 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
> > spin_lock_init(&sched->job_list_lock);
> > atomic_set(&sched->hw_rq_count, 0);
> > INIT_DELAYED_WORK(&sched->work_tdr, drm_sched_job_timedout);
> > - INIT_WORK(&sched->work_submit, drm_sched_main);
> > + INIT_WORK(&sched->work_run_job, drm_sched_run_job_work);
> > + INIT_WORK(&sched->work_free_job, drm_sched_free_job_work);
> > atomic_set(&sched->_score, 0);
> > atomic64_set(&sched->job_id_count, 0);
> > sched->pause_submit = false;
> > @@ -1286,7 +1348,8 @@ EXPORT_SYMBOL(drm_sched_submit_ready);
> > void drm_sched_submit_stop(struct drm_gpu_scheduler *sched)
> > {
> > WRITE_ONCE(sched->pause_submit, true);
> > - cancel_work_sync(&sched->work_submit);
> > + cancel_work_sync(&sched->work_run_job);
> > + cancel_work_sync(&sched->work_free_job);
> > }
> > EXPORT_SYMBOL(drm_sched_submit_stop);
> >
> > @@ -1298,6 +1361,7 @@ EXPORT_SYMBOL(drm_sched_submit_stop);
> > void drm_sched_submit_start(struct drm_gpu_scheduler *sched)
> > {
> > WRITE_ONCE(sched->pause_submit, false);
> > - queue_work(sched->submit_wq, &sched->work_submit);
> > + queue_work(sched->submit_wq, &sched->work_run_job);
> > + queue_work(sched->submit_wq, &sched->work_free_job);
> > }
> > EXPORT_SYMBOL(drm_sched_submit_start);
> > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > index 655675f797ea..7e6c121003ca 100644
> > --- a/include/drm/gpu_scheduler.h
> > +++ b/include/drm/gpu_scheduler.h
> > @@ -487,9 +487,10 @@ struct drm_sched_backend_ops {
> > * finished.
> > * @hw_rq_count: the number of jobs currently in the hardware queue.
> > * @job_id_count: used to assign unique id to the each job.
> > - * @submit_wq: workqueue used to queue @work_submit
> > + * @submit_wq: workqueue used to queue @work_run_job and @work_free_job
> > * @timeout_wq: workqueue used to queue @work_tdr
> > - * @work_submit: schedules jobs and cleans up entities
> > + * @work_run_job: schedules jobs
> > + * @work_free_job: cleans up jobs
> > * @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the
> > * timeout interval is over.
> > * @pending_list: the list of jobs which are currently in the job queue.
> > @@ -519,7 +520,8 @@ struct drm_gpu_scheduler {
> > atomic64_t job_id_count;
> > struct workqueue_struct *submit_wq;
> > struct workqueue_struct *timeout_wq;
> > - struct work_struct work_submit;
> > + struct work_struct work_run_job;
> > + struct work_struct work_free_job;
> > struct delayed_work work_tdr;
> > struct list_head pending_list;
> > spinlock_t job_list_lock;
>
> Yeah, so this is a good patch. Thanks for doing this!
Thanks for the review.
Matt
> --
> Regards,
> Luben
>
