[PATCH 1/2] drm/sched: Add helper to check job dependencies

[PATCH 1/2] drm/sched: Add helper to check job dependencies

[Tvrtko Ursulin]

Lets isolate scheduler internals from drivers such as pvr which currently
walks the dependency array to look for fences.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
Reviewed-by: Matt Coster <matt.coster@imgtec.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 23 +++++++++++++++++++++++
 include/drm/gpu_scheduler.h            |  3 ++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 57da84908752..e6f1f2a8e033 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -997,6 +997,29 @@ int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job,
 }
 EXPORT_SYMBOL(drm_sched_job_add_implicit_dependencies);
 
+/**
+ * drm_sched_job_has_dependency - check whether fence is the job's dependency
+ * @job: scheduler job to check
+ * @fence: fence to look for
+ *
+ * Returns:
+ * True if @fence is found within the job's dependencies, or otherwise false.
+ */
+bool drm_sched_job_has_dependency(struct drm_sched_job *job,
+				  struct dma_fence *fence)
+{
+	struct dma_fence *f;
+	unsigned long index;
+
+	xa_for_each(&job->dependencies, index, f) {
+		if (f == fence)
+			return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL(drm_sched_job_has_dependency);
+
 /**
  * drm_sched_job_cleanup - clean up scheduler job resources
  * @job: scheduler job to clean up
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 95e17504e46a..d118a0a57ab1 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -577,7 +577,8 @@ int drm_sched_job_add_resv_dependencies(struct drm_sched_job *job,
 int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job,
 					    struct drm_gem_object *obj,
 					    bool write);
-
+bool drm_sched_job_has_dependency(struct drm_sched_job *job,
+				  struct dma_fence *fence);
 
 void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
 				    struct drm_gpu_scheduler **sched_list,
-- 
2.47.1

[PATCH 2/2] drm/imagination: Use the drm_sched_job_has_dependency helper

[Tvrtko Ursulin]

Instead of manually peeking into the DRM scheduler implementation details
lets use the previously added helper.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
Cc: Frank Binns <frank.binns@imgtec.com>
Cc: Matt Coster <matt.coster@imgtec.com>
Reviewed-by: Matt Coster <matt.coster@imgtec.com>
---
 drivers/gpu/drm/imagination/pvr_job.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_job.c b/drivers/gpu/drm/imagination/pvr_job.c
index 618503a212a7..1cdb3cfd058d 100644
--- a/drivers/gpu/drm/imagination/pvr_job.c
+++ b/drivers/gpu/drm/imagination/pvr_job.c
@@ -597,8 +597,6 @@ update_job_resvs_for_each(struct pvr_job_data *job_data, u32 job_count)
 static bool can_combine_jobs(struct pvr_job *a, struct pvr_job *b)
 {
 	struct pvr_job *geom_job = a, *frag_job = b;
-	struct dma_fence *fence;
-	unsigned long index;
 
 	/* Geometry and fragment jobs can be combined if they are queued to the
 	 * same context and targeting the same HWRT.
@@ -609,13 +607,9 @@ static bool can_combine_jobs(struct pvr_job *a, struct pvr_job *b)
 	    a->hwrt != b->hwrt)
 		return false;
 
-	xa_for_each(&frag_job->base.dependencies, index, fence) {
-		/* We combine when we see an explicit geom -> frag dep. */
-		if (&geom_job->base.s_fence->scheduled == fence)
-			return true;
-	}
-
-	return false;
+	/* We combine when we see an explicit geom -> frag dep. */
+	return drm_sched_job_has_dependency(&frag_job->base,
+					    &geom_job->base.s_fence->scheduled);
 }
 
 static struct dma_fence *
-- 
2.47.1

Re: [PATCH 2/2] drm/imagination: Use the drm_sched_job_has_dependency helper

[Philipp Stanner]

On Mon, 2025-01-13 at 10:33 +0000, Tvrtko Ursulin wrote:
> Instead of manually peeking into the DRM scheduler implementation
> details
> lets use the previously added helper.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@redhat.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <pstanner@redhat.com>
> Cc: Frank Binns <frank.binns@imgtec.com>
> Cc: Matt Coster <matt.coster@imgtec.com>
> Reviewed-by: Matt Coster <matt.coster@imgtec.com>

Matt, just to be sure, you're OK with me pulling those two into drm-
misc-next, aren't you?

P.

> ---
>  drivers/gpu/drm/imagination/pvr_job.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_job.c
> b/drivers/gpu/drm/imagination/pvr_job.c
> index 618503a212a7..1cdb3cfd058d 100644
> --- a/drivers/gpu/drm/imagination/pvr_job.c
> +++ b/drivers/gpu/drm/imagination/pvr_job.c
> @@ -597,8 +597,6 @@ update_job_resvs_for_each(struct pvr_job_data
> *job_data, u32 job_count)
>  static bool can_combine_jobs(struct pvr_job *a, struct pvr_job *b)
>  {
>  	struct pvr_job *geom_job = a, *frag_job = b;
> -	struct dma_fence *fence;
> -	unsigned long index;
>  
>  	/* Geometry and fragment jobs can be combined if they are
> queued to the
>  	 * same context and targeting the same HWRT.
> @@ -609,13 +607,9 @@ static bool can_combine_jobs(struct pvr_job *a,
> struct pvr_job *b)
>  	    a->hwrt != b->hwrt)
>  		return false;
>  
> -	xa_for_each(&frag_job->base.dependencies, index, fence) {
> -		/* We combine when we see an explicit geom -> frag
> dep. */
> -		if (&geom_job->base.s_fence->scheduled == fence)
> -			return true;
> -	}
> -
> -	return false;
> +	/* We combine when we see an explicit geom -> frag dep. */
> +	return drm_sched_job_has_dependency(&frag_job->base,
> +					    &geom_job->base.s_fence-
> >scheduled);
>  }
>  
>  static struct dma_fence *

Re: [PATCH 2/2] drm/imagination: Use the drm_sched_job_has_dependency helper

[Matt Coster]

[-- Attachment #1.1: Type: text/plain, Size: 2252 bytes --]

On 16/01/2025 10:02, Philipp Stanner wrote:
> On Mon, 2025-01-13 at 10:33 +0000, Tvrtko Ursulin wrote:
>> Instead of manually peeking into the DRM scheduler implementation
>> details
>> lets use the previously added helper.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Danilo Krummrich <dakr@redhat.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Philipp Stanner <pstanner@redhat.com>
>> Cc: Frank Binns <frank.binns@imgtec.com>
>> Cc: Matt Coster <matt.coster@imgtec.com>
>> Reviewed-by: Matt Coster <matt.coster@imgtec.com>
> 
> Matt, just to be sure, you're OK with me pulling those two into drm-
> misc-next, aren't you?

Yup, all good! Thanks for checking :)

-- 
Matt Coster
E: matt.coster@imgtec.com

> 
> P.
> 
>> ---
>>  drivers/gpu/drm/imagination/pvr_job.c | 12 +++---------
>>  1 file changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imagination/pvr_job.c
>> b/drivers/gpu/drm/imagination/pvr_job.c
>> index 618503a212a7..1cdb3cfd058d 100644
>> --- a/drivers/gpu/drm/imagination/pvr_job.c
>> +++ b/drivers/gpu/drm/imagination/pvr_job.c
>> @@ -597,8 +597,6 @@ update_job_resvs_for_each(struct pvr_job_data
>> *job_data, u32 job_count)
>>  static bool can_combine_jobs(struct pvr_job *a, struct pvr_job *b)
>>  {
>>  	struct pvr_job *geom_job = a, *frag_job = b;
>> -	struct dma_fence *fence;
>> -	unsigned long index;
>>  
>>  	/* Geometry and fragment jobs can be combined if they are
>> queued to the
>>  	 * same context and targeting the same HWRT.
>> @@ -609,13 +607,9 @@ static bool can_combine_jobs(struct pvr_job *a,
>> struct pvr_job *b)
>>  	    a->hwrt != b->hwrt)
>>  		return false;
>>  
>> -	xa_for_each(&frag_job->base.dependencies, index, fence) {
>> -		/* We combine when we see an explicit geom -> frag
>> dep. */
>> -		if (&geom_job->base.s_fence->scheduled == fence)
>> -			return true;
>> -	}
>> -
>> -	return false;
>> +	/* We combine when we see an explicit geom -> frag dep. */
>> +	return drm_sched_job_has_dependency(&frag_job->base,
>> +					    &geom_job->base.s_fence-
>>> scheduled);
>>  }
>>  
>>  static struct dma_fence *
> 

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH 2/2] drm/imagination: Use the drm_sched_job_has_dependency helper

[Philipp Stanner]

On Thu, 2025-01-16 at 12:58 +0000, Matt Coster wrote:
> On 16/01/2025 10:02, Philipp Stanner wrote:
> > On Mon, 2025-01-13 at 10:33 +0000, Tvrtko Ursulin wrote:
> > > Instead of manually peeking into the DRM scheduler implementation
> > > details
> > > lets use the previously added helper.
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > > Cc: Christian König <christian.koenig@amd.com>
> > > Cc: Danilo Krummrich <dakr@redhat.com>
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Cc: Philipp Stanner <pstanner@redhat.com>
> > > Cc: Frank Binns <frank.binns@imgtec.com>
> > > Cc: Matt Coster <matt.coster@imgtec.com>
> > > Reviewed-by: Matt Coster <matt.coster@imgtec.com>

Applied to drm-misc-next

Thx
P.

> > 
> > Matt, just to be sure, you're OK with me pulling those two into
> > drm-
> > misc-next, aren't you?
> 
> Yup, all good! Thanks for checking :)
> 

Re: [PATCH 1/2] drm/sched: Add helper to check job dependencies

[Danilo Krummrich]

On 13.01.2025 11:33, Tvrtko Ursulin wrote:
> Lets isolate scheduler internals from drivers such as pvr which currently
> walks the dependency array to look for fences.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@redhat.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <pstanner@redhat.com>
> Reviewed-by: Matt Coster <matt.coster@imgtec.com>

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

> ---
>  drivers/gpu/drm/scheduler/sched_main.c | 23 +++++++++++++++++++++++
>  include/drm/gpu_scheduler.h            |  3 ++-
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 57da84908752..e6f1f2a8e033 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -997,6 +997,29 @@ int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job,
>  }
>  EXPORT_SYMBOL(drm_sched_job_add_implicit_dependencies);
>  
> +/**
> + * drm_sched_job_has_dependency - check whether fence is the job's dependency
> + * @job: scheduler job to check
> + * @fence: fence to look for
> + *
> + * Returns:
> + * True if @fence is found within the job's dependencies, or otherwise false.
> + */
> +bool drm_sched_job_has_dependency(struct drm_sched_job *job,
> +				  struct dma_fence *fence)
> +{
> +	struct dma_fence *f;
> +	unsigned long index;
> +
> +	xa_for_each(&job->dependencies, index, f) {
> +		if (f == fence)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(drm_sched_job_has_dependency);
> +
>  /**
>   * drm_sched_job_cleanup - clean up scheduler job resources
>   * @job: scheduler job to clean up
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 95e17504e46a..d118a0a57ab1 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -577,7 +577,8 @@ int drm_sched_job_add_resv_dependencies(struct drm_sched_job *job,
>  int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job,
>  					    struct drm_gem_object *obj,
>  					    bool write);
> -
> +bool drm_sched_job_has_dependency(struct drm_sched_job *job,
> +				  struct dma_fence *fence);
>  
>  void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
>  				    struct drm_gpu_scheduler **sched_list,

Re: [PATCH 1/2] drm/sched: Add helper to check job dependencies

[Tvrtko Ursulin]

Hi,

On 13/01/2025 13:12, Danilo Krummrich wrote:
> On 13.01.2025 11:33, Tvrtko Ursulin wrote:
>> Lets isolate scheduler internals from drivers such as pvr which currently
>> walks the dependency array to look for fences.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Danilo Krummrich <dakr@redhat.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Philipp Stanner <pstanner@redhat.com>
>> Reviewed-by: Matt Coster <matt.coster@imgtec.com>
> 
> Acked-by: Danilo Krummrich <dakr@kernel.org>

Thanks! Would you also be happy to merge the two?

Regards,

Tvrtko

>> ---
>>   drivers/gpu/drm/scheduler/sched_main.c | 23 +++++++++++++++++++++++
>>   include/drm/gpu_scheduler.h            |  3 ++-
>>   2 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>> index 57da84908752..e6f1f2a8e033 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -997,6 +997,29 @@ int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job,
>>   }
>>   EXPORT_SYMBOL(drm_sched_job_add_implicit_dependencies);
>>   
>> +/**
>> + * drm_sched_job_has_dependency - check whether fence is the job's dependency
>> + * @job: scheduler job to check
>> + * @fence: fence to look for
>> + *
>> + * Returns:
>> + * True if @fence is found within the job's dependencies, or otherwise false.
>> + */
>> +bool drm_sched_job_has_dependency(struct drm_sched_job *job,
>> +				  struct dma_fence *fence)
>> +{
>> +	struct dma_fence *f;
>> +	unsigned long index;
>> +
>> +	xa_for_each(&job->dependencies, index, f) {
>> +		if (f == fence)
>> +			return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +EXPORT_SYMBOL(drm_sched_job_has_dependency);
>> +
>>   /**
>>    * drm_sched_job_cleanup - clean up scheduler job resources
>>    * @job: scheduler job to clean up
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index 95e17504e46a..d118a0a57ab1 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -577,7 +577,8 @@ int drm_sched_job_add_resv_dependencies(struct drm_sched_job *job,
>>   int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job,
>>   					    struct drm_gem_object *obj,
>>   					    bool write);
>> -
>> +bool drm_sched_job_has_dependency(struct drm_sched_job *job,
>> +				  struct dma_fence *fence);
>>   
>>   void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
>>   				    struct drm_gpu_scheduler **sched_list,
>