[PATCH 2/5] drm/amdgpu/vcn:set vcn encode ring priority level

[Sharma, Shashank]

On 8/26/2021 6:04 PM, Christian König wrote:
> 
> Am 26.08.21 um 14:31 schrieb Sharma, Shashank:
>> On 8/26/2021 5:54 PM, Christian König wrote:
>>> Am 26.08.21 um 13:32 schrieb Sharma, Shashank:
>>>> Hi Satyajit,
>>>>
>>>> On 8/26/2021 12:43 PM, Satyajit Sahu wrote:
>>>>> There are multiple rings available in VCN encode. Map each ring
>>>>> to different priority.
>>>>>
>>>>> Signed-off-by: Satyajit Sahu <satyajit.sahu at amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 14 ++++++++++++++
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  9 +++++++++
>>>>>   2 files changed, 23 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>>>>> index 6780df0fb265..ce40e7a3ce05 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>>>>> @@ -951,3 +951,17 @@ int amdgpu_vcn_enc_ring_test_ib(struct 
>>>>> amdgpu_ring *ring, long timeout)
>>>>>         return r;
>>>>>   }
>>>>> +
>>>>> +enum vcn_enc_ring_priority amdgpu_vcn_get_enc_ring_prio(int index)
>>>>> +{
>>>>> +    switch(index) {
>>>>> +    case 0:
>>>>
>>>> As discussed in the previous patches, its far better to have MACROS 
>>>> or enums instead of having 0/1/2 cases. As a matter of fact, we can 
>>>> always call it RING_0 RING_1 and so on.
>>>
>>> I strongly disagree. Adding macros or enums just to have names for 
>>> the numbered rings doesn't gives you any advantage at all. That's 
>>> just extra loc.
>>>
>>
>> Honestly, when I just see case '0', its a magic number for me, and is 
>> making code less readable, harder for review, and even harder to 
>> debug. RING_0 tells me that we are mapping a ring to a priority, and 
>> clarifies the intention.
> 
> Well we should probably rename the variable then, e.g. like ring_idx or 
> just ring.
> 
> A switch on the variable named "ring" with a value of 0 has the same 
> meaning than RING_0, it's just not so much code to maintain.
> 
> Christian.

Perfect, sounds as good as anything.

- Shashank

> 
>>
>> - Shashank
>>
>>> We could use the ring pointers to identify a ring instead, but using 
>>> the switch here which is then used inside the init loop is perfectly 
>>> fine.
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>>
>>>> If this is being done just for the traditional reasons, we can have 
>>>> a separate patch to replace it across the driver as well.
>>>>
>>>> - Shashank
>>>>
>>>>
>>>>> +        return AMDGPU_VCN_ENC_PRIO_NORMAL;
>>>>> +    case 1:
>>>>> +        return AMDGPU_VCN_ENC_PRIO_HIGH;
>>>>> +    case 2:
>>>>> +        return AMDGPU_VCN_ENC_PRIO_VERY_HIGH;
>>>>> +    default:
>>>>> +        return AMDGPU_VCN_ENC_PRIO_NORMAL;
>>>>> +    }
>>>>> +}
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>>>>> index d74c62b49795..938ee73dfbfc 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>>>>> @@ -290,6 +290,13 @@ enum vcn_ring_type {
>>>>>       VCN_UNIFIED_RING,
>>>>>   };
>>>>>   +enum vcn_enc_ring_priority {
>>>>> +    AMDGPU_VCN_ENC_PRIO_NORMAL = 1,
>>>>> +    AMDGPU_VCN_ENC_PRIO_HIGH,
>>>>> +    AMDGPU_VCN_ENC_PRIO_VERY_HIGH,
>>>>> +    AMDGPU_VCN_ENC_PRIO_MAX
>>>>> +};
>>>>> +
>>>>>   int amdgpu_vcn_sw_init(struct amdgpu_device *adev);
>>>>>   int amdgpu_vcn_sw_fini(struct amdgpu_device *adev);
>>>>>   int amdgpu_vcn_suspend(struct amdgpu_device *adev);
>>>>> @@ -308,4 +315,6 @@ int amdgpu_vcn_dec_sw_ring_test_ib(struct 
>>>>> amdgpu_ring *ring, long timeout);
>>>>>   int amdgpu_vcn_enc_ring_test_ring(struct amdgpu_ring *ring);
>>>>>   int amdgpu_vcn_enc_ring_test_ib(struct amdgpu_ring *ring, long 
>>>>> timeout);
>>>>>   +enum vcn_enc_ring_priority amdgpu_vcn_get_enc_ring_prio(int index);
>>>>> +
>>>>>   #endif
>>>>>
>>>
>