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....@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
