Re: [PATCH v2 1/1] drm/amdgpu: Fix MMIO HDP flush on SRIOV

[Felix Kuehling <felix.kuehling@amd.com>]

Am 2021-11-10 um 11:11 a.m. schrieb Lazar, Lijo:
>
> [Public]
>
>
> >(... && !amdgpu_sriov_vf(adev))
>
> 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.

Except that's not really true. There is still common code (setting
adev->rmmio_remap.bus_addr) for handling MMIO remapping on VFs in
soc15.c and nv.c. I'm not comfortable with duplicating all of that
across all the IP version-specific files.

I also think it's very unlikely that a small IP version bump is going to
change this logic. So I'd rather prefer to keep the code more general
and conservatively correct.

Regards,
  Felix


>
> Thanks,
> Lijo
> ------------------------------------------------------------------------
> *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
>  
> 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 */
> >>