[PATCH v2 1/1] drm/amdgpu: detach ring priority from gfx priority

[PATCH v2 1/1] drm/amdgpu: detach ring priority from gfx priority

[Nirmoy Das]

Currently AMDGPU_RING_PRIO_MAX is redefinition of a
max gfx hwip priority, this won't work well when we will
have a hwip with different set of priorities than gfx.
Also, HW ring priorities are different from ring priorities.

Create a global enum for ring priority levels which each
HWIP can use to define its own priority levels.

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h  | 5 ++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 9 +++++++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
index d43fe2ed8116..7f747a4291f3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
@@ -43,9 +43,8 @@
 #define AMDGPU_MAX_COMPUTE_QUEUES KGD_MAX_QUEUES

 enum gfx_pipe_priority {
-	AMDGPU_GFX_PIPE_PRIO_NORMAL = 1,
-	AMDGPU_GFX_PIPE_PRIO_HIGH,
-	AMDGPU_GFX_PIPE_PRIO_MAX
+	AMDGPU_GFX_PIPE_PRIO_NORMAL = AMDGPU_RING_PRIO_1,
+	AMDGPU_GFX_PIPE_PRIO_HIGH = AMDGPU_RING_PRIO_2
 };

 /* Argument for PPSMC_MSG_GpuChangeState */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index e713d31619fe..88d80eb3fea1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -36,8 +36,13 @@
 #define AMDGPU_MAX_VCE_RINGS		3
 #define AMDGPU_MAX_UVD_ENC_RINGS	2

-#define AMDGPU_RING_PRIO_DEFAULT	1
-#define AMDGPU_RING_PRIO_MAX		AMDGPU_GFX_PIPE_PRIO_MAX
+enum amdgpu_ring_priority_level {
+	AMDGPU_RING_PRIO_0,
+	AMDGPU_RING_PRIO_1,
+	AMDGPU_RING_PRIO_DEFAULT = 1,
+	AMDGPU_RING_PRIO_2,
+	AMDGPU_RING_PRIO_MAX
+};

 /* some special values for the owner field */
 #define AMDGPU_FENCE_OWNER_UNDEFINED	((void *)0ul)
--
2.32.0

Re: [PATCH v2 1/1] drm/amdgpu: detach ring priority from gfx priority

[Christian König]

Am 26.08.21 um 12:08 schrieb Nirmoy Das:
> Currently AMDGPU_RING_PRIO_MAX is redefinition of a
> max gfx hwip priority, this won't work well when we will
> have a hwip with different set of priorities than gfx.
> Also, HW ring priorities are different from ring priorities.
>
> Create a global enum for ring priority levels which each
> HWIP can use to define its own priority levels.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h  | 5 ++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 9 +++++++--
>   2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> index d43fe2ed8116..7f747a4291f3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> @@ -43,9 +43,8 @@
>   #define AMDGPU_MAX_COMPUTE_QUEUES KGD_MAX_QUEUES
>
>   enum gfx_pipe_priority {

While at it can you add an amdgpu_ prefix before the enum name?

And if the enum isn't really used maybe even replace the enum with defines?

Thanks,
Christian.

> -	AMDGPU_GFX_PIPE_PRIO_NORMAL = 1,
> -	AMDGPU_GFX_PIPE_PRIO_HIGH,
> -	AMDGPU_GFX_PIPE_PRIO_MAX
> +	AMDGPU_GFX_PIPE_PRIO_NORMAL = AMDGPU_RING_PRIO_1,
> +	AMDGPU_GFX_PIPE_PRIO_HIGH = AMDGPU_RING_PRIO_2
>   };
>
>   /* Argument for PPSMC_MSG_GpuChangeState */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index e713d31619fe..88d80eb3fea1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -36,8 +36,13 @@
>   #define AMDGPU_MAX_VCE_RINGS		3
>   #define AMDGPU_MAX_UVD_ENC_RINGS	2
>
> -#define AMDGPU_RING_PRIO_DEFAULT	1
> -#define AMDGPU_RING_PRIO_MAX		AMDGPU_GFX_PIPE_PRIO_MAX
> +enum amdgpu_ring_priority_level {
> +	AMDGPU_RING_PRIO_0,
> +	AMDGPU_RING_PRIO_1,
> +	AMDGPU_RING_PRIO_DEFAULT = 1,
> +	AMDGPU_RING_PRIO_2,
> +	AMDGPU_RING_PRIO_MAX
> +};
>
>   /* some special values for the owner field */
>   #define AMDGPU_FENCE_OWNER_UNDEFINED	((void *)0ul)
> --
> 2.32.0
>

Re: [PATCH v2 1/1] drm/amdgpu: detach ring priority from gfx priority

[Das, Nirmoy]

On 8/26/2021 12:48 PM, Christian König wrote:
>
>
> Am 26.08.21 um 12:08 schrieb Nirmoy Das:
>> Currently AMDGPU_RING_PRIO_MAX is redefinition of a
>> max gfx hwip priority, this won't work well when we will
>> have a hwip with different set of priorities than gfx.
>> Also, HW ring priorities are different from ring priorities.
>>
>> Create a global enum for ring priority levels which each
>> HWIP can use to define its own priority levels.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h  | 5 ++---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 9 +++++++--
>>   2 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> index d43fe2ed8116..7f747a4291f3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> @@ -43,9 +43,8 @@
>>   #define AMDGPU_MAX_COMPUTE_QUEUES KGD_MAX_QUEUES
>>
>>   enum gfx_pipe_priority {
>
> While at it can you add an amdgpu_ prefix before the enum name?
>
> And if the enum isn't really used maybe even replace the enum with 
> defines?


Yes makes sense, I will resend with defines.

>
> Thanks,
> Christian.
>
>> -    AMDGPU_GFX_PIPE_PRIO_NORMAL = 1,
>> -    AMDGPU_GFX_PIPE_PRIO_HIGH,
>> -    AMDGPU_GFX_PIPE_PRIO_MAX
>> +    AMDGPU_GFX_PIPE_PRIO_NORMAL = AMDGPU_RING_PRIO_1,
>> +    AMDGPU_GFX_PIPE_PRIO_HIGH = AMDGPU_RING_PRIO_2
>>   };
>>
>>   /* Argument for PPSMC_MSG_GpuChangeState */
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> index e713d31619fe..88d80eb3fea1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> @@ -36,8 +36,13 @@
>>   #define AMDGPU_MAX_VCE_RINGS        3
>>   #define AMDGPU_MAX_UVD_ENC_RINGS    2
>>
>> -#define AMDGPU_RING_PRIO_DEFAULT    1
>> -#define AMDGPU_RING_PRIO_MAX        AMDGPU_GFX_PIPE_PRIO_MAX
>> +enum amdgpu_ring_priority_level {
>> +    AMDGPU_RING_PRIO_0,
>> +    AMDGPU_RING_PRIO_1,
>> +    AMDGPU_RING_PRIO_DEFAULT = 1,
>> +    AMDGPU_RING_PRIO_2,
>> +    AMDGPU_RING_PRIO_MAX
>> +};
>>
>>   /* some special values for the owner field */
>>   #define AMDGPU_FENCE_OWNER_UNDEFINED    ((void *)0ul)
>> -- 
>> 2.32.0
>>
>

Re: [PATCH v2 1/1] drm/amdgpu: detach ring priority from gfx priority

[Lazar, Lijo]

On 8/26/2021 4:21 PM, Das, Nirmoy wrote:
> 
> On 8/26/2021 12:48 PM, Christian König wrote:
>>
>>
>> Am 26.08.21 um 12:08 schrieb Nirmoy Das:
>>> Currently AMDGPU_RING_PRIO_MAX is redefinition of a
>>> max gfx hwip priority, this won't work well when we will
>>> have a hwip with different set of priorities than gfx.
>>> Also, HW ring priorities are different from ring priorities.
>>>
>>> Create a global enum for ring priority levels which each
>>> HWIP can use to define its own priority levels.
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>> Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h  | 5 ++---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 9 +++++++--
>>>   2 files changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>>> index d43fe2ed8116..7f747a4291f3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>>> @@ -43,9 +43,8 @@
>>>   #define AMDGPU_MAX_COMPUTE_QUEUES KGD_MAX_QUEUES
>>>
>>>   enum gfx_pipe_priority {
>>
>> While at it can you add an amdgpu_ prefix before the enum name?
>>
>> And if the enum isn't really used maybe even replace the enum with 
>> defines?
> 
> 
> Yes makes sense, I will resend with defines.
> 

Recommend against that so that ctx_to_ip priority returns a typed enum 
for that IP instead of something mapped randomly.

Thanks,
Lijo

>>
>> Thanks,
>> Christian.
>>
>>> -    AMDGPU_GFX_PIPE_PRIO_NORMAL = 1,
>>> -    AMDGPU_GFX_PIPE_PRIO_HIGH,
>>> -    AMDGPU_GFX_PIPE_PRIO_MAX
>>> +    AMDGPU_GFX_PIPE_PRIO_NORMAL = AMDGPU_RING_PRIO_1,
>>> +    AMDGPU_GFX_PIPE_PRIO_HIGH = AMDGPU_RING_PRIO_2
>>>   };
>>>
>>>   /* Argument for PPSMC_MSG_GpuChangeState */
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> index e713d31619fe..88d80eb3fea1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> @@ -36,8 +36,13 @@
>>>   #define AMDGPU_MAX_VCE_RINGS        3
>>>   #define AMDGPU_MAX_UVD_ENC_RINGS    2
>>>
>>> -#define AMDGPU_RING_PRIO_DEFAULT    1
>>> -#define AMDGPU_RING_PRIO_MAX        AMDGPU_GFX_PIPE_PRIO_MAX
>>> +enum amdgpu_ring_priority_level {
>>> +    AMDGPU_RING_PRIO_0,
>>> +    AMDGPU_RING_PRIO_1,
>>> +    AMDGPU_RING_PRIO_DEFAULT = 1,
>>> +    AMDGPU_RING_PRIO_2,
>>> +    AMDGPU_RING_PRIO_MAX
>>> +};
>>>
>>>   /* some special values for the owner field */
>>>   #define AMDGPU_FENCE_OWNER_UNDEFINED    ((void *)0ul)
>>> -- 
>>> 2.32.0
>>>
>>