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

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

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