[PATCH 3/9] drm/amdkfd: Make IOMMUv2 code conditional

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Feb 5, 2018 at 9:00 PM, Christian König
<ckoenig.leichtzumerken at gmail.com> wrote:
> Looks good to me on first glance.
>
> You probably don't mind that I'm going to pull a good part of that into
> amdgpu as next step?
>

That indeed looks better then the first approach.
Felix, I've applied all other patches from the dGPU topology patchset.
Could you send this new patch after you tested it ?
Thanks.


Christian, I'm going to pull this patch (after its tested and sent
formally) to amdkfd next for 4.17, so if you will pull it to amdgpu we
will have a collision.

Oded


> Regards,
> Christian.
>
>
> Am 03.02.2018 um 03:29 schrieb Felix Kuehling:
>
> The attached patch is my attempt to keep most of the IOMMU code in one
> place (new kfd_iommu.c) to avoid #ifdefs all over the place. This way I
> can still conditionally compile a bunch of KFD code that is only needed
> for IOMMU handling, with stub functions for kernel configs without IOMMU
> support. About 300 lines of conditionally compiled code got moved to
> kfd_iommu.c.
>
> The only piece I didn't move into kfd_iommu.c is
> kfd_signal_iommu_event. I prefer to keep that in kfd_events.c because it
> doesn't call any IOMMU driver functions, and because it's closely
> related to the rest of the event handling logic. It could be compiled
> unconditionally, but it would be dead code without IOMMU support.
>
> And I moved pdd->bound to a place where it doesn't consume extra space
> (on 64-bit systems due to structure alignment) instead of making it
> conditional.
>
> This is only compile-tested for now.
>
> If you like this approach, I'll do more testing and squash it with "Make
> IOMMUv2 code conditional".
>
> Regards,
>   Felix
>
>
> On 2018-01-31 10:00 AM, Oded Gabbay wrote:
>
> 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.
>
> 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
>
>


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux