Am 31.01.2018 um 16:00 schrieb Oded Gabbay:
> On Wed, Jan 31, 2018 at 4:56 PM, Oded Gabbay <oded.gabbay at gmail.com> wrote:
>> Hi Felix,
>> Please don't spread 19 #ifdefs throughout the code.
>> I suggest to put one #ifdef in linux/amd-iommu.h itself around all the
>> functions declarations and in the #else section put macros with empty
>> implementations. This is much more readable and maintainable.
>>
>> Oded
> To emphasize my point, there is a call to amd_iommu_bind_pasid in
> kfd_bind_processes_to_device() which isn't wrapped with the #ifdef so
> the compliation breaks. Putting the #ifdefs around the calls is simply
> not scalable.
I agree with Oded on that.
Additional to this you need to add "imply AMD_IOMMU_V2" to the Kconfig.
Otherwise you can compile amdkfd into the kernel and amd_iommu_v2 as
module which would result in a kernel with unresolved symbols.
Christian.
>
> Oded
>
>>
>> On Fri, Jan 5, 2018 at 12:17 AM, Felix Kuehling <Felix.Kuehling at amd.com> wrote:
>>> dGPUs work without IOMMUv2. Make IOMMUv2 initialization dependent on
>>> ASIC information. Also allow building KFD without IOMMUv2 support.
>>> This is still useful for dGPUs and prepares for enabling KFD on
>>> architectures that don't support AMD IOMMUv2.
>>>
>>> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdkfd/Kconfig | 2 +-
>>> drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 8 +++-
>>> drivers/gpu/drm/amd/amdkfd/kfd_device.c | 62 +++++++++++++++++++++----------
>>> drivers/gpu/drm/amd/amdkfd/kfd_events.c | 2 +
>>> drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 5 +++
>>> drivers/gpu/drm/amd/amdkfd/kfd_process.c | 17 ++++++---
>>> drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 2 +
>>> drivers/gpu/drm/amd/amdkfd/kfd_topology.h | 2 +
>>> 8 files changed, 74 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/Kconfig b/drivers/gpu/drm/amd/amdkfd/Kconfig
>>> index bc5a294..5bbeb95 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/Kconfig
>>> +++ b/drivers/gpu/drm/amd/amdkfd/Kconfig
>>> @@ -4,6 +4,6 @@
>>>
>>> config HSA_AMD
>>> tristate "HSA kernel driver for AMD GPU devices"
>>> - depends on DRM_AMDGPU && AMD_IOMMU_V2 && X86_64
>>> + depends on DRM_AMDGPU && X86_64
>>> help
>>> Enable this if you want to use HSA features on AMD GPU devices.
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
>>> index 2bc2816..3478270 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
>>> @@ -22,7 +22,9 @@
>>>
>>> #include <linux/pci.h>
>>> #include <linux/acpi.h>
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> #include <linux/amd-iommu.h>
>>> +#endif
>>> #include "kfd_crat.h"
>>> #include "kfd_priv.h"
>>> #include "kfd_topology.h"
>>> @@ -1037,15 +1039,17 @@ static int kfd_create_vcrat_image_gpu(void *pcrat_image,
>>> struct crat_subtype_generic *sub_type_hdr;
>>> struct crat_subtype_computeunit *cu;
>>> struct kfd_cu_info cu_info;
>>> - struct amd_iommu_device_info iommu_info;
>>> int avail_size = *size;
>>> uint32_t total_num_of_cu;
>>> int num_of_cache_entries = 0;
>>> int cache_mem_filled = 0;
>>> int ret = 0;
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> + struct amd_iommu_device_info iommu_info;
>>> const u32 required_iommu_flags = AMD_IOMMU_DEVICE_FLAG_ATS_SUP |
>>> AMD_IOMMU_DEVICE_FLAG_PRI_SUP |
>>> AMD_IOMMU_DEVICE_FLAG_PASID_SUP;
>>> +#endif
>>> struct kfd_local_mem_info local_mem_info;
>>>
>>> if (!pcrat_image || avail_size < VCRAT_SIZE_FOR_GPU)
>>> @@ -1106,12 +1110,14 @@ static int kfd_create_vcrat_image_gpu(void *pcrat_image,
>>> /* Check if this node supports IOMMU. During parsing this flag will
>>> * translate to HSA_CAP_ATS_PRESENT
>>> */
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> iommu_info.flags = 0;
>>> if (amd_iommu_device_info(kdev->pdev, &iommu_info) == 0) {
>>> if ((iommu_info.flags & required_iommu_flags) ==
>>> required_iommu_flags)
>>> cu->hsa_capability |= CRAT_CU_FLAGS_IOMMU_PRESENT;
>>> }
>>> +#endif
>>>
>>> crat_table->length += sub_type_hdr->length;
>>> crat_table->total_entries++;
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>> index fafe971..5205b34 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>> @@ -20,7 +20,9 @@
>>> * OTHER DEALINGS IN THE SOFTWARE.
>>> */
>>>
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> #include <linux/amd-iommu.h>
>>> +#endif
>>> #include <linux/bsearch.h>
>>> #include <linux/pci.h>
>>> #include <linux/slab.h>
>>> @@ -31,6 +33,7 @@
>>>
>>> #define MQD_SIZE_ALIGNED 768
>>>
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> static const struct kfd_device_info kaveri_device_info = {
>>> .asic_family = CHIP_KAVERI,
>>> .max_pasid_bits = 16,
>>> @@ -41,6 +44,7 @@ static const struct kfd_device_info kaveri_device_info = {
>>> .num_of_watch_points = 4,
>>> .mqd_size_aligned = MQD_SIZE_ALIGNED,
>>> .supports_cwsr = false,
>>> + .needs_iommu_device = true,
>>> .needs_pci_atomics = false,
>>> };
>>>
>>> @@ -54,8 +58,10 @@ static const struct kfd_device_info carrizo_device_info = {
>>> .num_of_watch_points = 4,
>>> .mqd_size_aligned = MQD_SIZE_ALIGNED,
>>> .supports_cwsr = true,
>>> + .needs_iommu_device = true,
>>> .needs_pci_atomics = false,
>>> };
>>> +#endif
>>>
>>> struct kfd_deviceid {
>>> unsigned short did;
>>> @@ -64,6 +70,7 @@ struct kfd_deviceid {
>>>
>>> /* Please keep this sorted by increasing device id. */
>>> static const struct kfd_deviceid supported_devices[] = {
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> { 0x1304, &kaveri_device_info }, /* Kaveri */
>>> { 0x1305, &kaveri_device_info }, /* Kaveri */
>>> { 0x1306, &kaveri_device_info }, /* Kaveri */
>>> @@ -91,6 +98,7 @@ static const struct kfd_deviceid supported_devices[] = {
>>> { 0x9875, &carrizo_device_info }, /* Carrizo */
>>> { 0x9876, &carrizo_device_info }, /* Carrizo */
>>> { 0x9877, &carrizo_device_info } /* Carrizo */
>>> +#endif
>>> };
>>>
>>> static int kfd_gtt_sa_init(struct kfd_dev *kfd, unsigned int buf_size,
>>> @@ -161,6 +169,7 @@ struct kfd_dev *kgd2kfd_probe(struct kgd_dev *kgd,
>>> return kfd;
>>> }
>>>
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> static bool device_iommu_pasid_init(struct kfd_dev *kfd)
>>> {
>>> const u32 required_iommu_flags = AMD_IOMMU_DEVICE_FLAG_ATS_SUP |
>>> @@ -231,6 +240,7 @@ static int iommu_invalid_ppr_cb(struct pci_dev *pdev, int pasid,
>>>
>>> return AMD_IOMMU_INV_PRI_RSP_INVALID;
>>> }
>>> +#endif /* CONFIG_AMD_IOMMU_V2 */
>>>
>>> static void kfd_cwsr_init(struct kfd_dev *kfd)
>>> {
>>> @@ -321,12 +331,14 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
>>> goto device_queue_manager_error;
>>> }
>>>
>>> - if (!device_iommu_pasid_init(kfd)) {
>>> - dev_err(kfd_device,
>>> - "Error initializing iommuv2 for device %x:%x\n",
>>> - kfd->pdev->vendor, kfd->pdev->device);
>>> - goto device_iommu_pasid_error;
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> + if (kfd->device_info->needs_iommu_device) {
>>> + if (!device_iommu_pasid_init(kfd)) {
>>> + dev_err(kfd_device, "Error initializing iommuv2\n");
>>> + goto device_iommu_pasid_error;
>>> + }
>>> }
>>> +#endif
>>>
>>> kfd_cwsr_init(kfd);
>>>
>>> @@ -386,11 +398,16 @@ void kgd2kfd_suspend(struct kfd_dev *kfd)
>>>
>>> kfd->dqm->ops.stop(kfd->dqm);
>>>
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> + if (!kfd->device_info->needs_iommu_device)
>>> + return;
>>> +
>>> kfd_unbind_processes_from_device(kfd);
>>>
>>> amd_iommu_set_invalidate_ctx_cb(kfd->pdev, NULL);
>>> amd_iommu_set_invalid_ppr_cb(kfd->pdev, NULL);
>>> amd_iommu_free_device(kfd->pdev);
>>> +#endif
>>> }
>>>
>>> int kgd2kfd_resume(struct kfd_dev *kfd)
>>> @@ -405,19 +422,24 @@ int kgd2kfd_resume(struct kfd_dev *kfd)
>>> static int kfd_resume(struct kfd_dev *kfd)
>>> {
>>> int err = 0;
>>> - unsigned int pasid_limit = kfd_get_pasid_limit();
>>>
>>> - err = amd_iommu_init_device(kfd->pdev, pasid_limit);
>>> - if (err)
>>> - return -ENXIO;
>>> - amd_iommu_set_invalidate_ctx_cb(kfd->pdev,
>>> - iommu_pasid_shutdown_callback);
>>> - amd_iommu_set_invalid_ppr_cb(kfd->pdev,
>>> - iommu_invalid_ppr_cb);
>>> -
>>> - err = kfd_bind_processes_to_device(kfd);
>>> - if (err)
>>> - goto processes_bind_error;
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> + if (kfd->device_info->needs_iommu_device) {
>>> + unsigned int pasid_limit = kfd_get_pasid_limit();
>>> +
>>> + err = amd_iommu_init_device(kfd->pdev, pasid_limit);
>>> + if (err)
>>> + return -ENXIO;
>>> + amd_iommu_set_invalidate_ctx_cb(kfd->pdev,
>>> + iommu_pasid_shutdown_callback);
>>> + amd_iommu_set_invalid_ppr_cb(kfd->pdev,
>>> + iommu_invalid_ppr_cb);
>>> +
>>> + err = kfd_bind_processes_to_device(kfd);
>>> + if (err)
>>> + goto processes_bind_error;
>>> + }
>>> +#endif
>>>
>>> err = kfd->dqm->ops.start(kfd->dqm);
>>> if (err) {
>>> @@ -431,8 +453,10 @@ static int kfd_resume(struct kfd_dev *kfd)
>>>
>>> dqm_start_error:
>>> processes_bind_error:
>>> - amd_iommu_free_device(kfd->pdev);
>>> -
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> + if (kfd->device_info->needs_iommu_device)
>>> + amd_iommu_free_device(kfd->pdev);
>>> +#endif
>>> return err;
>>> }
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>> index 93aae5c..f770dc7 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>> @@ -837,6 +837,7 @@ static void lookup_events_by_type_and_signal(struct kfd_process *p,
>>> }
>>> }
>>>
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> void kfd_signal_iommu_event(struct kfd_dev *dev, unsigned int pasid,
>>> unsigned long address, bool is_write_requested,
>>> bool is_execute_requested)
>>> @@ -905,6 +906,7 @@ void kfd_signal_iommu_event(struct kfd_dev *dev, unsigned int pasid,
>>> mutex_unlock(&p->event_mutex);
>>> kfd_unref_process(p);
>>> }
>>> +#endif /* CONFIG_AMD_IOMMU_V2_MODULE */
>>>
>>> void kfd_signal_hw_exception_event(unsigned int pasid)
>>> {
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> index eebfb1e..9f4766c 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> @@ -158,6 +158,7 @@ struct kfd_device_info {
>>> uint8_t num_of_watch_points;
>>> uint16_t mqd_size_aligned;
>>> bool supports_cwsr;
>>> + bool needs_iommu_device;
>>> bool needs_pci_atomics;
>>> };
>>>
>>> @@ -617,9 +618,11 @@ void kfd_unref_process(struct kfd_process *p);
>>>
>>> struct kfd_process_device *kfd_bind_process_to_device(struct kfd_dev *dev,
>>> struct kfd_process *p);
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> int kfd_bind_processes_to_device(struct kfd_dev *dev);
>>> void kfd_unbind_processes_from_device(struct kfd_dev *dev);
>>> void kfd_process_iommu_unbind_callback(struct kfd_dev *dev, unsigned int pasid);
>>> +#endif
>>> struct kfd_process_device *kfd_get_process_device_data(struct kfd_dev *dev,
>>> struct kfd_process *p);
>>> struct kfd_process_device *kfd_create_process_device_data(struct kfd_dev *dev,
>>> @@ -784,9 +787,11 @@ int kfd_wait_on_events(struct kfd_process *p,
>>> uint32_t *wait_result);
>>> void kfd_signal_event_interrupt(unsigned int pasid, uint32_t partial_id,
>>> uint32_t valid_id_bits);
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> void kfd_signal_iommu_event(struct kfd_dev *dev,
>>> unsigned int pasid, unsigned long address,
>>> bool is_write_requested, bool is_execute_requested);
>>> +#endif
>>> void kfd_signal_hw_exception_event(unsigned int pasid);
>>> int kfd_set_event(struct kfd_process *p, uint32_t event_id);
>>> int kfd_reset_event(struct kfd_process *p, uint32_t event_id);
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> index a22fb071..1d0e02c 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> @@ -173,14 +173,17 @@ static void kfd_process_wq_release(struct work_struct *work)
>>> {
>>> struct kfd_process *p = container_of(work, struct kfd_process,
>>> release_work);
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> struct kfd_process_device *pdd;
>>>
>>> pr_debug("Releasing process (pasid %d) in workqueue\n", p->pasid);
>>>
>>> list_for_each_entry(pdd, &p->per_device_data, per_device_list) {
>>> - if (pdd->bound == PDD_BOUND)
>>> + if (pdd->bound == PDD_BOUND &&
>>> + pdd->dev->device_info->needs_iommu_device)
>>> amd_iommu_unbind_pasid(pdd->dev->pdev, p->pasid);
>>> }
>>> +#endif
>>>
>>> kfd_process_destroy_pdds(p);
>>>
>>> @@ -421,7 +424,6 @@ struct kfd_process_device *kfd_bind_process_to_device(struct kfd_dev *dev,
>>> struct kfd_process *p)
>>> {
>>> struct kfd_process_device *pdd;
>>> - int err;
>>>
>>> pdd = kfd_get_process_device_data(dev, p);
>>> if (!pdd) {
>>> @@ -436,9 +438,14 @@ struct kfd_process_device *kfd_bind_process_to_device(struct kfd_dev *dev,
>>> return ERR_PTR(-EINVAL);
>>> }
>>>
>>> - err = amd_iommu_bind_pasid(dev->pdev, p->pasid, p->lead_thread);
>>> - if (err < 0)
>>> - return ERR_PTR(err);
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> + if (dev->device_info->needs_iommu_device) {
>>> + int err = amd_iommu_bind_pasid(dev->pdev, p->pasid,
>>> + p->lead_thread);
>>> + if (err < 0)
>>> + return ERR_PTR(err);
>>> + }
>>> +#endif
>>>
>>> pdd->bound = PDD_BOUND;
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
>>> index c6a7609..f57c305 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
>>> @@ -875,6 +875,7 @@ static void find_system_memory(const struct dmi_header *dm,
>>> */
>>> static int kfd_add_perf_to_topology(struct kfd_topology_device *kdev)
>>> {
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> struct kfd_perf_properties *props;
>>>
>>> if (amd_iommu_pc_supported()) {
>>> @@ -886,6 +887,7 @@ static int kfd_add_perf_to_topology(struct kfd_topology_device *kdev)
>>> amd_iommu_pc_get_max_counters(0); /* assume one iommu */
>>> list_add_tail(&props->list, &kdev->perf_props);
>>> }
>>> +#endif
>>>
>>> return 0;
>>> }
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.h b/drivers/gpu/drm/amd/amdkfd/kfd_topology.h
>>> index 53fca1f..111fda2 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.h
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.h
>>> @@ -183,8 +183,10 @@ struct kfd_topology_device *kfd_create_topology_device(
>>> struct list_head *device_list);
>>> void kfd_release_topology_device_list(struct list_head *device_list);
>>>
>>> +#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>> extern bool amd_iommu_pc_supported(void);
>>> extern u8 amd_iommu_pc_get_max_banks(u16 devid);
>>> extern u8 amd_iommu_pc_get_max_counters(u16 devid);
>>> +#endif
>>>
>>> #endif /* __KFD_TOPOLOGY_H__ */
>>> --
>>> 2.7.4
>>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
