From: Boris Brezillon <boris.brezillon@collabora.com>
To: Matthew Brost <matthew.brost@intel.com>
Cc: robdclark@chromium.org, sarah.walker@imgtec.com,
ketil.johnsen@arm.com, Liviu.Dudau@arm.com, mcanal@igalia.com,
frank.binns@imgtec.com, dri-devel@lists.freedesktop.org,
christian.koenig@amd.com, luben.tuikov@amd.com,
donald.robson@imgtec.com, daniel@ffwll.ch, lina@asahilina.net,
airlied@gmail.com, intel-xe@lists.freedesktop.org,
faith.ekstrand@collabora.com
Subject: Re: [Intel-xe] [PATCH v3 05/13] drm/sched: Split free_job into own work item
Date: Tue, 12 Sep 2023 10:08:33 +0200 [thread overview]
Message-ID: <20230912100833.792eacb6@collabora.com> (raw)
In-Reply-To: <20230912021615.2086698-6-matthew.brost@intel.com>
On Mon, 11 Sep 2023 19:16:07 -0700
Matthew Brost <matthew.brost@intel.com> 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)
>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
> drivers/gpu/drm/scheduler/sched_main.c | 143 ++++++++++++++++++-------
> include/drm/gpu_scheduler.h | 8 +-
> 2 files changed, 110 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 3820e9ae12c8..d28b6751256e 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
> *
> * 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);
> + }
> 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) {
> + 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
> *
> * 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) {
> + rq->current_entity = entity;
> + reinit_completion(&entity->entity_idle);
> + }
> break;
> }
> }
> @@ -282,13 +290,54 @@ 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
> */
> -static void drm_sched_submit_queue(struct drm_gpu_scheduler *sched)
> +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_submit);
> + queue_work(sched->submit_wq, &sched->work_run_job);
> +}
> +
> +static struct drm_sched_entity *
> +drm_sched_select_entity(struct drm_gpu_scheduler *sched, bool dequeue);
Nit: Can you drop this forward declaration and move the function here?
> +
> +/**
> + * drm_sched_run_job_queue_if_ready - queue job submission if ready
> + * @sched: scheduler instance
> + */
> +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
> + *
> + * @sched: scheduler instance to queue free job
> + */
> +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_free_job);
> +}
> +
> +/**
> + * drm_sched_free_job_queue_if_ready - queue free job if ready
> + *
> + * @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 +359,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);
> }
>
> /**
> @@ -906,18 +955,19 @@ 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_run_job_queue(sched);
> }
>
> /**
> * drm_sched_select_entity - Select next entity to process
> *
> * @sched: scheduler instance
> + * @dequeue: dequeue selected entity
> *
> * 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)
> +drm_sched_select_entity(struct drm_gpu_scheduler *sched, bool dequeue)
> {
> struct drm_sched_entity *entity;
> int i;
> @@ -936,8 +986,10 @@ drm_sched_select_entity(struct drm_gpu_scheduler *sched)
> /* 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]);
> + 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;
> }
> @@ -974,8 +1026,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;
Looks like you are changing the behavior here (unconditional ->
conditional timestamp update)? Probably something that should go in a
separate patch.
> /* start TO timer for next job */
> drm_sched_start_timeout(sched);
> }
> @@ -1025,30 +1079,44 @@ 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 (cleanup_job) {
> + sched->ops->free_job(cleanup_job);
> +
> + drm_sched_free_job_queue_if_ready(sched);
> + drm_sched_run_job_queue_if_ready(sched);
> + }
> +}
>
> - if (!entity && !cleanup_job)
> - return; /* No more work */
> +/**
> + * 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;
> + int r;
>
> - if (cleanup_job)
> - sched->ops->free_job(cleanup_job);
> + if (READ_ONCE(sched->pause_submit))
> + return;
>
> + entity = drm_sched_select_entity(sched, true);
Nit:
if (!entity)
return;
then you can save an indentation level for the rest of the function.
> if (entity) {
> struct dma_fence *fence;
> struct drm_sched_fence *s_fence;
> @@ -1057,9 +1125,7 @@ static void drm_sched_main(struct work_struct *w)
> 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;
> + return; /* No more work */
> }
>
> s_fence = sched_job->s_fence;
> @@ -1089,10 +1155,8 @@ static void drm_sched_main(struct work_struct *w)
> }
>
> wake_up(&sched->job_scheduled);
> + drm_sched_run_job_queue_if_ready(sched);
> }
> -
> -again:
> - drm_sched_submit_queue(sched);
> }
>
> /**
> @@ -1151,7 +1215,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;
> @@ -1276,7 +1341,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);
>
> @@ -1288,6 +1354,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 04eec2d7635f..fbc083a92757 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.
> @@ -518,7 +519,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;
next prev parent reply other threads:[~2023-09-12 8:08 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-12 2:16 [Intel-xe] [PATCH v3 00/13] DRM scheduler changes for Xe Matthew Brost
2023-09-12 2:16 ` [Intel-xe] [PATCH v3 01/13] drm/sched: Add drm_sched_submit_* helpers Matthew Brost
2023-09-12 2:16 ` [Intel-xe] [PATCH v3 02/13] drm/sched: Convert drm scheduler to use a work queue rather than kthread Matthew Brost
2023-09-12 7:29 ` Boris Brezillon
2023-09-12 15:02 ` Matthew Brost
2023-09-14 3:41 ` Luben Tuikov
2023-09-14 3:35 ` Luben Tuikov
2023-09-16 17:07 ` Danilo Krummrich
2023-09-12 2:16 ` [Intel-xe] [PATCH v3 03/13] drm/sched: Move schedule policy to scheduler / entity Matthew Brost
2023-09-12 7:37 ` Boris Brezillon
2023-09-12 15:14 ` Matthew Brost
2023-09-12 14:11 ` kernel test robot
2023-09-12 15:17 ` Matthew Brost
2023-09-14 4:18 ` Luben Tuikov
2023-09-14 4:23 ` Luben Tuikov
2023-09-14 15:48 ` Matthew Brost
2023-09-14 15:49 ` Matthew Brost
2023-09-12 2:16 ` [Intel-xe] [PATCH v3 04/13] drm/sched: Add DRM_SCHED_POLICY_SINGLE_ENTITY scheduling policy Matthew Brost
2023-09-13 12:30 ` kernel test robot
2023-09-12 2:16 ` [Intel-xe] [PATCH v3 05/13] drm/sched: Split free_job into own work item Matthew Brost
2023-09-12 8:08 ` Boris Brezillon [this message]
2023-09-12 14:37 ` Matthew Brost
2023-09-12 14:53 ` Boris Brezillon
2023-09-12 14:55 ` Matthew Brost
2023-09-12 2:16 ` [Intel-xe] [PATCH v3 06/13] drm/sched: Add generic scheduler message interface Matthew Brost
2023-09-12 8:23 ` Boris Brezillon
2023-09-12 14:50 ` Matthew Brost
2023-09-12 2:16 ` [Intel-xe] [PATCH v3 07/13] drm/sched: Add drm_sched_start_timeout_unlocked helper Matthew Brost
2023-09-12 2:16 ` [Intel-xe] [PATCH v3 08/13] drm/sched: Start run wq before TDR in drm_sched_start Matthew Brost
2023-09-12 2:16 ` [Intel-xe] [PATCH v3 09/13] drm/sched: Submit job before starting TDR Matthew Brost
2023-09-14 2:56 ` Luben Tuikov
2023-09-14 17:48 ` Matthew Brost
2023-09-21 3:35 ` Luben Tuikov
2023-09-12 2:16 ` [Intel-xe] [PATCH v3 10/13] drm/sched: Add helper to set TDR timeout Matthew Brost
2023-09-14 2:38 ` Luben Tuikov
2023-09-14 17:36 ` Matthew Brost
2023-09-12 2:16 ` [Intel-xe] [PATCH v3 11/13] drm/sched: Waiting for pending jobs to complete in scheduler kill Matthew Brost
2023-09-12 8:44 ` Boris Brezillon
2023-09-12 9:57 ` Christian König
2023-09-12 14:47 ` Matthew Brost
2023-09-16 17:52 ` Danilo Krummrich
2023-09-18 11:03 ` Christian König
2023-09-18 14:57 ` Danilo Krummrich
2023-09-19 5:55 ` Christian König
2023-09-12 10:28 ` Boris Brezillon
2023-09-12 14:54 ` Matthew Brost
2023-09-12 2:16 ` [Intel-xe] [PATCH v3 12/13] drm/sched/doc: Add Entity teardown documentaion Matthew Brost
2023-09-13 15:04 ` Christian König
2023-09-14 2:06 ` Luben Tuikov
2023-09-16 18:06 ` Danilo Krummrich
2023-09-12 2:16 ` [Intel-xe] [PATCH v3 13/13] drm/sched: Update maintainers of GPU scheduler Matthew Brost
2023-09-12 2:20 ` [Intel-xe] ✗ CI.Patch_applied: failure for DRM scheduler changes for Xe (rev5) Patchwork
2023-09-14 1:45 ` [Intel-xe] [PATCH v3 00/13] DRM scheduler changes for Xe Luben Tuikov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230912100833.792eacb6@collabora.com \
--to=boris.brezillon@collabora.com \
--cc=Liviu.Dudau@arm.com \
--cc=airlied@gmail.com \
--cc=christian.koenig@amd.com \
--cc=daniel@ffwll.ch \
--cc=donald.robson@imgtec.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=faith.ekstrand@collabora.com \
--cc=frank.binns@imgtec.com \
--cc=intel-xe@lists.freedesktop.org \
--cc=ketil.johnsen@arm.com \
--cc=lina@asahilina.net \
--cc=luben.tuikov@amd.com \
--cc=matthew.brost@intel.com \
--cc=mcanal@igalia.com \
--cc=robdclark@chromium.org \
--cc=sarah.walker@imgtec.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox