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

[Alex Deucher]

On Tue, Mar 31, 2026 at 7:34 AM Christian König
<christian.koenig@xxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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
> >
>