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

[Alex Deucher]

On Thu, Apr 23, 2026 at 4:52 AM Gabriel Almeida
<gabrielsousa230@xxxxxxxxx> wrote:
>
> Several helper functions are duplicated across multiple files with
> identical logic.
>
> Move these helpers to more appropriate locations based on their
> functionality:
> - move read_indexed_register to amdgpu_reg_access.c as
>   amdgpu_read_indexed_register
> - move program_aspm to amdgpu_nbio.c as
>   amdgpu_nbio_program_aspm

Can you split this into two patches?  one for the nbio change and one
for the read_indexed_register change?

Thanks!

Alex

>
> No functional changes intended.
>
> Signed-off-by: Gabriel Almeida <gabrielsousa230@xxxxxxxxx>
> ---
> v2:
> - Move read_indexed_register to amdgpu_reg_access.c instead of a new file
> - Move program_aspm to amdgpu_nbio.c
> - Drop amdgpu_common.[ch]
> - Drop changes to common_sw_init
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu_nbio.c      | 10 +++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_nbio.h      |  2 ++
>  .../gpu/drm/amd/amdgpu/amdgpu_reg_access.c    | 18 +++++++++++
>  .../gpu/drm/amd/amdgpu/amdgpu_reg_access.h    |  3 ++
>  drivers/gpu/drm/amd/amdgpu/nv.c               | 30 ++-----------------
>  drivers/gpu/drm/amd/amdgpu/soc15.c            | 30 ++-----------------
>  drivers/gpu/drm/amd/amdgpu/soc21.c            | 30 ++-----------------
>  drivers/gpu/drm/amd/amdgpu/soc24.c            | 21 +------------
>  drivers/gpu/drm/amd/amdgpu/soc_v1_0.c         | 20 +------------
>  9 files changed, 41 insertions(+), 123 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_nbio.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_nbio.c
> index a97426583..e4c8e9872 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_nbio.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_nbio.c
> @@ -84,3 +84,13 @@ int amdgpu_nbio_ras_late_init(struct amdgpu_device *adev, struct ras_common_if *
>         amdgpu_ras_block_late_fini(adev, ras_block);
>         return r;
>  }
> +
> +
> +void amdgpu_nbio_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);
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_nbio.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_nbio.h
> index b528de6a0..a61f3a6e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_nbio.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_nbio.h
> @@ -121,4 +121,6 @@ u64 amdgpu_nbio_get_pcie_replay_count(struct amdgpu_device *adev);
>
>  bool amdgpu_nbio_is_replay_cnt_supported(struct amdgpu_device *adev);
>
> +void amdgpu_nbio_program_aspm(struct amdgpu_device *adev);
> +
>  #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.c
> index 540040c76..daefbeeee 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.c
> @@ -956,3 +956,21 @@ uint32_t amdgpu_device_wait_on_rreg(struct amdgpu_device *adev, uint32_t inst,
>         }
>         return ret;
>  }
> +
> +
> +uint32_t amdgpu_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;
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.h
> index 4d88e5cd1..a1011af6b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.h
> @@ -160,4 +160,7 @@ uint32_t amdgpu_device_wait_on_rreg(struct amdgpu_device *adev, uint32_t inst,
>                                     uint32_t reg_addr, char reg_name[],
>                                     uint32_t expected_value, uint32_t mask);
>
> +uint32_t amdgpu_read_indexed_register(struct amdgpu_device *adev,
> +                                       u32 se_num, u32 sh_num, u32 reg_offset);
> +
>  #endif /* __AMDGPU_REG_ACCESS_H__ */
> diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c b/drivers/gpu/drm/amd/amdgpu/nv.c
> index 7ce1a1b95..1ab9d450e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/nv.c
> @@ -354,29 +354,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 amdgpu_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 +495,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,
> @@ -984,7 +958,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);
> +       amdgpu_nbio_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..87b398dd0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
> @@ -401,29 +401,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 amdgpu_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 +678,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 +1258,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);
> +       amdgpu_nbio_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..93c002e51 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc21.c
> +++ b/drivers/gpu/drm/amd/amdgpu/soc21.c
> @@ -306,29 +306,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 amdgpu_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 +453,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,
> @@ -925,7 +899,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);
> +       amdgpu_nbio_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..265db9331 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc24.c
> +++ b/drivers/gpu/drm/amd/amdgpu/soc24.c
> @@ -132,31 +132,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 amdgpu_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)
> diff --git a/drivers/gpu/drm/amd/amdgpu/soc_v1_0.c b/drivers/gpu/drm/amd/amdgpu/soc_v1_0.c
> index 709b1669b..4a5fe8e9d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/soc_v1_0.c
> @@ -184,31 +184,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 amdgpu_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
>