[PATCH] drm/amdgpu: remove vm sanity check from amdgpu_vm_make_compute

[PATCH] drm/amdgpu: remove vm sanity check from amdgpu_vm_make_compute

[Xiaogang.Chen]

From: Xiaogang Chen <xiaogang.chen@amd.com>

Since we allow kfd and graphic operate on same GPU VM to have interoperation
between them GPU VM may have been used by graphic vm operations before kfd turns
a GPU VM into a compute VM. Remove vm clean checking at amdgpu_vm_make_compute.

Signed-off-by: Xiaogang Chen<Xiaogang.Chen@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index eff73c428b12..291977b93b1d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2245,16 +2245,16 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 	if (r)
 		return r;
 
-	/* Sanity checks */
-	if (!amdgpu_vm_pt_is_root_clean(adev, vm)) {
-		r = -EINVAL;
-		goto unreserve_bo;
-	}
-
 	/* Check if PD needs to be reinitialized and do it before
 	 * changing any other state, in case it fails.
 	 */
 	if (pte_support_ats != vm->pte_support_ats) {
+		/* Sanity checks */
+		if (!amdgpu_vm_pt_is_root_clean(adev, vm)) {
+			r = -EINVAL;
+			goto unreserve_bo;
+		}
+
 		vm->pte_support_ats = pte_support_ats;
 		r = amdgpu_vm_pt_clear(adev, vm, to_amdgpu_bo_vm(vm->root.bo),
 				       false);
-- 
2.25.1

Re: [PATCH] drm/amdgpu: remove vm sanity check from amdgpu_vm_make_compute

[Felix Kuehling]

On 2023-06-19 17:28, Xiaogang.Chen wrote:
> From: Xiaogang Chen <xiaogang.chen@amd.com>
>
> Since we allow kfd and graphic operate on same GPU VM to have interoperation
> between them GPU VM may have been used by graphic vm operations before kfd turns
> a GPU VM into a compute VM. Remove vm clean checking at amdgpu_vm_make_compute.
>
> Signed-off-by: Xiaogang Chen<Xiaogang.Chen@amd.com>

Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

As discussed, we can follow this up with a change that enables ATS for 
graphics VMs as well, so we don't need to enable ATS in 
amdgpu_vm_make_compute. This would improve interop for Raven. We only 
enable ATS for the lower half of the address space, so it should not 
affect graphics client that use the upper half.

Thanks,
   Felix


> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index eff73c428b12..291977b93b1d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2245,16 +2245,16 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   	if (r)
>   		return r;
>   
> -	/* Sanity checks */
> -	if (!amdgpu_vm_pt_is_root_clean(adev, vm)) {
> -		r = -EINVAL;
> -		goto unreserve_bo;
> -	}
> -
>   	/* Check if PD needs to be reinitialized and do it before
>   	 * changing any other state, in case it fails.
>   	 */
>   	if (pte_support_ats != vm->pte_support_ats) {
> +		/* Sanity checks */
> +		if (!amdgpu_vm_pt_is_root_clean(adev, vm)) {
> +			r = -EINVAL;
> +			goto unreserve_bo;
> +		}
> +
>   		vm->pte_support_ats = pte_support_ats;
>   		r = amdgpu_vm_pt_clear(adev, vm, to_amdgpu_bo_vm(vm->root.bo),
>   				       false);

Re: [PATCH] drm/amdgpu: remove vm sanity check from amdgpu_vm_make_compute

[Felix Kuehling]

On 2023-06-19 15:06, Xiaogang.Chen wrote:
> From: Xiaogang Chen <xiaogang.chen@amd.com>
>
> Since we allow kfd and graphic operate on same GPU VM to have interoperation
> between them GPU VM may have been used by graphic vm operations before kfd turn
> a GFX VM into a compute VM. Remove vm clean checking at amdgpu_vm_make_compute.
>
> Signed-off-by: Xiaogang Chen<Xiaogang.Chen@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index eff73c428b12..33f05297ab7e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2246,7 +2246,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   		return r;
>   
>   	/* Sanity checks */
> -	if (!amdgpu_vm_pt_is_root_clean(adev, vm)) {
> +	if (pte_support_ats && !amdgpu_vm_pt_is_root_clean(adev, vm)) {

I think the correct condition here would be "pte_support_ats != 
vm->pte_support_ats", because that's what's used to reinitialize the 
page table just below. I think it would be even cleaner if you moved 
that check inside the "if (pte_support_ats != vm->pte_support_ats)" 
block below.

Regards,
   Felix


>   		r = -EINVAL;
>   		goto unreserve_bo;
>   	}

[PATCH] drm/amdgpu: remove vm sanity check from amdgpu_vm_make_compute

[Xiaogang.Chen]

From: Xiaogang Chen <xiaogang.chen@amd.com>

Since we allow kfd and graphic operate on same GPU VM to have interoperation
between them GPU VM may have been used by graphic vm operations before kfd turn
a GFX VM into a compute VM. Remove vm clean checking at amdgpu_vm_make_compute.

Signed-off-by: Xiaogang Chen<Xiaogang.Chen@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index eff73c428b12..33f05297ab7e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2246,7 +2246,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 		return r;
 
 	/* Sanity checks */
-	if (!amdgpu_vm_pt_is_root_clean(adev, vm)) {
+	if (pte_support_ats && !amdgpu_vm_pt_is_root_clean(adev, vm)) {
 		r = -EINVAL;
 		goto unreserve_bo;
 	}
-- 
2.25.1