Re: [PATCH v3 2/2] drm/amdgpu: move read_indexed_register to amdgpu_reg_access

[Alex Deucher]

Applied the series.  Thanks!

Alex

On Fri, Apr 24, 2026 at 3:49 AM Christian König
<christian.koenig@xxxxxxx> wrote:
>
> On 4/24/26 02:49, Gabriel Almeida wrote:
> > The read_indexed_register helper is duplicated across multiple files
> > with identical logic.
> >
> > Move it to amdgpu_reg_access.c as
> > amdgpu_read_indexed_register and update all users accordingly.
> >
> > No functional changes intended.
>
> Yeah, as far as I can see we have already abstracted the HW generation depending parts in amdgpu_gfx_select_se_sh(), so the change is most likely a valid cleanup.
>
> > Signed-off-by: Gabriel Almeida <gabrielsousa230@xxxxxxxxx>
> > ---
> > v3:
> > - split into two patches as requested
> >
> > v2:
> > - move read_indexed_register to amdgpu_reg_access.c
> > - drop amdgpu_common
> >
> >  .../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               | 19 +----------------
> >  drivers/gpu/drm/amd/amdgpu/soc15.c            | 19 +----------------
> >  drivers/gpu/drm/amd/amdgpu/soc21.c            | 19 +----------------
> >  drivers/gpu/drm/amd/amdgpu/soc24.c            | 21 +------------------
> >  drivers/gpu/drm/amd/amdgpu/soc_v1_0.c         | 20 +-----------------
> >  7 files changed, 26 insertions(+), 93 deletions(-)
> >
> > 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;
> >  }
> > +
> > +
>
> A bit kerneldoc here would be nice to have, but not mandatory.
>
> With that done Reviewed-by: Christian König <christian.koenig@xxxxxxx>
>
> Regards,
> Christian.
>
> > +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 030d80664..72edf5326 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/nv.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/nv.c
> > @@ -354,29 +354,12 @@ 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;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
> > index 27bcbbae5..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;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/soc21.c b/drivers/gpu/drm/amd/amdgpu/soc21.c
> > index 7e4353d0c..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;
> > 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)
>