drm/sched: Fix memory leaks with cancel_job() callback

Since its inception, the GPU scheduler can leak memory if the driver calls drm_sched_fini() while there are still jobs in flight.

The simplest way to solve this in a backwards compatible manner is by adding a new callback, drm_sched_backend_ops.cancel_job(), which instructs the driver to signal the hardware fence associated with the job. Afterwards, the scheduler can savely use the established free_job() callback for freeing the job.

Implement the new backend_ops callback cancel_job().

Suggested-by Tvrtko Ursulin <[email protected]>

Link https://lore.kernel.org/dri-devel/[email protected]/ open here

Signed-off-by Philipp Stanner <[email protected]>

---
  drivers/gpu/drm/scheduler/sched_main.c | 34 ++++++++++++++++----------
  include/drm/gpu_scheduler.h            | 18 ++++++++++++++
  2 files changed, 39 insertions(+), 13 deletions(-)

drivers/gpu/drm/scheduler/sched_main.c +0 -0

index c63543132f9d..1239954f5f7c 100644
@@ -1353,6 +1353,18 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, const struct drm_sched_init_
  }

drivers/gpu/drm/scheduler/sched_main.c +13 -13

+		sched->ops->free_job(job);
+	}
+}
+
  /**
   * drm_sched_fini - Destroy a gpu scheduler
   *

@@ -1360,19 +1372,11 @@ EXPORT_SYMBOL(drm_sched_init);
   *
   * Tears down and cleans up the scheduler.
   *
- * This stops submission of new jobs to the hardware through
- * drm_sched_backend_ops.run_job(). Consequently, drm_sched_backend_ops.free_job()
- * will not be called for all jobs still in drm_gpu_scheduler.pending_list.
- * There is no solution for this currently. Thus, it is up to the driver to make
- * sure that:
- *
- *  a) drm_sched_fini() is only called after for all submitted jobs
- *     drm_sched_backend_ops.free_job() has been called or that
- *  b) the jobs for which drm_sched_backend_ops.free_job() has not been called
- *     after drm_sched_fini() ran are freed manually.
- *
- * FIXME: Take care of the above problem and prevent this function from leaking
- * the jobs in drm_gpu_scheduler.pending_list under any circumstances.
+ * This stops submission of new jobs to the hardware through &struct
+ * drm_sched_backend_ops.run_job. If &struct drm_sched_backend_ops.cancel_job
+ * is implemented, all jobs will be canceled through it and afterwards cleaned
+ * up through &struct drm_sched_backend_ops.free_job. If cancel_job is not
+ * implemented, memory could leak.
   */
  void drm_sched_fini(struct drm_gpu_scheduler *sched)
  {

@@ -1402,6 +1406,10 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched)
  	/* Confirm no work left behind accessing device structures */
  	cancel_delayed_work_sync(&sched->work_tdr);
 
+	/* Avoid memory leaks if supported by the driver. */
+	if (sched->ops->cancel_job)
+		drm_sched_cancel_remaining_jobs(sched);
+
  	if (sched->own_submit_wq)
  		destroy_workqueue(sched->submit_wq);
  	sched->ready = false;

include/drm/gpu_scheduler.h +18 -0

index e62a7214e052..190844370f48 100644
@@ -512,6 +512,24 @@ struct drm_sched_backend_ops {
           * and it's time to clean it up.
  	 */
  	void (*free_job)(struct drm_sched_job *sched_job);
+
+	/**
+	 * @cancel_job: Used by the scheduler to guarantee remaining jobs' fences
+	 * get signaled in drm_sched_fini().
+	 *
+	 * Used by the scheduler to cancel all jobs that have not been executed
+	 * with &struct drm_sched_backend_ops.run_job by the time
+	 * drm_sched_fini() gets invoked.
+	 *
+	 * Drivers need to signal the passed job's hardware fence with an
+	 * appropriate error code (e.g., -ECANCELED) in this callback. They
+	 * must not free the job.
+	 *
+	 * The scheduler will only call this callback once it stopped calling
+	 * all other callbacks forever, with the exception of &struct
+	 * drm_sched_backend_ops.free_job.
+	 */
+	void (*cancel_job)(struct drm_sched_job *sched_job);
  };
 
  /**

Since its inception, the GPU scheduler can leak memory if the driver calls drm_sched_fini() while there are still jobs in flight.

The simplest way to solve this in a backwards compatible manner is by adding a new callback, drm_sched_backend_ops.cancel_job(), which instructs the driver to signal the hardware fence associated with the job. Afterwards, the scheduler can savely use the established free_job()

callback for freeing the job.

Implement the new backend_ops callback cancel_job().

Suggested-by Tvrtko Ursulin <[email protected]>

Link https://lore.kernel.org/dri-devel/[email protected]/ open here

Signed-off-by Philipp Stanner <[email protected]>

The GPU Scheduler now supports a new callback, cancel_job(), which lets the scheduler cancel all jobs which might not yet be freed when drm_sched_fini() runs. Using this callback allows for significantly simplifying the mock scheduler teardown code.

Implement the cancel_job() callback and adjust the code where necessary.

Signed-off-by Philipp Stanner <[email protected]>

---
  .../gpu/drm/scheduler/tests/mock_scheduler.c  | 66 +++++++------------
  1 file changed, 23 insertions(+), 43 deletions(-)

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +9 -9

index 49d067fecd67..2d3169d95200 100644
@@ -63,7 +63,7 @@ static void drm_mock_sched_job_complete(struct drm_mock_sched_job *job)
  	lockdep_assert_held(&sched->lock);
 
  	job->flags |= DRM_MOCK_SCHED_JOB_DONE;
-	list_move_tail(&job->link, &sched->done_list);
+	list_del(&job->link);
  	dma_fence_signal_locked(&job->hw_fence);
  	complete(&job->done);
  }

@@ -236,26 +236,39 @@ mock_sched_timedout_job(struct drm_sched_job *sched_job)
 
  static void mock_sched_free_job(struct drm_sched_job *sched_job)
  {
-	struct drm_mock_scheduler *sched =
-			drm_sched_to_mock_sched(sched_job->sched);
  	struct drm_mock_sched_job *job = drm_sched_job_to_mock_job(sched_job);
-	unsigned long flags;
 
-	/* Remove from the scheduler done list. */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_del(&job->link);
-	spin_unlock_irqrestore(&sched->lock, flags);
  	dma_fence_put(&job->hw_fence);
-
  	drm_sched_job_cleanup(sched_job);
 
  	/* Mock job itself is freed by the kunit framework. */
  }
 
+static void mock_sched_cancel_job(struct drm_sched_job *sched_job)
+{
+	struct drm_mock_scheduler *sched = drm_sched_to_mock_sched(sched_job->sched);
+	struct drm_mock_sched_job *job = drm_sched_job_to_mock_job(sched_job);
+	unsigned long flags;
+
+	hrtimer_cancel(&job->timer);
+

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +4 -34

+}
+
  static const struct drm_sched_backend_ops drm_mock_scheduler_ops = {
  	.run_job = mock_sched_run_job,
  	.timedout_job = mock_sched_timedout_job,
-	.free_job = mock_sched_free_job
+	.free_job = mock_sched_free_job,
+	.cancel_job = mock_sched_cancel_job,
  };
 
  /**

@@ -289,7 +302,6 @@ struct drm_mock_scheduler *drm_mock_sched_new(struct kunit *test, long timeout)
  	sched->hw_timeline.context = dma_fence_context_alloc(1);
  	atomic_set(&sched->hw_timeline.next_seqno, 0);
  	INIT_LIST_HEAD(&sched->job_list);
-	INIT_LIST_HEAD(&sched->done_list);
  	spin_lock_init(&sched->lock);
 
  	return sched;

@@ -304,38 +316,6 @@ struct drm_mock_scheduler *drm_mock_sched_new(struct kunit *test, long timeout)
   */
  void drm_mock_sched_fini(struct drm_mock_scheduler *sched)
  {
-	struct drm_mock_sched_job *job, *next;
-	unsigned long flags;
-	LIST_HEAD(list);
-
-	drm_sched_wqueue_stop(&sched->base);
-
-	/* Force complete all unfinished jobs. */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &sched->job_list, link)
-		list_move_tail(&job->link, &list);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	list_for_each_entry(job, &list, link)
-		hrtimer_cancel(&job->timer);
-
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &list, link)
-		drm_mock_sched_job_complete(job);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	/*
-	 * Free completed jobs and jobs not yet processed by the DRM scheduler
-	 * free worker.
-	 */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &sched->done_list, link)
-		list_move_tail(&job->link, &list);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	list_for_each_entry_safe(job, next, &list, link)
-		mock_sched_free_job(&job->base);
-
  	drm_sched_fini(&sched->base);
  }
 

Quoted 8 lines · Philipp Stanner

The GPU Scheduler now supports a new callback, cancel_job(), which lets
the scheduler cancel all jobs which might not yet be freed when drm_sched_fini() runs. Using this callback allows for significantly simplifying the mock scheduler teardown code.

Implement the cancel_job() callback and adjust the code where necessary.

Cross referencing against my version I think you missed this hunk:

I also had this:

drivers/gpu/drm/scheduler/tests/sched_tests.h +2 -1

@@ -97,7 +96,8 @@ struct drm_mock_sched_job {
   	struct completion	done;
 
   #define DRM_MOCK_SCHED_JOB_DONE		0x1
-#define DRM_MOCK_SCHED_JOB_TIMEDOUT	0x2
+#define DRM_MOCK_SCHED_JOB_CANCELED	0x2
+#define DRM_MOCK_SCHED_JOB_TIMEDOUT	0x4
 

And was setting it in the callback. And since we should add a test to explicitly cover the new callback, and just the callback, that could make it very easy to do it.

Quoted 64 lines

Signed-off-by Philipp Stanner <[email protected]>

---
  .../gpu/drm/scheduler/tests/mock_scheduler.c  | 66 +++++++-------
-----

  1 file changed, 23 insertions(+), 43 deletions(-)

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -0

index 49d067fecd67..2d3169d95200 100644
@@ -63,7 +63,7 @@ static void drm_mock_sched_job_complete(struct
 drm_mock_sched_job *job)
   	lockdep_assert_held(&sched->lock);

   job->flags |= DRM_MOCK_SCHED_JOB_DONE;

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +1 -1

-	list_move_tail(&job->link, &sched->done_list);
+	list_del(&job->link);

   dma_fence_signal_locked(&job->hw_fence);
   complete(&job->done);
  }

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -0

@@ -236,26 +236,39 @@ mock_sched_timedout_job(struct drm_sched_job
 *sched_job)

  static void mock_sched_free_job(struct drm_sched_job *sched_job) {

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -2

-	struct drm_mock_scheduler *sched =
-			drm_sched_to_mock_sched(sched_job->sched);

   struct drm_mock_sched_job *job =
drm_sched_job_to_mock_job(sched_job);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -1

-	unsigned long flags;

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -4

-	/* Remove from the scheduler done list. */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_del(&job->link);
-	spin_unlock_irqrestore(&sched->lock, flags);

   dma_fence_put(&job->hw_fence);
-
   drm_sched_job_cleanup(sched_job);

   /* Mock job itself is freed by the kunit framework. */ }

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +3 -0

+static void mock_sched_cancel_job(struct drm_sched_job *sched_job)
+{
+	struct drm_mock_scheduler *sched =

drm_sched_to_mock_sched(sched_job->sched);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +1 -0

+	struct drm_mock_sched_job *job =

drm_sched_job_to_mock_job(sched_job);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +13 -0

+	unsigned long flags;
+
+	hrtimer_cancel(&job->timer);
+
+	spin_lock_irqsave(&sched->lock, flags);
+	if (!dma_fence_is_signaled_locked(&job->hw_fence)) {
+		list_del(&job->link);
+		dma_fence_set_error(&job->hw_fence, -ECANCELED);
+		dma_fence_signal_locked(&job->hw_fence);
+	}
+	spin_unlock_irqrestore(&sched->lock, flags);
+
+	/* The GPU Scheduler will call

drm_sched_backend_ops.free_job(), still.

drivers/gpu/drm/scheduler/tests/mock_scheduler.c

Quoted 1 line

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +1 -0

+	 * Mock job itself is freed by the kunit framework. */

/*
  * Multiline comment style to stay consistent, at least in this file.
  */

The rest looks good, but I need to revisit the timeout/free handling since it has been a while and you changed it recently.

Regards,

Tvrtko

Quoted 64 lines

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +2 -0

+}
+

  static const struct drm_sched_backend_ops drm_mock_scheduler_ops = {
   .run_job = mock_sched_run_job,
   .timedout_job = mock_sched_timedout_job,

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +2 -1

-	.free_job = mock_sched_free_job
+	.free_job = mock_sched_free_job,
+	.cancel_job = mock_sched_cancel_job,

  };

  /**

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -1

@@ -289,7 +302,6 @@ struct drm_mock_scheduler
 *drm_mock_sched_new(struct kunit *test, long timeout)
   	sched->hw_timeline.context = dma_fence_context_alloc(1);
   	atomic_set(&sched->hw_timeline.next_seqno, 0);
   	INIT_LIST_HEAD(&sched->job_list);
-	INIT_LIST_HEAD(&sched->done_list);
   	spin_lock_init(&sched->lock);

   return sched;

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -22

@@ -304,38 +316,6 @@ struct drm_mock_scheduler
 *drm_mock_sched_new(struct kunit *test, long timeout)
    */
   void drm_mock_sched_fini(struct drm_mock_scheduler *sched)
   {
-	struct drm_mock_sched_job *job, *next;
-	unsigned long flags;
-	LIST_HEAD(list);
-
-	drm_sched_wqueue_stop(&sched->base);
-
-	/* Force complete all unfinished jobs. */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &sched->job_list,
 link)
-		list_move_tail(&job->link, &list);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	list_for_each_entry(job, &list, link)
-		hrtimer_cancel(&job->timer);
-
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &list, link)
-		drm_mock_sched_job_complete(job);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	/*
-	 * Free completed jobs and jobs not yet processed by the

DRM scheduler

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -4

-	 * free worker.
-	 */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &sched->done_list,

link)

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -6

-		list_move_tail(&job->link, &list);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	list_for_each_entry_safe(job, next, &list, link)
-		mock_sched_free_job(&job->base);
-

   drm_sched_fini(&sched->base);
  }

Quoted 17 lines · Tvrtko Ursulin

Quoted 8 lines · Philipp Stanner

The GPU Scheduler now supports a new callback, cancel_job(), which lets
the scheduler cancel all jobs which might not yet be freed when drm_sched_fini() runs. Using this callback allows for significantly simplifying the mock scheduler teardown code.

Implement the cancel_job() callback and adjust the code where necessary.

Cross referencing against my version I think you missed this hunk:

drivers/gpu/drm/scheduler/tests/sched_tests.h struct drm_mock_scheduler { · line 54

Quoted 7 lines

drivers/gpu/drm/scheduler/tests/sched_tests.h +0 -1

@@ -49,7 +49,6 @@ struct drm_mock_scheduler {
 
   	spinlock_t		lock;
   	struct list_head	job_list;
-	struct list_head	done_list;
 
   	struct {
   		u64		context;

Right, overlooked that one.

Quoted 11 lines

I also had this:

drivers/gpu/drm/scheduler/tests/sched_tests.h +2 -1

@@ -97,7 +96,8 @@ struct drm_mock_sched_job {
   	struct completion	done;
 
   #define DRM_MOCK_SCHED_JOB_DONE		0x1
-#define DRM_MOCK_SCHED_JOB_TIMEDOUT	0x2
+#define DRM_MOCK_SCHED_JOB_CANCELED	0x2
+#define DRM_MOCK_SCHED_JOB_TIMEDOUT	0x4
 

Quoted 3 lines

And was setting it in the callback. And since we should add a test to explicitly cover the new callback, and just the callback, that could make it very easy to do it.

What do you imagine that to look like? The scheduler only invokes the callback on tear down.

We also don't have tests that only test free_job() and the like, do we?

You cannot test a callback for the scheduler, because the callback is implemented in the driver.

Callbacks are tested by using the scheduler. In this case, it's tested the intended way by the unit tests invoking drm_sched_free().

Quoted 142 lines

Quoted 63 lines

Signed-off-by Philipp Stanner <[email protected]>

---
   .../gpu/drm/scheduler/tests/mock_scheduler.c  | 66 +++++++-------
-----
   1 file changed, 23 insertions(+), 43 deletions(-)

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +1 -1

index 49d067fecd67..2d3169d95200 100644
@@ -63,7 +63,7 @@ static void drm_mock_sched_job_complete(struct
 drm_mock_sched_job *job)
   	lockdep_assert_held(&sched->lock);
 
   	job->flags |= DRM_MOCK_SCHED_JOB_DONE;
-	list_move_tail(&job->link, &sched->done_list);
+	list_del(&job->link);
   	dma_fence_signal_locked(&job->hw_fence);
   	complete(&job->done);

  }

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +17 -8

@@ -236,26 +236,39 @@ mock_sched_timedout_job(struct drm_sched_job
 *sched_job)
 
   static void mock_sched_free_job(struct drm_sched_job *sched_job)
   {
-	struct drm_mock_scheduler *sched =
-			drm_sched_to_mock_sched(sched_job->sched);
   	struct drm_mock_sched_job *job =
 drm_sched_job_to_mock_job(sched_job);
-	unsigned long flags;
 
-	/* Remove from the scheduler done list. */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_del(&job->link);
-	spin_unlock_irqrestore(&sched->lock, flags);
   	dma_fence_put(&job->hw_fence);
-
   	drm_sched_job_cleanup(sched_job);
 
   	/* Mock job itself is freed by the kunit framework. */
   }
 
+static void mock_sched_cancel_job(struct drm_sched_job *sched_job)
+{
+	struct drm_mock_scheduler *sched =
 drm_sched_to_mock_sched(sched_job->sched);
+	struct drm_mock_sched_job *job =
 drm_sched_job_to_mock_job(sched_job);
+	unsigned long flags;
+
+	hrtimer_cancel(&job->timer);
+
+	spin_lock_irqsave(&sched->lock, flags);
+	if (!dma_fence_is_signaled_locked(&job->hw_fence)) {
+		list_del(&job->link);
+		dma_fence_set_error(&job->hw_fence, -ECANCELED);
+		dma_fence_signal_locked(&job->hw_fence);
+	}
+	spin_unlock_irqrestore(&sched->lock, flags);
+
+	/* The GPU Scheduler will call

drm_sched_backend_ops.free_job(), still.

drivers/gpu/drm/scheduler/tests/mock_scheduler.c

Quoted 1 line

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +1 -0

+	 * Mock job itself is freed by the kunit framework. */

/*
  * Multiline comment style to stay consistent, at least in this
file.
  */

The rest looks good, but I need to revisit the timeout/free handling since it has been a while and you changed it recently.

Regards,

Tvrtko

Quoted 64 lines

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +2 -0

+}
+
   static const struct drm_sched_backend_ops drm_mock_scheduler_ops

= {
   .run_job = mock_sched_run_job,
   .timedout_job = mock_sched_timedout_job,

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +2 -2

-	.free_job = mock_sched_free_job
+	.free_job = mock_sched_free_job,
+	.cancel_job = mock_sched_cancel_job,
   };
 
   /**

@@ -289,7 +302,6 @@ struct drm_mock_scheduler
 *drm_mock_sched_new(struct kunit *test, long timeout)
   	sched->hw_timeline.context = dma_fence_context_alloc(1);
   	atomic_set(&sched->hw_timeline.next_seqno, 0);
   	INIT_LIST_HEAD(&sched->job_list);
-	INIT_LIST_HEAD(&sched->done_list);
   	spin_lock_init(&sched->lock);
 

   return sched;

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -22

@@ -304,38 +316,6 @@ struct drm_mock_scheduler
 *drm_mock_sched_new(struct kunit *test, long timeout)
    */
   void drm_mock_sched_fini(struct drm_mock_scheduler *sched)
   {
-	struct drm_mock_sched_job *job, *next;
-	unsigned long flags;
-	LIST_HEAD(list);
-
-	drm_sched_wqueue_stop(&sched->base);
-
-	/* Force complete all unfinished jobs. */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &sched->job_list,
 link)
-		list_move_tail(&job->link, &list);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	list_for_each_entry(job, &list, link)
-		hrtimer_cancel(&job->timer);
-
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &list, link)
-		drm_mock_sched_job_complete(job);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	/*
-	 * Free completed jobs and jobs not yet processed by the

DRM scheduler

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -4

-	 * free worker.
-	 */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &sched->done_list,

link)

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -6

-		list_move_tail(&job->link, &list);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	list_for_each_entry_safe(job, next, &list, link)
-		mock_sched_free_job(&job->base);
-
   	drm_sched_fini(&sched->base);
   }

Quoted 55 lines · Philipp Stanner

Quoted 20 lines · Tvrtko Ursulin

Quoted 10 lines · Philipp Stanner

The GPU Scheduler now supports a new callback, cancel_job(), which
lets
the scheduler cancel all jobs which might not yet be freed when drm_sched_fini() runs. Using this callback allows for significantly
simplifying the mock scheduler teardown code.

Implement the cancel_job() callback and adjust the code where necessary.

Cross referencing against my version I think you missed this hunk:

drivers/gpu/drm/scheduler/tests/sched_tests.h struct drm_mock_scheduler { · line 54

Quoted 7 lines

drivers/gpu/drm/scheduler/tests/sched_tests.h +0 -1

@@ -49,7 +49,6 @@ struct drm_mock_scheduler {
 
    	spinlock_t		lock;
    	struct list_head	job_list;
-	struct list_head	done_list;
 
    	struct {
    		u64		context;

Right, overlooked that one.

Quoted 11 lines

I also had this:

drivers/gpu/drm/scheduler/tests/sched_tests.h +2 -1

@@ -97,7 +96,8 @@ struct drm_mock_sched_job {
    	struct completion	done;
 
    #define DRM_MOCK_SCHED_JOB_DONE		0x1
-#define DRM_MOCK_SCHED_JOB_TIMEDOUT	0x2
+#define DRM_MOCK_SCHED_JOB_CANCELED	0x2
+#define DRM_MOCK_SCHED_JOB_TIMEDOUT	0x4
 

Quoted 5 lines

And was setting it in the callback. And since we should add a test to
explicitly cover the new callback, and just the callback, that could
make it very easy to do it.

What do you imagine that to look like? The scheduler only invokes the
callback on tear down.

We also don't have tests that only test free_job() and the like, do we?

Quoted 7 lines

You cannot test a callback for the scheduler, because the callback is
implemented in the driver.

Callbacks are tested by using the scheduler. In this case, it's tested
the intended way by the unit tests invoking drm_sched_free().

Something like (untested):

static void drm_sched_test_cleanup(struct kunit *test)

{
	struct drm_mock_sched_entity *entity;
	struct drm_mock_scheduler *sched;
	struct drm_mock_sched_job job;
	bool done;

	/*
	 * Check that the job cancel callback gets invoked by the
scheduler.
	 */

	sched = drm_mock_sched_new(test, MAX_SCHEDULE_TIMEOUT);
	entity = drm_mock_sched_entity_new(test,
					  
DRM_SCHED_PRIORITY_NORMAL,
					   sched);

	job = drm_mock_sched_job_new(test, entity);
	drm_mock_sched_job_submit(job);
	done = drm_mock_sched_job_wait_scheduled(job, HZ);
	KUNIT_ASSERT_TRUE(test, done);

	drm_mock_sched_entity_free(entity);
	drm_mock_sched_fini(sched);

	KUNIT_ASSERT_TRUE(test, job->flags &

DRM_MOCK_SCHED_JOB_CANCELED);
}

Or via the hw fence status.

Regards,

Tvrtko

Quoted 154 lines

Quoted 153 lines

Quoted 70 lines

Signed-off-by Philipp Stanner <[email protected]>

---
   .../gpu/drm/scheduler/tests/mock_scheduler.c  | 66 +++++++--
-----
-----

   1 file changed, 23 insertions(+), 43 deletions(-)

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -0

index 49d067fecd67..2d3169d95200 100644
@@ -63,7 +63,7 @@ static void

drm_mock_sched_job_complete(struct
drm_mock_sched_job *job)
    lockdep_assert_held(&sched->lock);

    job->flags |= DRM_MOCK_SCHED_JOB_DONE;

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +1 -1

-	list_move_tail(&job->link, &sched->done_list);
+	list_del(&job->link);

    dma_fence_signal_locked(&job->hw_fence);
    complete(&job->done);
   }

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -0

@@ -236,26 +236,39 @@ mock_sched_timedout_job(struct

drm_sched_job
*sched_job)

   static void mock_sched_free_job(struct drm_sched_job *sched_job)
   {

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -2

-	struct drm_mock_scheduler *sched =
-			drm_sched_to_mock_sched(sched_job-

Quoted 1 line

sched);

    struct drm_mock_sched_job *job =
drm_sched_job_to_mock_job(sched_job);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -1

-	unsigned long flags;

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -4

-	/* Remove from the scheduler done list. */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_del(&job->link);
-	spin_unlock_irqrestore(&sched->lock, flags);

    dma_fence_put(&job->hw_fence);
-
    drm_sched_job_cleanup(sched_job);

    /* Mock job itself is freed by the kunit framework. */ }

+static void mock_sched_cancel_job(struct drm_sched_job *sched_job)

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +2 -0

+{
+	struct drm_mock_scheduler *sched =

drm_sched_to_mock_sched(sched_job->sched);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +1 -0

+	struct drm_mock_sched_job *job =

drm_sched_job_to_mock_job(sched_job);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +8 -0

+	unsigned long flags;
+
+	hrtimer_cancel(&job->timer);
+
+	spin_lock_irqsave(&sched->lock, flags);
+	if (!dma_fence_is_signaled_locked(&job->hw_fence)) {
+		list_del(&job->link);
+		dma_fence_set_error(&job->hw_fence, -

ECANCELED);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +5 -0

+		dma_fence_signal_locked(&job->hw_fence);
+	}
+	spin_unlock_irqrestore(&sched->lock, flags);
+
+	/* The GPU Scheduler will call

drm_sched_backend_ops.free_job(), still.

drivers/gpu/drm/scheduler/tests/mock_scheduler.c

Quoted 1 line

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +1 -0

+	 * Mock job itself is freed by the kunit framework. */

/*
   * Multiline comment style to stay consistent, at least in this file.
   */

The rest looks good, but I need to revisit the timeout/free handling
since it has been a while and you changed it recently.

Regards,

Tvrtko

Quoted 67 lines

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +2 -0

+}
+

   static const struct drm_sched_backend_ops
drm_mock_scheduler_ops
= {
    .run_job = mock_sched_run_job,
    .timedout_job = mock_sched_timedout_job,

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +2 -1

-	.free_job = mock_sched_free_job
+	.free_job = mock_sched_free_job,
+	.cancel_job = mock_sched_cancel_job,

   };

   /**

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -0

@@ -289,7 +302,6 @@ struct drm_mock_scheduler
 *drm_mock_sched_new(struct kunit *test, long timeout)

    sched->hw_timeline.context = dma_fence_context_alloc(1); atomic_set(&sched->hw_timeline.next_seqno, 0); INIT_LIST_HEAD(&sched->job_list);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -1

-	INIT_LIST_HEAD(&sched->done_list);

    spin_lock_init(&sched->lock);

    return sched;

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -22

@@ -304,38 +316,6 @@ struct drm_mock_scheduler
 *drm_mock_sched_new(struct kunit *test, long timeout)
     */
    void drm_mock_sched_fini(struct drm_mock_scheduler *sched)
    {
-	struct drm_mock_sched_job *job, *next;
-	unsigned long flags;
-	LIST_HEAD(list);
-
-	drm_sched_wqueue_stop(&sched->base);
-
-	/* Force complete all unfinished jobs. */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &sched->job_list,
 link)
-		list_move_tail(&job->link, &list);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	list_for_each_entry(job, &list, link)
-		hrtimer_cancel(&job->timer);
-
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &list, link)
-		drm_mock_sched_job_complete(job);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	/*
-	 * Free completed jobs and jobs not yet processed by

the
DRM scheduler

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -4

-	 * free worker.
-	 */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &sched->done_list,

link)

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -6

-		list_move_tail(&job->link, &list);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	list_for_each_entry_safe(job, next, &list, link)
-		mock_sched_free_job(&job->base);
-

    drm_sched_fini(&sched->base);
   }

Quoted 97 lines · Tvrtko Ursulin

Quoted 57 lines · Philipp Stanner

Quoted 21 lines · Tvrtko Ursulin

Quoted 11 lines · Philipp Stanner

The GPU Scheduler now supports a new callback, cancel_job(), which
lets
the scheduler cancel all jobs which might not yet be freed when
drm_sched_fini() runs. Using this callback allows for significantly
simplifying the mock scheduler teardown code.

Implement the cancel_job() callback and adjust the code where necessary.

Cross referencing against my version I think you missed this hunk:

drivers/gpu/drm/scheduler/tests/sched_tests.h struct drm_mock_scheduler { · line 54

Quoted 7 lines

drivers/gpu/drm/scheduler/tests/sched_tests.h +0 -1

@@ -49,7 +49,6 @@ struct drm_mock_scheduler {
 
    	spinlock_t		lock;
    	struct list_head	job_list;
-	struct list_head	done_list;
 
    	struct {
    		u64		context;

Right, overlooked that one.

Quoted 11 lines

I also had this:

drivers/gpu/drm/scheduler/tests/sched_tests.h +2 -1

@@ -97,7 +96,8 @@ struct drm_mock_sched_job {
    	struct completion	done;
 
    #define DRM_MOCK_SCHED_JOB_DONE		0x1
-#define DRM_MOCK_SCHED_JOB_TIMEDOUT	0x2
+#define DRM_MOCK_SCHED_JOB_CANCELED	0x2
+#define DRM_MOCK_SCHED_JOB_TIMEDOUT	0x4
 

Quoted 5 lines

And was setting it in the callback. And since we should add a test to
explicitly cover the new callback, and just the callback, that could
make it very easy to do it.

What do you imagine that to look like? The scheduler only invokes the
callback on tear down.

We also don't have tests that only test free_job() and the like, do
we?

Quoted 8 lines

You cannot test a callback for the scheduler, because the callback
is
implemented in the driver.

Callbacks are tested by using the scheduler. In this case, it's tested
the intended way by the unit tests invoking drm_sched_free().

Something like (untested):

static void drm_sched_test_cleanup(struct kunit *test)

{
	struct drm_mock_sched_entity *entity;
	struct drm_mock_scheduler *sched;
	struct drm_mock_sched_job job;
	bool done;

	/*
	 * Check that the job cancel callback gets invoked by the
scheduler.
	 */

	sched = drm_mock_sched_new(test, MAX_SCHEDULE_TIMEOUT);
	entity = drm_mock_sched_entity_new(test,
					  
DRM_SCHED_PRIORITY_NORMAL,
					   sched);

	job = drm_mock_sched_job_new(test, entity);
	drm_mock_sched_job_submit(job);
	done = drm_mock_sched_job_wait_scheduled(job, HZ);
	KUNIT_ASSERT_TRUE(test, done);

	drm_mock_sched_entity_free(entity);
	drm_mock_sched_fini(sched);

	KUNIT_ASSERT_TRUE(test, job->flags &

Quoted 2 lines

DRM_MOCK_SCHED_JOB_CANCELED);
}

That could work – but it's racy.

I wonder if we want to introduce a mechanism into the mock scheduler through which we can enforce a simulated GPU hang. Then it would never
race, and that might be useful for more advanced tests for reset recovery and those things.

Opinions?

P.

Quoted 168 lines

Or via the hw fence status.

Regards,

Tvrtko

Quoted 160 lines

Quoted 159 lines

Quoted 73 lines

Signed-off-by Philipp Stanner <[email protected]>

---
   .../gpu/drm/scheduler/tests/mock_scheduler.c  | 66
+++++++--
-----
-----

   1 file changed, 23 insertions(+), 43 deletions(-)

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -0

index 49d067fecd67..2d3169d95200 100644
@@ -63,7 +63,7 @@ static void

drm_mock_sched_job_complete(struct
drm_mock_sched_job *job)
    lockdep_assert_held(&sched->lock);

    job->flags |= DRM_MOCK_SCHED_JOB_DONE;

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +1 -1

-	list_move_tail(&job->link, &sched->done_list);
+	list_del(&job->link);

    dma_fence_signal_locked(&job->hw_fence);
    complete(&job->done);
   }

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -0

@@ -236,26 +236,39 @@ mock_sched_timedout_job(struct

drm_sched_job
*sched_job)

   static void mock_sched_free_job(struct drm_sched_job *sched_job)
   {

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -2

-	struct drm_mock_scheduler *sched =
-			drm_sched_to_mock_sched(sched_job-

Quoted 1 line

sched);

    struct drm_mock_sched_job *job =
drm_sched_job_to_mock_job(sched_job);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -1

-	unsigned long flags;

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -4

-	/* Remove from the scheduler done list. */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_del(&job->link);
-	spin_unlock_irqrestore(&sched->lock, flags);

    dma_fence_put(&job->hw_fence);
-
    drm_sched_job_cleanup(sched_job);

    /* Mock job itself is freed by the kunit framework. */
   }

+static void mock_sched_cancel_job(struct drm_sched_job *sched_job)

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +2 -0

+{
+	struct drm_mock_scheduler *sched =

drm_sched_to_mock_sched(sched_job->sched);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +1 -0

+	struct drm_mock_sched_job *job =

drm_sched_job_to_mock_job(sched_job);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +8 -0

+	unsigned long flags;
+
+	hrtimer_cancel(&job->timer);
+
+	spin_lock_irqsave(&sched->lock, flags);
+	if (!dma_fence_is_signaled_locked(&job->hw_fence)) {
+		list_del(&job->link);
+		dma_fence_set_error(&job->hw_fence, -

ECANCELED);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +5 -0

+		dma_fence_signal_locked(&job->hw_fence);
+	}
+	spin_unlock_irqrestore(&sched->lock, flags);
+
+	/* The GPU Scheduler will call

drm_sched_backend_ops.free_job(), still.

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +1 -0

+	 * Mock job itself is freed by the kunit framework.

Quoted 1 line

*/

/*
   * Multiline comment style to stay consistent, at least in this
file.
   */

The rest looks good, but I need to revisit the timeout/free handling
since it has been a while and you changed it recently.

Regards,

Tvrtko

Quoted 69 lines

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +2 -0

+}
+

   static const struct drm_sched_backend_ops
drm_mock_scheduler_ops
= {
    .run_job = mock_sched_run_job,
    .timedout_job = mock_sched_timedout_job,

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +2 -1

-	.free_job = mock_sched_free_job
+	.free_job = mock_sched_free_job,
+	.cancel_job = mock_sched_cancel_job,

   };

   /**

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -0

@@ -289,7 +302,6 @@ struct drm_mock_scheduler
 *drm_mock_sched_new(struct kunit *test, long timeout)

    sched->hw_timeline.context = dma_fence_context_alloc(1); atomic_set(&sched->hw_timeline.next_seqno, 0); INIT_LIST_HEAD(&sched->job_list);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -1

-	INIT_LIST_HEAD(&sched->done_list);

    spin_lock_init(&sched->lock);

    return sched;

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -22

@@ -304,38 +316,6 @@ struct drm_mock_scheduler
 *drm_mock_sched_new(struct kunit *test, long timeout)
     */
    void drm_mock_sched_fini(struct drm_mock_scheduler *sched)
    {
-	struct drm_mock_sched_job *job, *next;
-	unsigned long flags;
-	LIST_HEAD(list);
-
-	drm_sched_wqueue_stop(&sched->base);
-
-	/* Force complete all unfinished jobs. */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &sched-
 >job_list,
 link)
-		list_move_tail(&job->link, &list);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	list_for_each_entry(job, &list, link)
-		hrtimer_cancel(&job->timer);
-
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &list, link)
-		drm_mock_sched_job_complete(job);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	/*
-	 * Free completed jobs and jobs not yet processed by

the
DRM scheduler

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -4

-	 * free worker.
-	 */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &sched-

Quoted 1 line

done_list,

link)

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -6

-		list_move_tail(&job->link, &list);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	list_for_each_entry_safe(job, next, &list, link)
-		mock_sched_free_job(&job->base);
-

    drm_sched_fini(&sched->base);
   }

Quoted 109 lines · Philipp Stanner

Quoted 97 lines · Tvrtko Ursulin

Quoted 57 lines · Philipp Stanner

Quoted 21 lines · Tvrtko Ursulin

Quoted 11 lines · Philipp Stanner

The GPU Scheduler now supports a new callback, cancel_job(), which
lets
the scheduler cancel all jobs which might not yet be freed when
drm_sched_fini() runs. Using this callback allows for significantly
simplifying the mock scheduler teardown code.

Implement the cancel_job() callback and adjust the code where necessary.

Cross referencing against my version I think you missed this hunk:

drivers/gpu/drm/scheduler/tests/sched_tests.h struct drm_mock_scheduler { · line 54

Quoted 7 lines

drivers/gpu/drm/scheduler/tests/sched_tests.h +0 -1

@@ -49,7 +49,6 @@ struct drm_mock_scheduler {
 
    	spinlock_t		lock;
    	struct list_head	job_list;
-	struct list_head	done_list;
 
    	struct {
    		u64		context;

Right, overlooked that one.

Quoted 11 lines

I also had this:

drivers/gpu/drm/scheduler/tests/sched_tests.h +2 -1

@@ -97,7 +96,8 @@ struct drm_mock_sched_job {
    	struct completion	done;
 
    #define DRM_MOCK_SCHED_JOB_DONE		0x1
-#define DRM_MOCK_SCHED_JOB_TIMEDOUT	0x2
+#define DRM_MOCK_SCHED_JOB_CANCELED	0x2
+#define DRM_MOCK_SCHED_JOB_TIMEDOUT	0x4
 

Quoted 5 lines

And was setting it in the callback. And since we should add a test to
explicitly cover the new callback, and just the callback, that could
make it very easy to do it.

What do you imagine that to look like? The scheduler only invokes the
callback on tear down.

We also don't have tests that only test free_job() and the like, do
we?

Quoted 8 lines

You cannot test a callback for the scheduler, because the callback
is
implemented in the driver.

Callbacks are tested by using the scheduler. In this case, it's tested
the intended way by the unit tests invoking drm_sched_free().

Something like (untested):

static void drm_sched_test_cleanup(struct kunit *test)

{
	struct drm_mock_sched_entity *entity;
	struct drm_mock_scheduler *sched;
	struct drm_mock_sched_job job;
	bool done;

	/*
	 * Check that the job cancel callback gets invoked by the
scheduler.
	 */

	sched = drm_mock_sched_new(test, MAX_SCHEDULE_TIMEOUT);
	entity = drm_mock_sched_entity_new(test,

DRM_SCHED_PRIORITY_NORMAL,

					   sched);

	job = drm_mock_sched_job_new(test, entity);
	drm_mock_sched_job_submit(job);
	done = drm_mock_sched_job_wait_scheduled(job, HZ);
	KUNIT_ASSERT_TRUE(test, done);

	drm_mock_sched_entity_free(entity);
	drm_mock_sched_fini(sched);

	KUNIT_ASSERT_TRUE(test, job->flags &

Quoted 2 lines

DRM_MOCK_SCHED_JOB_CANCELED);
}

That could work – but it's racy.

I wonder if we want to introduce a mechanism into the mock scheduler through which we can enforce a simulated GPU hang. Then it would never
race, and that might be useful for more advanced tests for reset recovery and those things.

Quoted 1 line

Opinions?

Ah wait, we can do that with job_duration = 0

Very good, I'll include something like that in v2.

Quoted 168 lines

Quoted 167 lines

Or via the hw fence status.

Regards,

Tvrtko

Quoted 160 lines

Quoted 159 lines

Quoted 73 lines

Signed-off-by Philipp Stanner <[email protected]>

---
    .../gpu/drm/scheduler/tests/mock_scheduler.c  | 66
+++++++--
-----
-----
    1 file changed, 23 insertions(+), 43 deletions(-)

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -0

index 49d067fecd67..2d3169d95200 100644
@@ -63,7 +63,7 @@ static void

drm_mock_sched_job_complete(struct
drm_mock_sched_job *job)
    lockdep_assert_held(&sched->lock);

    job->flags |= DRM_MOCK_SCHED_JOB_DONE;

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +1 -1

-	list_move_tail(&job->link, &sched->done_list);
+	list_del(&job->link);
    	dma_fence_signal_locked(&job->hw_fence);
    	complete(&job->done);
    }

@@ -236,26 +236,39 @@ mock_sched_timedout_job(struct

drm_sched_job
*sched_job)

   static void mock_sched_free_job(struct drm_sched_job
*sched_job)
   {

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -2

-	struct drm_mock_scheduler *sched =
-			drm_sched_to_mock_sched(sched_job-

Quoted 1 line

sched);

    struct drm_mock_sched_job *job =
drm_sched_job_to_mock_job(sched_job);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -6

-	unsigned long flags;
 
-	/* Remove from the scheduler done list. */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_del(&job->link);
-	spin_unlock_irqrestore(&sched->lock, flags);
    	dma_fence_put(&job->hw_fence);
-
    	drm_sched_job_cleanup(sched_job);
 
    	/* Mock job itself is freed by the kunit framework.

*/
   }

+static void mock_sched_cancel_job(struct drm_sched_job *sched_job)

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +2 -0

+{
+	struct drm_mock_scheduler *sched =

drm_sched_to_mock_sched(sched_job->sched);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +1 -0

+	struct drm_mock_sched_job *job =

drm_sched_job_to_mock_job(sched_job);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +8 -0

+	unsigned long flags;
+
+	hrtimer_cancel(&job->timer);
+
+	spin_lock_irqsave(&sched->lock, flags);
+	if (!dma_fence_is_signaled_locked(&job->hw_fence)) {
+		list_del(&job->link);
+		dma_fence_set_error(&job->hw_fence, -

ECANCELED);

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +5 -0

+		dma_fence_signal_locked(&job->hw_fence);
+	}
+	spin_unlock_irqrestore(&sched->lock, flags);
+
+	/* The GPU Scheduler will call

drm_sched_backend_ops.free_job(), still.

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +1 -0

+	 * Mock job itself is freed by the kunit framework.

Quoted 1 line

*/

/*
   * Multiline comment style to stay consistent, at least in
this
file.
   */

The rest looks good, but I need to revisit the timeout/free handling
since it has been a while and you changed it recently.

Regards,

Tvrtko

Quoted 69 lines

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +2 -0

+}
+
    static const struct drm_sched_backend_ops

drm_mock_scheduler_ops
= {
    .run_job = mock_sched_run_job,
    .timedout_job = mock_sched_timedout_job,

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +2 -2

-	.free_job = mock_sched_free_job
+	.free_job = mock_sched_free_job,
+	.cancel_job = mock_sched_cancel_job,
    };
 
    /**

@@ -289,7 +302,6 @@ struct drm_mock_scheduler
 *drm_mock_sched_new(struct kunit *test, long timeout)
    	sched->hw_timeline.context =
 dma_fence_context_alloc(1);
    	atomic_set(&sched->hw_timeline.next_seqno, 0);
    	INIT_LIST_HEAD(&sched->job_list);
-	INIT_LIST_HEAD(&sched->done_list);
    	spin_lock_init(&sched->lock);

    return sched;

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -22

@@ -304,38 +316,6 @@ struct drm_mock_scheduler
 *drm_mock_sched_new(struct kunit *test, long timeout)
     */
    void drm_mock_sched_fini(struct drm_mock_scheduler *sched)
    {
-	struct drm_mock_sched_job *job, *next;
-	unsigned long flags;
-	LIST_HEAD(list);
-
-	drm_sched_wqueue_stop(&sched->base);
-
-	/* Force complete all unfinished jobs. */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &sched-
 > job_list,
 link)
-		list_move_tail(&job->link, &list);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	list_for_each_entry(job, &list, link)
-		hrtimer_cancel(&job->timer);
-
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &list, link)
-		drm_mock_sched_job_complete(job);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	/*
-	 * Free completed jobs and jobs not yet processed by

the
DRM scheduler

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -4

-	 * free worker.
-	 */
-	spin_lock_irqsave(&sched->lock, flags);
-	list_for_each_entry_safe(job, next, &sched-

Quoted 1 line

done_list,

link)

drivers/gpu/drm/scheduler/tests/mock_scheduler.c +0 -6

-		list_move_tail(&job->link, &list);
-	spin_unlock_irqrestore(&sched->lock, flags);
-
-	list_for_each_entry_safe(job, next, &list, link)
-		mock_sched_free_job(&job->base);
-
    	drm_sched_fini(&sched->base);
    }