[PATCH v4] drm/amdgpu: add HW_IP_VCN_UNIFIED type

[Dong, Ruijing]

[AMD Official Use Only - General]

No, we don't plan to clone another one.  I will modify the comment only and remove the bias.

Thanks
Ruijing

-----Original Message-----
From: Koenig, Christian <Christian.Koenig at amd.com>
Sent: Monday, July 18, 2022 10:37 AM
To: Dong, Ruijing <Ruijing.Dong at amd.com>; Liu, Leo <Leo.Liu at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
Subject: Re: [PATCH v4] drm/amdgpu: add HW_IP_VCN_UNIFIED type

Am 18.07.22 um 16:14 schrieb Dong, Ruijing:
> [AMD Official Use Only - General]
>
>>> 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.
> Just to clarify the format difference between legacy encoding and unified queue, we can do either way in the code.
>
> Unified queue requires a different format. The original encoding/decoding format cannot be used in unified queue, which requires to have two new headers, "engine info" to indicate the coming IB package type, encoding or decoding; and the "signature header" to guarantee the coming IB package's integrity, if this failed, the whole IB package will be discarded by VCN FW. That is why I was thinking to have a bias of AMDGPU_HW_IP_VCN_ENC could be better in the future.

Yeah, but the remaining packet format is the same. And as far as I know you guys don't plan do clone that for some reason, don't you?

Adding another name for the same enum value can potentially be much more confusing than the fact that the encode ring now takes a bunch of more commands to do both encoding and decoding.

Regards,
Christian.

>
> Thanks,
> Ruijing
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig at amd.com>
> Sent: Monday, July 18, 2022 9:54 AM
> To: Liu, Leo <Leo.Liu at amd.com>; Dong, Ruijing <Ruijing.Dong at amd.com>;
> amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
> Subject: Re: [PATCH v4] drm/amdgpu: add HW_IP_VCN_UNIFIED type
>
> 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/commit
>>>> s
>>>>
>>>> Signed-off-by: Ruijing Dong <ruijing.dong at 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