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

[Sahu, Satyajit]

On 8/26/2021 6:06 PM, Sharma, Shashank wrote:
>
>
> 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

I'll take care in v2.

regards,

Satyajit

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