[RFC] drm/scheduler: Remove mention of TDR from scheduler API
Christian König
christian.koenig at amd.com
Wed Feb 5 12:44:48 UTC 2025
Am 05.02.25 um 12:14 schrieb Tvrtko Ursulin:
> Christian suggests scheduler should not use the term TDR because it only
> can do basic timeout detection on it's own, not the full blown timeout-
> detection-and-recovery (TDR) as the term is generally understood.
There is even more to the term TDR on Windows than
timeout-detection-and-recovery.
For example it includes a certain signaling of errors which works
totally different on Linux.
> Attempt to rename it to a more basic drm_sched_trigger_timeout.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
> Suggested-by: Christian König <christian.koenig at amd.com>
> Cc: Christian König <christian.koenig at amd.com>
> Cc: Danilo Krummrich <dakr at redhat.com>
> Cc: Matthew Brost <matthew.brost at intel.com>
> Cc: Philipp Stanner <pstanner at redhat.com>
> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> Cc: "Thomas Hellström" <thomas.hellstrom at linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: intel-xe at lists.freedesktop.org
> ---
> While doing this I have however noticed the pre-existing drm_sched_fault
> API sitting right below drm_sched_tdr_queue_imm() added by
> 3c6c7ca4508b ("drm/sched: Add a helper to queue TDR immediately").
>
> It does not appear to be documented in the kernel doc why is the
> newer API setting sched->timeout permanently to zero, or why are
> both needed and what are the considerations to use one versus the
> other. Perhaps Matt can clarify as the author of the newer API.
Oh, good point. I wasn't aware of that duplication.
Regards,
Christian.
> ---
> drivers/gpu/drm/scheduler/sched_main.c | 32 ++++++++++++++------------
> drivers/gpu/drm/xe/xe_gpu_scheduler.h | 4 ++--
> drivers/gpu/drm/xe/xe_guc_submit.c | 8 +++----
> include/drm/gpu_scheduler.h | 8 +++----
> 4 files changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index a48be16ab84f..b01792fe6141 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -433,7 +433,8 @@ static void drm_sched_start_timeout(struct drm_gpu_scheduler *sched)
>
> if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
> !list_empty(&sched->pending_list))
> - mod_delayed_work(sched->timeout_wq, &sched->work_tdr, sched->timeout);
> + mod_delayed_work(sched->timeout_wq, &sched->work_timeout,
> + sched->timeout);
> }
>
> static void drm_sched_start_timeout_unlocked(struct drm_gpu_scheduler *sched)
> @@ -444,20 +445,20 @@ static void drm_sched_start_timeout_unlocked(struct drm_gpu_scheduler *sched)
> }
>
> /**
> - * drm_sched_tdr_queue_imm: - immediately start job timeout handler
> + * drm_sched_trigger_timeout: - immediately start job timeout handler
> *
> * @sched: scheduler for which the timeout handling should be started.
> *
> * Start timeout handling immediately for the named scheduler.
> */
> -void drm_sched_tdr_queue_imm(struct drm_gpu_scheduler *sched)
> +void drm_sched_trigger_timeout(struct drm_gpu_scheduler *sched)
> {
> spin_lock(&sched->job_list_lock);
> sched->timeout = 0;
> drm_sched_start_timeout(sched);
> spin_unlock(&sched->job_list_lock);
> }
> -EXPORT_SYMBOL(drm_sched_tdr_queue_imm);
> +EXPORT_SYMBOL(drm_sched_trigger_timeout);
>
> /**
> * drm_sched_fault - immediately start timeout handler
> @@ -469,7 +470,7 @@ EXPORT_SYMBOL(drm_sched_tdr_queue_imm);
> void drm_sched_fault(struct drm_gpu_scheduler *sched)
> {
> if (sched->timeout_wq)
> - mod_delayed_work(sched->timeout_wq, &sched->work_tdr, 0);
> + mod_delayed_work(sched->timeout_wq, &sched->work_timeout, 0);
> }
> EXPORT_SYMBOL(drm_sched_fault);
>
> @@ -489,14 +490,15 @@ unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler *sched)
> {
> unsigned long sched_timeout, now = jiffies;
>
> - sched_timeout = sched->work_tdr.timer.expires;
> + sched_timeout = sched->work_timeout.timer.expires;
>
> /*
> * Modify the timeout to an arbitrarily large value. This also prevents
> * the timeout to be restarted when new submissions arrive
> */
> - if (mod_delayed_work(sched->timeout_wq, &sched->work_tdr, MAX_SCHEDULE_TIMEOUT)
> - && time_after(sched_timeout, now))
> + if (mod_delayed_work(sched->timeout_wq, &sched->work_timeout,
> + MAX_SCHEDULE_TIMEOUT) &&
> + time_after(sched_timeout, now))
> return sched_timeout - now;
> else
> return sched->timeout;
> @@ -517,9 +519,9 @@ void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
> spin_lock(&sched->job_list_lock);
>
> if (list_empty(&sched->pending_list))
> - cancel_delayed_work(&sched->work_tdr);
> + cancel_delayed_work(&sched->work_timeout);
> else
> - mod_delayed_work(sched->timeout_wq, &sched->work_tdr, remaining);
> + mod_delayed_work(sched->timeout_wq, &sched->work_timeout, remaining);
>
> spin_unlock(&sched->job_list_lock);
> }
> @@ -541,7 +543,7 @@ static void drm_sched_job_timedout(struct work_struct *work)
> struct drm_sched_job *job;
> enum drm_gpu_sched_stat status = DRM_GPU_SCHED_STAT_NOMINAL;
>
> - sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
> + sched = container_of(work, struct drm_gpu_scheduler, work_timeout.work);
>
> /* Protects against concurrent deletion in drm_sched_get_finished_job */
> spin_lock(&sched->job_list_lock);
> @@ -659,7 +661,7 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
> * this TDR finished and before the newly restarted jobs had a
> * chance to complete.
> */
> - cancel_delayed_work(&sched->work_tdr);
> + cancel_delayed_work(&sched->work_timeout);
> }
> EXPORT_SYMBOL(drm_sched_stop);
>
> @@ -1107,7 +1109,7 @@ drm_sched_get_finished_job(struct drm_gpu_scheduler *sched)
> list_del_init(&job->list);
>
> /* cancel this job's TO timer */
> - cancel_delayed_work(&sched->work_tdr);
> + cancel_delayed_work(&sched->work_timeout);
> /* make the scheduled timestamp more accurate */
> next = list_first_entry_or_null(&sched->pending_list,
> typeof(*next), list);
> @@ -1325,7 +1327,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
> INIT_LIST_HEAD(&sched->pending_list);
> spin_lock_init(&sched->job_list_lock);
> atomic_set(&sched->credit_count, 0);
> - INIT_DELAYED_WORK(&sched->work_tdr, drm_sched_job_timedout);
> + INIT_DELAYED_WORK(&sched->work_timeout, drm_sched_job_timedout);
> 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);
> @@ -1395,7 +1397,7 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched)
> wake_up_all(&sched->job_scheduled);
>
> /* Confirm no work left behind accessing device structures */
> - cancel_delayed_work_sync(&sched->work_tdr);
> + cancel_delayed_work_sync(&sched->work_timeout);
>
> if (sched->own_submit_wq)
> destroy_workqueue(sched->submit_wq);
> diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.h b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> index c250ea773491..3fd728b1bfd6 100644
> --- a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> @@ -44,9 +44,9 @@ static inline void xe_sched_stop(struct xe_gpu_scheduler *sched)
> drm_sched_stop(&sched->base, NULL);
> }
>
> -static inline void xe_sched_tdr_queue_imm(struct xe_gpu_scheduler *sched)
> +static inline void xe_sched_trigger_timeout(struct xe_gpu_scheduler *sched)
> {
> - drm_sched_tdr_queue_imm(&sched->base);
> + drm_sched_trigger_timeout(&sched->base);
> }
>
> static inline void xe_sched_resubmit_jobs(struct xe_gpu_scheduler *sched)
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 913c74d6e2ae..968709fd6db4 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -778,7 +778,7 @@ static void disable_scheduling_deregister(struct xe_guc *guc,
> xe_gt_warn(q->gt, "Pending enable/disable failed to respond\n");
> xe_sched_submission_start(sched);
> xe_gt_reset_async(q->gt);
> - xe_sched_tdr_queue_imm(sched);
> + xe_sched_trigger_timeout(sched);
> return;
> }
>
> @@ -807,7 +807,7 @@ static void xe_guc_exec_queue_trigger_cleanup(struct xe_exec_queue *q)
> if (xe_exec_queue_is_lr(q))
> queue_work(guc_to_gt(guc)->ordered_wq, &q->guc->lr_tdr);
> else
> - xe_sched_tdr_queue_imm(&q->guc->sched);
> + xe_sched_trigger_timeout(&q->guc->sched);
> }
>
> /**
> @@ -986,7 +986,7 @@ static void enable_scheduling(struct xe_exec_queue *q)
> xe_gt_warn(guc_to_gt(guc), "Schedule enable failed to respond");
> set_exec_queue_banned(q);
> xe_gt_reset_async(q->gt);
> - xe_sched_tdr_queue_imm(&q->guc->sched);
> + xe_sched_trigger_timeout(&q->guc->sched);
> }
> }
>
> @@ -1144,7 +1144,7 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> xe_exec_queue_get(q); /* GT reset owns this */
> set_exec_queue_banned(q);
> xe_gt_reset_async(q->gt);
> - xe_sched_tdr_queue_imm(sched);
> + xe_sched_trigger_timeout(sched);
> goto rearm;
> }
> }
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index cf88f2bd020f..666968cf505d 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -494,10 +494,10 @@ struct drm_sched_backend_ops {
> * finished.
> * @job_id_count: used to assign unique id to the each job.
> * @submit_wq: workqueue used to queue @work_run_job and @work_free_job
> - * @timeout_wq: workqueue used to queue @work_tdr
> + * @timeout_wq: workqueue used to queue @work_timeout
> * @work_run_job: work which calls run_job op of each scheduler.
> * @work_free_job: work which calls free_job op of each scheduler.
> - * @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the
> + * @work_timeout: 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.
> * @job_list_lock: lock to protect the pending_list.
> @@ -527,7 +527,7 @@ struct drm_gpu_scheduler {
> struct workqueue_struct *timeout_wq;
> struct work_struct work_run_job;
> struct work_struct work_free_job;
> - struct delayed_work work_tdr;
> + struct delayed_work work_timeout;
> struct list_head pending_list;
> spinlock_t job_list_lock;
> int hang_limit;
> @@ -571,7 +571,7 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
> struct drm_gpu_scheduler **sched_list,
> unsigned int num_sched_list);
>
> -void drm_sched_tdr_queue_imm(struct drm_gpu_scheduler *sched);
> +void drm_sched_trigger_timeout(struct drm_gpu_scheduler *sched);
> void drm_sched_job_cleanup(struct drm_sched_job *job);
> void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
> bool drm_sched_wqueue_ready(struct drm_gpu_scheduler *sched);
More information about the dri-devel
mailing list