RE: [PATCH] drm/amd/amdgpu: fix the inst passed to reg read write under sriov
[AMD Official Use Only - AMD Internal Distribution Only]
Thanks for pointing that out. You are right, kiq is taking logical xcc id and
should not be affected.
I'm sending out a new patch, please help review again.
Thanks,
Victor
-Original Message-
From: Lazar, Lijo
Sent: Monday, May 20, 2024 8:02 PM
To: Zhao, Victor ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amd/amdgpu: fix the inst passed to reg read write
under sriov
On 5/20/2024 4:44 PM, Victor Zhao wrote:
> the inst passed to reg read/write should be physical instance.
> Fix the miss matched code.
>
> Signed-off-by: Victor Zhao
> ---
> .../drm/amd/amdgpu/amdgpu_amdkfd_gc_9_4_3.c | 6 ++---
> .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c | 8 +++---
> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 26 +--
> 4 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gc_9_4_3.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gc_9_4_3.c
> index a5c7259cf2a3..319e6793053a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gc_9_4_3.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gc_9_4_3.c
> @@ -300,7 +300,7 @@ static int kgd_gfx_v9_4_3_hqd_load(struct amdgpu_device
> *adev, void *mqd,
> hqd_end = SOC15_REG_OFFSET(GC, GET_INST(GC, inst),
> regCP_HQD_AQL_DISPATCH_ID_HI);
>
> for (reg = hqd_base; reg <= hqd_end; reg++)
> - WREG32_XCC(reg, mqd_hqd[reg - hqd_base], inst);
> + WREG32_XCC(reg, mqd_hqd[reg - hqd_base], GET_INST(GC, inst));
Why this needs to be done? Isn't the expectation that it goes to the right
KIQ/RLCG as those are also indexed by logical XCC ids?
Thanks,
Lijo
>
>
> /* Activate doorbell logic before triggering WPTR poll. */ @@
> -493,12 +493,12 @@ static uint32_t kgd_gfx_v9_4_3_set_address_watch(
> WREG32_XCC((SOC15_REG_OFFSET(GC, GET_INST(GC, inst),
> regTCP_WATCH0_ADDR_H) +
> (watch_id * TCP_WATCH_STRIDE)),
> - watch_address_high, inst);
> + watch_address_high, GET_INST(GC, inst));
>
> WREG32_XCC((SOC15_REG_OFFSET(GC, GET_INST(GC, inst),
> regTCP_WATCH0_ADDR_L) +
> (watch_id * TCP_WATCH_STRIDE)),
> - watch_address_low, inst);
> + watch_address_low, GET_INST(GC, inst));
>
> return watch_address_cntl;
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> index 5a35a8ca8922..76be23dcea31 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> @@ -239,7 +239,7 @@ int kgd_gfx_v9_hqd_load(struct amdgpu_device
> *adev, void *mqd,
>
> for (reg = hqd_base;
>reg <= SOC15_REG_OFFSET(GC, GET_INST(GC, inst),
> mmCP_HQD_PQ_WPTR_HI); reg++)
> - WREG32_XCC(reg, mqd_hqd[reg - hqd_base], inst);
> + WREG32_XCC(reg, mqd_hqd[reg - hqd_base], GET_INST(GC, inst));
>
>
> /* Activate doorbell logic before triggering WPTR poll. */ diff
> --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> index 07b299ec7169..349ece5a27ee 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> @@ -2812,16 +2812,16 @@ static void
> gfx_v9_4_3_xcc_set_compute_eop_interrupt_state(
>
> switch (state) {
> case AMDGPU_IRQ_STATE_DISABLE:
> - mec_int_cntl = RREG32_XCC(mec_int_cntl_reg, xcc_id);
> + mec_int_cntl = RREG32_XCC(mec_int_cntl_reg, GET_INST(GC,
> xcc_id));
> mec_int_cntl = REG_SET_FIELD(mec_int_cntl,
> CP_ME1_PIPE0_INT_CNTL,
>TIME_STAMP_INT_ENABLE, 0);
> - WREG32_XCC(mec_int_cntl_reg, mec_int_cntl, xcc_id);
> + WREG32_XCC(mec_int_cntl_reg, mec_int_cntl, GET_INST(GC,
> xcc_id));
> break;
> case AMDGPU_IRQ_STATE_ENABLE:
> - mec_int_cntl = RREG32_XCC(mec_int_cntl_reg, xcc_id);
> + mec_int_cntl = RREG32_XCC(mec_int_cntl_reg, GET_INST(GC,
> xcc_id));
> mec_int_cntl = REG_SET_FIELD(mec_int_cntl,
> CP_ME1_PIPE0_INT_CNTL,
>TIME_STAMP_INT_ENABLE, 1);
> - WREG32_XCC(mec_int_cntl_reg, mec_int_cntl, xcc_id);
> + WREG32_XCC(mec_int_cntl_reg, mec_int_cntl, GET_INST(GC,
> xcc_id));
> break;
> default:
> break;
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc
Re: [PATCH] drm/amd/amdgpu: fix the inst passed to reg read write under sriov
On 5/20/2024 4:44 PM, Victor Zhao wrote:
> the inst passed to reg read/write should be physical instance.
> Fix the miss matched code.
>
> Signed-off-by: Victor Zhao
> ---
> .../drm/amd/amdgpu/amdgpu_amdkfd_gc_9_4_3.c | 6 ++---
> .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c | 8 +++---
> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 26 +--
> 4 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gc_9_4_3.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gc_9_4_3.c
> index a5c7259cf2a3..319e6793053a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gc_9_4_3.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gc_9_4_3.c
> @@ -300,7 +300,7 @@ static int kgd_gfx_v9_4_3_hqd_load(struct amdgpu_device
> *adev, void *mqd,
> hqd_end = SOC15_REG_OFFSET(GC, GET_INST(GC, inst),
> regCP_HQD_AQL_DISPATCH_ID_HI);
>
> for (reg = hqd_base; reg <= hqd_end; reg++)
> - WREG32_XCC(reg, mqd_hqd[reg - hqd_base], inst);
> + WREG32_XCC(reg, mqd_hqd[reg - hqd_base], GET_INST(GC, inst));
Why this needs to be done? Isn't the expectation that it goes to the
right KIQ/RLCG as those are also indexed by logical XCC ids?
Thanks,
Lijo
>
>
> /* Activate doorbell logic before triggering WPTR poll. */
> @@ -493,12 +493,12 @@ static uint32_t kgd_gfx_v9_4_3_set_address_watch(
> WREG32_XCC((SOC15_REG_OFFSET(GC, GET_INST(GC, inst),
> regTCP_WATCH0_ADDR_H) +
> (watch_id * TCP_WATCH_STRIDE)),
> - watch_address_high, inst);
> + watch_address_high, GET_INST(GC, inst));
>
> WREG32_XCC((SOC15_REG_OFFSET(GC, GET_INST(GC, inst),
> regTCP_WATCH0_ADDR_L) +
> (watch_id * TCP_WATCH_STRIDE)),
> - watch_address_low, inst);
> + watch_address_low, GET_INST(GC, inst));
>
> return watch_address_cntl;
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> index 5a35a8ca8922..76be23dcea31 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> @@ -239,7 +239,7 @@ int kgd_gfx_v9_hqd_load(struct amdgpu_device *adev, void
> *mqd,
>
> for (reg = hqd_base;
>reg <= SOC15_REG_OFFSET(GC, GET_INST(GC, inst),
> mmCP_HQD_PQ_WPTR_HI); reg++)
> - WREG32_XCC(reg, mqd_hqd[reg - hqd_base], inst);
> + WREG32_XCC(reg, mqd_hqd[reg - hqd_base], GET_INST(GC, inst));
>
>
> /* Activate doorbell logic before triggering WPTR poll. */
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> index 07b299ec7169..349ece5a27ee 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> @@ -2812,16 +2812,16 @@ static void
> gfx_v9_4_3_xcc_set_compute_eop_interrupt_state(
>
> switch (state) {
> case AMDGPU_IRQ_STATE_DISABLE:
> - mec_int_cntl = RREG32_XCC(mec_int_cntl_reg, xcc_id);
> + mec_int_cntl = RREG32_XCC(mec_int_cntl_reg, GET_INST(GC,
> xcc_id));
> mec_int_cntl = REG_SET_FIELD(mec_int_cntl,
> CP_ME1_PIPE0_INT_CNTL,
>TIME_STAMP_INT_ENABLE, 0);
> - WREG32_XCC(mec_int_cntl_reg, mec_int_cntl, xcc_id);
> + WREG32_XCC(mec_int_cntl_reg, mec_int_cntl, GET_INST(GC,
> xcc_id));
> break;
> case AMDGPU_IRQ_STATE_ENABLE:
> - mec_int_cntl = RREG32_XCC(mec_int_cntl_reg, xcc_id);
> + mec_int_cntl = RREG32_XCC(mec_int_cntl_reg, GET_INST(GC,
> xcc_id));
> mec_int_cntl = REG_SET_FIELD(mec_int_cntl,
> CP_ME1_PIPE0_INT_CNTL,
>TIME_STAMP_INT_ENABLE, 1);
> - WREG32_XCC(mec_int_cntl_reg, mec_int_cntl, xcc_id);
> + WREG32_XCC(mec_int_cntl_reg, mec_int_cntl, GET_INST(GC,
> xcc_id));
> break;
> default:
> break;
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 094c08cb98e7..aca842668c56 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -496,14 +496,14 @@ static int gmc_v9_0_vm_fault_interrupt_state(struct
> amdgpu_device *adev,
> if (j >= AMDGPU_MMHUB0(0))
> tmp = RREG32_SOC15_IP(MMHUB, reg);
> else
> - tmp = RREG32_XCC(reg, j);
> + tmp = RREG32_XCC(reg, GET_INST(GC, j));
>
> tmp &= ~bits;
>
> if (j >= AMDGPU_MMHUB0(0))
>
