Attachment 143882 Details for Bug 109692 – 0001-drm-scheduler-rework-job-destruction.patch

[patch] 0001-drm-scheduler-rework-job-destruction.patch

0001-drm-scheduler-rework-job-destruction.patch (text/plain), 10.30 KB, created by Andrey Grodzovsky on 2019-04-05 20:41:52 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Andrey Grodzovsky

Created: 2019-04-05 20:41:52 UTC

Size: 10.30 KB

patch

obsolete

>From acdcbdb624f1adfa17be0b946bf736558571d814 Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Christian=20K=C3=B6nig?= <christian.koenig@amd.com>
>Date: Thu, 21 Mar 2019 13:08:07 +0100
>Subject: drm/scheduler: rework job destruction
>MIME-Version: 1.0
>Content-Type: text/plain; charset=UTF-8
>Content-Transfer-Encoding: 8bit
>
>We now destroy finished jobs from the worker thread to make sure that
>we never destroy a job currently in timeout processing.
>
>Signed-off-by: Christian KÃ¶nig <christian.koenig@amd.com>
>Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>---
> drivers/gpu/drm/scheduler/sched_main.c | 140 +++++++++++++++++++--------------
> include/drm/gpu_scheduler.h            |   6 +-
> 2 files changed, 82 insertions(+), 64 deletions(-)
>
>diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>index 19fc601..c215cde 100644
>--- a/drivers/gpu/drm/scheduler/sched_main.c
>+++ b/drivers/gpu/drm/scheduler/sched_main.c
>@@ -191,6 +191,8 @@ EXPORT_SYMBOL(drm_sched_dependency_optimized);
>  */
> static void drm_sched_start_timeout(struct drm_gpu_scheduler *sched)
> {
>+	unsigned long flags;
>+
> 	if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
> 	    !list_empty(&sched->ring_mirror_list))
> 		schedule_delayed_work(&sched->work_tdr, sched->timeout);
>@@ -265,32 +267,6 @@ void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
> }
> EXPORT_SYMBOL(drm_sched_resume_timeout);
> 
>-/* job_finish is called after hw fence signaled
>- */
>-static void drm_sched_job_finish(struct work_struct *work)
>-{
>-	struct drm_sched_job *s_job = container_of(work, struct drm_sched_job,
>-						   finish_work);
>-	struct drm_gpu_scheduler *sched = s_job->sched;
>-	unsigned long flags;
>-
>-	/*
>-	 * Canceling the timeout without removing our job from the ring mirror
>-	 * list is safe, as we will only end up in this worker if our jobs
>-	 * finished fence has been signaled. So even if some another worker
>-	 * manages to find this job as the next job in the list, the fence
>-	 * signaled check below will prevent the timeout to be restarted.
>-	 */
>-	cancel_delayed_work_sync(&sched->work_tdr);
>-
>-	spin_lock_irqsave(&sched->job_list_lock, flags);
>-	/* queue TDR for next job */
>-	drm_sched_start_timeout(sched);
>-	spin_unlock_irqrestore(&sched->job_list_lock, flags);
>-
>-	sched->ops->free_job(s_job);
>-}
>-
> static void drm_sched_job_begin(struct drm_sched_job *s_job)
> {
> 	struct drm_gpu_scheduler *sched = s_job->sched;
>@@ -371,23 +347,26 @@ EXPORT_SYMBOL(drm_sched_increase_karma);
>  * @sched: scheduler instance
>  * @bad: bad scheduler job
>  *
>+ * Stop the scheduler and also removes and frees all completed jobs.
>+ * Note: bad job will not be freed as it might be used later and so it's
>+ * callers responsibility to release it manually if it's not part of the
>+ * mirror list any more.
>+ *
>  */
>-void drm_sched_stop(struct drm_gpu_scheduler *sched)
>+void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
> {
>-	struct drm_sched_job *s_job;
>+	struct drm_sched_job *s_job, *tmp;
> 	unsigned long flags;
>-	struct dma_fence *last_fence =  NULL;
> 
> 	kthread_park(sched->thread);
> 
> 	/*
>-	 * Verify all the signaled jobs in mirror list are removed from the ring
>-	 * by waiting for the latest job to enter the list. This should insure that
>-	 * also all the previous jobs that were in flight also already singaled
>-	 * and removed from the list.
>+	 * Iterate the job list from later to  earlier one and either deactive
>+	 * their HW callbacks or remove them from mirror list if they already
>+	 * signaled.
>+	 * This iteration is thread safe as sched thread is stopped.
> 	 */
>-	spin_lock_irqsave(&sched->job_list_lock, flags);
>-	list_for_each_entry_reverse(s_job, &sched->ring_mirror_list, node) {
>+	list_for_each_entry_safe_reverse(s_job, tmp, &sched->ring_mirror_list, node) {
> 		if (s_job->s_fence->parent &&
> 		    dma_fence_remove_callback(s_job->s_fence->parent,
> 					      &s_job->cb)) {
>@@ -395,16 +374,30 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched)
> 			s_job->s_fence->parent = NULL;
> 			atomic_dec(&sched->hw_rq_count);
> 		} else {
>-			 last_fence = dma_fence_get(&s_job->s_fence->finished);
>-			 break;
>+			/*
>+			 * remove job from ring_mirror_list.
>+			 * Locking here is for concurrent resume timeout
>+			 */
>+			spin_lock_irqsave(&sched->job_list_lock, flags);
>+			list_del_init(&s_job->node);
>+			spin_unlock_irqrestore(&sched->job_list_lock, flags);
>+
>+			/*
>+			 * Wait for job's HW fence callback to finish using s_job
>+			 * before releasing it.
>+			 *
>+			 * Job is still alive so fence refcount at least 1
>+			 */
>+			dma_fence_wait(&s_job->s_fence->finished, false);
>+
>+			/*
>+			 * We must keep bad job alive for later use during
>+			 * recovery by some of the drivers
>+			 */
>+			if (bad != s_job)
>+				sched->ops->free_job(s_job);
> 		}
> 	}
>-	spin_unlock_irqrestore(&sched->job_list_lock, flags);
>-
>-	if (last_fence) {
>-		dma_fence_wait(last_fence, false);
>-		dma_fence_put(last_fence);
>-	}
> }
> 
> EXPORT_SYMBOL(drm_sched_stop);
>@@ -418,6 +411,7 @@ EXPORT_SYMBOL(drm_sched_stop);
> void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
> {
> 	struct drm_sched_job *s_job, *tmp;
>+	unsigned long flags;
> 	int r;
> 
> 	if (!full_recovery)
>@@ -425,9 +419,7 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
> 
> 	/*
> 	 * Locking the list is not required here as the sched thread is parked
>-	 * so no new jobs are being pushed in to HW and in drm_sched_stop we
>-	 * flushed all the jobs who were still in mirror list but who already
>-	 * signaled and removed them self from the list. Also concurrent
>+	 * so no new jobs are being inserted or removed. Also concurrent
> 	 * GPU recovers can't run in parallel.
> 	 */
> 	list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) {
>@@ -445,7 +437,9 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
> 			drm_sched_process_job(NULL, &s_job->cb);
> 	}
> 
>+	spin_lock_irqsave(&sched->job_list_lock, flags);
> 	drm_sched_start_timeout(sched);
>+	spin_unlock_irqrestore(&sched->job_list_lock, flags);
> 
> unpark:
> 	kthread_unpark(sched->thread);
>@@ -464,7 +458,6 @@ void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)
> 	uint64_t guilty_context;
> 	bool found_guilty = false;
> 
>-	/*TODO DO we need spinlock here ? */
> 	list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) {
> 		struct drm_sched_fence *s_fence = s_job->s_fence;
> 
>@@ -514,7 +507,6 @@ int drm_sched_job_init(struct drm_sched_job *job,
> 		return -ENOMEM;
> 	job->id = atomic64_inc_return(&sched->job_id_count);
> 
>-	INIT_WORK(&job->finish_work, drm_sched_job_finish);
> 	INIT_LIST_HEAD(&job->node);
> 
> 	return 0;
>@@ -597,24 +589,53 @@ static void drm_sched_process_job(struct dma_fence *f, struct dma_fence_cb *cb)
> 	struct drm_sched_job *s_job = container_of(cb, struct drm_sched_job, cb);
> 	struct drm_sched_fence *s_fence = s_job->s_fence;
> 	struct drm_gpu_scheduler *sched = s_fence->sched;
>-	unsigned long flags;
>-
>-	cancel_delayed_work(&sched->work_tdr);
> 
> 	atomic_dec(&sched->hw_rq_count);
> 	atomic_dec(&sched->num_jobs);
> 
>-	spin_lock_irqsave(&sched->job_list_lock, flags);
>-	/* remove job from ring_mirror_list */
>-	list_del_init(&s_job->node);
>-	spin_unlock_irqrestore(&sched->job_list_lock, flags);
>+	trace_drm_sched_process_job(s_fence);
> 
> 	drm_sched_fence_finished(s_fence);
>-
>-	trace_drm_sched_process_job(s_fence);
> 	wake_up_interruptible(&sched->wake_up_worker);
>+}
>+
>+/**
>+ * drm_sched_cleanup_jobs - destroy finished jobs
>+ *
>+ * @sched: scheduler instance
>+ *
>+ * Remove all finished jobs from the mirror list and destroy them.
>+ */
>+static void drm_sched_cleanup_jobs(struct drm_gpu_scheduler *sched)
>+{
>+	unsigned long flags;
>+
>+	/* Don't destroy jobs while the timeout worker is running */
>+	if (!cancel_delayed_work(&sched->work_tdr))
>+		return;
>+
>+
>+	while (!list_empty(&sched->ring_mirror_list)) {
>+		struct drm_sched_job *job;
>+
>+		job = list_first_entry(&sched->ring_mirror_list,
>+				       struct drm_sched_job, node);
>+		if (!dma_fence_is_signaled(&job->s_fence->finished))
>+			break;
>+
>+		spin_lock_irqsave(&sched->job_list_lock, flags);
>+		/* remove job from ring_mirror_list */
>+		list_del_init(&job->node);
>+		spin_unlock_irqrestore(&sched->job_list_lock, flags);
>+
>+		sched->ops->free_job(job);
>+	}
>+
>+	/* queue timeout for next job */
>+	spin_lock_irqsave(&sched->job_list_lock, flags);
>+	drm_sched_start_timeout(sched);
>+	spin_unlock_irqrestore(&sched->job_list_lock, flags);
> 
>-	schedule_work(&s_job->finish_work);
> }
> 
> /**
>@@ -656,9 +677,10 @@ static int drm_sched_main(void *param)
> 		struct dma_fence *fence;
> 
> 		wait_event_interruptible(sched->wake_up_worker,
>+					 (drm_sched_cleanup_jobs(sched),
> 					 (!drm_sched_blocked(sched) &&
> 					  (entity = drm_sched_select_entity(sched))) ||
>-					 kthread_should_stop());
>+					 kthread_should_stop()));
> 
> 		if (!entity)
> 			continue;
>diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>index 0daca4d..9ee0f27 100644
>--- a/include/drm/gpu_scheduler.h
>+++ b/include/drm/gpu_scheduler.h
>@@ -167,9 +167,6 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f);
>  * @sched: the scheduler instance on which this job is scheduled.
>  * @s_fence: contains the fences for the scheduling of job.
>  * @finish_cb: the callback for the finished fence.
>- * @finish_work: schedules the function @drm_sched_job_finish once the job has
>- *               finished to remove the job from the
>- *               @drm_gpu_scheduler.ring_mirror_list.
>  * @node: used to append this struct to the @drm_gpu_scheduler.ring_mirror_list.
>  * @id: a unique id assigned to each job scheduled on the scheduler.
>  * @karma: increment on every hang caused by this job. If this exceeds the hang
>@@ -188,7 +185,6 @@ struct drm_sched_job {
> 	struct drm_gpu_scheduler	*sched;
> 	struct drm_sched_fence		*s_fence;
> 	struct dma_fence_cb		finish_cb;
>-	struct work_struct		finish_work;
> 	struct list_head		node;
> 	uint64_t			id;
> 	atomic_t			karma;
>@@ -296,7 +292,7 @@ int drm_sched_job_init(struct drm_sched_job *job,
> 		       void *owner);
> void drm_sched_job_cleanup(struct drm_sched_job *job);
> void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
>-void drm_sched_stop(struct drm_gpu_scheduler *sched);
>+void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad);
> void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery);
> void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
> void drm_sched_increase_karma(struct drm_sched_job *bad);
>-- 
>2.7.4
>

Actions: View | Splinter Review

Attachments on bug 109692: 143419 | 143422 | 143423 | 143476 | 143485 | 143497 | 143882 | 143883 | 143886 | 143897 | 143898 | 143904 | 143908 | 143927 | 143928 | 144242

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.