Re: [PATCH v5 06/16] drm/sched: Consolidate drm_sched_rq_select_entity_rr

["Maíra Canal" <mcanal@igalia.com>]

Hi Tvrtko,

On 23/06/25 09:27, Tvrtko Ursulin wrote:
> Extract out two copies of the identical code to function epilogue to make
> it smaller and more readable.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <phasta@kernel.org>
> ---
>   drivers/gpu/drm/scheduler/sched_main.c | 48 +++++++++++---------------
>   1 file changed, 20 insertions(+), 28 deletions(-)
> 

I'd consider dropping this patch. The function
drm_sched_rq_select_entity_rr() will be deleted in the next patch
anyway. Therefore, we could land 7/16 in drm-misc-next (even before
landing fair scheduling) and get rid of drm_sched_rq_select_entity_rr().

Best Regards,
- Maíra

> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 0627a82c02dc..436d58073823 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -263,38 +263,14 @@ drm_sched_rq_select_entity_rr(struct drm_gpu_scheduler *sched,
>   	entity = rq->current_entity;
>   	if (entity) {
>   		list_for_each_entry_continue(entity, &rq->entities, list) {
> -			if (drm_sched_entity_is_ready(entity)) {
> -				/* If we can't queue yet, preserve the current
> -				 * entity in terms of fairness.
> -				 */
> -				if (!drm_sched_can_queue(sched, entity)) {
> -					spin_unlock(&rq->lock);
> -					return ERR_PTR(-ENOSPC);
> -				}
> -
> -				rq->current_entity = entity;
> -				reinit_completion(&entity->entity_idle);
> -				spin_unlock(&rq->lock);
> -				return entity;
> -			}
> +			if (drm_sched_entity_is_ready(entity))
> +				goto found;
>   		}
>   	}
>   
>   	list_for_each_entry(entity, &rq->entities, list) {
> -		if (drm_sched_entity_is_ready(entity)) {
> -			/* If we can't queue yet, preserve the current entity in
> -			 * terms of fairness.
> -			 */
> -			if (!drm_sched_can_queue(sched, entity)) {
> -				spin_unlock(&rq->lock);
> -				return ERR_PTR(-ENOSPC);
> -			}
> -
> -			rq->current_entity = entity;
> -			reinit_completion(&entity->entity_idle);
> -			spin_unlock(&rq->lock);
> -			return entity;
> -		}
> +		if (drm_sched_entity_is_ready(entity))
> +			goto found;
>   
>   		if (entity == rq->current_entity)
>   			break;
> @@ -303,6 +279,22 @@ drm_sched_rq_select_entity_rr(struct drm_gpu_scheduler *sched,
>   	spin_unlock(&rq->lock);
>   
>   	return NULL;
> +
> +found:
> +	if (!drm_sched_can_queue(sched, entity)) {
> +		/*
> +		 * If scheduler cannot take more jobs signal the caller to not
> +		 * consider lower priority queues.
> +		 */
> +		entity = ERR_PTR(-ENOSPC);
> +	} else {
> +		rq->current_entity = entity;
> +		reinit_completion(&entity->entity_idle);
> +	}
> +
> +	spin_unlock(&rq->lock);
> +
> +	return entity;
>   }
>   
>   /**