Re: [PATCH 6.12.y 1/3] drm/sched: Optimise drm_sched_entity_push_job

["Christian König" <christian.koenig@amd.com>]

On 22.09.25 17:30, Philipp Stanner wrote:
> On Mon, 2025-09-22 at 15:09 +0200, Jules Maselbas wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>
>> commit d42a254633c773921884a19e8a1a0f53a31150c3 upstream.
>>
>> In FIFO mode (which is the default), both drm_sched_entity_push_job() and
>> drm_sched_rq_update_fifo(), where the latter calls the former, are
>> currently taking and releasing the same entity->rq_lock.
>>
>> We can avoid that design inelegance, and also have a miniscule
>> efficiency improvement on the submit from idle path, by introducing a new
>> drm_sched_rq_update_fifo_locked() helper and pulling up the lock taking to
>> its callers.
>>
>> v2:
>>  * Remove drm_sched_rq_update_fifo() altogether. (Christian)
>>
>> v3:
>>  * Improved commit message. (Philipp)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Luben Tuikov <ltuikov89@gmail.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Philipp Stanner <pstanner@redhat.com>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Philipp Stanner <pstanner@redhat.com>
>> Link: https://patchwork.freedesktop.org/patch/msgid/20241016122013.7857-2-tursulin@igalia.com
>> (cherry picked from commit d42a254633c773921884a19e8a1a0f53a31150c3)
>> Signed-off-by: Jules Maselbas <jmaselbas@zdiv.net>
> 
> Am I interpreting this mail correctly: you want to get this patch into
> stable?
> 
> Why? It doesn't fix a bug.

Patch #3 in this series depends on the other two, but I agree that isn't a good idea.

We should just adjust patch #3 to apply on the older kernel as well instead of backporting patches #1 and #2.

Regards,
Christian.

> 
> 
> P.
> 
>> ---
>>  drivers/gpu/drm/scheduler/sched_entity.c | 13 +++++++++----
>>  drivers/gpu/drm/scheduler/sched_main.c   |  6 +++---
>>  include/drm/gpu_scheduler.h              |  2 +-
>>  3 files changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
>> index 3e75fc1f6607..9dbae7b08bc9 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -505,8 +505,12 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
>>  		struct drm_sched_job *next;
>>  
>>  		next = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
>> -		if (next)
>> -			drm_sched_rq_update_fifo(entity, next->submit_ts);
>> +		if (next) {
>> +			spin_lock(&entity->rq_lock);
>> +			drm_sched_rq_update_fifo_locked(entity,
>> +							next->submit_ts);
>> +			spin_unlock(&entity->rq_lock);
>> +		}
>>  	}
>>  
>>  	/* Jobs and entities might have different lifecycles. Since we're
>> @@ -606,10 +610,11 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
>>  		sched = rq->sched;
>>  
>>  		drm_sched_rq_add_entity(rq, entity);
>> -		spin_unlock(&entity->rq_lock);
>>  
>>  		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>> -			drm_sched_rq_update_fifo(entity, submit_ts);
>> +			drm_sched_rq_update_fifo_locked(entity, submit_ts);
>> +
>> +		spin_unlock(&entity->rq_lock);
>>  
>>  		drm_sched_wakeup(sched);
>>  	}
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>> index 416590ea0dc3..3609d5a8fecd 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -169,14 +169,15 @@ static inline void drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *enti
>>  	}
>>  }
>>  
>> -void drm_sched_rq_update_fifo(struct drm_sched_entity *entity, ktime_t ts)
>> +void drm_sched_rq_update_fifo_locked(struct drm_sched_entity *entity, ktime_t ts)
>>  {
>>  	/*
>>  	 * Both locks need to be grabbed, one to protect from entity->rq change
>>  	 * for entity from within concurrent drm_sched_entity_select_rq and the
>>  	 * other to update the rb tree structure.
>>  	 */
>> -	spin_lock(&entity->rq_lock);
>> +	lockdep_assert_held(&entity->rq_lock);
>> +
>>  	spin_lock(&entity->rq->lock);
>>  
>>  	drm_sched_rq_remove_fifo_locked(entity);
>> @@ -187,7 +188,6 @@ void drm_sched_rq_update_fifo(struct drm_sched_entity *entity, ktime_t ts)
>>  		      drm_sched_entity_compare_before);
>>  
>>  	spin_unlock(&entity->rq->lock);
>> -	spin_unlock(&entity->rq_lock);
>>  }
>>  
>>  /**
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index 9c437a057e5d..346a3c261b43 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -593,7 +593,7 @@ void drm_sched_rq_add_entity(struct drm_sched_rq *rq,
>>  void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
>>  				struct drm_sched_entity *entity);
>>  
>> -void drm_sched_rq_update_fifo(struct drm_sched_entity *entity, ktime_t ts);
>> +void drm_sched_rq_update_fifo_locked(struct drm_sched_entity *entity, ktime_t ts);
>>  
>>  int drm_sched_entity_init(struct drm_sched_entity *entity,
>>  			  enum drm_sched_priority priority,
>