<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> </head> <body> [Public] <div> <div style="color: rgb(33, 33, 33); background-color: rgb(255, 255, 255);" dir="auto"> >(... && !amdgpu_sriov_vf(adev))</div> <div style="color: rgb(33, 33, 33); background-color: rgb(255, 255, 255);" dir="auto"> </div> <div style="color: rgb(33, 33, 33); background-color: rgb(255, 255, 255);" dir="auto"> This kind of closes the door for all versions. My thought was - having it in the same function provides a logical grouping for how it's handled for different cases - VF vs non-VF - for a particular IP version.</div> <div id="ms-outlook-mobile-signature"> <div> </div> Thanks, Lijo</div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Kuehling, Felix <Felix.Kuehling@amd.com> Sent: Wednesday, November 10, 2021 9:27:22 PM To: Lazar, Lijo <Lijo.Lazar@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org> Cc: Zhang, Bokun <Bokun.Zhang@amd.com> Subject: Re: [PATCH v2 1/1] drm/amdgpu: Fix MMIO HDP flush on SRIOV <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">Am 2021-11-10 um 4:14 a.m. schrieb Lazar, Lijo: > > > On 11/10/2021 8:00 AM, Felix Kuehling wrote: >> Disable HDP register remapping on SRIOV and set rmmio_remap.reg_offset >> to the fixed address of the VF register for hdp_v*_flush_hdp. >> >> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c | 4 ++++ >> drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c | 4 ++++ >> drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c | 4 +++- >> drivers/gpu/drm/amd/amdgpu/nbio_v7_2.c | 4 ++++ >> drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c | 4 +++- >> drivers/gpu/drm/amd/amdgpu/nv.c | 8 +++++--- >> drivers/gpu/drm/amd/amdgpu/soc15.c | 8 +++++--- >> 7 files changed, 28 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c >> b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c >> index 4ecd2b5808ce..ee7cab37dfd5 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c >> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c >> @@ -359,6 +359,10 @@ static void nbio_v2_3_init_registers(struct >> amdgpu_device *adev) >> if (def != data) >> WREG32_PCIE(smnPCIE_CONFIG_CNTL, data); >> + >> + if (amdgpu_sriov_vf(adev)) >> + adev->rmmio_remap.reg_offset = SOC15_REG_OFFSET(NBIO, 0, >> + mmBIF_BX_DEV0_EPF0_VF0_HDP_MEM_COHERENCY_FLUSH_CNTL) << 2; > > Wouldn't it be better to do this assignment inside > remap_hdp_registers()? Return with a comment saying no remap is done > for VFs. That looks easier to manage per IP version. I was considering that. I felt it was clearer not to have that hidden side effect in remap_hdp_registers and to have the explicit condition (... && !amdgpu_sriov_vf(adev)) around the remap_hdp_registers call in soc15/nv_common_hw_init. Regards, Felix > > Thanks, > Lijo > >> } >> #define NAVI10_PCIE__LC_L0S_INACTIVITY_DEFAULT 0x00000000 >> // off by default, no gains over L1 >> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c >> b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c >> index 0d2d629e2d6a..4bbacf1be25a 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c >> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c >> @@ -276,6 +276,10 @@ static void nbio_v6_1_init_registers(struct >> amdgpu_device *adev) >> if (def != data) >> WREG32_PCIE(smnPCIE_CI_CNTL, data); >> + >> + if (amdgpu_sriov_vf(adev)) >> + adev->rmmio_remap.reg_offset = SOC15_REG_OFFSET(NBIO, 0, >> + mmBIF_BX_DEV0_EPF0_VF0_HDP_MEM_COHERENCY_FLUSH_CNTL) << 2; >> } >> static void nbio_v6_1_program_ltr(struct amdgpu_device *adev) >> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c >> b/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c >> index 3c00666a13e1..37a4039fdfc5 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c >> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c >> @@ -273,7 +273,9 @@ const struct nbio_hdp_flush_reg >> nbio_v7_0_hdp_flush_reg = { >> static void nbio_v7_0_init_registers(struct amdgpu_device *adev) >> { >> - >> + if (amdgpu_sriov_vf(adev)) >> + adev->rmmio_remap.reg_offset = >> + SOC15_REG_OFFSET(NBIO, 0, >> mmHDP_MEM_COHERENCY_FLUSH_CNTL) << 2; >> } >> const struct amdgpu_nbio_funcs nbio_v7_0_funcs = { >> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_2.c >> b/drivers/gpu/drm/amd/amdgpu/nbio_v7_2.c >> index 8f2a315e7c73..3444332ea110 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v7_2.c >> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v7_2.c >> @@ -371,6 +371,10 @@ static void nbio_v7_2_init_registers(struct >> amdgpu_device *adev) >> if (def != data) >> WREG32_PCIE_PORT(SOC15_REG_OFFSET(NBIO, 0, >> regPCIE_CONFIG_CNTL), data); >> } >> + >> + if (amdgpu_sriov_vf(adev)) >> + adev->rmmio_remap.reg_offset = SOC15_REG_OFFSET(NBIO, 0, >> + regBIF_BX_PF0_HDP_MEM_COHERENCY_FLUSH_CNTL) << 2; >> } >> const struct amdgpu_nbio_funcs nbio_v7_2_funcs = { >> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c >> b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c >> index b8bd03d16dba..e96516d3fd45 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c >> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c >> @@ -362,7 +362,9 @@ const struct nbio_hdp_flush_reg >> nbio_v7_4_hdp_flush_reg_ald = { >> static void nbio_v7_4_init_registers(struct amdgpu_device *adev) >> { >> - >> + if (amdgpu_sriov_vf(adev)) >> + adev->rmmio_remap.reg_offset = SOC15_REG_OFFSET(NBIO, 0, >> + mmBIF_BX_DEV0_EPF0_VF0_HDP_MEM_COHERENCY_FLUSH_CNTL) << 2; >> } >> static void >> nbio_v7_4_handle_ras_controller_intr_no_bifring(struct amdgpu_device >> *adev) >> diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c >> b/drivers/gpu/drm/amd/amdgpu/nv.c >> index febc903adf58..7088528079c6 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/nv.c >> +++ b/drivers/gpu/drm/amd/amdgpu/nv.c >> @@ -730,8 +730,10 @@ static int nv_common_early_init(void *handle) >> #define MMIO_REG_HOLE_OFFSET (0x80000 - PAGE_SIZE) >> struct amdgpu_device *adev = (struct amdgpu_device *)handle; >> - adev->rmmio_remap.reg_offset = MMIO_REG_HOLE_OFFSET; >> - adev->rmmio_remap.bus_addr = adev->rmmio_base + >> MMIO_REG_HOLE_OFFSET; >> + if (!amdgpu_sriov_vf(adev)) { >> + adev->rmmio_remap.reg_offset = MMIO_REG_HOLE_OFFSET; >> + adev->rmmio_remap.bus_addr = adev->rmmio_base + >> MMIO_REG_HOLE_OFFSET; >> + } >> adev->smc_rreg = NULL; >> adev->smc_wreg = NULL; >> adev->pcie_rreg = &nv_pcie_rreg; >> @@ -1031,7 +1033,7 @@ static int nv_common_hw_init(void *handle) >> * for the purpose of expose those registers >> * to process space >> */ >> - if (adev->nbio.funcs->remap_hdp_registers) >> + if (adev->nbio.funcs->remap_hdp_registers && >> !amdgpu_sriov_vf(adev)) >> adev->nbio.funcs->remap_hdp_registers(adev); >> /* enable the doorbell aperture */ >> nv_enable_doorbell_aperture(adev, true); >> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c >> b/drivers/gpu/drm/amd/amdgpu/soc15.c >> index 0c316a2d42ed..de9b55383e9f 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c >> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c >> @@ -971,8 +971,10 @@ static int soc15_common_early_init(void *handle) >> #define MMIO_REG_HOLE_OFFSET (0x80000 - PAGE_SIZE) >> struct amdgpu_device *adev = (struct amdgpu_device *)handle; >> - adev->rmmio_remap.reg_offset = MMIO_REG_HOLE_OFFSET; >> - adev->rmmio_remap.bus_addr = adev->rmmio_base + >> MMIO_REG_HOLE_OFFSET; >> + if (!amdgpu_sriov_vf(adev)) { >> + adev->rmmio_remap.reg_offset = MMIO_REG_HOLE_OFFSET; >> + adev->rmmio_remap.bus_addr = adev->rmmio_base + >> MMIO_REG_HOLE_OFFSET; >> + } >> adev->smc_rreg = NULL; >> adev->smc_wreg = NULL; >> adev->pcie_rreg = &soc15_pcie_rreg; >> @@ -1285,7 +1287,7 @@ static int soc15_common_hw_init(void *handle) >> * for the purpose of expose those registers >> * to process space >> */ >> - if (adev->nbio.funcs->remap_hdp_registers) >> + if (adev->nbio.funcs->remap_hdp_registers && >> !amdgpu_sriov_vf(adev)) >> adev->nbio.funcs->remap_hdp_registers(adev); >> /* enable the doorbell aperture */ >> </div> </div> </div> </body> </html>