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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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






[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux