[v2] drm/sched/tests: Use one lock for fence context

[PATCH v2] drm/sched/tests: Use one lock for fence context

Posted by Philipp Stanner 6 months, 3 weeks ago

There is no need for separate locks for single jobs and the entire
scheduler. The dma_fence context can be protected by the scheduler lock,
allowing for removing the jobs' locks. This simplifies things and
reduces the likelyhood of deadlocks etc.

Replace the jobs' locks with the mock scheduler lock.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
Changes in v2:
  - Make commit message more neutral by stating it's about simplifying
    the code. (Tvrtko)
---
 drivers/gpu/drm/scheduler/tests/mock_scheduler.c | 5 ++---
 drivers/gpu/drm/scheduler/tests/sched_tests.h    | 1 -
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
index f999c8859cf7..17023276f4b0 100644
--- a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
+++ b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
@@ -64,7 +64,7 @@ static void drm_mock_sched_job_complete(struct drm_mock_sched_job *job)
 
 	job->flags |= DRM_MOCK_SCHED_JOB_DONE;
 	list_move_tail(&job->link, &sched->done_list);
-	dma_fence_signal(&job->hw_fence);
+	dma_fence_signal_locked(&job->hw_fence);
 	complete(&job->done);
 }
 
@@ -123,7 +123,6 @@ drm_mock_sched_job_new(struct kunit *test,
 	job->test = test;
 
 	init_completion(&job->done);
-	spin_lock_init(&job->lock);
 	INIT_LIST_HEAD(&job->link);
 	hrtimer_setup(&job->timer, drm_mock_sched_job_signal_timer,
 		      CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
@@ -169,7 +168,7 @@ static struct dma_fence *mock_sched_run_job(struct drm_sched_job *sched_job)
 
 	dma_fence_init(&job->hw_fence,
 		       &drm_mock_sched_hw_fence_ops,
-		       &job->lock,
+		       &sched->lock,
 		       sched->hw_timeline.context,
 		       atomic_inc_return(&sched->hw_timeline.next_seqno));
 
diff --git a/drivers/gpu/drm/scheduler/tests/sched_tests.h b/drivers/gpu/drm/scheduler/tests/sched_tests.h
index 27caf8285fb7..fbba38137f0c 100644
--- a/drivers/gpu/drm/scheduler/tests/sched_tests.h
+++ b/drivers/gpu/drm/scheduler/tests/sched_tests.h
@@ -106,7 +106,6 @@ struct drm_mock_sched_job {
 	unsigned int		duration_us;
 	ktime_t			finish_at;
 
-	spinlock_t		lock;
 	struct dma_fence	hw_fence;
 
 	struct kunit		*test;
-- 
2.49.0

Re: [PATCH v2] drm/sched/tests: Use one lock for fence context

Posted by Philipp Stanner 6 months, 2 weeks ago

On Tue, 2025-05-27 at 12:10 +0200, Philipp Stanner wrote:
> There is no need for separate locks for single jobs and the entire
> scheduler. The dma_fence context can be protected by the scheduler
> lock,
> allowing for removing the jobs' locks. This simplifies things and
> reduces the likelyhood of deadlocks etc.
> 
> Replace the jobs' locks with the mock scheduler lock.
> 
> Signed-off-by: Philipp Stanner <phasta@kernel.org>

Applied to drm-misc-next

P.

> ---
> Changes in v2:
>   - Make commit message more neutral by stating it's about
> simplifying
>     the code. (Tvrtko)
> ---
>  drivers/gpu/drm/scheduler/tests/mock_scheduler.c | 5 ++---
>  drivers/gpu/drm/scheduler/tests/sched_tests.h    | 1 -
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> index f999c8859cf7..17023276f4b0 100644
> --- a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> +++ b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> @@ -64,7 +64,7 @@ static void drm_mock_sched_job_complete(struct
> drm_mock_sched_job *job)
>  
>  	job->flags |= DRM_MOCK_SCHED_JOB_DONE;
>  	list_move_tail(&job->link, &sched->done_list);
> -	dma_fence_signal(&job->hw_fence);
> +	dma_fence_signal_locked(&job->hw_fence);
>  	complete(&job->done);
>  }
>  
> @@ -123,7 +123,6 @@ drm_mock_sched_job_new(struct kunit *test,
>  	job->test = test;
>  
>  	init_completion(&job->done);
> -	spin_lock_init(&job->lock);
>  	INIT_LIST_HEAD(&job->link);
>  	hrtimer_setup(&job->timer, drm_mock_sched_job_signal_timer,
>  		      CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
> @@ -169,7 +168,7 @@ static struct dma_fence
> *mock_sched_run_job(struct drm_sched_job *sched_job)
>  
>  	dma_fence_init(&job->hw_fence,
>  		       &drm_mock_sched_hw_fence_ops,
> -		       &job->lock,
> +		       &sched->lock,
>  		       sched->hw_timeline.context,
>  		       atomic_inc_return(&sched-
> >hw_timeline.next_seqno));
>  
> diff --git a/drivers/gpu/drm/scheduler/tests/sched_tests.h
> b/drivers/gpu/drm/scheduler/tests/sched_tests.h
> index 27caf8285fb7..fbba38137f0c 100644
> --- a/drivers/gpu/drm/scheduler/tests/sched_tests.h
> +++ b/drivers/gpu/drm/scheduler/tests/sched_tests.h
> @@ -106,7 +106,6 @@ struct drm_mock_sched_job {
>  	unsigned int		duration_us;
>  	ktime_t			finish_at;
>  
> -	spinlock_t		lock;
>  	struct dma_fence	hw_fence;
>  
>  	struct kunit		*test;

Re: [PATCH v2] drm/sched/tests: Use one lock for fence context

Posted by Tvrtko Ursulin 6 months, 2 weeks ago

On 27/05/2025 11:10, Philipp Stanner wrote:
> There is no need for separate locks for single jobs and the entire
> scheduler. The dma_fence context can be protected by the scheduler lock,
> allowing for removing the jobs' locks. This simplifies things and
> reduces the likelyhood of deadlocks etc.
> 
> Replace the jobs' locks with the mock scheduler lock.
> 
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
> Changes in v2:
>    - Make commit message more neutral by stating it's about simplifying
>      the code. (Tvrtko)
> ---
>   drivers/gpu/drm/scheduler/tests/mock_scheduler.c | 5 ++---
>   drivers/gpu/drm/scheduler/tests/sched_tests.h    | 1 -
>   2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> index f999c8859cf7..17023276f4b0 100644
> --- a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> +++ b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c
> @@ -64,7 +64,7 @@ static void drm_mock_sched_job_complete(struct drm_mock_sched_job *job)
>   
>   	job->flags |= DRM_MOCK_SCHED_JOB_DONE;
>   	list_move_tail(&job->link, &sched->done_list);
> -	dma_fence_signal(&job->hw_fence);
> +	dma_fence_signal_locked(&job->hw_fence);
>   	complete(&job->done);
>   }
>   
> @@ -123,7 +123,6 @@ drm_mock_sched_job_new(struct kunit *test,
>   	job->test = test;
>   
>   	init_completion(&job->done);
> -	spin_lock_init(&job->lock);
>   	INIT_LIST_HEAD(&job->link);
>   	hrtimer_setup(&job->timer, drm_mock_sched_job_signal_timer,
>   		      CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
> @@ -169,7 +168,7 @@ static struct dma_fence *mock_sched_run_job(struct drm_sched_job *sched_job)
>   
>   	dma_fence_init(&job->hw_fence,
>   		       &drm_mock_sched_hw_fence_ops,
> -		       &job->lock,
> +		       &sched->lock,
>   		       sched->hw_timeline.context,
>   		       atomic_inc_return(&sched->hw_timeline.next_seqno));
>   
> diff --git a/drivers/gpu/drm/scheduler/tests/sched_tests.h b/drivers/gpu/drm/scheduler/tests/sched_tests.h
> index 27caf8285fb7..fbba38137f0c 100644
> --- a/drivers/gpu/drm/scheduler/tests/sched_tests.h
> +++ b/drivers/gpu/drm/scheduler/tests/sched_tests.h
> @@ -106,7 +106,6 @@ struct drm_mock_sched_job {
>   	unsigned int		duration_us;
>   	ktime_t			finish_at;
>   
> -	spinlock_t		lock;
>   	struct dma_fence	hw_fence;
>   
>   	struct kunit		*test;

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Regards,

Tvrtko