From: "Christian König" <christian.koenig@amd.com>
To: Gabriel Almeida <gabrielsousa230@gmail.com>,
alexander.deucher@amd.com, airlied@gmail.com, simona@ffwll.ch
Cc: linux-kernel@vger.kernel.org, amd-gfx@lists.freedesktop.org,
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v3 2/2] drm/amdgpu: move read_indexed_register to amdgpu_reg_access
Date: Fri, 24 Apr 2026 09:24:19 +0200 [thread overview]
Message-ID: <975a945c-1bb8-4dbe-9767-d39f3c98e778@amd.com> (raw)
In-Reply-To: <20260424004910.21003-2-gabrielsousa230@gmail.com>
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@gmail.com>
> ---
> 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@amd.com>
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)
next prev parent reply other threads:[~2026-04-24 7:24 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-24 0:49 [PATCH v3 1/2] drm/amdgpu: move program_aspm to amdgpu_nbio Gabriel Almeida
2026-04-24 0:49 ` [PATCH v3 2/2] drm/amdgpu: move read_indexed_register to amdgpu_reg_access Gabriel Almeida
2026-04-24 7:24 ` Christian König [this message]
2026-04-24 14:27 ` Alex Deucher
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=975a945c-1bb8-4dbe-9767-d39f3c98e778@amd.com \
--to=christian.koenig@amd.com \
--cc=airlied@gmail.com \
--cc=alexander.deucher@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=gabrielsousa230@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=simona@ffwll.ch \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®