[PATCH 1/5] drm/amdgpu/vce:set vce ring priority level

[PATCH 1/5] drm/amdgpu/vce:set vce ring priority level

[Satyajit Sahu]

There are multiple rings available in VCE. Map each ring
to different priority.

Signed-off-by: Satyajit Sahu <satyajit.sahu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c | 14 ++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h | 14 ++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
index 1ae7f824adc7..b68411caeac2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
@@ -1168,3 +1168,17 @@ int amdgpu_vce_ring_test_ib(struct amdgpu_ring *ring, long timeout)
 	amdgpu_bo_free_kernel(&bo, NULL, NULL);
 	return r;
 }
+
+enum vce_enc_ring_priority amdgpu_vce_get_ring_prio(int index)
+{
+	switch(index) {
+	case AMDGPU_VCE_GENERAL_PURPOSE:
+		return AMDGPU_VCE_ENC_PRIO_NORMAL;
+	case AMDGPU_VCE_LOW_LATENCY:
+		return AMDGPU_VCE_ENC_PRIO_HIGH;
+	case AMDGPU_VCE_REALTIME:
+		return AMDGPU_VCE_ENC_PRIO_VERY_HIGH;
+	default:
+		return AMDGPU_VCE_ENC_PRIO_NORMAL;
+	}
+}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
index d6d83a3ec803..60525887e9e3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
@@ -32,6 +32,19 @@
 
 #define AMDGPU_VCE_FW_53_45	((53 << 24) | (45 << 16))
 
+enum vce_enc_ring_priority {
+	AMDGPU_VCE_ENC_PRIO_NORMAL = 1,
+	AMDGPU_VCE_ENC_PRIO_HIGH,
+	AMDGPU_VCE_ENC_PRIO_VERY_HIGH,
+	AMDGPU_VCE_ENC_PRIO_MAX
+};
+
+enum vce_enc_ring_type {
+	AMDGPU_VCE_GENERAL_PURPOSE,
+	AMDGPU_VCE_LOW_LATENCY,
+	AMDGPU_VCE_REALTIME
+};
+
 struct amdgpu_vce {
 	struct amdgpu_bo	*vcpu_bo;
 	uint64_t		gpu_addr;
@@ -71,5 +84,6 @@ void amdgpu_vce_ring_begin_use(struct amdgpu_ring *ring);
 void amdgpu_vce_ring_end_use(struct amdgpu_ring *ring);
 unsigned amdgpu_vce_ring_get_emit_ib_size(struct amdgpu_ring *ring);
 unsigned amdgpu_vce_ring_get_dma_frame_size(struct amdgpu_ring *ring);
+enum vce_enc_ring_priority amdgpu_vce_get_ring_prio(int index);
 
 #endif
-- 
2.25.1

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

[Satyajit Sahu]

There are multiple rings available in VCN encode. Map each ring
to different priority.

Signed-off-by: Satyajit Sahu <satyajit.sahu@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:
+		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
-- 
2.25.1

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

[Christian König]

Am 26.08.21 um 09:13 schrieb Satyajit Sahu:
> There are multiple rings available in VCN encode. Map each ring
> to different priority.
>
> Signed-off-by: Satyajit Sahu <satyajit.sahu@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:
> +		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 {

Please name that amdgpu_vcn_...

Christian.

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

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

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

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
> 

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

[Christian König]

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

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

Re: [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@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
>>>
> 

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

[Christian König]

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

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

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

[Sharma, Shashank]

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

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

Re: [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@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
>>>>>>
>>>>
>>

[PATCH 3/5] drm/amdgpu/vce:set ring priorities

[Satyajit Sahu]

Set proper ring priority while initializing the ring.

Signed-off-by: Satyajit Sahu <satyajit.sahu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/vce_v2_0.c | 4 +++-
 drivers/gpu/drm/amd/amdgpu/vce_v3_0.c | 4 +++-
 drivers/gpu/drm/amd/amdgpu/vce_v4_0.c | 4 +++-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c b/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
index c7d28c169be5..8ce37e2d5ffd 100644
--- a/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
@@ -431,10 +431,12 @@ static int vce_v2_0_sw_init(void *handle)
 		return r;
 
 	for (i = 0; i < adev->vce.num_rings; i++) {
+		unsigned int hw_prio = amdgpu_vce_get_ring_prio(i);
+
 		ring = &adev->vce.ring[i];
 		sprintf(ring->name, "vce%d", i);
 		r = amdgpu_ring_init(adev, ring, 512, &adev->vce.irq, 0,
-				     AMDGPU_RING_PRIO_DEFAULT, NULL);
+				     hw_prio, NULL);
 		if (r)
 			return r;
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
index 3b82fb289ef6..e0bc42e1e2b3 100644
--- a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
@@ -440,10 +440,12 @@ static int vce_v3_0_sw_init(void *handle)
 		return r;
 
 	for (i = 0; i < adev->vce.num_rings; i++) {
+		unsigned int hw_prio = amdgpu_vce_get_ring_prio(i);
+
 		ring = &adev->vce.ring[i];
 		sprintf(ring->name, "vce%d", i);
 		r = amdgpu_ring_init(adev, ring, 512, &adev->vce.irq, 0,
-				     AMDGPU_RING_PRIO_DEFAULT, NULL);
+				     hw_prio, NULL);
 		if (r)
 			return r;
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
index 90910d19db12..931d3ae09c65 100644
--- a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
@@ -463,6 +463,8 @@ static int vce_v4_0_sw_init(void *handle)
 	}
 
 	for (i = 0; i < adev->vce.num_rings; i++) {
+		unsigned int hw_prio = amdgpu_vce_get_ring_prio(i);
+
 		ring = &adev->vce.ring[i];
 		sprintf(ring->name, "vce%d", i);
 		if (amdgpu_sriov_vf(adev)) {
@@ -478,7 +480,7 @@ static int vce_v4_0_sw_init(void *handle)
 				ring->doorbell_index = adev->doorbell_index.uvd_vce.vce_ring2_3 * 2 + 1;
 		}
 		r = amdgpu_ring_init(adev, ring, 512, &adev->vce.irq, 0,
-				     AMDGPU_RING_PRIO_DEFAULT, NULL);
+				     hw_prio, NULL);
 		if (r)
 			return r;
 	}
-- 
2.25.1

Re: [PATCH 3/5] drm/amdgpu/vce:set ring priorities

[Christian König]

Am 26.08.21 um 09:13 schrieb Satyajit Sahu:
> Set proper ring priority while initializing the ring.

Might be merged with patch #1, apart from that looks good to me.

Christian.

>
> Signed-off-by: Satyajit Sahu <satyajit.sahu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/vce_v2_0.c | 4 +++-
>   drivers/gpu/drm/amd/amdgpu/vce_v3_0.c | 4 +++-
>   drivers/gpu/drm/amd/amdgpu/vce_v4_0.c | 4 +++-
>   3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c b/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
> index c7d28c169be5..8ce37e2d5ffd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
> @@ -431,10 +431,12 @@ static int vce_v2_0_sw_init(void *handle)
>   		return r;
>   
>   	for (i = 0; i < adev->vce.num_rings; i++) {
> +		unsigned int hw_prio = amdgpu_vce_get_ring_prio(i);
> +
>   		ring = &adev->vce.ring[i];
>   		sprintf(ring->name, "vce%d", i);
>   		r = amdgpu_ring_init(adev, ring, 512, &adev->vce.irq, 0,
> -				     AMDGPU_RING_PRIO_DEFAULT, NULL);
> +				     hw_prio, NULL);
>   		if (r)
>   			return r;
>   	}
> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> index 3b82fb289ef6..e0bc42e1e2b3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> @@ -440,10 +440,12 @@ static int vce_v3_0_sw_init(void *handle)
>   		return r;
>   
>   	for (i = 0; i < adev->vce.num_rings; i++) {
> +		unsigned int hw_prio = amdgpu_vce_get_ring_prio(i);
> +
>   		ring = &adev->vce.ring[i];
>   		sprintf(ring->name, "vce%d", i);
>   		r = amdgpu_ring_init(adev, ring, 512, &adev->vce.irq, 0,
> -				     AMDGPU_RING_PRIO_DEFAULT, NULL);
> +				     hw_prio, NULL);
>   		if (r)
>   			return r;
>   	}
> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> index 90910d19db12..931d3ae09c65 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> @@ -463,6 +463,8 @@ static int vce_v4_0_sw_init(void *handle)
>   	}
>   
>   	for (i = 0; i < adev->vce.num_rings; i++) {
> +		unsigned int hw_prio = amdgpu_vce_get_ring_prio(i);
> +
>   		ring = &adev->vce.ring[i];
>   		sprintf(ring->name, "vce%d", i);
>   		if (amdgpu_sriov_vf(adev)) {
> @@ -478,7 +480,7 @@ static int vce_v4_0_sw_init(void *handle)
>   				ring->doorbell_index = adev->doorbell_index.uvd_vce.vce_ring2_3 * 2 + 1;
>   		}
>   		r = amdgpu_ring_init(adev, ring, 512, &adev->vce.irq, 0,
> -				     AMDGPU_RING_PRIO_DEFAULT, NULL);
> +				     hw_prio, NULL);
>   		if (r)
>   			return r;
>   	}

Re: [PATCH 3/5] drm/amdgpu/vce:set ring priorities

[Sharma, Shashank]

Hi Satyajit,

On 8/26/2021 1:51 PM, Christian König wrote:
> Am 26.08.21 um 09:13 schrieb Satyajit Sahu:
>> Set proper ring priority while initializing the ring.
> 
> Might be merged with patch #1, apart from that looks good to me.
> 
> Christian.

Actually it was my suggestion to him to split the patch in such a way 
that all IP sw_init changes to go into single patch, as patch 1 was 
getting too big with that.

If it is not a problem with Christian, LGTM
Feel free to use: Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>

- Shashank
> 
>>
>> Signed-off-by: Satyajit Sahu <satyajit.sahu@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/vce_v2_0.c | 4 +++-
>>   drivers/gpu/drm/amd/amdgpu/vce_v3_0.c | 4 +++-
>>   drivers/gpu/drm/amd/amdgpu/vce_v4_0.c | 4 +++-
>>   3 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c 
>> b/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
>> index c7d28c169be5..8ce37e2d5ffd 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
>> @@ -431,10 +431,12 @@ static int vce_v2_0_sw_init(void *handle)
>>           return r;
>>       for (i = 0; i < adev->vce.num_rings; i++) {
>> +        unsigned int hw_prio = amdgpu_vce_get_ring_prio(i);
>> +
>>           ring = &adev->vce.ring[i];
>>           sprintf(ring->name, "vce%d", i);
>>           r = amdgpu_ring_init(adev, ring, 512, &adev->vce.irq, 0,
>> -                     AMDGPU_RING_PRIO_DEFAULT, NULL);
>> +                     hw_prio, NULL);
>>           if (r)
>>               return r;
>>       }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c 
>> b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
>> index 3b82fb289ef6..e0bc42e1e2b3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
>> @@ -440,10 +440,12 @@ static int vce_v3_0_sw_init(void *handle)
>>           return r;
>>       for (i = 0; i < adev->vce.num_rings; i++) {
>> +        unsigned int hw_prio = amdgpu_vce_get_ring_prio(i);
>> +
>>           ring = &adev->vce.ring[i];
>>           sprintf(ring->name, "vce%d", i);
>>           r = amdgpu_ring_init(adev, ring, 512, &adev->vce.irq, 0,
>> -                     AMDGPU_RING_PRIO_DEFAULT, NULL);
>> +                     hw_prio, NULL);
>>           if (r)
>>               return r;
>>       }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c 
>> b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
>> index 90910d19db12..931d3ae09c65 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
>> @@ -463,6 +463,8 @@ static int vce_v4_0_sw_init(void *handle)
>>       }
>>       for (i = 0; i < adev->vce.num_rings; i++) {
>> +        unsigned int hw_prio = amdgpu_vce_get_ring_prio(i);
>> +
>>           ring = &adev->vce.ring[i];
>>           sprintf(ring->name, "vce%d", i);
>>           if (amdgpu_sriov_vf(adev)) {
>> @@ -478,7 +480,7 @@ static int vce_v4_0_sw_init(void *handle)
>>                   ring->doorbell_index = 
>> adev->doorbell_index.uvd_vce.vce_ring2_3 * 2 + 1;
>>           }
>>           r = amdgpu_ring_init(adev, ring, 512, &adev->vce.irq, 0,
>> -                     AMDGPU_RING_PRIO_DEFAULT, NULL);
>> +                     hw_prio, NULL);
>>           if (r)
>>               return r;
>>       }
> 

[PATCH 4/5] drm/amdgpu/vcn:set ring priorities

[Satyajit Sahu]

Set proper ring priority while initializing the ring.

Signed-off-by: Satyajit Sahu <satyajit.sahu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 4 +++-
 drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c | 4 +++-
 drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c | 4 +++-
 drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c | 5 +++--
 4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
index 284bb42d6c86..51c46c9e7e0d 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
@@ -145,10 +145,12 @@ static int vcn_v1_0_sw_init(void *handle)
 		SOC15_REG_OFFSET(UVD, 0, mmUVD_NO_OP);
 
 	for (i = 0; i < adev->vcn.num_enc_rings; ++i) {
+		unsigned int hw_prio = amdgpu_vcn_get_enc_ring_prio(i);
+
 		ring = &adev->vcn.inst->ring_enc[i];
 		sprintf(ring->name, "vcn_enc%d", i);
 		r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst->irq, 0,
-				     AMDGPU_RING_PRIO_DEFAULT, NULL);
+				     hw_prio, NULL);
 		if (r)
 			return r;
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
index 8af567c546db..720a69322f7c 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
@@ -159,6 +159,8 @@ static int vcn_v2_0_sw_init(void *handle)
 	adev->vcn.inst->external.nop = SOC15_REG_OFFSET(UVD, 0, mmUVD_NO_OP);
 
 	for (i = 0; i < adev->vcn.num_enc_rings; ++i) {
+		unsigned int hw_prio = amdgpu_vcn_get_enc_ring_prio(i);
+
 		ring = &adev->vcn.inst->ring_enc[i];
 		ring->use_doorbell = true;
 		if (!amdgpu_sriov_vf(adev))
@@ -167,7 +169,7 @@ static int vcn_v2_0_sw_init(void *handle)
 			ring->doorbell_index = (adev->doorbell_index.vcn.vcn_ring0_1 << 1) + 1 + i;
 		sprintf(ring->name, "vcn_enc%d", i);
 		r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst->irq, 0,
-				     AMDGPU_RING_PRIO_DEFAULT, NULL);
+				     hw_prio, NULL);
 		if (r)
 			return r;
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
index 888b17d84691..6837f5fc729e 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
@@ -194,6 +194,8 @@ static int vcn_v2_5_sw_init(void *handle)
 			return r;
 
 		for (i = 0; i < adev->vcn.num_enc_rings; ++i) {
+			unsigned int hw_prio = amdgpu_vcn_get_enc_ring_prio(i);
+
 			ring = &adev->vcn.inst[j].ring_enc[i];
 			ring->use_doorbell = true;
 
@@ -203,7 +205,7 @@ static int vcn_v2_5_sw_init(void *handle)
 			sprintf(ring->name, "vcn_enc_%d.%d", j, i);
 			r = amdgpu_ring_init(adev, ring, 512,
 					     &adev->vcn.inst[j].irq, 0,
-					     AMDGPU_RING_PRIO_DEFAULT, NULL);
+					     hw_prio, NULL);
 			if (r)
 				return r;
 		}
diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
index 47d4f04cbd69..e6e5d476ae9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
@@ -227,6 +227,8 @@ static int vcn_v3_0_sw_init(void *handle)
 			return r;
 
 		for (j = 0; j < adev->vcn.num_enc_rings; ++j) {
+			unsigned int hw_prio = amdgpu_vcn_get_enc_ring_prio(j);
+
 			/* VCN ENC TRAP */
 			r = amdgpu_irq_add_id(adev, amdgpu_ih_clientid_vcns[i],
 				j + VCN_2_0__SRCID__UVD_ENC_GENERAL_PURPOSE, &adev->vcn.inst[i].irq);
@@ -242,8 +244,7 @@ static int vcn_v3_0_sw_init(void *handle)
 			}
 			sprintf(ring->name, "vcn_enc_%d.%d", i, j);
 			r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst[i].irq, 0,
-					     AMDGPU_RING_PRIO_DEFAULT,
-					     &adev->vcn.inst[i].sched_score);
+					     hw_prio, &adev->vcn.inst[i].sched_score);
 			if (r)
 				return r;
 		}
-- 
2.25.1

Re: [PATCH 4/5] drm/amdgpu/vcn:set ring priorities

[Sharma, Shashank]

On 8/26/2021 12:43 PM, Satyajit Sahu wrote:
> Set proper ring priority while initializing the ring.
> 
> Signed-off-by: Satyajit Sahu <satyajit.sahu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 4 +++-
>   drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c | 4 +++-
>   drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c | 4 +++-
>   drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c | 5 +++--
>   4 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> index 284bb42d6c86..51c46c9e7e0d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> @@ -145,10 +145,12 @@ static int vcn_v1_0_sw_init(void *handle)
>   		SOC15_REG_OFFSET(UVD, 0, mmUVD_NO_OP);
>   
>   	for (i = 0; i < adev->vcn.num_enc_rings; ++i) {
> +		unsigned int hw_prio = amdgpu_vcn_get_enc_ring_prio(i);
> +
>   		ring = &adev->vcn.inst->ring_enc[i];
>   		sprintf(ring->name, "vcn_enc%d", i);
>   		r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst->irq, 0,
> -				     AMDGPU_RING_PRIO_DEFAULT, NULL);
> +				     hw_prio, NULL);
>   		if (r)
>   			return r;
>   	}
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> index 8af567c546db..720a69322f7c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> @@ -159,6 +159,8 @@ static int vcn_v2_0_sw_init(void *handle)
>   	adev->vcn.inst->external.nop = SOC15_REG_OFFSET(UVD, 0, mmUVD_NO_OP);
>   
>   	for (i = 0; i < adev->vcn.num_enc_rings; ++i) {
> +		unsigned int hw_prio = amdgpu_vcn_get_enc_ring_prio(i);
> +
>   		ring = &adev->vcn.inst->ring_enc[i];
>   		ring->use_doorbell = true;
>   		if (!amdgpu_sriov_vf(adev))
> @@ -167,7 +169,7 @@ static int vcn_v2_0_sw_init(void *handle)
>   			ring->doorbell_index = (adev->doorbell_index.vcn.vcn_ring0_1 << 1) + 1 + i;
>   		sprintf(ring->name, "vcn_enc%d", i);
>   		r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst->irq, 0,
> -				     AMDGPU_RING_PRIO_DEFAULT, NULL);
> +				     hw_prio, NULL);
>   		if (r)
>   			return r;
>   	}
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
> index 888b17d84691..6837f5fc729e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
> @@ -194,6 +194,8 @@ static int vcn_v2_5_sw_init(void *handle)
>   			return r;
>   
>   		for (i = 0; i < adev->vcn.num_enc_rings; ++i) {
> +			unsigned int hw_prio = amdgpu_vcn_get_enc_ring_prio(i);
> +
>   			ring = &adev->vcn.inst[j].ring_enc[i];
>   			ring->use_doorbell = true;
>   
> @@ -203,7 +205,7 @@ static int vcn_v2_5_sw_init(void *handle)
>   			sprintf(ring->name, "vcn_enc_%d.%d", j, i);
>   			r = amdgpu_ring_init(adev, ring, 512,
>   					     &adev->vcn.inst[j].irq, 0,
> -					     AMDGPU_RING_PRIO_DEFAULT, NULL);
> +					     hw_prio, NULL);
>   			if (r)
>   				return r;
>   		}
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
> index 47d4f04cbd69..e6e5d476ae9e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
> @@ -227,6 +227,8 @@ static int vcn_v3_0_sw_init(void *handle)
>   			return r;
>   
>   		for (j = 0; j < adev->vcn.num_enc_rings; ++j) {
> +			unsigned int hw_prio = amdgpu_vcn_get_enc_ring_prio(j);
> +
>   			/* VCN ENC TRAP */
>   			r = amdgpu_irq_add_id(adev, amdgpu_ih_clientid_vcns[i],
>   				j + VCN_2_0__SRCID__UVD_ENC_GENERAL_PURPOSE, &adev->vcn.inst[i].irq);
> @@ -242,8 +244,7 @@ static int vcn_v3_0_sw_init(void *handle)
>   			}
>   			sprintf(ring->name, "vcn_enc_%d.%d", i, j);
>   			r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst[i].irq, 0,
> -					     AMDGPU_RING_PRIO_DEFAULT,
> -					     &adev->vcn.inst[i].sched_score);
> +					     hw_prio, &adev->vcn.inst[i].sched_score);
>   			if (r)
>   				return r;
>   		}
> 

Please feel free to use:
Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>

- Shashank

[PATCH 5/5] drm/amdgpu:schedule vce/vcn encode based on priority

[Satyajit Sahu]

Schedule the encode job in VCE/VCN encode ring
based on the priority set by UMD.

Signed-off-by: Satyajit Sahu <satyajit.sahu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 30 +++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index c88c5c6c54a2..4e6e4b6ea471 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -120,6 +120,30 @@ static enum gfx_pipe_priority amdgpu_ctx_prio_to_compute_prio(int32_t prio)
 	}
 }
 
+static enum gfx_pipe_priority amdgpu_ctx_sched_prio_to_vce_prio(int32_t prio)
+{
+	switch (prio) {
+	case AMDGPU_CTX_PRIORITY_HIGH:
+		return AMDGPU_VCE_ENC_PRIO_HIGH;
+	case AMDGPU_CTX_PRIORITY_VERY_HIGH:
+		return AMDGPU_VCE_ENC_PRIO_VERY_HIGH;
+	default:
+		return AMDGPU_VCE_ENC_PRIO_NORMAL;
+	}
+}
+
+static enum gfx_pipe_priority amdgpu_ctx_sched_prio_to_vcn_prio(int32_t prio)
+{
+	switch (prio) {
+	case AMDGPU_CTX_PRIORITY_HIGH:
+		return AMDGPU_VCN_ENC_PRIO_HIGH;
+	case AMDGPU_CTX_PRIORITY_VERY_HIGH:
+		return AMDGPU_VCN_ENC_PRIO_VERY_HIGH;
+	default:
+		return AMDGPU_VCN_ENC_PRIO_NORMAL;
+	}
+}
+
 static unsigned int amdgpu_ctx_get_hw_prio(struct amdgpu_ctx *ctx, u32 hw_ip)
 {
 	struct amdgpu_device *adev = ctx->adev;
@@ -133,6 +157,12 @@ static unsigned int amdgpu_ctx_get_hw_prio(struct amdgpu_ctx *ctx, u32 hw_ip)
 	case AMDGPU_HW_IP_COMPUTE:
 		hw_prio = amdgpu_ctx_prio_to_compute_prio(ctx_prio);
 		break;
+	case AMDGPU_HW_IP_VCE:
+		hw_prio = amdgpu_ctx_sched_prio_to_vce_prio(ctx_prio);
+		break;
+	case AMDGPU_HW_IP_VCN_ENC:
+		hw_prio = amdgpu_ctx_sched_prio_to_vcn_prio(ctx_prio);
+		break;
 	default:
 		hw_prio = AMDGPU_RING_PRIO_DEFAULT;
 		break;
-- 
2.25.1

Re: [PATCH 5/5] drm/amdgpu:schedule vce/vcn encode based on priority

[Christian König]

Am 26.08.21 um 09:13 schrieb Satyajit Sahu:
> Schedule the encode job in VCE/VCN encode ring
> based on the priority set by UMD.
>
> Signed-off-by: Satyajit Sahu <satyajit.sahu@amd.com>

Some nit pick comments on the other patches, but in general that set 
looks really clean now.

Reviewed-by: Christian König <christian.koenig@amd.com> for this one here.

Thanks,
Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 30 +++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index c88c5c6c54a2..4e6e4b6ea471 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -120,6 +120,30 @@ static enum gfx_pipe_priority amdgpu_ctx_prio_to_compute_prio(int32_t prio)
>   	}
>   }
>   
> +static enum gfx_pipe_priority amdgpu_ctx_sched_prio_to_vce_prio(int32_t prio)
> +{
> +	switch (prio) {
> +	case AMDGPU_CTX_PRIORITY_HIGH:
> +		return AMDGPU_VCE_ENC_PRIO_HIGH;
> +	case AMDGPU_CTX_PRIORITY_VERY_HIGH:
> +		return AMDGPU_VCE_ENC_PRIO_VERY_HIGH;
> +	default:
> +		return AMDGPU_VCE_ENC_PRIO_NORMAL;
> +	}
> +}
> +
> +static enum gfx_pipe_priority amdgpu_ctx_sched_prio_to_vcn_prio(int32_t prio)
> +{
> +	switch (prio) {
> +	case AMDGPU_CTX_PRIORITY_HIGH:
> +		return AMDGPU_VCN_ENC_PRIO_HIGH;
> +	case AMDGPU_CTX_PRIORITY_VERY_HIGH:
> +		return AMDGPU_VCN_ENC_PRIO_VERY_HIGH;
> +	default:
> +		return AMDGPU_VCN_ENC_PRIO_NORMAL;
> +	}
> +}
> +
>   static unsigned int amdgpu_ctx_get_hw_prio(struct amdgpu_ctx *ctx, u32 hw_ip)
>   {
>   	struct amdgpu_device *adev = ctx->adev;
> @@ -133,6 +157,12 @@ static unsigned int amdgpu_ctx_get_hw_prio(struct amdgpu_ctx *ctx, u32 hw_ip)
>   	case AMDGPU_HW_IP_COMPUTE:
>   		hw_prio = amdgpu_ctx_prio_to_compute_prio(ctx_prio);
>   		break;
> +	case AMDGPU_HW_IP_VCE:
> +		hw_prio = amdgpu_ctx_sched_prio_to_vce_prio(ctx_prio);
> +		break;
> +	case AMDGPU_HW_IP_VCN_ENC:
> +		hw_prio = amdgpu_ctx_sched_prio_to_vcn_prio(ctx_prio);
> +		break;
>   	default:
>   		hw_prio = AMDGPU_RING_PRIO_DEFAULT;
>   		break;

Re: [PATCH 5/5] drm/amdgpu:schedule vce/vcn encode based on priority

[Sharma, Shashank]

On 8/26/2021 12:43 PM, Satyajit Sahu wrote:
> Schedule the encode job in VCE/VCN encode ring
> based on the priority set by UMD.
> 
> Signed-off-by: Satyajit Sahu <satyajit.sahu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 30 +++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index c88c5c6c54a2..4e6e4b6ea471 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -120,6 +120,30 @@ static enum gfx_pipe_priority amdgpu_ctx_prio_to_compute_prio(int32_t prio)
>   	}
>   }
>   
> +static enum gfx_pipe_priority amdgpu_ctx_sched_prio_to_vce_prio(int32_t prio)
> +{
> +	switch (prio) {
> +	case AMDGPU_CTX_PRIORITY_HIGH:
> +		return AMDGPU_VCE_ENC_PRIO_HIGH;
> +	case AMDGPU_CTX_PRIORITY_VERY_HIGH:
> +		return AMDGPU_VCE_ENC_PRIO_VERY_HIGH;
> +	default:
> +		return AMDGPU_VCE_ENC_PRIO_NORMAL;
> +	}
> +}
> +
> +static enum gfx_pipe_priority amdgpu_ctx_sched_prio_to_vcn_prio(int32_t prio)
> +{
> +	switch (prio) {
> +	case AMDGPU_CTX_PRIORITY_HIGH:
> +		return AMDGPU_VCN_ENC_PRIO_HIGH;
> +	case AMDGPU_CTX_PRIORITY_VERY_HIGH:
> +		return AMDGPU_VCN_ENC_PRIO_VERY_HIGH;
> +	default:
> +		return AMDGPU_VCN_ENC_PRIO_NORMAL;
> +	}
> +}
> +
>   static unsigned int amdgpu_ctx_get_hw_prio(struct amdgpu_ctx *ctx, u32 hw_ip)
>   {
>   	struct amdgpu_device *adev = ctx->adev;
> @@ -133,6 +157,12 @@ static unsigned int amdgpu_ctx_get_hw_prio(struct amdgpu_ctx *ctx, u32 hw_ip)
>   	case AMDGPU_HW_IP_COMPUTE:
>   		hw_prio = amdgpu_ctx_prio_to_compute_prio(ctx_prio);
>   		break;
> +	case AMDGPU_HW_IP_VCE:
> +		hw_prio = amdgpu_ctx_sched_prio_to_vce_prio(ctx_prio);
> +		break;
> +	case AMDGPU_HW_IP_VCN_ENC:
> +		hw_prio = amdgpu_ctx_sched_prio_to_vcn_prio(ctx_prio);
> +		break;
>   	default:
>   		hw_prio = AMDGPU_RING_PRIO_DEFAULT;
>   		break;
> 

IMO, this patch can be split and merged into patches 3 and 4 
respectively, but is not a dealbreaker for me.

- Shashank

Re: [PATCH 5/5] drm/amdgpu:schedule vce/vcn encode based on priority

[Christian König]

Am 26.08.21 um 13:44 schrieb Sharma, Shashank:
> On 8/26/2021 12:43 PM, Satyajit Sahu wrote:
>> Schedule the encode job in VCE/VCN encode ring
>> based on the priority set by UMD.
>>
>> Signed-off-by: Satyajit Sahu <satyajit.sahu@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 30 +++++++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> index c88c5c6c54a2..4e6e4b6ea471 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> @@ -120,6 +120,30 @@ static enum gfx_pipe_priority 
>> amdgpu_ctx_prio_to_compute_prio(int32_t prio)
>>       }
>>   }
>>   +static enum gfx_pipe_priority 
>> amdgpu_ctx_sched_prio_to_vce_prio(int32_t prio)
>> +{
>> +    switch (prio) {
>> +    case AMDGPU_CTX_PRIORITY_HIGH:
>> +        return AMDGPU_VCE_ENC_PRIO_HIGH;
>> +    case AMDGPU_CTX_PRIORITY_VERY_HIGH:
>> +        return AMDGPU_VCE_ENC_PRIO_VERY_HIGH;
>> +    default:
>> +        return AMDGPU_VCE_ENC_PRIO_NORMAL;
>> +    }
>> +}
>> +
>> +static enum gfx_pipe_priority 
>> amdgpu_ctx_sched_prio_to_vcn_prio(int32_t prio)
>> +{
>> +    switch (prio) {
>> +    case AMDGPU_CTX_PRIORITY_HIGH:
>> +        return AMDGPU_VCN_ENC_PRIO_HIGH;
>> +    case AMDGPU_CTX_PRIORITY_VERY_HIGH:
>> +        return AMDGPU_VCN_ENC_PRIO_VERY_HIGH;
>> +    default:
>> +        return AMDGPU_VCN_ENC_PRIO_NORMAL;
>> +    }
>> +}
>> +
>>   static unsigned int amdgpu_ctx_get_hw_prio(struct amdgpu_ctx *ctx, 
>> u32 hw_ip)
>>   {
>>       struct amdgpu_device *adev = ctx->adev;
>> @@ -133,6 +157,12 @@ static unsigned int 
>> amdgpu_ctx_get_hw_prio(struct amdgpu_ctx *ctx, u32 hw_ip)
>>       case AMDGPU_HW_IP_COMPUTE:
>>           hw_prio = amdgpu_ctx_prio_to_compute_prio(ctx_prio);
>>           break;
>> +    case AMDGPU_HW_IP_VCE:
>> +        hw_prio = amdgpu_ctx_sched_prio_to_vce_prio(ctx_prio);
>> +        break;
>> +    case AMDGPU_HW_IP_VCN_ENC:
>> +        hw_prio = amdgpu_ctx_sched_prio_to_vcn_prio(ctx_prio);
>> +        break;
>>       default:
>>           hw_prio = AMDGPU_RING_PRIO_DEFAULT;
>>           break;
>>
>
> IMO, this patch can be split and merged into patches 3 and 4 
> respectively, but is not a dealbreaker for me.

I would rather keep that separated. The other patches add the 
functionality into the backend while this one here modifies the frontend.

Christian.

>
> - Shashank

Re: [PATCH 5/5] drm/amdgpu:schedule vce/vcn encode based on priority

[Sharma, Shashank]

On 8/26/2021 5:55 PM, Christian König wrote:
> Am 26.08.21 um 13:44 schrieb Sharma, Shashank:
>> On 8/26/2021 12:43 PM, Satyajit Sahu wrote:
>>> Schedule the encode job in VCE/VCN encode ring
>>> based on the priority set by UMD.
>>>
>>> Signed-off-by: Satyajit Sahu <satyajit.sahu@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 30 +++++++++++++++++++++++++
>>>   1 file changed, 30 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> index c88c5c6c54a2..4e6e4b6ea471 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> @@ -120,6 +120,30 @@ static enum gfx_pipe_priority 
>>> amdgpu_ctx_prio_to_compute_prio(int32_t prio)
>>>       }
>>>   }
>>>   +static enum gfx_pipe_priority 
>>> amdgpu_ctx_sched_prio_to_vce_prio(int32_t prio)
>>> +{
>>> +    switch (prio) {
>>> +    case AMDGPU_CTX_PRIORITY_HIGH:
>>> +        return AMDGPU_VCE_ENC_PRIO_HIGH;
>>> +    case AMDGPU_CTX_PRIORITY_VERY_HIGH:
>>> +        return AMDGPU_VCE_ENC_PRIO_VERY_HIGH;
>>> +    default:
>>> +        return AMDGPU_VCE_ENC_PRIO_NORMAL;
>>> +    }
>>> +}
>>> +
>>> +static enum gfx_pipe_priority 
>>> amdgpu_ctx_sched_prio_to_vcn_prio(int32_t prio)
>>> +{
>>> +    switch (prio) {
>>> +    case AMDGPU_CTX_PRIORITY_HIGH:
>>> +        return AMDGPU_VCN_ENC_PRIO_HIGH;
>>> +    case AMDGPU_CTX_PRIORITY_VERY_HIGH:
>>> +        return AMDGPU_VCN_ENC_PRIO_VERY_HIGH;
>>> +    default:
>>> +        return AMDGPU_VCN_ENC_PRIO_NORMAL;
>>> +    }
>>> +}
>>> +
>>>   static unsigned int amdgpu_ctx_get_hw_prio(struct amdgpu_ctx *ctx, 
>>> u32 hw_ip)
>>>   {
>>>       struct amdgpu_device *adev = ctx->adev;
>>> @@ -133,6 +157,12 @@ static unsigned int 
>>> amdgpu_ctx_get_hw_prio(struct amdgpu_ctx *ctx, u32 hw_ip)
>>>       case AMDGPU_HW_IP_COMPUTE:
>>>           hw_prio = amdgpu_ctx_prio_to_compute_prio(ctx_prio);
>>>           break;
>>> +    case AMDGPU_HW_IP_VCE:
>>> +        hw_prio = amdgpu_ctx_sched_prio_to_vce_prio(ctx_prio);
>>> +        break;
>>> +    case AMDGPU_HW_IP_VCN_ENC:
>>> +        hw_prio = amdgpu_ctx_sched_prio_to_vcn_prio(ctx_prio);
>>> +        break;
>>>       default:
>>>           hw_prio = AMDGPU_RING_PRIO_DEFAULT;
>>>           break;
>>>
>>
>> IMO, this patch can be split and merged into patches 3 and 4 
>> respectively, but is not a dealbreaker for me.
> 
> I would rather keep that separated. The other patches add the 
> functionality into the backend while this one here modifies the frontend.
> 
> Christian.
> 

Sure, that too works for me.

- Shashank

>>
>> - Shashank
> 

Re: [PATCH 5/5] drm/amdgpu:schedule vce/vcn encode based on priority

[Lazar, Lijo]

On 8/26/2021 12:43 PM, Satyajit Sahu wrote:
> Schedule the encode job in VCE/VCN encode ring
> based on the priority set by UMD.
> 
> Signed-off-by: Satyajit Sahu <satyajit.sahu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 30 +++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index c88c5c6c54a2..4e6e4b6ea471 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -120,6 +120,30 @@ static enum gfx_pipe_priority amdgpu_ctx_prio_to_compute_prio(int32_t prio)
>   	}
>   }
>   
> +static enum gfx_pipe_priority amdgpu_ctx_sched_prio_to_vce_prio(int32_t prio)

Well, there it is..enum gfx_pipe_priority. I really thought there is 
some type check protection from compiler, looks like implicit conversion 
from integral type.

Thanks,
Lijo

> +{
> +	switch (prio) {
> +	case AMDGPU_CTX_PRIORITY_HIGH:
> +		return AMDGPU_VCE_ENC_PRIO_HIGH;
> +	case AMDGPU_CTX_PRIORITY_VERY_HIGH:
> +		return AMDGPU_VCE_ENC_PRIO_VERY_HIGH;
> +	default:
> +		return AMDGPU_VCE_ENC_PRIO_NORMAL;
> +	}
> +}
> +
> +static enum gfx_pipe_priority amdgpu_ctx_sched_prio_to_vcn_prio(int32_t prio)
> +{
> +	switch (prio) {
> +	case AMDGPU_CTX_PRIORITY_HIGH:
> +		return AMDGPU_VCN_ENC_PRIO_HIGH;
> +	case AMDGPU_CTX_PRIORITY_VERY_HIGH:
> +		return AMDGPU_VCN_ENC_PRIO_VERY_HIGH;
> +	default:
> +		return AMDGPU_VCN_ENC_PRIO_NORMAL;
> +	}
> +}
> +
>   static unsigned int amdgpu_ctx_get_hw_prio(struct amdgpu_ctx *ctx, u32 hw_ip)
>   {
>   	struct amdgpu_device *adev = ctx->adev;
> @@ -133,6 +157,12 @@ static unsigned int amdgpu_ctx_get_hw_prio(struct amdgpu_ctx *ctx, u32 hw_ip)
>   	case AMDGPU_HW_IP_COMPUTE:
>   		hw_prio = amdgpu_ctx_prio_to_compute_prio(ctx_prio);
>   		break;
> +	case AMDGPU_HW_IP_VCE:
> +		hw_prio = amdgpu_ctx_sched_prio_to_vce_prio(ctx_prio);
> +		break;
> +	case AMDGPU_HW_IP_VCN_ENC:
> +		hw_prio = amdgpu_ctx_sched_prio_to_vcn_prio(ctx_prio);
> +		break;
>   	default:
>   		hw_prio = AMDGPU_RING_PRIO_DEFAULT;
>   		break;
> 

Re: [PATCH 5/5] drm/amdgpu:schedule vce/vcn encode based on priority

[Sahu, Satyajit]

On 8/26/2021 5:31 PM, Lazar, Lijo wrote:
>
>
> On 8/26/2021 12:43 PM, Satyajit Sahu wrote:
>> Schedule the encode job in VCE/VCN encode ring
>> based on the priority set by UMD.
>>
>> Signed-off-by: Satyajit Sahu <satyajit.sahu@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 30 +++++++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> index c88c5c6c54a2..4e6e4b6ea471 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> @@ -120,6 +120,30 @@ static enum gfx_pipe_priority 
>> amdgpu_ctx_prio_to_compute_prio(int32_t prio)
>>       }
>>   }
>>   +static enum gfx_pipe_priority 
>> amdgpu_ctx_sched_prio_to_vce_prio(int32_t prio)
>
> Well, there it is..enum gfx_pipe_priority. I really thought there is 
> some type check protection from compiler, looks like implicit 
> conversion from integral type.
>
> Thanks,
> Lijo
>
Will change the return type to amdgpu_ring_priority_level in v2 based on 
the Nirmoy's patch.

regards,

Satyajit

>> +{
>> +    switch (prio) {
>> +    case AMDGPU_CTX_PRIORITY_HIGH:
>> +        return AMDGPU_VCE_ENC_PRIO_HIGH;
>> +    case AMDGPU_CTX_PRIORITY_VERY_HIGH:
>> +        return AMDGPU_VCE_ENC_PRIO_VERY_HIGH;
>> +    default:
>> +        return AMDGPU_VCE_ENC_PRIO_NORMAL;
>> +    }
>> +}
>> +
>> +static enum gfx_pipe_priority 
>> amdgpu_ctx_sched_prio_to_vcn_prio(int32_t prio)
>> +{
>> +    switch (prio) {
>> +    case AMDGPU_CTX_PRIORITY_HIGH:
>> +        return AMDGPU_VCN_ENC_PRIO_HIGH;
>> +    case AMDGPU_CTX_PRIORITY_VERY_HIGH:
>> +        return AMDGPU_VCN_ENC_PRIO_VERY_HIGH;
>> +    default:
>> +        return AMDGPU_VCN_ENC_PRIO_NORMAL;
>> +    }
>> +}
>> +
>>   static unsigned int amdgpu_ctx_get_hw_prio(struct amdgpu_ctx *ctx, 
>> u32 hw_ip)
>>   {
>>       struct amdgpu_device *adev = ctx->adev;
>> @@ -133,6 +157,12 @@ static unsigned int 
>> amdgpu_ctx_get_hw_prio(struct amdgpu_ctx *ctx, u32 hw_ip)
>>       case AMDGPU_HW_IP_COMPUTE:
>>           hw_prio = amdgpu_ctx_prio_to_compute_prio(ctx_prio);
>>           break;
>> +    case AMDGPU_HW_IP_VCE:
>> +        hw_prio = amdgpu_ctx_sched_prio_to_vce_prio(ctx_prio);
>> +        break;
>> +    case AMDGPU_HW_IP_VCN_ENC:
>> +        hw_prio = amdgpu_ctx_sched_prio_to_vcn_prio(ctx_prio);
>> +        break;
>>       default:
>>           hw_prio = AMDGPU_RING_PRIO_DEFAULT;
>>           break;
>>

Re: [PATCH 1/5] drm/amdgpu/vce:set vce ring priority level

[Christian König]

Am 26.08.21 um 09:13 schrieb Satyajit Sahu:
> There are multiple rings available in VCE. Map each ring
> to different priority.
>
> Signed-off-by: Satyajit Sahu <satyajit.sahu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c | 14 ++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h | 14 ++++++++++++++
>   2 files changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> index 1ae7f824adc7..b68411caeac2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> @@ -1168,3 +1168,17 @@ int amdgpu_vce_ring_test_ib(struct amdgpu_ring *ring, long timeout)
>   	amdgpu_bo_free_kernel(&bo, NULL, NULL);
>   	return r;
>   }
> +
> +enum vce_enc_ring_priority amdgpu_vce_get_ring_prio(int index)
> +{
> +	switch(index) {
> +	case AMDGPU_VCE_GENERAL_PURPOSE:
> +		return AMDGPU_VCE_ENC_PRIO_NORMAL;
> +	case AMDGPU_VCE_LOW_LATENCY:
> +		return AMDGPU_VCE_ENC_PRIO_HIGH;
> +	case AMDGPU_VCE_REALTIME:
> +		return AMDGPU_VCE_ENC_PRIO_VERY_HIGH;
> +	default:
> +		return AMDGPU_VCE_ENC_PRIO_NORMAL;
> +	}
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> index d6d83a3ec803..60525887e9e3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> @@ -32,6 +32,19 @@
>   
>   #define AMDGPU_VCE_FW_53_45	((53 << 24) | (45 << 16))
>   
> +enum vce_enc_ring_priority {

Please name that enamu amdgpu_vce_...

> +	AMDGPU_VCE_ENC_PRIO_NORMAL = 1,
> +	AMDGPU_VCE_ENC_PRIO_HIGH,
> +	AMDGPU_VCE_ENC_PRIO_VERY_HIGH,

Please use the defines Nirmoy added for that here.

> +	AMDGPU_VCE_ENC_PRIO_MAX

I don't think we need this any more.

> +};
> +
> +enum vce_enc_ring_type {
> +	AMDGPU_VCE_GENERAL_PURPOSE,
> +	AMDGPU_VCE_LOW_LATENCY,
> +	AMDGPU_VCE_REALTIME
> +};

Same here, I don't think we need this any more.

Regards,
Christian.

> +
>   struct amdgpu_vce {
>   	struct amdgpu_bo	*vcpu_bo;
>   	uint64_t		gpu_addr;
> @@ -71,5 +84,6 @@ void amdgpu_vce_ring_begin_use(struct amdgpu_ring *ring);
>   void amdgpu_vce_ring_end_use(struct amdgpu_ring *ring);
>   unsigned amdgpu_vce_ring_get_emit_ib_size(struct amdgpu_ring *ring);
>   unsigned amdgpu_vce_ring_get_dma_frame_size(struct amdgpu_ring *ring);
> +enum vce_enc_ring_priority amdgpu_vce_get_ring_prio(int index);
>   
>   #endif

Re: [PATCH 1/5] drm/amdgpu/vce:set vce ring priority level

[Sahu, Satyajit]

On 8/26/2021 1:49 PM, Christian König wrote:
>
>
> Am 26.08.21 um 09:13 schrieb Satyajit Sahu:
>> There are multiple rings available in VCE. Map each ring
>> to different priority.
>>
>> Signed-off-by: Satyajit Sahu <satyajit.sahu@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c | 14 ++++++++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h | 14 ++++++++++++++
>>   2 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
>> index 1ae7f824adc7..b68411caeac2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
>> @@ -1168,3 +1168,17 @@ int amdgpu_vce_ring_test_ib(struct amdgpu_ring 
>> *ring, long timeout)
>>       amdgpu_bo_free_kernel(&bo, NULL, NULL);
>>       return r;
>>   }
>> +
>> +enum vce_enc_ring_priority amdgpu_vce_get_ring_prio(int index)
>> +{
>> +    switch(index) {
>> +    case AMDGPU_VCE_GENERAL_PURPOSE:
>> +        return AMDGPU_VCE_ENC_PRIO_NORMAL;
>> +    case AMDGPU_VCE_LOW_LATENCY:
>> +        return AMDGPU_VCE_ENC_PRIO_HIGH;
>> +    case AMDGPU_VCE_REALTIME:
>> +        return AMDGPU_VCE_ENC_PRIO_VERY_HIGH;
>> +    default:
>> +        return AMDGPU_VCE_ENC_PRIO_NORMAL;
>> +    }
>> +}
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
>> index d6d83a3ec803..60525887e9e3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
>> @@ -32,6 +32,19 @@
>>     #define AMDGPU_VCE_FW_53_45    ((53 << 24) | (45 << 16))
>>   +enum vce_enc_ring_priority {
>
> Please name that enamu amdgpu_vce_...
>
>> +    AMDGPU_VCE_ENC_PRIO_NORMAL = 1,
>> +    AMDGPU_VCE_ENC_PRIO_HIGH,
>> +    AMDGPU_VCE_ENC_PRIO_VERY_HIGH,
>
> Please use the defines Nirmoy added for that here.

I'll wait till Nirmoy's patch is merged, then rebase my changes on top 
of that.

regards,

Satyajit

>
>> +    AMDGPU_VCE_ENC_PRIO_MAX
>
> I don't think we need this any more.
>
>> +};
>> +
>> +enum vce_enc_ring_type {
>> +    AMDGPU_VCE_GENERAL_PURPOSE,
>> +    AMDGPU_VCE_LOW_LATENCY,
>> +    AMDGPU_VCE_REALTIME
>> +};
>
> Same here, I don't think we need this any more.
>
> Regards,
> Christian.
>
>> +
>>   struct amdgpu_vce {
>>       struct amdgpu_bo    *vcpu_bo;
>>       uint64_t        gpu_addr;
>> @@ -71,5 +84,6 @@ void amdgpu_vce_ring_begin_use(struct amdgpu_ring 
>> *ring);
>>   void amdgpu_vce_ring_end_use(struct amdgpu_ring *ring);
>>   unsigned amdgpu_vce_ring_get_emit_ib_size(struct amdgpu_ring *ring);
>>   unsigned amdgpu_vce_ring_get_dma_frame_size(struct amdgpu_ring *ring);
>> +enum vce_enc_ring_priority amdgpu_vce_get_ring_prio(int index);
>>     #endif
>