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@xxxxxxx> > --- > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx