Re: [PATCH v4] drm/amdgpu: add HW_IP_VCN_UNIFIED type

["Christian König" <christian.koenig@amd.com>]

Am 18.07.22 um 15:48 schrieb Leo Liu:
>
> On 2022-07-18 02:57, Christian König wrote:
>> Am 15.07.22 um 22:04 schrieb Ruijing Dong:
>>>  From VCN4, AMDGPU_HW_IP_VCN_UNIFIED is used to support
>>> both encoding and decoding jobs, it re-uses the same
>>> queue number of AMDGPU_HW_IP_VCN_ENC.
>>>
>>> link: 
>>> https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/245/commits
>>>
>>> Signed-off-by: Ruijing Dong <ruijing.dong@amd.com>
>>> ---
>>>   include/uapi/drm/amdgpu_drm.h | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/include/uapi/drm/amdgpu_drm.h 
>>> b/include/uapi/drm/amdgpu_drm.h
>>> index 18d3246d636e..e268cd3cdb12 100644
>>> --- a/include/uapi/drm/amdgpu_drm.h
>>> +++ b/include/uapi/drm/amdgpu_drm.h
>>> @@ -560,6 +560,12 @@ struct drm_amdgpu_gem_va {
>>>   #define AMDGPU_HW_IP_UVD_ENC      5
>>>   #define AMDGPU_HW_IP_VCN_DEC      6
>>>   #define AMDGPU_HW_IP_VCN_ENC      7
>>> +/**
>>
>> Please don't use "/**" here, that is badly formated for a kerneldoc 
>> comment.
>>
>>> + * From VCN4, AMDGPU_HW_IP_VCN_UNIFIED is used to support
>>> + * both encoding and decoding jobs, it re-uses the same
>>> + * queue number of AMDGPU_HW_IP_VCN_ENC.
>>> + */
>>> +#define AMDGPU_HW_IP_VCN_UNIFIED  AMDGPU_HW_IP_VCN_ENC
>>
>> I'm still in doubt that adding another define with the same value as 
>> AMDGPU_HW_IP_VCN_ENC is a good idea.
>
> Hi Christian,
>
> From VCN4, there is no VCN dec and enc ring type any more, the 
> decode/encode will go through the unified queue, so using 
> AMDGPU_HW_IP_VCN_ENC is no longer accurate . Keeping 
> AMDGPU_HW_IP_VCN_ENC type is for legacy HW, and the new 
> AMDGPU_HW_IP_VCN_UNIFIED just happen to use the same HW ring as legacy 
> encode ring, so reuse the value, and that is the whole idea.

Yeah, I understand your reasoning I just don't see it this way.

What happened is that the encode ring was extended with decode 
functionality. In other words we still use the same format for encoding, 
we just added another one for decoding as well.

Renaming the enum and adding AMDGPU_HW_IP_VCN_UNIFIED suggests that this 
is something completely new, which is not the case here. The encoding 
commands stay the same, don't they?

So to sum it up my suggestion is to stick with AMDGPU_HW_IP_VCN_ENC and 
just document on the definition that this is used for both encode as 
well as decode starting with VCN4.

Regards,
Christian.

>
> Thanks,
>
> Leo
>
>
>>
>>
>> Instead we should just add the comment to AMDGPU_HW_IP_VCN_ENC.
>>
>> Regards,
>> Christian.
>>
>>>   #define AMDGPU_HW_IP_VCN_JPEG     8
>>>   #define AMDGPU_HW_IP_NUM          9
>>