[AMD Official Use Only]
> -----Original Message-----
> From: Kuehling, Felix <Felix.Kuehling@xxxxxxx>
> Sent: Friday, November 19, 2021 4:20 PM
> To: Sider, Graham <Graham.Sider@xxxxxxx>; amd-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> Subject: Re: [PATCH v2 2/4] drm/amdkfd: add kfd_device_info_init function
>
> On 2021-11-19 2:52 p.m., Graham Sider wrote:
> > Initializes device_info structs given either asic_type (enum) if GFX
> > version is less than GFX9, or GC IP version if greater. Also takes in
> > vf and the target compiler gfx version.
> >
> > Inclusion/exclusion to certain conditions for certain GC IP versions
> > may be necessary on npi bringup on a case-by-case basis, but for the
> > most part should be minimal (e.g. adding one || asic_version ==
> IP_VERSION(X ,X, X) case).
> >
> > Signed-off-by: Graham Sider <Graham.Sider@xxxxxxx>
> > ---
> > drivers/gpu/drm/amd/amdkfd/kfd_device.c | 61
> +++++++++++++++++++++++++
> > 1 file changed, 61 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > index e11fc4e20c32..676cb9c3166c 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > @@ -511,6 +511,67 @@ static void kfd_gtt_sa_fini(struct kfd_dev *kfd);
> >
> > static int kfd_resume(struct kfd_dev *kfd);
> >
> > +static void kfd_device_info_init(struct kfd_dev *kfd,
> > + struct kfd_device_info *device_info,
> > + bool vf, uint32_t gfx_target_version)
>
> This will give you a compile warning about an unused static function.
> Maybe squash this with the commit that actually starts using this function.
>
Sounds good.
>
> > +{
> > + uint32_t gc_version = KFD_GC_VERSION(kfd);
> > + uint32_t asic_type = kfd->adev->asic_type;
> > +
> > + device_info->max_pasid_bits = 16;
> > + device_info->max_no_of_hqd = 24;
> > + device_info->num_of_watch_points = 4;
> > + device_info->mqd_size_aligned = MQD_SIZE_ALIGNED;
> > + device_info->gfx_target_version = gfx_target_version;
> > +
> > + if (KFD_IS_SOC15(kfd)) {
> > + device_info->doorbell_size = 8;
> > + device_info->ih_ring_entry_size = 8 * sizeof(uint32_t);
> > + device_info->event_interrupt_class =
> &event_interrupt_class_v9;
> > + device_info->supports_cwsr = true;
> > +
> > + if ((gc_version >= IP_VERSION(9, 0, 1) &&
> > + gc_version <= IP_VERSION(9, 3, 0)) ||
> > + gc_version == IP_VERSION(10, 3, 1) ||
> > + gc_version == IP_VERSION(10, 3, 3))
> > + device_info->num_sdma_queues_per_engine = 2;
> > + else
> > + device_info->num_sdma_queues_per_engine = 8;
>
> I feel this should be based on the SDMA IP version, not the GC IP version.
>
Can the SDMA queues/engine be determined by the SDMA IP versions? I would have thought those were instead done on a chip-by-chip basis. E.g. in amdgpu_discovery.c this is how the number of SDMA instances is defined.
>
> > +
> > + /* Navi2x+, Navi1x+ */
> > + if (gc_version >= IP_VERSION(10, 3, 0))
>
> There needs to be a maximum check here. This case should not automatically
> apply to future ASICs e.g. GFX11.
>
Just a thought on this: assuming on future asics this field is going to continue to be populated, might it be better to just continue adding cases here as they arise? Adding a check for e.g. < GFX11, would require eventually bumping that check to < GFX12 alongside another check for >= GFX11. So at the end of the day, if a >= check is going to be needed anyway, is a maximum check necessary? Of course this wouldn't apply to below regarding the needs_pci_atomics bool, since as you mention on future asics it can be kept as defaulted to false.
>
> > + device_info->no_atomic_fw_version = 145;
> > + else if (gc_version >= IP_VERSION(10, 1, 1))
> > + device_info->no_atomic_fw_version = 92;
> > +
> > + /* Raven */
> > + if (gc_version == IP_VERSION(9, 1, 0) ||
> > + gc_version == IP_VERSION(9, 2, 2))
> > + device_info->needs_iommu_device = true;
> > +
> > + /* Navi1x+ */
> > + if (gc_version >= IP_VERSION(10, 1, 1))
>
> There needs to be a maximum check here. On future ASICs (maybe GFX11) I
> would not expect atomics to be required.
>
See above, agreed here.
> Regards,
> Felix
>
Best,
Graham
>
> > + device_info->needs_pci_atomics = true;
> > + } else {
> > + device_info->doorbell_size = 4;
> > + device_info->ih_ring_entry_size = 4 * sizeof(uint32_t);
> > + device_info->event_interrupt_class =
> &event_interrupt_class_cik;
> > + device_info->num_sdma_queues_per_engine = 2;
> > +
> > + if (asic_type != CHIP_KAVERI &&
> > + asic_type != CHIP_HAWAII &&
> > + asic_type != CHIP_TONGA)
> > + device_info->supports_cwsr = true;
> > +
> > + if (asic_type == CHIP_KAVERI ||
> > + asic_type == CHIP_CARRIZO)
> > + device_info->needs_iommu_device = true;
> > +
> > + if (asic_type != CHIP_HAWAII && !vf)
> > + device_info->needs_pci_atomics = true;
> > + }
> > +}
> > +
> > struct kfd_dev *kgd2kfd_probe(struct amdgpu_device *adev, bool vf)
> > {
> > struct kfd_dev *kfd;
