[PATCH v4] drm/amdgpu: add HW_IP_VCN_UNIFIED type

[PATCH v4] drm/amdgpu: add HW_IP_VCN_UNIFIED type

[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
+/**
+ * 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
 #define AMDGPU_HW_IP_VCN_JPEG     8
 #define AMDGPU_HW_IP_NUM          9
 
-- 
2.25.1

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

[Liu, Leo]

[AMD Official Use Only - General]

Reviewed-by: Leo Liu <leo.liu@amd.com>

-----Original Message-----
From: Dong, Ruijing <Ruijing.Dong@amd.com>
Sent: July 15, 2022 4:04 PM
To: Koenig, Christian <Christian.Koenig@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Liu, Leo <Leo.Liu@amd.com>; Dong, Ruijing <Ruijing.Dong@amd.com>
Subject: [PATCH v4] drm/amdgpu: add HW_IP_VCN_UNIFIED type

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
+/**
+ * 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
 #define AMDGPU_HW_IP_VCN_JPEG     8
 #define AMDGPU_HW_IP_NUM          9

--
2.25.1

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

[Christian König]

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.

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
>   

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

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

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
>

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

[Christian König]

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

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

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

Thanks,
Ruijing

-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Monday, July 18, 2022 9:54 AM
To: Liu, Leo <Leo.Liu@amd.com>; Dong, Ruijing <Ruijing.Dong@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@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/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
>>

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

[Christian König]

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@amd.com>
> Sent: Monday, July 18, 2022 9:54 AM
> To: Liu, Leo <Leo.Liu@amd.com>; Dong, Ruijing <Ruijing.Dong@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@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/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

RE: [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@amd.com>
Sent: Monday, July 18, 2022 10:37 AM
To: Dong, Ruijing <Ruijing.Dong@amd.com>; Liu, Leo <Leo.Liu@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@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@amd.com>
> Sent: Monday, July 18, 2022 9:54 AM
> To: Liu, Leo <Leo.Liu@amd.com>; Dong, Ruijing <Ruijing.Dong@amd.com>;
> amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@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@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