Re: [PATCH 11/13] drm/amdgpu: add DRM_AMDGPU_ATC config option

[Jean-Philippe Brucker]

Hi Christian,

On 31/01/18 13:04, Christian König wrote:
> Adding Jean and the IOMMU list as well.
> 
> Am 31.01.2018 um 13:43 schrieb Oded Gabbay:
>> On Tue, Jan 30, 2018 at 6:53 PM, Felix Kuehling  
>> wrote:
>>> [SNIP]
>> There was some discussion last year about a generic PASID allocator in
>> the iommu subsystem:
>> https://lists.linuxfoundation.org/pipermail/iommu/2017-June/022159.html
>>
>> Unfortunately, I don't think it got any further then that.

I also sent an RFCv2 in October:
https://www.spinics.net/lists/arm-kernel/msg609771.html

> Yeah, that sounds exactly like what I had in mind as well.
> 
> Jean any updates on that topic? We are rather interested in common PASID 
> handling as well.

Good to hear, I plan to send a new version of that series after the merge
window.

Thanks,
Jean
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 11/13] drm/amdgpu: add DRM_AMDGPU_ATC config option

[Christian König]

Adding Jean and the IOMMU list as well.

Am 31.01.2018 um 13:43 schrieb Oded Gabbay:

On Tue, Jan 30, 2018 at 6:53 PM, Felix Kuehling  wrote:

[SNIP]

There was some discussion last year about a generic PASID allocator in
the iommu subsystem:
https://lists.linuxfoundation.org/pipermail/iommu/2017-June/022159.html

Unfortunately, I don't think it got any further then that.


Yeah, that sounds exactly like what I had in mind as well.

Jean any updates on that topic? We are rather interested in common PASID 
handling as well.


Anybody already working on this? Sounds mostly like moving code around 
and creating a common interface.


Thanks,
Christian.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 11/13] drm/amdgpu: add DRM_AMDGPU_ATC config option

[Oded Gabbay]

On Tue, Jan 30, 2018 at 6:53 PM, Felix Kuehling  wrote:
>
> On 2018-01-30 08:59 AM, Christian König wrote:
>> Am 29.01.2018 um 23:08 schrieb Felix Kuehling:
>>> On 2018-01-26 03:13 PM, Christian König wrote:
 [SNIP]
 +#ifdef CONFIG_DRM_AMDGPU_ATC
 +r = amd_iommu_init_device(adev->pdev, 0x1);
>>> KFD queries how many PASIDs the IOMMU can support with
>>> amd_iommu_device_info. KFD only assigns PASIDs within that range. It can
>>> be much smaller than the 16-bits supported by the GPU.
>>>
>>> For a VM that uses ATC, you need to make sure it gets a PASID in the
>>> range supported by the IOMMU. The PASID manager already supports that
>>> and keeps smaller PASIDs for users that really need them.
>>
>> Yeah, seen that and I'm not really keen about it.
>>
>> Especially since we need multiple types of PASIDs here:
>> 1. For GPUVM debugging and HMM faults, where we can use the full 16bit
>> range without worrying about what IOMMU can do.
>> 2. For ATC use case where we need to keep the IOMMU in the picture.
>>
>> Are there any hardware limitations which blocks us from using a per
>> device PASID? That would simplify the whole handling quite a bit.
>
> Conceptually PASIDs are device-specific process IDs. KFD uses the same
> PASID on all devices, but that's not necessary.
>
> Currently you allocate PASIDs per VM or per open device file. So you'll
> end up with different PASIDs on each device. I think you can even end up
> with multiple PASIDs in each process even on the same device, if you
> create multiple VMs. That doesn't seem to be a problem for the IOMMU driver.
>
>>
>> Additional to that we don't really want this direct relationship
>> between amdgpu/amdkfd and the amd_iommu_v2 driver.
>
> Not sure what you mean. Right now amdgpu and amdkfd don't coordinate
> their PASIDs (other than using a common allocator to avoid using the
> same PASID for different things).
>
>>
>> So what do you think about moving the PASID handling into the IOMMU
>> driver? And abstracting which driver is in use through the iommu_ops?
>
> What if both drivers want to use the IOMMU?
>

There was some discussion last year about a generic PASID allocator in
the iommu subsystem:
https://lists.linuxfoundation.org/pipermail/iommu/2017-June/022159.html

Unfortunately, I don't think it got any further then that.

Oded

> Regards,
>   Felix
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>>Felix
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 11/13] drm/amdgpu: add DRM_AMDGPU_ATC config option

[Felix Kuehling]

On 2018-01-30 08:59 AM, Christian König wrote:
> Am 29.01.2018 um 23:08 schrieb Felix Kuehling:
>> On 2018-01-26 03:13 PM, Christian König wrote:
>>> [SNIP]
>>> +#ifdef CONFIG_DRM_AMDGPU_ATC
>>> +    r = amd_iommu_init_device(adev->pdev, 0x1);
>> KFD queries how many PASIDs the IOMMU can support with
>> amd_iommu_device_info. KFD only assigns PASIDs within that range. It can
>> be much smaller than the 16-bits supported by the GPU.
>>
>> For a VM that uses ATC, you need to make sure it gets a PASID in the
>> range supported by the IOMMU. The PASID manager already supports that
>> and keeps smaller PASIDs for users that really need them.
>
> Yeah, seen that and I'm not really keen about it.
>
> Especially since we need multiple types of PASIDs here:
> 1. For GPUVM debugging and HMM faults, where we can use the full 16bit
> range without worrying about what IOMMU can do.
> 2. For ATC use case where we need to keep the IOMMU in the picture.
>
> Are there any hardware limitations which blocks us from using a per
> device PASID? That would simplify the whole handling quite a bit.

Conceptually PASIDs are device-specific process IDs. KFD uses the same
PASID on all devices, but that's not necessary.

Currently you allocate PASIDs per VM or per open device file. So you'll
end up with different PASIDs on each device. I think you can even end up
with multiple PASIDs in each process even on the same device, if you
create multiple VMs. That doesn't seem to be a problem for the IOMMU driver.

>
> Additional to that we don't really want this direct relationship
> between amdgpu/amdkfd and the amd_iommu_v2 driver.

Not sure what you mean. Right now amdgpu and amdkfd don't coordinate
their PASIDs (other than using a common allocator to avoid using the
same PASID for different things).

>
> So what do you think about moving the PASID handling into the IOMMU
> driver? And abstracting which driver is in use through the iommu_ops?

What if both drivers want to use the IOMMU?

Regards,
  Felix

>
> Regards,
> Christian.
>
>>
>> Regards,
>>    Felix

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 11/13] drm/amdgpu: add DRM_AMDGPU_ATC config option

[Christian König]

Am 29.01.2018 um 23:08 schrieb Felix Kuehling:

On 2018-01-26 03:13 PM, Christian König wrote:

[SNIP]
+#ifdef CONFIG_DRM_AMDGPU_ATC
+   r = amd_iommu_init_device(adev->pdev, 0x1);

KFD queries how many PASIDs the IOMMU can support with
amd_iommu_device_info. KFD only assigns PASIDs within that range. It can
be much smaller than the 16-bits supported by the GPU.

For a VM that uses ATC, you need to make sure it gets a PASID in the
range supported by the IOMMU. The PASID manager already supports that
and keeps smaller PASIDs for users that really need them.


Yeah, seen that and I'm not really keen about it.

Especially since we need multiple types of PASIDs here:
1. For GPUVM debugging and HMM faults, where we can use the full 16bit 
range without worrying about what IOMMU can do.

2. For ATC use case where we need to keep the IOMMU in the picture.

Are there any hardware limitations which blocks us from using a per 
device PASID? That would simplify the whole handling quite a bit.


Additional to that we don't really want this direct relationship between 
amdgpu/amdkfd and the amd_iommu_v2 driver.


So what do you think about moving the PASID handling into the IOMMU 
driver? And abstracting which driver is in use through the iommu_ops?


Regards,
Christian.



Regards,
   Felix

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 11/13] drm/amdgpu: add DRM_AMDGPU_ATC config option

[Felix Kuehling]

On 2018-01-26 03:13 PM, Christian König wrote:
> Move amd_iommu_v2 initialization into amdgpu when it is enabled.
>
> This is WIP and really ugly since amdgpu should not depend directly on
> amd_iommu_v2.
>
> Signed-off-by: Christian König 
> ---
>  drivers/gpu/drm/amd/amdgpu/Kconfig |  8 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 
>  drivers/gpu/drm/amd/amdkfd/kfd_device.c| 11 ++-
>  3 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig 
> b/drivers/gpu/drm/amd/amdgpu/Kconfig
> index e8af1f5e8a79..7d3fdcbf0acb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
> +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
> @@ -40,5 +40,13 @@ config DRM_AMDGPU_GART_DEBUGFS
> Selecting this option creates a debugfs file to inspect the mapped
> pages. Uses more memory for housekeeping, enable only for debugging.
>  
> +config DRM_AMDGPU_ATC
> + bool "Enable the ATC to provide SVM support"
> + depends on DRM_AMDGPU
> + depends on AMD_IOMMU_V2
> + default y
> + help
> + This enables support for the ATC to provide a shared virtual 
> memory implementation.
> +
>  source "drivers/gpu/drm/amd/acp/Kconfig"
>  source "drivers/gpu/drm/amd/display/Kconfig"
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 84281ee34a25..04205236cc5e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -35,6 +35,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "amdgpu.h"
>  #include "amdgpu_trace.h"
>  #include "amdgpu_i2c.h"
> @@ -1355,6 +1356,12 @@ static int amdgpu_device_ip_init(struct amdgpu_device 
> *adev)
>   adev->ip_blocks[i].status.hw = true;
>   }
>  
> +#ifdef CONFIG_DRM_AMDGPU_ATC
> + r = amd_iommu_init_device(adev->pdev, 0x1);

KFD queries how many PASIDs the IOMMU can support with
amd_iommu_device_info. KFD only assigns PASIDs within that range. It can
be much smaller than the 16-bits supported by the GPU.

For a VM that uses ATC, you need to make sure it gets a PASID in the
range supported by the IOMMU. The PASID manager already supports that
and keeps smaller PASIDs for users that really need them.

Regards,
  Felix

> + if (r)
> + DRM_ERROR("amd_iommu_init_device error %d\n", r);
> +#endif
> +
>   amdgpu_amdkfd_device_init(adev);
>  
>   if (amdgpu_sriov_vf(adev))
> @@ -1428,6 +1435,11 @@ static int amdgpu_device_ip_fini(struct amdgpu_device 
> *adev)
>   int i, r;
>  
>   amdgpu_amdkfd_device_fini(adev);
> +
> +#ifdef CONFIG_DRM_AMDGPU_ATC
> + amd_iommu_free_device(adev->pdev);
> +#endif
> +
>   /* need to disable SMC first */
>   for (i = 0; i < adev->num_ip_blocks; i++) {
>   if (!adev->ip_blocks[i].status.hw)
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> index a8fa33a08de3..7ed090f7c5f0 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> @@ -373,7 +373,10 @@ void kgd2kfd_suspend(struct kfd_dev *kfd)
>  
>   amd_iommu_set_invalidate_ctx_cb(kfd->pdev, NULL);
>   amd_iommu_set_invalid_ppr_cb(kfd->pdev, NULL);
> +
> +#ifndef CONFIG_DRM_AMDGPU_ATC
>   amd_iommu_free_device(kfd->pdev);
> +#endif
>  }
>  
>  int kgd2kfd_resume(struct kfd_dev *kfd)
> @@ -388,11 +391,15 @@ int kgd2kfd_resume(struct kfd_dev *kfd)
>  static int kfd_resume(struct kfd_dev *kfd)
>  {
>   int err = 0;
> +
> +#ifndef CONFIG_DRM_AMDGPU_ATC
>   unsigned int pasid_limit = kfd_get_pasid_limit();
>  
>   err = amd_iommu_init_device(kfd->pdev, pasid_limit);
>   if (err)
>   return -ENXIO;
> +#endif
> +
>   amd_iommu_set_invalidate_ctx_cb(kfd->pdev,
>   iommu_pasid_shutdown_callback);
>   amd_iommu_set_invalid_ppr_cb(kfd->pdev,
> @@ -414,8 +421,10 @@ static int kfd_resume(struct kfd_dev *kfd)
>  
>  dqm_start_error:
>  processes_bind_error:
> - amd_iommu_free_device(kfd->pdev);
>  
> +#ifndef CONFIG_DRM_AMDGPU_ATC
> + amd_iommu_free_device(kfd->pdev);
> +#endif
>   return err;
>  }
>  

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx