Re: [PATCH v1 1/2] drm/amdgpu: increment share sched score on entity selection

[Pierre-Eric Pelloux-Prayer <pierre-eric@damsy.net>]

Hi,

Le 01/09/2025 à 11:02, Tvrtko Ursulin a écrit :
> 
> On 22/08/2025 14:43, Pierre-Eric Pelloux-Prayer wrote:
>> For hw engines that can't load balance jobs, entities are
>> "statically" load balanced: on their first submit, they select
>> the best scheduler based on its score.
>> The score is made up of 2 parts:
>> * the job queue depth (how much jobs are executing/waiting)
>> * the number of entities assigned
>>
>> The second part is only relevant for the static load balance:
>> it's a way to consider how many entities are attached to this
>> scheduler, knowing that if they ever submit jobs they will go
>> to this one.
>>
>> For rings that can load balance jobs freely, idle entities
>> aren't a concern and shouldn't impact the scheduler's decisions.
>>
>> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 23 ++++++++++++++++++-----
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h |  1 +
>>   2 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/ 
>> amdgpu/amdgpu_ctx.c
>> index f5d5c45ddc0d..4a078d2d98c5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> @@ -206,9 +206,11 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, 
>> u32 hw_ip,
>>   {
>>       struct drm_gpu_scheduler **scheds = NULL, *sched = NULL;
>>       struct amdgpu_device *adev = ctx->mgr->adev;
>> +    bool static_load_balancing = false;
>>       struct amdgpu_ctx_entity *entity;
>>       enum drm_sched_priority drm_prio;
>>       unsigned int hw_prio, num_scheds;
>> +    struct amdgpu_ring *aring;
>>       int32_t ctx_prio;
>>       int r;
>> @@ -236,17 +238,22 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx 
>> *ctx, u32 hw_ip,
>>           r = amdgpu_xcp_select_scheds(adev, hw_ip, hw_prio, fpriv,
>>                           &num_scheds, &scheds);
>>           if (r)
>> -            goto cleanup_entity;
>> +            goto error_free_entity;
> 
> Indeed, previously this was calling drm_sched_entity_fini() before 
> drm_sched_entity_init() and it only worked because of kzalloc.
> 
>>       }
>>       /* disable load balance if the hw engine retains context among dependent 
>> jobs */
>> -    if (hw_ip == AMDGPU_HW_IP_VCN_ENC ||
>> -        hw_ip == AMDGPU_HW_IP_VCN_DEC ||
>> -        hw_ip == AMDGPU_HW_IP_UVD_ENC ||
>> -        hw_ip == AMDGPU_HW_IP_UVD) {
>> +    static_load_balancing = hw_ip == AMDGPU_HW_IP_VCN_ENC ||
>> +                hw_ip == AMDGPU_HW_IP_VCN_DEC ||
>> +                hw_ip == AMDGPU_HW_IP_UVD_ENC ||
>> +                hw_ip == AMDGPU_HW_IP_UVD;
>> +
>> +    if (static_load_balancing) {
>>           sched = drm_sched_pick_best(scheds, num_scheds);
>>           scheds = &sched;
>>           num_scheds = 1;
>> +        aring = container_of(sched, struct amdgpu_ring, sched);
>> +        entity->sched_ring_score = aring->sched_score;
>> +        atomic_inc(entity->sched_ring_score);
> 
> If we were to bike-shed we could find a way to avoid the new local variables. 
> Keeping the if as is and assign to entity->sched_ring_score directly, and then 
> checking for that on the cleanup path. Still works due kzalloc. Or if relying on 
> kzalloc is not desired, at least bool static_load_balance could be replaced by 
> re-naming the aring local as static_aring and using it like the name suggests.
> 
> Could also move the atomic_inc to the success path to avoid having to add code 
> to error unwind.
> 

Christian, Alex: any preferences on the code style to use here?

> Both cases are I think equally racy in the sense that parallel 
> amdgpu_ctx_init_entity invocations can all pick the same sched. But that is true 
> today AFAICT because score is not incremented until later in the job submit 
> process.
> 
> I suppose one way to make the assignment more robust would be to "rotate" (or 
> randomize) the sched list atomically before calling drm_sched_pick_best. Thoughts?
> 

IMO without the ability to do job level load balancing, there will always be 
cases where the scheduler choice is not optimal. For this reason I prefered to 
keep the code as simple as possible.

Thanks,
Pierre-Eric


> Regards,
> 
> Tvrtko
> 
>>       }
>>       r = drm_sched_entity_init(&entity->entity, drm_prio, scheds, num_scheds,
>> @@ -264,6 +271,9 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, 
>> u32 hw_ip,
>>       drm_sched_entity_fini(&entity->entity);
>>   error_free_entity:
>> +    if (static_load_balancing)
>> +        atomic_dec(entity->sched_ring_score);
>> +
>>       kfree(entity);
>>       return r;
>> @@ -514,6 +524,9 @@ static void amdgpu_ctx_do_release(struct kref *ref)
>>               if (!ctx->entities[i][j])
>>                   continue;
>> +            if (ctx->entities[i][j]->sched_ring_score)
>> +                atomic_dec(ctx->entities[i][j]->sched_ring_score);
>> +
>>               drm_sched_entity_destroy(&ctx->entities[i][j]->entity);
>>           }
>>       }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h b/drivers/gpu/drm/amd/ 
>> amdgpu/amdgpu_ctx.h
>> index 090dfe86f75b..076a0e165ce0 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
>> @@ -39,6 +39,7 @@ struct amdgpu_ctx_entity {
>>       uint32_t        hw_ip;
>>       uint64_t        sequence;
>>       struct drm_sched_entity    entity;
>> +    atomic_t        *sched_ring_score;
>>       struct dma_fence    *fences[];
>>   };