[AMD Official Use Only]
Thanks for the review . I accepted your comments and will sent another change list for review once your change is in.
Regards
Shaoyun.liu
-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling@xxxxxxx>
Sent: Thursday, September 9, 2021 12:18 PM
To: Liu, Shaoyun <Shaoyun.Liu@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH] drm/amdgpu: Get atomicOps info from Host for sriov setup
Am 2021-09-09 um 11:59 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
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 20 ++++++++++++++++++--
> drivers/gpu/drm/amd/amdgpu/amdgv_sriovmsg.h | 4 +++-
> 2 files changed, 21 insertions(+), 3 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..a0d2b9eb84fc
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2167,8 +2167,6 @@ static int amdgpu_device_ip_early_init(struct amdgpu_device *adev)
> return -EINVAL;
> }
>
> - amdgpu_amdkfd_device_probe(adev);
> -
> adev->pm.pp_feature = amdgpu_pp_feature_mask;
> if (amdgpu_sriov_vf(adev) || sched_policy == KFD_SCHED_POLICY_NO_HWS)
> adev->pm.pp_feature &= ~PP_GFXOFF_MASK; @@ -3562,6 +3560,24 @@ int
> amdgpu_device_init(struct amdgpu_device *adev,
> if (r)
> return r;
>
> + /* enable PCIE atomic ops */
> + if (amdgpu_sriov_bios(adev))
> + 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))
> + ? TRUE : FALSE;
Please don't use this "condition ? TRUE : FALSE" idiom. Just "condition"
is good enough.
> + else
> + adev->have_atomics_support =
> + pci_enable_atomic_ops_to_root(adev->pdev,
> + PCI_EXP_DEVCAP2_ATOMIC_COMP32 |
> + PCI_EXP_DEVCAP2_ATOMIC_COMP64)
> + ? FALSE : TRUE;
Same as above, but in this case it's "!condition". Also, I would have expected that you remove the other call to pci_enable_atomic_ops_to_root from this function.
> + if (adev->have_atomics_support = false )
This should be "==", but even better would be "if
(!adev->have_atomics_support) ...
That said, the message below may be redundant. The PCIe atomic check in kgd2kfd_device_init already prints an error message if atomics are required by the GPU but not supported. If you really want to print it for information on GPUs where it's not required, use dev_info so the message clearly shows which GPU in a multi-GPU system it refers to.
> + DRM_INFO("PCIE atomic ops is not supported\n");
> +
> + amdgpu_amdkfd_device_probe(adev);
This should not be necessary. I just sent another patch for review that moves the PCIe atomic check in KFD into kgd2kfd_device_init:
"drm/amdkfd: make needs_pcie_atomics FW-version dependent". So amdgpu_amdkfd_device_probe can stay where it is, if you can wait a few days for my change to go in first.
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 {
