Re: [PATCH v5 09/28] drm/sched: Add fair scheduling policy

["Danilo Krummrich" <dakr@kernel.org>]

On Fri Dec 19, 2025 at 2:53 PM CET, Tvrtko Ursulin wrote:
> diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
> index 2d1f579d8352..2fde309d02a6 100644
> --- a/drivers/gpu/drm/scheduler/sched_rq.c
> +++ b/drivers/gpu/drm/scheduler/sched_rq.c
> @@ -16,6 +16,35 @@ drm_sched_entity_compare_before(struct rb_node *a, const struct rb_node *b)
>  	return ktime_before(ea->oldest_job_waiting, eb->oldest_job_waiting);
>  }
>  
> +static void drm_sched_rq_update_prio(struct drm_sched_rq *rq)
> +{
> +	enum drm_sched_priority prio = DRM_SCHED_PRIORITY_INVALID;
> +	struct rb_node *rb;
> +
> +	lockdep_assert_held(&rq->lock);
> +
> +	rb = rb_first_cached(&rq->rb_tree_root);
> +	if (rb) {
> +		struct drm_sched_entity *entity =
> +			rb_entry(rb, typeof(*entity), rb_tree_node);
> +
> +		/*
> +		 * The normal locking order is entity then run-queue so taking
> +		 * the entity lock here would be a locking inversion for the
> +		 * case when the current head of the run-queue is different from
> +		 * the one we already have locked. The unlocked read is fine
> +		 * though, because if the priority had just changed it is no big
> +		 * deal for our algorithm, but just a transient reachable only
> +		 * by drivers with userspace dynamic priority changes API. Equal
> +		 * in effect to the priority change becoming visible a few
> +		 * instructions later.
> +		 */
> +		prio = READ_ONCE(entity->priority);

I still think that we should address the root cause of the lock inversion
problem instead.

I previously mentioned that I can take a look at this beginning of this year,
which I can do soon.

In the meantime, can you please explain what's the problem with this specific
case? This function is only ever called from drm_sched_rq_remove_fifo_locked()
and drm_sched_rq_update_fifo_locked(), which already seem to hold both locks.

> +	}
> +
> +	rq->head_prio = prio;
> +}
> +
>  static void drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
>  					    struct drm_sched_rq *rq)
>  {
> @@ -25,6 +54,7 @@ static void drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
>  	if (!RB_EMPTY_NODE(&entity->rb_tree_node)) {
>  		rb_erase_cached(&entity->rb_tree_node, &rq->rb_tree_root);
>  		RB_CLEAR_NODE(&entity->rb_tree_node);
> +		drm_sched_rq_update_prio(rq);
>  	}
>  }
>  
> @@ -46,6 +76,7 @@ static void drm_sched_rq_update_fifo_locked(struct drm_sched_entity *entity,
>  
>  	rb_add_cached(&entity->rb_tree_node, &rq->rb_tree_root,
>  		      drm_sched_entity_compare_before);
> +	drm_sched_rq_update_prio(rq);
>  }