* [PATCH] drm/amdgpu: deduplicate register access and helper routines
@ 2026-03-30 22:45 Gabriel Almeida
2026-03-31 11:24 ` Christian König
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Gabriel Almeida @ 2026-03-30 22:45 UTC (permalink / raw)
To: alexander.deucher, christian.koenig, airlied, simona
Cc: linux-kernel, amd-gfx, dri-devel, gabrielsousa230
Some helper functions are implemented multiple times with identical
logic across different source files.
Extract these implementations into a shared helper file
(amdgpu_common.c) and update existing code to reuse them.
This simplifies the codebase and avoids duplication without
changing behavior.
No functional changes intended.
Signed-off-by: Gabriel Almeida <gabrielsousa230@gmail.com>
---
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 +++++++
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
+#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)
+{
+ 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 */
+#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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amdgpu: deduplicate register access and helper routines
2026-03-30 22:45 [PATCH] drm/amdgpu: deduplicate register access and helper routines Gabriel Almeida
@ 2026-03-31 11:24 ` Christian König
2026-03-31 13:30 ` Alex Deucher
2026-03-31 22:02 ` Claude review: " Claude Code Review Bot
2026-03-31 22:02 ` Claude Code Review Bot
2 siblings, 1 reply; 6+ messages in thread
From: Christian König @ 2026-03-31 11:24 UTC (permalink / raw)
To: Gabriel Almeida, alexander.deucher, airlied, simona
Cc: linux-kernel, amd-gfx, dri-devel
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 <gabrielsousa230@gmail.com>
> ---
> 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 +++++++
> 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
> +#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)
> +{
> + 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 */
> +#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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amdgpu: deduplicate register access and helper routines
2026-03-31 11:24 ` Christian König
@ 2026-03-31 13:30 ` Alex Deucher
[not found] ` <CALsHKmUeB3=H9=Nq=+jOvtmwXxSg=wtHrASZNvVVPQwpyem2ug@mail.gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Alex Deucher @ 2026-03-31 13:30 UTC (permalink / raw)
To: Christian König
Cc: Gabriel Almeida, alexander.deucher, airlied, simona, linux-kernel,
amd-gfx, dri-devel
On Tue, Mar 31, 2026 at 7:34 AM Christian König
<christian.koenig@amd.com> 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 <gabrielsousa230@gmail.com>
> > ---
> > 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
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amdgpu: deduplicate register access and helper routines
[not found] ` <CALsHKmUeB3=H9=Nq=+jOvtmwXxSg=wtHrASZNvVVPQwpyem2ug@mail.gmail.com>
@ 2026-03-31 21:21 ` Alex Deucher
0 siblings, 0 replies; 6+ messages in thread
From: Alex Deucher @ 2026-03-31 21:21 UTC (permalink / raw)
To: Gabriel Almeida
Cc: Christian König, alexander.deucher, airlied, simona,
linux-kernel, amd-gfx, dri-devel
On Tue, Mar 31, 2026 at 5:07 PM Gabriel Almeida
<gabrielsousa230@gmail.com> 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().
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
> <alexdeucher@gmail.com> escreveu:
> >
> > On Tue, Mar 31, 2026 at 7:34 AM Christian König
> > <christian.koenig@amd.com> 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 <gabrielsousa230@gmail.com>
> > > > ---
> > > > 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
> > > >
> > >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Claude review: drm/amdgpu: deduplicate register access and helper routines
2026-03-30 22:45 [PATCH] drm/amdgpu: deduplicate register access and helper routines Gabriel Almeida
2026-03-31 11:24 ` Christian König
@ 2026-03-31 22:02 ` Claude Code Review Bot
2026-03-31 22:02 ` Claude Code Review Bot
2 siblings, 0 replies; 6+ messages in thread
From: Claude Code Review Bot @ 2026-03-31 22:02 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: drm/amdgpu: deduplicate register access and helper routines
Author: Gabriel Almeida <gabrielsousa230@gmail.com>
Patches: 4
Reviewed: 2026-04-01T08:02:08.456948
---
**NAK.** This patch has good intentions (deduplicating identical helper functions) but introduces two **functional regressions** and has naming/design issues that need to be addressed.
### Critical Issues
1. **`common_sw_init()` breaks `soc15.c`**: The `soc15_common_sw_init()` function calls `xgpu_ai_mailbox_add_irq_id()` (for Vega/AI GPUs), **not** `xgpu_nv_mailbox_add_irq_id()` (for Navi+ GPUs). The patch does not touch `soc15_common_sw_init()` in the diff context, but the shared `common_sw_init()` unconditionally calls `xgpu_nv_mailbox_add_irq_id()`. This means `soc15.c` cannot use it — yet the function is presented as a universal replacement. More importantly, `soc15_common_sw_init()` also contains additional logic (`adev->df.funcs->sw_init()`) that is not in the shared version, so it is **not** actually a duplicate.
2. **`common_sw_init()` breaks `soc_v1_0.c`**: The existing `soc_v1_0_common_sw_init()` in the tree is simply `return 0;` — it does **not** call `xgpu_nv_mailbox_add_irq_id()`. The patch replaces it with `common_sw_init()` which adds a spurious `xgpu_nv_mailbox_add_irq_id()` call for soc_v1_0 hardware that never had it. This is a **functional change** on hardware that may not support the NV mailbox at all, contradicting the commit message's claim of "no functional changes intended."
### Design/Naming Issues
3. **Overly generic names**: `read_indexed_register`, `program_aspm`, and `common_sw_init` are extremely generic names in the global symbol namespace. In a codebase as large as amdgpu, these names risk collisions and are hard to grep for. They should have an `amdgpu_` prefix at minimum (e.g., `amdgpu_common_read_indexed_register`).
4. **File naming**: `amdgpu_common.c/h` is vague. Consider whether these truly belong together or whether the register helper belongs in the existing `amdgpu_reg_access.c` and the ASPM helper belongs near the NBIO code.
5. **Unnecessary `#include <linux/module.h>`** in `amdgpu_common.c` — nothing in this file uses module macros.
6. **Missing forward declarations in the header**: `amdgpu_common.h` uses `struct amdgpu_device` and `struct amdgpu_ip_block` without including or forward-declaring them. It works only because every consumer happens to include `amdgpu.h` first, but the header is not self-contained.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 6+ messages in thread
* Claude review: drm/amdgpu: deduplicate register access and helper routines
2026-03-30 22:45 [PATCH] drm/amdgpu: deduplicate register access and helper routines Gabriel Almeida
2026-03-31 11:24 ` Christian König
2026-03-31 22:02 ` Claude review: " Claude Code Review Bot
@ 2026-03-31 22:02 ` Claude Code Review Bot
2 siblings, 0 replies; 6+ messages in thread
From: Claude Code Review Bot @ 2026-03-31 22:02 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**`amdgpu_common.c` — `common_sw_init()`:**
```c
int 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;
}
```
This is **not** a universal replacement:
- `soc15_common_sw_init()` calls `xgpu_ai_mailbox_add_irq_id()` (different function) and also has `adev->df.funcs->sw_init()` logic. The patch doesn't replace `soc15_common_sw_init()` in the diff, but the function is misleadingly named as if it were universal.
- `soc_v1_0_common_sw_init()` is just `return 0;` — it should **not** gain an `xgpu_nv_mailbox_add_irq_id()` call.
**`soc_v1_0.c` change:**
```c
static int soc_v1_0_common_sw_init(struct amdgpu_ip_block *ip_block)
{
- return 0;
+ return common_sw_init(ip_block);
}
```
This introduces a **behavioral change** on soc_v1_0 hardware — the function now calls `xgpu_nv_mailbox_add_irq_id()` in SR-IOV VF mode, which it never did before.
**`read_indexed_register()` and `program_aspm()` deduplication**: These two are genuinely identical across all call sites and the mechanical replacement is correct. However, the function names need an `amdgpu_` prefix to avoid namespace pollution.
**`amdgpu_common.h`:**
```c
#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);
```
Missing forward declarations for `struct amdgpu_device` and `struct amdgpu_ip_block`. Add them or `#include "amdgpu.h"`.
**Recommendation**: Drop the `common_sw_init()` consolidation entirely (it's not actually a common function). Keep the `read_indexed_register` and `program_aspm` deduplication but add proper `amdgpu_` prefixes. Consider placing them in existing files rather than creating a new catch-all `amdgpu_common.c`.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-03-31 22:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-30 22:45 [PATCH] drm/amdgpu: deduplicate register access and helper routines Gabriel Almeida
2026-03-31 11:24 ` Christian König
2026-03-31 13:30 ` Alex Deucher
[not found] ` <CALsHKmUeB3=H9=Nq=+jOvtmwXxSg=wtHrASZNvVVPQwpyem2ug@mail.gmail.com>
2026-03-31 21:21 ` Alex Deucher
2026-03-31 22:02 ` Claude review: " Claude Code Review Bot
2026-03-31 22:02 ` Claude Code Review Bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox