[PATCH] drm/sched/tests: Make timedout_job callback a better role model

[PATCH] drm/sched/tests: Make timedout_job callback a better role model

[Philipp Stanner]

Since the drm_mock_scheduler does not have real users in userspace, nor
does it have real hardware or firmware rings, it's not necessary to
signal timedout fences nor free jobs - from a functional standpoint.

The unit tests, however, serve as a reference implementation and a first
example for new scheduler users. Therefore, they should approximate the
canonical usage as much as possible.

Make sure timed out hardware fences get signaled with the appropriate
error code.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 .../gpu/drm/scheduler/tests/mock_scheduler.c  | 26 ++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
index 7f947ab9d322..49d067fecd67 100644
--- a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
+++ b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
@@ -200,12 +200,36 @@ static struct dma_fence *mock_sched_run_job(struct drm_sched_job *sched_job)
 	return &job->hw_fence;
 }
 
+/*
+ * Normally, drivers would take appropriate measures in this callback, such as
+ * killing the entity the faulty job is associated with, resetting the hardware
+ * and / or resubmitting non-faulty jobs.
+ *
+ * For the mock scheduler, there are no hardware rings to be resetted nor jobs
+ * to be resubmitted. Thus, this function merely ensures that
+ *   a) timedout fences get signaled properly and removed from the pending list
+ *   b) the mock scheduler framework gets informed about the timeout via a flag
+ *   c) The drm_sched_job, not longer needed, gets freed
+ */
 static enum drm_gpu_sched_stat
 mock_sched_timedout_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;
 
-	job->flags |= DRM_MOCK_SCHED_JOB_TIMEDOUT;
+	spin_lock_irqsave(&sched->lock, flags);
+	if (!dma_fence_is_signaled_locked(&job->hw_fence)) {
+		list_del(&job->link);
+		job->flags |= DRM_MOCK_SCHED_JOB_TIMEDOUT;
+		dma_fence_set_error(&job->hw_fence, -ETIMEDOUT);
+		dma_fence_signal_locked(&job->hw_fence);
+	}
+	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. */
 
 	return DRM_GPU_SCHED_STAT_NOMINAL;
 }
-- 
2.49.0

Re: [PATCH] drm/sched/tests: Make timedout_job callback a better role model

[Tvrtko Ursulin]

On 05/06/2025 14:41, Philipp Stanner wrote:
> Since the drm_mock_scheduler does not have real users in userspace, nor
> does it have real hardware or firmware rings, it's not necessary to
> signal timedout fences nor free jobs - from a functional standpoint.
> 
> The unit tests, however, serve as a reference implementation and a first
> example for new scheduler users. Therefore, they should approximate the
> canonical usage as much as possible.
> 
> Make sure timed out hardware fences get signaled with the appropriate
> error code.

Code looks fine, but currently nothing is broken and I disagree with the 
goal that the _mock_^1 components should be role models. The idea is to 
implement as little in the mock components as it is required to exercise 
the tested functionality.

As the motivation for this patch, ie. things which start depending on 
adding this functionality, only appear in one upcoming series, and as 
mentioned in 
https://lore.kernel.org/dri-devel/18cd6b1f-8872-4a16-9ceb-50fd1ecfea39@igalia.com/, 
I think it makes most sense to stick with that philosophy and fold this 
patch into the respective series.

Also, there are various ways drivers use the scheduler API. Trying to 
make the mock scheduler a reference driver implementation would only be 
able to make it a reference for one possible use.

Regards,

Tvrtko

1)
mock
adjective
not authentic or real, but without the intention to deceive.
"a mock-Georgian red brick house"

> 
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
>   .../gpu/drm/scheduler/tests/mock_scheduler.c  | 26 ++++++++++++++++++-
>   1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> index 7f947ab9d322..49d067fecd67 100644
> --- a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> +++ b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> @@ -200,12 +200,36 @@ static struct dma_fence *mock_sched_run_job(struct drm_sched_job *sched_job)
>   	return &job->hw_fence;
>   }
>   
> +/*
> + * Normally, drivers would take appropriate measures in this callback, such as
> + * killing the entity the faulty job is associated with, resetting the hardware
> + * and / or resubmitting non-faulty jobs.
> + *
> + * For the mock scheduler, there are no hardware rings to be resetted nor jobs
> + * to be resubmitted. Thus, this function merely ensures that
> + *   a) timedout fences get signaled properly and removed from the pending list
> + *   b) the mock scheduler framework gets informed about the timeout via a flag
> + *   c) The drm_sched_job, not longer needed, gets freed
> + */
>   static enum drm_gpu_sched_stat
>   mock_sched_timedout_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;
>   
> -	job->flags |= DRM_MOCK_SCHED_JOB_TIMEDOUT;
> +	spin_lock_irqsave(&sched->lock, flags);
> +	if (!dma_fence_is_signaled_locked(&job->hw_fence)) {
> +		list_del(&job->link);
> +		job->flags |= DRM_MOCK_SCHED_JOB_TIMEDOUT;
> +		dma_fence_set_error(&job->hw_fence, -ETIMEDOUT);
> +		dma_fence_signal_locked(&job->hw_fence);
> +	}
> +	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. */
>   
>   	return DRM_GPU_SCHED_STAT_NOMINAL;
>   }

Re: [PATCH] drm/sched/tests: Make timedout_job callback a better role model

[Danilo Krummrich]

On Mon, Jun 16, 2025 at 11:57:47AM +0100, Tvrtko Ursulin wrote:
> Code looks fine, but currently nothing is broken and I disagree with the
> goal that the _mock_^1 components should be role models. The idea is to
> implement as little in the mock components as it is required to exercise the
> tested functionality.

No, please consider the following.

  1) When we write tests for common infrastructure we should be testing things
     as close as possible to how we intend real code to use this infrastructure.
     Relying on internals in creative ways is likely to underrun this testing.

  2) Being close to a reference design is a good thing, why wouldn't we want
     that? The reality is that people *will* look at this code for reference.

> Also, there are various ways drivers use the scheduler API. Trying to make
> the mock scheduler a reference driver implementation would only be able to
> make it a reference for one possible use.

Why? Nothing prevents us from covering all of them eventually.

If for now, we just implement one of them, that's better than none, so why not?

Re: [PATCH] drm/sched/tests: Make timedout_job callback a better role model

[Maíra Canal]

Hi Danilo,

On 16/06/25 08:14, Danilo Krummrich wrote:
> On Mon, Jun 16, 2025 at 11:57:47AM +0100, Tvrtko Ursulin wrote:
>> Code looks fine, but currently nothing is broken and I disagree with the
>> goal that the _mock_^1 components should be role models. The idea is to
>> implement as little in the mock components as it is required to exercise the
>> tested functionality.
> 
> No, please consider the following.
> 
>    1) When we write tests for common infrastructure we should be testing things
>       as close as possible to how we intend real code to use this infrastructure.
>       Relying on internals in creative ways is likely to underrun this testing.

Regarding unit testing (and KUnit is a unit testing framework), the core
principle is that individual components of the code should be tested in
isolation to validate that they perform as expected.

The "units" should be tested independently and we use those mocks to
simulate dependencies, ensuring that the test focuses solely on the unit
under scrutiny.

If we introduce more things into the mock, we end up losing the
isolation. The mock scheduler, from what I understand, is not suppose to
be a reference design or even something close to a driver.  It should
remain just a mock, a minimal interface to test the scheduler's
internals.

Best Regards,
- Maíra

> 
>    2) Being close to a reference design is a good thing, why wouldn't we want
>       that? The reality is that people *will* look at this code for reference.
> 
>> Also, there are various ways drivers use the scheduler API. Trying to make
>> the mock scheduler a reference driver implementation would only be able to
>> make it a reference for one possible use.
> 
> Why? Nothing prevents us from covering all of them eventually.
> 
> If for now, we just implement one of them, that's better than none, so why not?

Re: [PATCH] drm/sched/tests: Make timedout_job callback a better role model

[Philipp Stanner]

On Mon, 2025-06-16 at 09:49 -0300, Maíra Canal wrote:
> Hi Danilo,
> 
> On 16/06/25 08:14, Danilo Krummrich wrote:
> > On Mon, Jun 16, 2025 at 11:57:47AM +0100, Tvrtko Ursulin wrote:
> > > Code looks fine, but currently nothing is broken and I disagree
> > > with the
> > > goal that the _mock_^1 components should be role models. The idea
> > > is to
> > > implement as little in the mock components as it is required to
> > > exercise the
> > > tested functionality.
> > 
> > No, please consider the following.
> > 
> >    1) When we write tests for common infrastructure we should be
> > testing things
> >       as close as possible to how we intend real code to use this
> > infrastructure.
> >       Relying on internals in creative ways is likely to underrun
> > this testing.
> 
> Regarding unit testing (and KUnit is a unit testing framework), the
> core
> principle is that individual components of the code should be tested
> in
> isolation to validate that they perform as expected.
> 
> The "units" should be tested independently and we use those mocks to
> simulate dependencies, ensuring that the test focuses solely on the
> unit
> under scrutiny.
> 
> If we introduce more things into the mock, we end up losing the
> isolation.

That depends on what those "things" are.

In the presented case, the unit tests are just as isolated as before.
The particular test case itself is not even modified and it will still
do exactly one thing: See if the job actually timed out as expected.

The added overhead in the mock component is merely the guarantee that
the fence gets signaled and that the job is removed from the mock
component's list at the appropriate place.

0 downside, documentation upside.

Each unit test still tests exactly 1 functionality.


> The mock scheduler, from what I understand, is not suppose to
> be a reference design or even something close to a driver. 

We here in DRM define what it is supposed to be.

And since the scheduler has been infinitely abused and misused, at
least having one official, simple example "this is how you're supposed
to do it" is desirable, wouldn't you say so?


P.

>  It should
> remain just a mock, a minimal interface to test the scheduler's
> internals.
> 
> Best Regards,
> - Maíra
> 
> > 
> >    2) Being close to a reference design is a good thing, why
> > wouldn't we want
> >       that? The reality is that people *will* look at this code for
> > reference.
> > 
> > > Also, there are various ways drivers use the scheduler API.
> > > Trying to make
> > > the mock scheduler a reference driver implementation would only
> > > be able to
> > > make it a reference for one possible use.
> > 
> > Why? Nothing prevents us from covering all of them eventually.
> > 
> > If for now, we just implement one of them, that's better than none,
> > so why not?
> 

Re: [PATCH] drm/sched/tests: Make timedout_job callback a better role model

[Danilo Krummrich]

On Mon, Jun 16, 2025 at 09:49:46AM -0300, Maíra Canal wrote:
> Hi Danilo,
> 
> On 16/06/25 08:14, Danilo Krummrich wrote:
> > On Mon, Jun 16, 2025 at 11:57:47AM +0100, Tvrtko Ursulin wrote:
> > > Code looks fine, but currently nothing is broken and I disagree with the
> > > goal that the _mock_^1 components should be role models. The idea is to
> > > implement as little in the mock components as it is required to exercise the
> > > tested functionality.
> > 
> > No, please consider the following.
> > 
> >    1) When we write tests for common infrastructure we should be testing things
> >       as close as possible to how we intend real code to use this infrastructure.
> >       Relying on internals in creative ways is likely to underrun this testing.
> 
> Regarding unit testing (and KUnit is a unit testing framework), the core
> principle is that individual components of the code should be tested in
> isolation to validate that they perform as expected.
> 
> The "units" should be tested independently and we use those mocks to
> simulate dependencies, ensuring that the test focuses solely on the unit
> under scrutiny.
> 
> If we introduce more things into the mock, we end up losing the
> isolation. The mock scheduler, from what I understand, is not suppose to
> be a reference design or even something close to a driver.  It should
> remain just a mock, a minimal interface to test the scheduler's
> internals.

Nothing of what you say seems contradictive to what I say, no?

I'm just saying that while we're doing all this we can (and should) still try
to be as close as possible to how we intend real code to use the corresponding
APIs.

Re: [PATCH] drm/sched/tests: Make timedout_job callback a better role model

[Philipp Stanner]

On Thu, 2025-06-05 at 15:41 +0200, Philipp Stanner wrote:
> Since the drm_mock_scheduler does not have real users in userspace,
> nor
> does it have real hardware or firmware rings, it's not necessary to
> signal timedout fences nor free jobs - from a functional standpoint.
> 
> The unit tests, however, serve as a reference implementation and a
> first
> example for new scheduler users. Therefore, they should approximate
> the
> canonical usage as much as possible.
> 
> Make sure timed out hardware fences get signaled with the appropriate
> error code.
> 
> Signed-off-by: Philipp Stanner <phasta@kernel.org>

Any further objections with me merging this?

P.


> ---
>  .../gpu/drm/scheduler/tests/mock_scheduler.c  | 26
> ++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> index 7f947ab9d322..49d067fecd67 100644
> --- a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> +++ b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> @@ -200,12 +200,36 @@ static struct dma_fence
> *mock_sched_run_job(struct drm_sched_job *sched_job)
>  	return &job->hw_fence;
>  }
>  
> +/*
> + * Normally, drivers would take appropriate measures in this
> callback, such as
> + * killing the entity the faulty job is associated with, resetting
> the hardware
> + * and / or resubmitting non-faulty jobs.
> + *
> + * For the mock scheduler, there are no hardware rings to be
> resetted nor jobs
> + * to be resubmitted. Thus, this function merely ensures that
> + *   a) timedout fences get signaled properly and removed from the
> pending list
> + *   b) the mock scheduler framework gets informed about the timeout
> via a flag
> + *   c) The drm_sched_job, not longer needed, gets freed
> + */
>  static enum drm_gpu_sched_stat
>  mock_sched_timedout_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;
>  
> -	job->flags |= DRM_MOCK_SCHED_JOB_TIMEDOUT;
> +	spin_lock_irqsave(&sched->lock, flags);
> +	if (!dma_fence_is_signaled_locked(&job->hw_fence)) {
> +		list_del(&job->link);
> +		job->flags |= DRM_MOCK_SCHED_JOB_TIMEDOUT;
> +		dma_fence_set_error(&job->hw_fence, -ETIMEDOUT);
> +		dma_fence_signal_locked(&job->hw_fence);
> +	}
> +	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. */
>  
>  	return DRM_GPU_SCHED_STAT_NOMINAL;
>  }

Re: [PATCH] drm/sched/tests: Make timedout_job callback a better role model

[Danilo Krummrich]

On Thu, Jun 05, 2025 at 03:41:55PM +0200, Philipp Stanner wrote:
> Since the drm_mock_scheduler does not have real users in userspace, nor
> does it have real hardware or firmware rings, it's not necessary to
> signal timedout fences nor free jobs - from a functional standpoint.
> 
> The unit tests, however, serve as a reference implementation and a first
> example for new scheduler users. Therefore, they should approximate the
> canonical usage as much as possible.
> 
> Make sure timed out hardware fences get signaled with the appropriate
> error code.
> 
> Signed-off-by: Philipp Stanner <phasta@kernel.org>

Given the discussion on this patch we agree that, unit test should remain to be
unit tests, but at the same time (as far as possible) represent the intended
usage of the scheduler's APIs.

Given that, "reference implementation" might be slightly overstated and we
should just say something along the lines of "make them represent the intended
usage of the scheduler's APIs".

Also, since this patch is a prerequisite of your teardown series, please mention
that in the commit message.

With that:

	Acked-by: Danilo Krummrich <dakr@kernel.org>

Fine for me if you fix up those two nits when applying the patch.

- Danilo

Re: [PATCH] drm/sched/tests: Make timedout_job callback a better role model

[Philipp Stanner]

On Thu, 2025-06-05 at 15:41 +0200, Philipp Stanner wrote:
> Since the drm_mock_scheduler does not have real users in userspace,
> nor
> does it have real hardware or firmware rings, it's not necessary to
> signal timedout fences nor free jobs - from a functional standpoint.
> 
> The unit tests, however, serve as a reference implementation and a
> first
> example for new scheduler users. Therefore, they should approximate
> the
> canonical usage as much as possible.
> 
> Make sure timed out hardware fences get signaled with the appropriate
> error code.
> 
> Signed-off-by: Philipp Stanner <phasta@kernel.org>

Pushed that one to drm-misc-next

P.

> ---
>  .../gpu/drm/scheduler/tests/mock_scheduler.c  | 26
> ++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> index 7f947ab9d322..49d067fecd67 100644
> --- a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> +++ b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> @@ -200,12 +200,36 @@ static struct dma_fence
> *mock_sched_run_job(struct drm_sched_job *sched_job)
>  	return &job->hw_fence;
>  }
>  
> +/*
> + * Normally, drivers would take appropriate measures in this
> callback, such as
> + * killing the entity the faulty job is associated with, resetting
> the hardware
> + * and / or resubmitting non-faulty jobs.
> + *
> + * For the mock scheduler, there are no hardware rings to be
> resetted nor jobs
> + * to be resubmitted. Thus, this function merely ensures that
> + *   a) timedout fences get signaled properly and removed from the
> pending list
> + *   b) the mock scheduler framework gets informed about the timeout
> via a flag
> + *   c) The drm_sched_job, not longer needed, gets freed
> + */
>  static enum drm_gpu_sched_stat
>  mock_sched_timedout_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;
>  
> -	job->flags |= DRM_MOCK_SCHED_JOB_TIMEDOUT;
> +	spin_lock_irqsave(&sched->lock, flags);
> +	if (!dma_fence_is_signaled_locked(&job->hw_fence)) {
> +		list_del(&job->link);
> +		job->flags |= DRM_MOCK_SCHED_JOB_TIMEDOUT;
> +		dma_fence_set_error(&job->hw_fence, -ETIMEDOUT);
> +		dma_fence_signal_locked(&job->hw_fence);
> +	}
> +	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. */
>  
>  	return DRM_GPU_SCHED_STAT_NOMINAL;
>  }