[AMD Official Use Only]
Good catch . my editor seems has auto complete feature and I just select the first one . ☹
Thanks
Shaoyun.liu
-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling@xxxxxxx>
Sent: Friday, September 10, 2021 10:19 AM
To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Liu, Shaoyun <Shaoyun.Liu@xxxxxxx>
Subject: Re: [PATCH] drm/amdgpu: Get atomicOps info from Host for sriov setup
Am 2021-09-10 um 10:04 a.m. schrieb shaoyunl:
> The AtomicOp Requester Enable bit is reserved in VFs and the PF value
> applies to all associated VFs. so guest driver can not directly enable
> the atomicOps for VF, it depends on PF to enable it. In current
> design, amdgpu driver will get the enabled atomicOps bits through
> private pf2vf data
>
> Signed-off-by: shaoyunl <shaoyun.liu@xxxxxxx>
> Change-Id: Ifdbcb4396d64e3f3cbf6bcbf7ab9c7b2cb061052
Please remove the Change-Id.
In general, the change looks good to me. One question and one more nit-pick inline ...
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 25
> ++++++++++++--------- drivers/gpu/drm/amd/amdgpu/amdgv_sriovmsg.h |
> 4 +++-
> 2 files changed, 17 insertions(+), 12 deletions(-) mode change
> 100644 => 100755 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> mode change 100644 => 100755
> drivers/gpu/drm/amd/amdgpu/amdgv_sriovmsg.h
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> old mode 100644
> new mode 100755
> index 653bd8fdaa33..fc6a6491c1b6
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3529,17 +3529,6 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> DRM_INFO("register mmio base: 0x%08X\n", (uint32_t)adev->rmmio_base);
> DRM_INFO("register mmio size: %u\n", (unsigned)adev->rmmio_size);
>
> - /* enable PCIE atomic ops */
> - r = pci_enable_atomic_ops_to_root(adev->pdev,
> - PCI_EXP_DEVCAP2_ATOMIC_COMP32 |
> - PCI_EXP_DEVCAP2_ATOMIC_COMP64);
> - if (r) {
> - adev->have_atomics_support = false;
> - DRM_INFO("PCIE atomic ops is not supported\n");
> - } else {
> - adev->have_atomics_support = true;
> - }
> -
> amdgpu_device_get_pcie_info(adev);
>
> if (amdgpu_mcbp)
> @@ -3562,6 +3551,20 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> if (r)
> return r;
>
> + /* enable PCIE atomic ops */
> + if (amdgpu_sriov_bios(adev))
Is this the correct condition? I think this would be true for the PF as well. But on the PF we still need to call pci_enable_atomic_ops_to_root.
I would expect a condition that only applies to VFs.
> + adev->have_atomics_support = ((struct amd_sriov_msg_pf2vf_info *)
> + adev->virt.fw_reserve.p_pf2vf)->pcie_atomic_ops_enabled_flags ==
> + (PCI_EXP_DEVCAP2_ATOMIC_COMP32 | PCI_EXP_DEVCAP2_ATOMIC_COMP64);
> + else
> + adev->have_atomics_support =
> + !pci_enable_atomic_ops_to_root(adev->pdev,
> + PCI_EXP_DEVCAP2_ATOMIC_COMP32 |
> + PCI_EXP_DEVCAP2_ATOMIC_COMP64);
> + if (!adev->have_atomics_support)
> + dev_info(adev->dev, "PCIE atomic ops is not supported\n");
> +
> +
Double blank lines. One is enough.
Regards,
Felix
> /* doorbell bar mapping and doorbell index init*/
> amdgpu_device_doorbell_init(adev);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgv_sriovmsg.h
> b/drivers/gpu/drm/amd/amdgpu/amdgv_sriovmsg.h
> old mode 100644
> new mode 100755
> index a434c71fde8e..995899191288
> --- a/drivers/gpu/drm/amd/amdgpu/amdgv_sriovmsg.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgv_sriovmsg.h
> @@ -204,8 +204,10 @@ struct amd_sriov_msg_pf2vf_info {
> } mm_bw_management[AMD_SRIOV_MSG_RESERVE_VCN_INST];
> /* UUID info */
> struct amd_sriov_msg_uuid_info uuid_info;
> + /* pcie atomic Ops info */
> + uint32_t pcie_atomic_ops_enabled_flags;
> /* reserved */
> - uint32_t reserved[256 - 47];
> + uint32_t reserved[256 - 48];
> };
>
> struct amd_sriov_msg_vf2pf_info_header {
