Re: [RFC PATCH 3/4] drm/amdgpu: change hw sched list on ctx priority override

[Luben Tuikov <luben.tuikov@amd.com>]

On 2020-02-27 4:40 p.m., Nirmoy Das wrote:
> Switch to appropriate sched list for an entity on priority override.
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 54 ++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index a1742b1d4f9c..69a791430b25 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -508,11 +508,53 @@ struct dma_fence *amdgpu_ctx_get_fence(struct amdgpu_ctx *ctx,
>  	return fence;
>  }
>  
> +static int amdgpu_ctx_change_sched(struct amdgpu_ctx *ctx,
> +				   struct amdgpu_ctx_entity *aentity,
> +				   int hw_ip, enum drm_sched_priority priority)
> +{
> +	struct amdgpu_device *adev = ctx->adev;
> +	struct drm_gpu_scheduler **scheds = NULL;
> +	unsigned num_scheds = 0;
> +
> +	switch (hw_ip) {
> +		case AMDGPU_HW_IP_COMPUTE:

NAK. Don't indent the "case".

LKCS says that "case" must not be indented:
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#indentation

> +			if (priority > DRM_SCHED_PRIORITY_NORMAL &&
> +			    adev->gfx.num_compute_sched_high) {
> +				scheds = adev->gfx.compute_sched_high;
> +				num_scheds = adev->gfx.num_compute_sched_high;
> +			} else {
> +				scheds = adev->gfx.compute_sched;
> +				num_scheds = adev->gfx.num_compute_sched;
> +			}

I feel that this is a regression in that we're having an if-conditional
inside a switch-case. Could use just use maps to execute without
any branching?

Surely priority could be used as an index into a map of DRM_SCHED_PRIORITY_MAX
to find out which scheduler to use and the number thereof.

> +			break;
> +		default:
> +			return 0;
> +	}


> +
> +	return drm_sched_entity_modify_sched(&aentity->entity, scheds, num_scheds);
> +}
> +
> +static int amdgpu_ctx_hw_priority_override(struct amdgpu_ctx *ctx,
> +					    const u32 hw_ip,
> +					    enum drm_sched_priority priority)
> +{
> +	int r = 0, i;
> +
> +	for (i = 0; i < amdgpu_ctx_num_entities[hw_ip]; ++i) {
> +		if (!ctx->entities[hw_ip][i])
> +			continue;
> +		r = amdgpu_ctx_change_sched(ctx, ctx->entities[hw_ip][i],
> +					    hw_ip, priority);
> +	}
> +
> +	return r;
> +}
> +
>  void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
>  				  enum drm_sched_priority priority)
>  {
>  	enum drm_sched_priority ctx_prio;
> -	unsigned i, j;
> +	unsigned r, i, j;
>  
>  	ctx->override_priority = priority;
>  
> @@ -521,11 +563,21 @@ void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
>  	for (i = 0; i < AMDGPU_HW_IP_NUM; ++i) {
>  		for (j = 0; j < amdgpu_ctx_num_entities[i]; ++j) {
>  			struct drm_sched_entity *entity;
> +			struct amdgpu_ring *ring;
>  
>  			if (!ctx->entities[i][j])
>  				continue;
>  
>  			entity = &ctx->entities[i][j]->entity;
> +			ring = to_amdgpu_ring(entity->rq->sched);
> +
> +			if (ring->high_priority) {
> +				r = amdgpu_ctx_hw_priority_override(ctx, i,
> +								    ctx_prio);
> +				if (r)
> +					DRM_ERROR("Failed to override HW priority for %s",
> +						  ring->name);
> +			}

I can't see this an an improvement when we add branching inside a for-loop.
Perhaps if we remove if-conditionals and use indexing to eliminate
branching?

Regards,
Luben

>  			drm_sched_entity_set_priority(entity, ctx_prio);
>  		}
>  	}
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx