Re: [PATCH] drm/amdgpu: deduplicate register access and helper routines

Tue, 31 Mar 2026 23:58:35 -0700 



On 01-Apr-26 2:51 AM, Alex Deucher wrote:

On Tue, Mar 31, 2026 at 5:07 PM Gabriel Almeida
<[email protected]> wrote:


Hi Christian and Alex,

Thank you both for your feedback.

I understand that there can be differences between these functions due to
different macro values across hardware generations. I admit that I didn’t
fully take that into account in this patch.

Among the functions I modified, `program_aspm` and `common_sw_init` seem
to have identical behavior regardless of those macros, so I thought they
could be good candidates for shared helper functions. That said,
`common_sw_init` is currently only identical across NV, SOC21 and SOC24,
so I’m not sure if you would consider it generic enough for such use.

Regarding `read_indexed_register`, I’m still uncertain due to the use of
the `RREG32` macro. From what I’ve seen so far, it appears to behave
consistently across these implementations, but I may be missing some
subtleties.


You are correct.  the RREG32 and WREG32 macros are the same on all chips.



Also, when Christian mentioned “move them a layer up”, do you mean moving
these helpers into an existing common file such as `amdgpu_device.c`
instead of introducing a new file like `amdgpu_common.c/h`? I can rework
the patch accordingly and drop the new files if that is the preferred
approach.


I think something like  amdgpu_common_helpers.c is fine, although
thinking about it more, I think the program_aspm() function should
probably end up in amdgpu_nbio.c as something like
amdgpu_nbio_program_aspm().  read_indexed_register() could probably go
in amdgpu_device.c as amdgpu_device_read_indexed_register_helper().


For the grbm register access one, consider keeping it in amdgpu_reg_access.c

Thanks,
Lijo


And finally I'm not sure it's worth breaking out common_sw_init() as a
separate function.  Maybe drop that change.

Alex



I can also incorporate Alex’s suggestions regarding naming and licensing.

Given these points, I’d like to better understand which direction you
would prefer for this change.

Thanks again for your time and guidance.

Best regards,
Gabriel Almeida


Em ter., 31 de mar. de 2026 às 10:31, Alex Deucher
<[email protected]> escreveu:


On Tue, Mar 31, 2026 at 7:34 AM Christian König
<[email protected]> wrote:


On 3/31/26 00:45, Gabriel Almeida wrote:

Some helper functions are implemented multiple times with identical
logic across different source files.


And that is at least sometimes completely intentional.

Background is that different headers are included which define macros with 
different values for each HW generation.



Extract these implementations into a shared helper file
(amdgpu_common.c) and update existing code to reuse them.


Please don't when they are functional identical then move them a layer up 
instead of messing up the backends.

Regards,
Christian.



This simplifies the codebase and avoids duplication without
changing behavior.

No functional changes intended.

Signed-off-by: Gabriel Almeida <[email protected]>
---
  drivers/gpu/drm/amd/amdgpu/Makefile        |  2 ++
  drivers/gpu/drm/amd/amdgpu/amdgpu_common.c | 42 ++++++++++++++++++++++
  drivers/gpu/drm/amd/amdgpu/amdgpu_common.h | 12 +++++++


I think amdgpu_common_helper.c/h would be better.


  drivers/gpu/drm/amd/amdgpu/nv.c            | 38 +++-----------------
  drivers/gpu/drm/amd/amdgpu/soc15.c         | 31 ++--------------
  drivers/gpu/drm/amd/amdgpu/soc21.c         | 38 +++-----------------
  drivers/gpu/drm/amd/amdgpu/soc24.c         | 29 ++-------------
  drivers/gpu/drm/amd/amdgpu/soc_v1_0.c      | 21 ++---------
  8 files changed, 72 insertions(+), 141 deletions(-)
  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_common.c
  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_common.h

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
b/drivers/gpu/drm/amd/amdgpu/Makefile
index 6a7e9bfec..84cce03d7 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -69,6 +69,8 @@ amdgpu-y += amdgpu_device.o amdgpu_reg_access.o 
amdgpu_doorbell_mgr.o amdgpu_kms
         amdgpu_ring_mux.o amdgpu_xcp.o amdgpu_seq64.o amdgpu_aca.o 
amdgpu_dev_coredump.o \
         amdgpu_cper.o amdgpu_userq_fence.o amdgpu_eviction_fence.o amdgpu_ip.o

+amdgpu-y += amdgpu_common.o
+
  amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o

  amdgpu-$(CONFIG_PERF_EVENTS) += amdgpu_pmu.o
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_common.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_common.c
new file mode 100644
index 000000000..34ade6f63
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_common.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0


This should be MIT


+#include <linux/module.h>
+
+#include "amdgpu.h"
+#include "amdgpu_common.h"
+#include "mxgpu_nv.h"
+
+uint32_t read_indexed_register(struct amdgpu_device *adev,
+                              u32 se_num, u32 sh_num, u32 reg_offset)
+{
+       uint32_t val;
+
+       mutex_lock(&adev->grbm_idx_mutex);
+       if (se_num != 0xffffffff || sh_num != 0xffffffff)
+               amdgpu_gfx_select_se_sh(adev, se_num, sh_num, 0xffffffff, 0);
+
+       val = RREG32(reg_offset);
+
+       if (se_num != 0xffffffff || sh_num != 0xffffffff)
+               amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 
0xffffffff, 0);
+       mutex_unlock(&adev->grbm_idx_mutex);
+       return val;
+}
+
+void program_aspm(struct amdgpu_device *adev)
+{
+       if (!amdgpu_device_should_use_aspm(adev))
+               return;
+
+       if (adev->nbio.funcs->program_aspm)
+               adev->nbio.funcs->program_aspm(adev);
+}
+
+int common_sw_init(struct amdgpu_ip_block *ip_block)


Please prefix each of these functions with amdgpu_common_helper_


+{
+       struct amdgpu_device *adev = ip_block->adev;
+
+       if (amdgpu_sriov_vf(adev))
+               xgpu_nv_mailbox_add_irq_id(adev);
+
+       return 0;
+}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_common.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_common.h
new file mode 100644
index 000000000..314b3506b
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_common.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */


This should be MIT

Alex


+#ifndef __AMDGPU_COMMON_H__
+#define __AMDGPU_COMMON_H__
+
+uint32_t read_indexed_register(struct amdgpu_device *adev,
+                              u32 se_num, u32 sh_num, u32 reg_offset);
+
+void program_aspm(struct amdgpu_device *adev);
+
+int common_sw_init(struct amdgpu_ip_block *ip_block);
+
+#endif
diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c b/drivers/gpu/drm/amd/amdgpu/nv.c
index 7ce1a1b95..cf8052c73 100644
--- a/drivers/gpu/drm/amd/amdgpu/nv.c
+++ b/drivers/gpu/drm/amd/amdgpu/nv.c
@@ -29,6 +29,7 @@

  #include "amdgpu.h"
  #include "amdgpu_atombios.h"
+#include "amdgpu_common.h"
  #include "amdgpu_ih.h"
  #include "amdgpu_uvd.h"
  #include "amdgpu_vce.h"
@@ -354,29 +355,13 @@ static struct soc15_allowed_register_entry 
nv_allowed_read_registers[] = {
         { SOC15_REG_ENTRY(GC, 0, mmGB_ADDR_CONFIG)},
  };

-static uint32_t nv_read_indexed_register(struct amdgpu_device *adev, u32 
se_num,
-                                        u32 sh_num, u32 reg_offset)
-{
-       uint32_t val;
-
-       mutex_lock(&adev->grbm_idx_mutex);
-       if (se_num != 0xffffffff || sh_num != 0xffffffff)
-               amdgpu_gfx_select_se_sh(adev, se_num, sh_num, 0xffffffff, 0);
-
-       val = RREG32(reg_offset);
-
-       if (se_num != 0xffffffff || sh_num != 0xffffffff)
-               amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 
0xffffffff, 0);
-       mutex_unlock(&adev->grbm_idx_mutex);
-       return val;
-}

  static uint32_t nv_get_register_value(struct amdgpu_device *adev,
                                       bool indexed, u32 se_num,
                                       u32 sh_num, u32 reg_offset)
  {
         if (indexed) {
-               return nv_read_indexed_register(adev, se_num, sh_num, 
reg_offset);
+               return read_indexed_register(adev, se_num, sh_num, reg_offset);
         } else {
                 if (reg_offset == SOC15_REG_OFFSET(GC, 0, mmGB_ADDR_CONFIG))
                         return adev->gfx.config.gb_addr_config;
@@ -511,16 +496,6 @@ static int nv_set_vce_clocks(struct amdgpu_device *adev, 
u32 evclk, u32 ecclk)
         return 0;
  }

-static void nv_program_aspm(struct amdgpu_device *adev)
-{
-       if (!amdgpu_device_should_use_aspm(adev))
-               return;
-
-       if (adev->nbio.funcs->program_aspm)
-               adev->nbio.funcs->program_aspm(adev);
-
-}
-
  const struct amdgpu_ip_block_version nv_common_ip_block = {
         .type = AMD_IP_BLOCK_TYPE_COMMON,
         .major = 1,
@@ -965,12 +940,7 @@ static int nv_common_late_init(struct amdgpu_ip_block 
*ip_block)

  static int nv_common_sw_init(struct amdgpu_ip_block *ip_block)
  {
-       struct amdgpu_device *adev = ip_block->adev;
-
-       if (amdgpu_sriov_vf(adev))
-               xgpu_nv_mailbox_add_irq_id(adev);
-
-       return 0;
+       return common_sw_init(ip_block);
  }

  static int nv_common_hw_init(struct amdgpu_ip_block *ip_block)
@@ -984,7 +954,7 @@ static int nv_common_hw_init(struct amdgpu_ip_block 
*ip_block)
                 adev->nbio.funcs->apply_l1_link_width_reconfig_wa(adev);

         /* enable aspm */
-       nv_program_aspm(adev);
+       program_aspm(adev);
         /* setup nbio registers */
         adev->nbio.funcs->init_registers(adev);
         /* remap HDP registers to a hole in mmio space,
diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c 
b/drivers/gpu/drm/amd/amdgpu/soc15.c
index b456e4541..a6b91363d 100644
--- a/drivers/gpu/drm/amd/amdgpu/soc15.c
+++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
@@ -28,6 +28,7 @@
  #include <drm/amdgpu_drm.h>

  #include "amdgpu.h"
+#include "amdgpu_common.h"
  #include "amdgpu_ih.h"
  #include "amdgpu_uvd.h"
  #include "amdgpu_vce.h"
@@ -401,29 +402,12 @@ static struct soc15_allowed_register_entry 
soc15_allowed_read_registers[] = {
         { SOC15_REG_ENTRY(GC, 0, mmDB_DEBUG2)},
  };

-static uint32_t soc15_read_indexed_register(struct amdgpu_device *adev, u32 
se_num,
-                                        u32 sh_num, u32 reg_offset)
-{
-       uint32_t val;
-
-       mutex_lock(&adev->grbm_idx_mutex);
-       if (se_num != 0xffffffff || sh_num != 0xffffffff)
-               amdgpu_gfx_select_se_sh(adev, se_num, sh_num, 0xffffffff, 0);
-
-       val = RREG32(reg_offset);
-
-       if (se_num != 0xffffffff || sh_num != 0xffffffff)
-               amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 
0xffffffff, 0);
-       mutex_unlock(&adev->grbm_idx_mutex);
-       return val;
-}
-
  static uint32_t soc15_get_register_value(struct amdgpu_device *adev,
                                          bool indexed, u32 se_num,
                                          u32 sh_num, u32 reg_offset)
  {
         if (indexed) {
-               return soc15_read_indexed_register(adev, se_num, sh_num, 
reg_offset);
+               return read_indexed_register(adev, se_num, sh_num, reg_offset);
         } else {
                 if (reg_offset == SOC15_REG_OFFSET(GC, 0, mmGB_ADDR_CONFIG))
                         return adev->gfx.config.gb_addr_config;
@@ -695,15 +679,6 @@ static int soc15_set_vce_clocks(struct amdgpu_device 
*adev, u32 evclk, u32 ecclk
         return 0;
  }

-static void soc15_program_aspm(struct amdgpu_device *adev)
-{
-       if (!amdgpu_device_should_use_aspm(adev))
-               return;
-
-       if (adev->nbio.funcs->program_aspm)
-               adev->nbio.funcs->program_aspm(adev);
-}
-
  const struct amdgpu_ip_block_version vega10_common_ip_block =
  {
         .type = AMD_IP_BLOCK_TYPE_COMMON,
@@ -1284,7 +1259,7 @@ static int soc15_common_hw_init(struct amdgpu_ip_block 
*ip_block)
         struct amdgpu_device *adev = ip_block->adev;

         /* enable aspm */
-       soc15_program_aspm(adev);
+       program_aspm(adev);
         /* setup nbio registers */
         adev->nbio.funcs->init_registers(adev);
         /* remap HDP registers to a hole in mmio space,
diff --git a/drivers/gpu/drm/amd/amdgpu/soc21.c 
b/drivers/gpu/drm/amd/amdgpu/soc21.c
index fbd1d97f3..586d62202 100644
--- a/drivers/gpu/drm/amd/amdgpu/soc21.c
+++ b/drivers/gpu/drm/amd/amdgpu/soc21.c
@@ -27,6 +27,7 @@

  #include "amdgpu.h"
  #include "amdgpu_atombios.h"
+#include "amdgpu_common.h"
  #include "amdgpu_ih.h"
  #include "amdgpu_uvd.h"
  #include "amdgpu_vce.h"
@@ -306,29 +307,12 @@ static struct soc15_allowed_register_entry 
soc21_allowed_read_registers[] = {
         { SOC15_REG_ENTRY(GC, 0, regGB_ADDR_CONFIG)},
  };

-static uint32_t soc21_read_indexed_register(struct amdgpu_device *adev, u32 
se_num,
-                                        u32 sh_num, u32 reg_offset)
-{
-       uint32_t val;
-
-       mutex_lock(&adev->grbm_idx_mutex);
-       if (se_num != 0xffffffff || sh_num != 0xffffffff)
-               amdgpu_gfx_select_se_sh(adev, se_num, sh_num, 0xffffffff, 0);
-
-       val = RREG32(reg_offset);
-
-       if (se_num != 0xffffffff || sh_num != 0xffffffff)
-               amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 
0xffffffff, 0);
-       mutex_unlock(&adev->grbm_idx_mutex);
-       return val;
-}
-
  static uint32_t soc21_get_register_value(struct amdgpu_device *adev,
                                       bool indexed, u32 se_num,
                                       u32 sh_num, u32 reg_offset)
  {
         if (indexed) {
-               return soc21_read_indexed_register(adev, se_num, sh_num, 
reg_offset);
+               return read_indexed_register(adev, se_num, sh_num, reg_offset);
         } else {
                 if (reg_offset == SOC15_REG_OFFSET(GC, 0, regGB_ADDR_CONFIG) && 
adev->gfx.config.gb_addr_config)
                         return adev->gfx.config.gb_addr_config;
@@ -470,15 +454,6 @@ static int soc21_set_vce_clocks(struct amdgpu_device 
*adev, u32 evclk, u32 ecclk
         return 0;
  }

-static void soc21_program_aspm(struct amdgpu_device *adev)
-{
-       if (!amdgpu_device_should_use_aspm(adev))
-               return;
-
-       if (adev->nbio.funcs->program_aspm)
-               adev->nbio.funcs->program_aspm(adev);
-}
-
  const struct amdgpu_ip_block_version soc21_common_ip_block = {
         .type = AMD_IP_BLOCK_TYPE_COMMON,
         .major = 1,
@@ -912,12 +887,7 @@ static int soc21_common_late_init(struct amdgpu_ip_block 
*ip_block)

  static int soc21_common_sw_init(struct amdgpu_ip_block *ip_block)
  {
-       struct amdgpu_device *adev = ip_block->adev;
-
-       if (amdgpu_sriov_vf(adev))
-               xgpu_nv_mailbox_add_irq_id(adev);
-
-       return 0;
+       return common_sw_init(ip_block);
  }

  static int soc21_common_hw_init(struct amdgpu_ip_block *ip_block)
@@ -925,7 +895,7 @@ static int soc21_common_hw_init(struct amdgpu_ip_block 
*ip_block)
         struct amdgpu_device *adev = ip_block->adev;

         /* enable aspm */
-       soc21_program_aspm(adev);
+       program_aspm(adev);
         /* setup nbio registers */
         adev->nbio.funcs->init_registers(adev);
         /* remap HDP registers to a hole in mmio space,
diff --git a/drivers/gpu/drm/amd/amdgpu/soc24.c 
b/drivers/gpu/drm/amd/amdgpu/soc24.c
index d1adf19a5..f9341c0e4 100644
--- a/drivers/gpu/drm/amd/amdgpu/soc24.c
+++ b/drivers/gpu/drm/amd/amdgpu/soc24.c
@@ -26,6 +26,7 @@
  #include <linux/pci.h>

  #include "amdgpu.h"
+#include "amdgpu_common.h"
  #include "amdgpu_ih.h"
  #include "amdgpu_uvd.h"
  #include "amdgpu_vce.h"
@@ -132,31 +133,12 @@ static struct soc15_allowed_register_entry 
soc24_allowed_read_registers[] = {
         { SOC15_REG_ENTRY(GC, 0, regGB_ADDR_CONFIG)},
  };

-static uint32_t soc24_read_indexed_register(struct amdgpu_device *adev,
-                                           u32 se_num,
-                                           u32 sh_num,
-                                           u32 reg_offset)
-{
-       uint32_t val;
-
-       mutex_lock(&adev->grbm_idx_mutex);
-       if (se_num != 0xffffffff || sh_num != 0xffffffff)
-               amdgpu_gfx_select_se_sh(adev, se_num, sh_num, 0xffffffff, 0);
-
-       val = RREG32(reg_offset);
-
-       if (se_num != 0xffffffff || sh_num != 0xffffffff)
-               amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 
0xffffffff, 0);
-       mutex_unlock(&adev->grbm_idx_mutex);
-       return val;
-}
-
  static uint32_t soc24_get_register_value(struct amdgpu_device *adev,
                                          bool indexed, u32 se_num,
                                          u32 sh_num, u32 reg_offset)
  {
         if (indexed) {
-               return soc24_read_indexed_register(adev, se_num, sh_num, 
reg_offset);
+               return read_indexed_register(adev, se_num, sh_num, reg_offset);
         } else {
                 if (reg_offset == SOC15_REG_OFFSET(GC, 0, regGB_ADDR_CONFIG) &&
                     adev->gfx.config.gb_addr_config)
@@ -455,12 +437,7 @@ static int soc24_common_late_init(struct amdgpu_ip_block 
*ip_block)

  static int soc24_common_sw_init(struct amdgpu_ip_block *ip_block)
  {
-       struct amdgpu_device *adev = ip_block->adev;
-
-       if (amdgpu_sriov_vf(adev))
-               xgpu_nv_mailbox_add_irq_id(adev);
-
-       return 0;
+       return common_sw_init(ip_block);
  }

  static int soc24_common_hw_init(struct amdgpu_ip_block *ip_block)
diff --git a/drivers/gpu/drm/amd/amdgpu/soc_v1_0.c 
b/drivers/gpu/drm/amd/amdgpu/soc_v1_0.c
index 709b1669b..2f77fb0b6 100644
--- a/drivers/gpu/drm/amd/amdgpu/soc_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/soc_v1_0.c
@@ -21,6 +21,7 @@
   *
   */
  #include "amdgpu.h"
+#include "amdgpu_common.h"
  #include "soc15.h"
  #include "soc15_common.h"
  #include "soc_v1_0.h"
@@ -184,31 +185,13 @@ static struct soc15_allowed_register_entry 
soc_v1_0_allowed_read_registers[] = {
         { SOC15_REG_ENTRY(GC, 0, regGB_ADDR_CONFIG_1) },
  };

-static uint32_t soc_v1_0_read_indexed_register(struct amdgpu_device *adev,
-                                              u32 se_num,
-                                              u32 sh_num,
-                                              u32 reg_offset)
-{
-       uint32_t val;
-
-       mutex_lock(&adev->grbm_idx_mutex);
-       if (se_num != 0xffffffff || sh_num != 0xffffffff)
-               amdgpu_gfx_select_se_sh(adev, se_num, sh_num, 0xffffffff, 0);
-
-       val = RREG32(reg_offset);
-
-       if (se_num != 0xffffffff || sh_num != 0xffffffff)
-               amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 
0xffffffff, 0);
-       mutex_unlock(&adev->grbm_idx_mutex);
-       return val;
-}

  static uint32_t soc_v1_0_get_register_value(struct amdgpu_device *adev,
                                             bool indexed, u32 se_num,
                                             u32 sh_num, u32 reg_offset)
  {
         if (indexed) {
-               return soc_v1_0_read_indexed_register(adev, se_num, sh_num, 
reg_offset);
+               return read_indexed_register(adev, se_num, sh_num, reg_offset);
         } else {
                 if (reg_offset == SOC15_REG_OFFSET(GC, 0, regGB_ADDR_CONFIG_1) 
&&
                     adev->gfx.config.gb_addr_config)
--
2.43.0



























					Reply via email to