Hi,
On Mon, 2024-10-14 at 11:46 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx>
>
> In FIFO mode We can avoid dropping the lock only to immediately re-
> acquire
> by adding a new drm_sched_rq_update_fifo_locked() helper.
>
Please write detailed commit messages, as described here [1].
1. Describe the problem: current state and why it's bad.
2. Then, describe in imperative (present tense) form what the commit
does about the problem.
Optionally, in between can be information about why it's solved this
way and not another etc.
Applies to the other patches, too.
[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
> v2:
> * Remove drm_sched_rq_update_fifo() altogether. (Christian)
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx>
> Cc: Christian König <christian.koenig@xxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Cc: Luben Tuikov <ltuikov89@xxxxxxxxx>
> Cc: Matthew Brost <matthew.brost@xxxxxxxxx>
> Cc: Philipp Stanner <pstanner@xxxxxxxxxx>
> Reviewed-by: Christian König <christian.koenig@xxxxxxx>
> ---
> 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 2951fcc2e6b1..b72cba292839 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -514,8 +514,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
> @@ -613,10 +617,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 e32b0f7d7e94..bbd1630407e4 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)
Since you touch function name / signature already, would you mind
writing a small doc string that also mentions the locking requirements
or lack of the same?
> {
> /*
> * 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.
> */
It seems to me that the comment above is now out of date, no?
Thx for your efforts,
P.
> - 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 e9f075f51db3..3658a6cb048e 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,
