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

[Luben Tuikov]

On 2020-03-05 01:28, Nirmoy wrote:
> 
> On 3/4/20 11:25 PM, Luben Tuikov wrote:
>> On 2020-03-03 7:50 a.m., Nirmoy Das wrote:
>>> Switch to appropriate sched list for an entity on priority override.
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das at amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 32 +++++++++++++++++++++----
>>>   1 file changed, 28 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> index 8c52152e3a6e..a0bf14ab9d33 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> @@ -522,6 +522,32 @@ struct dma_fence *amdgpu_ctx_get_fence(struct amdgpu_ctx *ctx,
>>>   	return fence;
>>>   }
>>>
>>> +static void amdgpu_ctx_set_entity_priority(struct amdgpu_ctx *ctx,
>>> +				   struct amdgpu_ctx_entity *aentity,
>>> +				   int hw_ip, enum drm_sched_priority priority)
>>> +{
>>> +	struct amdgpu_device *adev = ctx->adev;
>>> +	enum gfx_pipe_priority hw_prio;
>>> +	struct drm_gpu_scheduler **scheds = NULL;
>>> +	unsigned num_scheds;
>>> +
>>> +	/* set sw priority */
>>> +	drm_sched_entity_set_priority(&aentity->entity, priority);
>>> +
>>> +	/* set hw priority */
>>> +	switch (hw_ip) {
>>> +	case AMDGPU_HW_IP_COMPUTE:
>>> +		hw_prio = amdgpu_ctx_sched_prio_to_compute_prio(priority);
>>> +		scheds = adev->gfx.compute_prio_sched[hw_prio];
>>> +		num_scheds = adev->gfx.num_compute_sched[hw_prio];
>>> +		break;
>>> +	default:
>>> +		return;
>>> +	}
>>> +
>>> +	drm_sched_entity_modify_sched(&aentity->entity, scheds, num_scheds);
>>> +}
>> I'd rather this not be over-engineered (in expectations of more case labels,
>> and a simple if-else to do it. Over-engineering it "just in case" creates
>> difficult to maintain code. I believe there is a document about this somewhere
>> in Documentation/.
> 
> This switch is in expectation of extending it for VCN and GFX but I 
> guess that can wait for now,.

No, please, do not over-engineer. Don't make decisions on what the next
programmer and implementation would be.

If you search the LKML you'll see plenty of emails from Linus and
other Linux leaders about the drawbacks of over-engineering.

Just have an if statement as I wrote which you remove without
adding "[snip]". Please add "[snip]" if you're removing content
when you reply. Everyone does this. It's only common courtesy.

Here it is again, because you removed it, conveniently:

You don't need a break only to execute one statement, which you can pull
into the case: label. If you did this you'll see that you just want to do:

static void amdgpu_ctx_set_entity_priority(struct amdgpu_ctx *ctx,
					   struct amdgpu_ctx_entity *aentity,
					   int hw_ip, enum drm_sched_priority priority)
{

	...

	/* Set software priority.
	 */
	drm_sched_entity_set_priority(&aentity->entity, priority);

	/* Set hardware priority.
	 */
	if (hw_ip == AMDGPU_HW_IP_COMPUTE) {
		hw_prio = s2p_prio_map(priority - 2);  ## or perhaps from a static inline from a header file, so we wouldn't care for the - 2 here
		scheds = adev->gfx.compute_prio_sched[hw_prio];
		num_scheds = adev->gfx.num_compute_sched[hw_prio];
		drm_sched_entity_modify_sched(&aentity->entity, scheds, num_scheds);
	}
}

Regards,
Luben


> 
> 
> thanks,
> 
> Nirmoy
>