[PATCH 1/6] drm/amdgpu: Fix possible null pointer dereference

[PATCH 1/6] drm/amdgpu: Fix possible null pointer dereference

[Felix Kuehling]

mem = bo->tbo.resource may be NULL in amdgpu_vm_bo_update.

Fixes: 180253782038 ("drm/ttm: stop allocating dummy resources during BO creation")
Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 7b2a7c9156f0..1442d97ddd0f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1015,8 +1015,8 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
 				bo = gem_to_amdgpu_bo(gobj);
 		}
 		mem = bo->tbo.resource;
-		if (mem->mem_type == TTM_PL_TT ||
-		    mem->mem_type == AMDGPU_PL_PREEMPT)
+		if (mem && (mem->mem_type == TTM_PL_TT ||
+			    mem->mem_type == AMDGPU_PL_PREEMPT))
 			pages_addr = bo->tbo.ttm->dma_address;
 	}
 
-- 
2.34.1

[PATCH 2/6] drm/amdgpu: New VM state for evicted user BOs

[Felix Kuehling]

Create a new VM state to track user BOs that are in the system domain.
In the next patch this will be used do conditionally re-validate them in
amdgpu_vm_handle_moved.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 +++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  5 ++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 1442d97ddd0f..0d685577243c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -232,6 +232,22 @@ static void amdgpu_vm_bo_invalidated(struct amdgpu_vm_bo_base *vm_bo)
 	spin_unlock(&vm_bo->vm->status_lock);
 }
 
+/**
+ * amdgpu_vm_bo_evicted_user - vm_bo is evicted
+ *
+ * @vm_bo: vm_bo which is evicted
+ *
+ * State for BOs used by user mode queues which are not at the location they
+ * should be.
+ */
+static void amdgpu_vm_bo_evicted_user(struct amdgpu_vm_bo_base *vm_bo)
+{
+	vm_bo->moved = true;
+	spin_lock(&vm_bo->vm->status_lock);
+	list_move(&vm_bo->vm_status, &vm_bo->vm->evicted_user);
+	spin_unlock(&vm_bo->vm->status_lock);
+}
+
 /**
  * amdgpu_vm_bo_relocated - vm_bo is reloacted
  *
@@ -2110,6 +2126,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, int32_t xcp
 	for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
 		vm->reserved_vmid[i] = NULL;
 	INIT_LIST_HEAD(&vm->evicted);
+	INIT_LIST_HEAD(&vm->evicted_user);
 	INIT_LIST_HEAD(&vm->relocated);
 	INIT_LIST_HEAD(&vm->moved);
 	INIT_LIST_HEAD(&vm->idle);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 12cac06fa289..939d0c2219c0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -286,9 +286,12 @@ struct amdgpu_vm {
 	/* Lock to protect vm_bo add/del/move on all lists of vm */
 	spinlock_t		status_lock;
 
-	/* BOs who needs a validation */
+	/* Per VM and PT BOs who needs a validation */
 	struct list_head	evicted;
 
+	/* BOs for user mode queues that need a validation */
+	struct list_head	evicted_user;
+
 	/* PT BOs which relocated and their parent need an update */
 	struct list_head	relocated;
 
-- 
2.34.1

Re: [PATCH 2/6] drm/amdgpu: New VM state for evicted user BOs

[Felix Kuehling]

Hi Christian,

I know you have objected to this patch before. I still think this is the 
best solution for what I need. I can talk you through my reasoning by 
email or offline. If I can't convince you, I will need your guidance for 
a better solution.

Thanks,
   Felix


On 2023-11-07 11:58, Felix Kuehling wrote:
> Create a new VM state to track user BOs that are in the system domain.
> In the next patch this will be used do conditionally re-validate them in
> amdgpu_vm_handle_moved.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 +++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  5 ++++-
>   2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 1442d97ddd0f..0d685577243c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -232,6 +232,22 @@ static void amdgpu_vm_bo_invalidated(struct amdgpu_vm_bo_base *vm_bo)
>   	spin_unlock(&vm_bo->vm->status_lock);
>   }
>   
> +/**
> + * amdgpu_vm_bo_evicted_user - vm_bo is evicted
> + *
> + * @vm_bo: vm_bo which is evicted
> + *
> + * State for BOs used by user mode queues which are not at the location they
> + * should be.
> + */
> +static void amdgpu_vm_bo_evicted_user(struct amdgpu_vm_bo_base *vm_bo)
> +{
> +	vm_bo->moved = true;
> +	spin_lock(&vm_bo->vm->status_lock);
> +	list_move(&vm_bo->vm_status, &vm_bo->vm->evicted_user);
> +	spin_unlock(&vm_bo->vm->status_lock);
> +}
> +
>   /**
>    * amdgpu_vm_bo_relocated - vm_bo is reloacted
>    *
> @@ -2110,6 +2126,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, int32_t xcp
>   	for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
>   		vm->reserved_vmid[i] = NULL;
>   	INIT_LIST_HEAD(&vm->evicted);
> +	INIT_LIST_HEAD(&vm->evicted_user);
>   	INIT_LIST_HEAD(&vm->relocated);
>   	INIT_LIST_HEAD(&vm->moved);
>   	INIT_LIST_HEAD(&vm->idle);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 12cac06fa289..939d0c2219c0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -286,9 +286,12 @@ struct amdgpu_vm {
>   	/* Lock to protect vm_bo add/del/move on all lists of vm */
>   	spinlock_t		status_lock;
>   
> -	/* BOs who needs a validation */
> +	/* Per VM and PT BOs who needs a validation */
>   	struct list_head	evicted;
>   
> +	/* BOs for user mode queues that need a validation */
> +	struct list_head	evicted_user;
> +
>   	/* PT BOs which relocated and their parent need an update */
>   	struct list_head	relocated;
>   

Re: [PATCH 2/6] drm/amdgpu: New VM state for evicted user BOs

[Christian König]

Not necessary objections to this patch here, but rather how this new 
state is used later on.

The fundamental problem is that re-validating things in 
amdgpu_vm_handle_moved() won't work in all cases.

The general approach for both classic CS IOCTL as well as user queues is 
the following:

1. Grab all the necessary locks
     The VM lock + either everything inside the VM (user queues) or just 
the BOs in the submission (CS IOCTL).

2. Validate everything locked
     It can be that additional BO locks are grabbed for eviction and 
even locked BOs are shuffled around during that.

3. Update the PDEs and PTEs
     This can be done only after validating everything because things 
can still shuffle around during validation.

4. Do your CS or make the userqueu / process runable etc...

5. Attach fences to the locked BOs.

6. Unlock everything again.

I think that is part of the problem why handling all updates in 
amdgpu_vm_handle_moved() for the CS doesn't work and sooner or later you 
might run into the same issue during process restore as well.

If I'm not completely mistaken this should be solvable by moving all 
validation to amdgpu_vm_validate_pt_bos() instead (but let us rename 
that function).

Regards,
Christian.

Am 07.11.23 um 23:11 schrieb Felix Kuehling:
> Hi Christian,
>
> I know you have objected to this patch before. I still think this is 
> the best solution for what I need. I can talk you through my reasoning 
> by email or offline. If I can't convince you, I will need your 
> guidance for a better solution.
>
> Thanks,
>   Felix
>
>
> On 2023-11-07 11:58, Felix Kuehling wrote:
>> Create a new VM state to track user BOs that are in the system domain.
>> In the next patch this will be used do conditionally re-validate them in
>> amdgpu_vm_handle_moved.
>>
>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 +++++++++++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  5 ++++-
>>   2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 1442d97ddd0f..0d685577243c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -232,6 +232,22 @@ static void amdgpu_vm_bo_invalidated(struct 
>> amdgpu_vm_bo_base *vm_bo)
>>       spin_unlock(&vm_bo->vm->status_lock);
>>   }
>>   +/**
>> + * amdgpu_vm_bo_evicted_user - vm_bo is evicted
>> + *
>> + * @vm_bo: vm_bo which is evicted
>> + *
>> + * State for BOs used by user mode queues which are not at the 
>> location they
>> + * should be.
>> + */
>> +static void amdgpu_vm_bo_evicted_user(struct amdgpu_vm_bo_base *vm_bo)
>> +{
>> +    vm_bo->moved = true;
>> +    spin_lock(&vm_bo->vm->status_lock);
>> +    list_move(&vm_bo->vm_status, &vm_bo->vm->evicted_user);
>> +    spin_unlock(&vm_bo->vm->status_lock);
>> +}
>> +
>>   /**
>>    * amdgpu_vm_bo_relocated - vm_bo is reloacted
>>    *
>> @@ -2110,6 +2126,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, 
>> struct amdgpu_vm *vm, int32_t xcp
>>       for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
>>           vm->reserved_vmid[i] = NULL;
>>       INIT_LIST_HEAD(&vm->evicted);
>> +    INIT_LIST_HEAD(&vm->evicted_user);
>>       INIT_LIST_HEAD(&vm->relocated);
>>       INIT_LIST_HEAD(&vm->moved);
>>       INIT_LIST_HEAD(&vm->idle);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index 12cac06fa289..939d0c2219c0 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -286,9 +286,12 @@ struct amdgpu_vm {
>>       /* Lock to protect vm_bo add/del/move on all lists of vm */
>>       spinlock_t        status_lock;
>>   -    /* BOs who needs a validation */
>> +    /* Per VM and PT BOs who needs a validation */
>>       struct list_head    evicted;
>>   +    /* BOs for user mode queues that need a validation */
>> +    struct list_head    evicted_user;
>> +
>>       /* PT BOs which relocated and their parent need an update */
>>       struct list_head    relocated;

Re: [PATCH 2/6] drm/amdgpu: New VM state for evicted user BOs

[Felix Kuehling]

On 2023-11-08 07:28, Christian König wrote:
> Not necessary objections to this patch here, but rather how this new 
> state is used later on.
>
> The fundamental problem is that re-validating things in 
> amdgpu_vm_handle_moved() won't work in all cases.
>
> The general approach for both classic CS IOCTL as well as user queues 
> is the following:
>
> 1. Grab all the necessary locks
>     The VM lock + either everything inside the VM (user queues) or 
> just the BOs in the submission (CS IOCTL).
>
> 2. Validate everything locked
>     It can be that additional BO locks are grabbed for eviction and 
> even locked BOs are shuffled around during that.
>
> 3. Update the PDEs and PTEs
>     This can be done only after validating everything because things 
> can still shuffle around during validation.
>
> 4. Do your CS or make the userqueu / process runable etc...
>
> 5. Attach fences to the locked BOs.
>
> 6. Unlock everything again.
>
> I think that is part of the problem why handling all updates in 
> amdgpu_vm_handle_moved() for the CS doesn't work and sooner or later 
> you might run into the same issue during process restore as well.
>
> If I'm not completely mistaken this should be solvable by moving all 
> validation to amdgpu_vm_validate_pt_bos() instead (but let us rename 
> that function).

I'm trying to understand what the problem is. As far as I can tell, 
amdgpu_vm_handle_moved just runs a bit later in the CS and 
process-restore code than amdgpu_vm_validate_pt_bos. But it runs with 
all the same reservations locked. My current implementation in 
amdgpu_vm_handle_moved has some failure cases when reservations cannot 
be acquired. I think your drm_exec patch series would make it possible 
to handle this more robustly. That said, in case of KFD process restore, 
we can retry in case of failures already.

Anyway, moving my re-validation code to a renamed version of 
amdgpu_vm_validate_pt_bos shouldn't be a big problem. I just don't 
understand what problem that solves. Maybe it just makes the code 
cleaner to handle both evicted states in one place? Or maybe you're 
pointing to a way to merge the two states into one?

Regards,
   Felix


>
> Regards,
> Christian.
>
> Am 07.11.23 um 23:11 schrieb Felix Kuehling:
>> Hi Christian,
>>
>> I know you have objected to this patch before. I still think this is 
>> the best solution for what I need. I can talk you through my 
>> reasoning by email or offline. If I can't convince you, I will need 
>> your guidance for a better solution.
>>
>> Thanks,
>>   Felix
>>
>>
>> On 2023-11-07 11:58, Felix Kuehling wrote:
>>> Create a new VM state to track user BOs that are in the system domain.
>>> In the next patch this will be used do conditionally re-validate 
>>> them in
>>> amdgpu_vm_handle_moved.
>>>
>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 +++++++++++++++++
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  5 ++++-
>>>   2 files changed, 21 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 1442d97ddd0f..0d685577243c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -232,6 +232,22 @@ static void amdgpu_vm_bo_invalidated(struct 
>>> amdgpu_vm_bo_base *vm_bo)
>>>       spin_unlock(&vm_bo->vm->status_lock);
>>>   }
>>>   +/**
>>> + * amdgpu_vm_bo_evicted_user - vm_bo is evicted
>>> + *
>>> + * @vm_bo: vm_bo which is evicted
>>> + *
>>> + * State for BOs used by user mode queues which are not at the 
>>> location they
>>> + * should be.
>>> + */
>>> +static void amdgpu_vm_bo_evicted_user(struct amdgpu_vm_bo_base *vm_bo)
>>> +{
>>> +    vm_bo->moved = true;
>>> +    spin_lock(&vm_bo->vm->status_lock);
>>> +    list_move(&vm_bo->vm_status, &vm_bo->vm->evicted_user);
>>> +    spin_unlock(&vm_bo->vm->status_lock);
>>> +}
>>> +
>>>   /**
>>>    * amdgpu_vm_bo_relocated - vm_bo is reloacted
>>>    *
>>> @@ -2110,6 +2126,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, 
>>> struct amdgpu_vm *vm, int32_t xcp
>>>       for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
>>>           vm->reserved_vmid[i] = NULL;
>>>       INIT_LIST_HEAD(&vm->evicted);
>>> +    INIT_LIST_HEAD(&vm->evicted_user);
>>>       INIT_LIST_HEAD(&vm->relocated);
>>>       INIT_LIST_HEAD(&vm->moved);
>>>       INIT_LIST_HEAD(&vm->idle);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> index 12cac06fa289..939d0c2219c0 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> @@ -286,9 +286,12 @@ struct amdgpu_vm {
>>>       /* Lock to protect vm_bo add/del/move on all lists of vm */
>>>       spinlock_t        status_lock;
>>>   -    /* BOs who needs a validation */
>>> +    /* Per VM and PT BOs who needs a validation */
>>>       struct list_head    evicted;
>>>   +    /* BOs for user mode queues that need a validation */
>>> +    struct list_head    evicted_user;
>>> +
>>>       /* PT BOs which relocated and their parent need an update */
>>>       struct list_head    relocated;
>

Re: [PATCH 2/6] drm/amdgpu: New VM state for evicted user BOs

[Christian König]

Am 08.11.23 um 22:23 schrieb Felix Kuehling:
>
> On 2023-11-08 07:28, Christian König wrote:
>> Not necessary objections to this patch here, but rather how this new 
>> state is used later on.
>>
>> The fundamental problem is that re-validating things in 
>> amdgpu_vm_handle_moved() won't work in all cases.
>>
>> The general approach for both classic CS IOCTL as well as user queues 
>> is the following:
>>
>> 1. Grab all the necessary locks
>>     The VM lock + either everything inside the VM (user queues) or 
>> just the BOs in the submission (CS IOCTL).
>>
>> 2. Validate everything locked
>>     It can be that additional BO locks are grabbed for eviction and 
>> even locked BOs are shuffled around during that.
>>
>> 3. Update the PDEs and PTEs
>>     This can be done only after validating everything because things 
>> can still shuffle around during validation.
>>
>> 4. Do your CS or make the userqueu / process runable etc...
>>
>> 5. Attach fences to the locked BOs.
>>
>> 6. Unlock everything again.
>>
>> I think that is part of the problem why handling all updates in 
>> amdgpu_vm_handle_moved() for the CS doesn't work and sooner or later 
>> you might run into the same issue during process restore as well.
>>
>> If I'm not completely mistaken this should be solvable by moving all 
>> validation to amdgpu_vm_validate_pt_bos() instead (but let us rename 
>> that function).
>
> I'm trying to understand what the problem is. As far as I can tell, 
> amdgpu_vm_handle_moved just runs a bit later in the CS and 
> process-restore code than amdgpu_vm_validate_pt_bos. But it runs with 
> all the same reservations locked. My current implementation in 
> amdgpu_vm_handle_moved has some failure cases when reservations cannot 
> be acquired. I think your drm_exec patch series would make it possible 
> to handle this more robustly. That said, in case of KFD process 
> restore, we can retry in case of failures already.
>
> Anyway, moving my re-validation code to a renamed version of 
> amdgpu_vm_validate_pt_bos shouldn't be a big problem. I just don't 
> understand what problem that solves. Maybe it just makes the code 
> cleaner to handle both evicted states in one place? Or maybe you're 
> pointing to a way to merge the two states into one?

Well yeah merging both state into one might be nice to have, but that 
isn't the fundamental problem.

What basically happens during validation of a BO is that this BO is move 
to a desired place (as described by the placement parameter). During 
this it can happen that we shuffle out other BOs.

Now the issue is that when you validate in amdgpu_vm_handle_moved() it 
can be that by validating BO we shuffle out other BOs which then end up 
of the evicted list again.

This most likely won't happen with KFD, but for GFX CS it's certainly 
possible. So the idea is that we first validate everything and then 
update all the page tables and not inter mix the two operations.

Regards,
Christian.

>
> Regards,
>   Felix
>
>
>>
>> Regards,
>> Christian.
>>
>> Am 07.11.23 um 23:11 schrieb Felix Kuehling:
>>> Hi Christian,
>>>
>>> I know you have objected to this patch before. I still think this is 
>>> the best solution for what I need. I can talk you through my 
>>> reasoning by email or offline. If I can't convince you, I will need 
>>> your guidance for a better solution.
>>>
>>> Thanks,
>>>   Felix
>>>
>>>
>>> On 2023-11-07 11:58, Felix Kuehling wrote:
>>>> Create a new VM state to track user BOs that are in the system domain.
>>>> In the next patch this will be used do conditionally re-validate 
>>>> them in
>>>> amdgpu_vm_handle_moved.
>>>>
>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 +++++++++++++++++
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  5 ++++-
>>>>   2 files changed, 21 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 1442d97ddd0f..0d685577243c 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -232,6 +232,22 @@ static void amdgpu_vm_bo_invalidated(struct 
>>>> amdgpu_vm_bo_base *vm_bo)
>>>>       spin_unlock(&vm_bo->vm->status_lock);
>>>>   }
>>>>   +/**
>>>> + * amdgpu_vm_bo_evicted_user - vm_bo is evicted
>>>> + *
>>>> + * @vm_bo: vm_bo which is evicted
>>>> + *
>>>> + * State for BOs used by user mode queues which are not at the 
>>>> location they
>>>> + * should be.
>>>> + */
>>>> +static void amdgpu_vm_bo_evicted_user(struct amdgpu_vm_bo_base 
>>>> *vm_bo)
>>>> +{
>>>> +    vm_bo->moved = true;
>>>> +    spin_lock(&vm_bo->vm->status_lock);
>>>> +    list_move(&vm_bo->vm_status, &vm_bo->vm->evicted_user);
>>>> +    spin_unlock(&vm_bo->vm->status_lock);
>>>> +}
>>>> +
>>>>   /**
>>>>    * amdgpu_vm_bo_relocated - vm_bo is reloacted
>>>>    *
>>>> @@ -2110,6 +2126,7 @@ int amdgpu_vm_init(struct amdgpu_device 
>>>> *adev, struct amdgpu_vm *vm, int32_t xcp
>>>>       for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
>>>>           vm->reserved_vmid[i] = NULL;
>>>>       INIT_LIST_HEAD(&vm->evicted);
>>>> +    INIT_LIST_HEAD(&vm->evicted_user);
>>>>       INIT_LIST_HEAD(&vm->relocated);
>>>>       INIT_LIST_HEAD(&vm->moved);
>>>>       INIT_LIST_HEAD(&vm->idle);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> index 12cac06fa289..939d0c2219c0 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> @@ -286,9 +286,12 @@ struct amdgpu_vm {
>>>>       /* Lock to protect vm_bo add/del/move on all lists of vm */
>>>>       spinlock_t        status_lock;
>>>>   -    /* BOs who needs a validation */
>>>> +    /* Per VM and PT BOs who needs a validation */
>>>>       struct list_head    evicted;
>>>>   +    /* BOs for user mode queues that need a validation */
>>>> +    struct list_head    evicted_user;
>>>> +
>>>>       /* PT BOs which relocated and their parent need an update */
>>>>       struct list_head    relocated;
>>

Re: [PATCH 2/6] drm/amdgpu: New VM state for evicted user BOs

[Felix Kuehling]

On 2023-11-09 03:12, Christian König wrote:
> Am 08.11.23 um 22:23 schrieb Felix Kuehling:
>>
>> On 2023-11-08 07:28, Christian König wrote:
>>> Not necessary objections to this patch here, but rather how this new 
>>> state is used later on.
>>>
>>> The fundamental problem is that re-validating things in 
>>> amdgpu_vm_handle_moved() won't work in all cases.
>>>
>>> The general approach for both classic CS IOCTL as well as user 
>>> queues is the following:
>>>
>>> 1. Grab all the necessary locks
>>>     The VM lock + either everything inside the VM (user queues) or 
>>> just the BOs in the submission (CS IOCTL).
>>>
>>> 2. Validate everything locked
>>>     It can be that additional BO locks are grabbed for eviction and 
>>> even locked BOs are shuffled around during that.
>>>
>>> 3. Update the PDEs and PTEs
>>>     This can be done only after validating everything because things 
>>> can still shuffle around during validation.
>>>
>>> 4. Do your CS or make the userqueu / process runable etc...
>>>
>>> 5. Attach fences to the locked BOs.
>>>
>>> 6. Unlock everything again.
>>>
>>> I think that is part of the problem why handling all updates in 
>>> amdgpu_vm_handle_moved() for the CS doesn't work and sooner or later 
>>> you might run into the same issue during process restore as well.
>>>
>>> If I'm not completely mistaken this should be solvable by moving all 
>>> validation to amdgpu_vm_validate_pt_bos() instead (but let us rename 
>>> that function).
>>
>> I'm trying to understand what the problem is. As far as I can tell, 
>> amdgpu_vm_handle_moved just runs a bit later in the CS and 
>> process-restore code than amdgpu_vm_validate_pt_bos. But it runs with 
>> all the same reservations locked. My current implementation in 
>> amdgpu_vm_handle_moved has some failure cases when reservations 
>> cannot be acquired. I think your drm_exec patch series would make it 
>> possible to handle this more robustly. That said, in case of KFD 
>> process restore, we can retry in case of failures already.
>>
>> Anyway, moving my re-validation code to a renamed version of 
>> amdgpu_vm_validate_pt_bos shouldn't be a big problem. I just don't 
>> understand what problem that solves. Maybe it just makes the code 
>> cleaner to handle both evicted states in one place? Or maybe you're 
>> pointing to a way to merge the two states into one?
>
> Well yeah merging both state into one might be nice to have, but that 
> isn't the fundamental problem.
>
> What basically happens during validation of a BO is that this BO is 
> move to a desired place (as described by the placement parameter). 
> During this it can happen that we shuffle out other BOs.
>
> Now the issue is that when you validate in amdgpu_vm_handle_moved() it 
> can be that by validating BO we shuffle out other BOs which then end 
> up of the evicted list again.
>
> This most likely won't happen with KFD, but for GFX CS it's certainly 
> possible. So the idea is that we first validate everything and then 
> update all the page tables and not inter mix the two operations.

I have something working now. But I think the fundamental problem is 
what you say, that BOs get can evicted again before we update the page 
tables. It won't be a problem for KFD, because I only care about DMABuf 
imports, and they don't get evicted if I have the original allocations 
reserved. I just have to make sure I validate/move the original 
allocations before I validate their DMABuf imports. This could still be 
a problem for future graphics interop.

The problem is, that I don't currently have a way to keep the validated 
BOs reserved to prevent their eviction. That is on amd-staging-drm-next, 
where I don't have your drm_exec patch series yet. Maybe this will get 
easier once the branch gets rebased on Linux 6.5 or newer.

Regards,
   Felix


>
> Regards,
> Christian.
>
>>
>> Regards,
>>   Felix
>>
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>> Am 07.11.23 um 23:11 schrieb Felix Kuehling:
>>>> Hi Christian,
>>>>
>>>> I know you have objected to this patch before. I still think this 
>>>> is the best solution for what I need. I can talk you through my 
>>>> reasoning by email or offline. If I can't convince you, I will need 
>>>> your guidance for a better solution.
>>>>
>>>> Thanks,
>>>>   Felix
>>>>
>>>>
>>>> On 2023-11-07 11:58, Felix Kuehling wrote:
>>>>> Create a new VM state to track user BOs that are in the system 
>>>>> domain.
>>>>> In the next patch this will be used do conditionally re-validate 
>>>>> them in
>>>>> amdgpu_vm_handle_moved.
>>>>>
>>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 +++++++++++++++++
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  5 ++++-
>>>>>   2 files changed, 21 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> index 1442d97ddd0f..0d685577243c 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> @@ -232,6 +232,22 @@ static void amdgpu_vm_bo_invalidated(struct 
>>>>> amdgpu_vm_bo_base *vm_bo)
>>>>>       spin_unlock(&vm_bo->vm->status_lock);
>>>>>   }
>>>>>   +/**
>>>>> + * amdgpu_vm_bo_evicted_user - vm_bo is evicted
>>>>> + *
>>>>> + * @vm_bo: vm_bo which is evicted
>>>>> + *
>>>>> + * State for BOs used by user mode queues which are not at the 
>>>>> location they
>>>>> + * should be.
>>>>> + */
>>>>> +static void amdgpu_vm_bo_evicted_user(struct amdgpu_vm_bo_base 
>>>>> *vm_bo)
>>>>> +{
>>>>> +    vm_bo->moved = true;
>>>>> +    spin_lock(&vm_bo->vm->status_lock);
>>>>> +    list_move(&vm_bo->vm_status, &vm_bo->vm->evicted_user);
>>>>> +    spin_unlock(&vm_bo->vm->status_lock);
>>>>> +}
>>>>> +
>>>>>   /**
>>>>>    * amdgpu_vm_bo_relocated - vm_bo is reloacted
>>>>>    *
>>>>> @@ -2110,6 +2126,7 @@ int amdgpu_vm_init(struct amdgpu_device 
>>>>> *adev, struct amdgpu_vm *vm, int32_t xcp
>>>>>       for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
>>>>>           vm->reserved_vmid[i] = NULL;
>>>>>       INIT_LIST_HEAD(&vm->evicted);
>>>>> +    INIT_LIST_HEAD(&vm->evicted_user);
>>>>>       INIT_LIST_HEAD(&vm->relocated);
>>>>>       INIT_LIST_HEAD(&vm->moved);
>>>>>       INIT_LIST_HEAD(&vm->idle);
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> index 12cac06fa289..939d0c2219c0 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> @@ -286,9 +286,12 @@ struct amdgpu_vm {
>>>>>       /* Lock to protect vm_bo add/del/move on all lists of vm */
>>>>>       spinlock_t        status_lock;
>>>>>   -    /* BOs who needs a validation */
>>>>> +    /* Per VM and PT BOs who needs a validation */
>>>>>       struct list_head    evicted;
>>>>>   +    /* BOs for user mode queues that need a validation */
>>>>> +    struct list_head    evicted_user;
>>>>> +
>>>>>       /* PT BOs which relocated and their parent need an update */
>>>>>       struct list_head    relocated;
>>>
>

[PATCH 3/6] drm/amdgpu: Auto-validate DMABuf imports in compute VMs

[Felix Kuehling]

DMABuf imports in compute VMs are not wrapped in a kgd_mem object on the
process_info->kfd_bo_list. There is no explicit KFD API call to validate
them or add eviction fences to them.

This patch automatically validates and fences dymanic DMABuf imports when
they are added to a compute VM. Revalidation after evictions is handled
in the VM code.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |   3 +
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  15 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        |   2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c   |   6 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  26 ++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 117 +++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   6 +-
 7 files changed, 164 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index fcf8a98ad15e..68d534a89942 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -178,6 +178,9 @@ int amdgpu_queue_mask_bit_to_set_resource_bit(struct amdgpu_device *adev,
 struct amdgpu_amdkfd_fence *amdgpu_amdkfd_fence_create(u64 context,
 				struct mm_struct *mm,
 				struct svm_range_bo *svm_bo);
+int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo,
+					uint32_t domain,
+					struct dma_fence *fence);
 #if defined(CONFIG_DEBUG_FS)
 int kfd_debugfs_kfd_mem_limits(struct seq_file *m, void *data);
 #endif
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 2e302956a279..0c1cb6048259 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -423,9 +423,9 @@ static int amdgpu_amdkfd_bo_validate(struct amdgpu_bo *bo, uint32_t domain,
 	return ret;
 }
 
-static int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo,
-					       uint32_t domain,
-					       struct dma_fence *fence)
+int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo,
+					uint32_t domain,
+					struct dma_fence *fence)
 {
 	int ret = amdgpu_bo_reserve(bo, false);
 
@@ -2948,7 +2948,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)
 		struct amdgpu_device *adev = amdgpu_ttm_adev(
 			peer_vm->root.bo->tbo.bdev);
 
-		ret = amdgpu_vm_handle_moved(adev, peer_vm, &ctx.ticket);
+		ret = amdgpu_vm_handle_moved(adev, peer_vm, &ctx.ticket, true);
 		if (ret) {
 			pr_debug("Memory eviction: handle moved failed. Try again\n");
 			goto validate_map_fail;
@@ -3001,7 +3001,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)
 				   &process_info->eviction_fence->base,
 				   DMA_RESV_USAGE_BOOKKEEP);
 	}
-	/* Attach eviction fence to PD / PT BOs */
+	/* Attach eviction fence to PD / PT BOs and DMABuf imports */
 	list_for_each_entry(peer_vm, &process_info->vm_list_head,
 			    vm_list_node) {
 		struct amdgpu_bo *bo = peer_vm->root.bo;
@@ -3009,6 +3009,11 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)
 		dma_resv_add_fence(bo->tbo.base.resv,
 				   &process_info->eviction_fence->base,
 				   DMA_RESV_USAGE_BOOKKEEP);
+
+		ret = amdgpu_vm_fence_imports(peer_vm, &ctx.ticket,
+					      &process_info->eviction_fence->base);
+		if (ret)
+			break;
 	}
 
 validate_map_fail:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index aafedb344c1b..20f4be8cd635 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1165,7 +1165,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 			return r;
 	}
 
-	r = amdgpu_vm_handle_moved(adev, vm, &p->ticket);
+	r = amdgpu_vm_handle_moved(adev, vm, &p->ticket, false);
 	if (r)
 		return r;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
index e7e87a3b2601..234244704f27 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
@@ -373,6 +373,10 @@ amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach)
 	struct amdgpu_vm_bo_base *bo_base;
 	int r;
 
+	/* FIXME: This should be after the "if", but needs a fix to make sure
+	 * DMABuf imports are initialized in the right VM list.
+	 */
+	amdgpu_vm_bo_invalidate(adev, bo, false);
 	if (!bo->tbo.resource || bo->tbo.resource->mem_type == TTM_PL_SYSTEM)
 		return;
 
@@ -409,7 +413,7 @@ amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach)
 		if (!r)
 			r = amdgpu_vm_clear_freed(adev, vm, NULL);
 		if (!r)
-			r = amdgpu_vm_handle_moved(adev, vm, ticket);
+			r = amdgpu_vm_handle_moved(adev, vm, ticket, false);
 
 		if (r && r != -EBUSY)
 			DRM_ERROR("Failed to invalidate VM page tables (%d))\n",
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 849fffbb367d..755cc3c559f5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -186,6 +186,32 @@ static int amdgpu_gem_object_open(struct drm_gem_object *obj,
 	else
 		++bo_va->ref_count;
 	amdgpu_bo_unreserve(abo);
+
+	/* Validate and add eviction fence to DMABuf imports with dymanic
+	 * attachment in compute VMs. Re-validation will be done by
+	 * amdgpu_vm_handle_moved and the fence will be updated by
+	 * amdgpu_vm_fence_imports in amdgpu_amdkfd_gpuvm_restore_process_bos.
+	 *
+	 * Nested locking below for the case that a GEM object is opened in
+	 * kfd_mem_export_dmabuf. Since the lock below is only taken for imports,
+	 * but not for export, this is a different lock class that cannot lead to
+	 * circular lock dependencies.
+	 */
+	if (!vm->is_compute_context || !vm->process_info)
+		return 0;
+	if (!obj->import_attach ||
+	    !dma_buf_is_dynamic(obj->import_attach->dmabuf))
+		return 0;
+	mutex_lock_nested(&vm->process_info->lock, 1);
+	if (!WARN_ON(!vm->process_info->eviction_fence)) {
+		r = amdgpu_amdkfd_bo_validate_and_fence(abo, AMDGPU_GEM_DOMAIN_GTT,
+							&vm->process_info->eviction_fence->base);
+		if (r)
+			dev_warn(adev->dev, "%d: validate_and_fence failed: %d\n",
+				 vm->task_info.pid, r);
+	}
+	mutex_unlock(&vm->process_info->lock);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 0d685577243c..b2c7449ab561 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1307,6 +1307,7 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
  * @adev: amdgpu_device pointer
  * @vm: requested vm
  * @ticket: optional reservation ticket used to reserve the VM
+ * @valitate: whether to auto-validate invalid DMABuf imports
  *
  * Make sure all BOs which are moved are updated in the PTs.
  *
@@ -1317,7 +1318,8 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
  */
 int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
 			   struct amdgpu_vm *vm,
-			   struct ww_acquire_ctx *ticket)
+			   struct ww_acquire_ctx *ticket,
+			   bool validate)
 {
 	struct amdgpu_bo_va *bo_va;
 	struct dma_resv *resv;
@@ -1337,6 +1339,12 @@ int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
 		spin_lock(&vm->status_lock);
 	}
 
+	/* If we're validating user BOs, splice all evicted user BOs into
+	 * the list of invalid BOs for revalidation
+	 */
+	if (validate)
+		list_splice_init(&vm->evicted_user, &vm->invalidated);
+
 	while (!list_empty(&vm->invalidated)) {
 		bo_va = list_first_entry(&vm->invalidated, struct amdgpu_bo_va,
 					 base.vm_status);
@@ -1357,17 +1365,120 @@ int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
 			unlock = false;
 		}
 
+		/* Automatically validate DMABuf imports in compute VMs, if we
+		 * have a reservation, or remember them for later validation.
+		 */
+		if (vm->is_compute_context && bo_va->base.bo &&
+		    bo_va->base.bo->tbo.base.import_attach &&
+		    (!bo_va->base.bo->tbo.resource ||
+		     bo_va->base.bo->tbo.resource->mem_type == TTM_PL_SYSTEM)) {
+			struct ttm_operation_ctx ctx = { true, false };
+			struct amdgpu_bo *bo = bo_va->base.bo;
+
+			if (!validate) {
+				r = amdgpu_vm_bo_update(adev, bo_va, clear);
+				if (!r)
+					amdgpu_vm_bo_evicted_user(&bo_va->base);
+				goto unlock;
+			}
+
+			if (clear) {
+				pr_warn_ratelimited("Invalid DMABuf import is busy in pid %d\n", vm->task_info.pid);
+				r = -EBUSY;
+				goto unlock;
+			}
+
+			amdgpu_bo_placement_from_domain(bo,
+							bo->preferred_domains);
+			r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
+			if (r)
+				goto unlock;
+			r = amdgpu_bo_sync_wait(bo, AMDGPU_FENCE_OWNER_KFD,
+						true);
+			if (r)
+				goto unlock;
+		}
+
 		r = amdgpu_vm_bo_update(adev, bo_va, clear);
+unlock:
+		if (unlock)
+			dma_resv_unlock(resv);
 		if (r)
 			return r;
+		spin_lock(&vm->status_lock);
+	}
+	spin_unlock(&vm->status_lock);
+
+	return 0;
+}
+
+/**
+ * amdgpu_vm_fence_imports - add fence to valid DMABuf imports
+ *
+ * @vm: requested vm
+ * @ticket: optional reservation ticket used to reserve the VM
+ * @fence: fence to add
+ *
+ * Add the specified fence to all dymanic DMABuf imports that are valid.
+ *
+ * Returns:
+ * 0 for success.
+ */
+int amdgpu_vm_fence_imports(struct amdgpu_vm *vm,
+			    struct ww_acquire_ctx *ticket,
+			    struct dma_fence *fence)
+{
+	struct amdgpu_bo_va *bo_va, *tmp;
+	struct dma_resv *resv;
+	LIST_HEAD(imports);
+	bool unlock;
+	int ret = 0;
+
+	if (!vm->is_compute_context)
+		return 0;
+
+	/* Move all the DMABuf imports to a private list so I can reserve
+	 * them while not holding te status_lock.
+	 */
+	spin_lock(&vm->status_lock);
+	list_for_each_entry_safe(bo_va, tmp, &vm->idle, base.vm_status) {
+		if (bo_va->base.bo && bo_va->base.bo->tbo.base.import_attach &&
+		    dma_buf_is_dynamic(bo_va->base.bo->tbo.base.import_attach->dmabuf))
+			list_move(&bo_va->base.vm_status, &imports);
+	}
+	spin_unlock(&vm->status_lock);
+
+	list_for_each_entry(bo_va, &imports, base.vm_status) {
+		resv = bo_va->base.bo->tbo.base.resv;
+
+		/* Try to reserve the BO */
+		if (dma_resv_trylock(resv)) {
+			unlock = true;
+		/* The caller is already holding the reservation lock */
+		} else if (ticket && dma_resv_locking_ctx(resv) == ticket) {
+			unlock = false;
+		} else {
+			WARN_ONCE(1, "Failed to reserve DMABuf import");
+			ret = -EBUSY;
+			break;
+		}
+
+		ret = dma_resv_reserve_fences(resv, 1);
+		if (!ret)
+			dma_resv_add_fence(resv, fence,
+					   DMA_RESV_USAGE_BOOKKEEP);
 
 		if (unlock)
 			dma_resv_unlock(resv);
-		spin_lock(&vm->status_lock);
+		if (ret)
+			break;
 	}
+
+	spin_lock(&vm->status_lock);
+	list_splice(&imports, &vm->idle);
 	spin_unlock(&vm->status_lock);
 
-	return 0;
+	return ret;
 }
 
 /**
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 939d0c2219c0..2db04b8fef97 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -443,7 +443,11 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
 			  struct dma_fence **fence);
 int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
 			   struct amdgpu_vm *vm,
-			   struct ww_acquire_ctx *ticket);
+			   struct ww_acquire_ctx *ticket,
+			   bool validate);
+int amdgpu_vm_fence_imports(struct amdgpu_vm *vm,
+			    struct ww_acquire_ctx *ticket,
+			    struct dma_fence *fence);
 int amdgpu_vm_flush_compute_tlb(struct amdgpu_device *adev,
 				struct amdgpu_vm *vm,
 				uint32_t flush_type,
-- 
2.34.1

RE: [PATCH 3/6] drm/amdgpu: Auto-validate DMABuf imports in compute VMs

[Joshi, Mukul]

[AMD Official Use Only - General]

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Felix
> Kuehling
> Sent: Tuesday, November 7, 2023 11:58 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Chen, Xiaogang <Xiaogang.Chen@amd.com>; Errabolu, Ramesh
> <Ramesh.Errabolu@amd.com>; Koenig, Christian
> <Christian.Koenig@amd.com>
> Subject: [PATCH 3/6] drm/amdgpu: Auto-validate DMABuf imports in
> compute VMs
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> DMABuf imports in compute VMs are not wrapped in a kgd_mem object on
> the process_info->kfd_bo_list. There is no explicit KFD API call to validate them
> or add eviction fences to them.
>
> This patch automatically validates and fences dymanic DMABuf imports when
> they are added to a compute VM. Revalidation after evictions is handled in the
> VM code.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |   3 +
>  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  15 ++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        |   2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c   |   6 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  26 ++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 117
> +++++++++++++++++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   6 +-
>  7 files changed, 164 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index fcf8a98ad15e..68d534a89942 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -178,6 +178,9 @@ int
> amdgpu_queue_mask_bit_to_set_resource_bit(struct amdgpu_device *adev,
> struct amdgpu_amdkfd_fence *amdgpu_amdkfd_fence_create(u64 context,
>                                 struct mm_struct *mm,
>                                 struct svm_range_bo *svm_bo);
> +int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo,
> +                                       uint32_t domain,
> +                                       struct dma_fence *fence);
>  #if defined(CONFIG_DEBUG_FS)
>  int kfd_debugfs_kfd_mem_limits(struct seq_file *m, void *data);  #endif diff --
> git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 2e302956a279..0c1cb6048259 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -423,9 +423,9 @@ static int amdgpu_amdkfd_bo_validate(struct
> amdgpu_bo *bo, uint32_t domain,
>         return ret;
>  }
>
> -static int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo,
> -                                              uint32_t domain,
> -                                              struct dma_fence *fence)
> +int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo,
> +                                       uint32_t domain,
> +                                       struct dma_fence *fence)
>  {
>         int ret = amdgpu_bo_reserve(bo, false);
>
> @@ -2948,7 +2948,7 @@ int
> amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence
> **ef)
>                 struct amdgpu_device *adev = amdgpu_ttm_adev(
>                         peer_vm->root.bo->tbo.bdev);
>
> -               ret = amdgpu_vm_handle_moved(adev, peer_vm, &ctx.ticket);
> +               ret = amdgpu_vm_handle_moved(adev, peer_vm, &ctx.ticket,
> + true);
>                 if (ret) {
>                         pr_debug("Memory eviction: handle moved failed. Try again\n");
>                         goto validate_map_fail; @@ -3001,7 +3001,7 @@ int
> amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence
> **ef)
>                                    &process_info->eviction_fence->base,
>                                    DMA_RESV_USAGE_BOOKKEEP);
>         }
> -       /* Attach eviction fence to PD / PT BOs */
> +       /* Attach eviction fence to PD / PT BOs and DMABuf imports */
>         list_for_each_entry(peer_vm, &process_info->vm_list_head,
>                             vm_list_node) {
>                 struct amdgpu_bo *bo = peer_vm->root.bo; @@ -3009,6 +3009,11
> @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct
> dma_fence **ef)
>                 dma_resv_add_fence(bo->tbo.base.resv,
>                                    &process_info->eviction_fence->base,
>                                    DMA_RESV_USAGE_BOOKKEEP);
> +
> +               ret = amdgpu_vm_fence_imports(peer_vm, &ctx.ticket,
> +                                             &process_info->eviction_fence->base);
> +               if (ret)
> +                       break;
>         }
>
>  validate_map_fail:
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index aafedb344c1b..20f4be8cd635 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1165,7 +1165,7 @@ static int amdgpu_cs_vm_handling(struct
> amdgpu_cs_parser *p)
>                         return r;
>         }
>
> -       r = amdgpu_vm_handle_moved(adev, vm, &p->ticket);
> +       r = amdgpu_vm_handle_moved(adev, vm, &p->ticket, false);
>         if (r)
>                 return r;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> index e7e87a3b2601..234244704f27 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> @@ -373,6 +373,10 @@ amdgpu_dma_buf_move_notify(struct
> dma_buf_attachment *attach)
>         struct amdgpu_vm_bo_base *bo_base;
>         int r;
>
> +       /* FIXME: This should be after the "if", but needs a fix to make sure
> +        * DMABuf imports are initialized in the right VM list.
> +        */
> +       amdgpu_vm_bo_invalidate(adev, bo, false);
>         if (!bo->tbo.resource || bo->tbo.resource->mem_type ==
> TTM_PL_SYSTEM)
>                 return;
>
> @@ -409,7 +413,7 @@ amdgpu_dma_buf_move_notify(struct
> dma_buf_attachment *attach)
>                 if (!r)
>                         r = amdgpu_vm_clear_freed(adev, vm, NULL);
>                 if (!r)
> -                       r = amdgpu_vm_handle_moved(adev, vm, ticket);
> +                       r = amdgpu_vm_handle_moved(adev, vm, ticket,
> + false);
>
>                 if (r && r != -EBUSY)
>                         DRM_ERROR("Failed to invalidate VM page tables (%d))\n", diff --
> git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 849fffbb367d..755cc3c559f5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -186,6 +186,32 @@ static int amdgpu_gem_object_open(struct
> drm_gem_object *obj,
>         else
>                 ++bo_va->ref_count;
>         amdgpu_bo_unreserve(abo);
> +
> +       /* Validate and add eviction fence to DMABuf imports with dymanic

dymanic -> dynamic

> +        * attachment in compute VMs. Re-validation will be done by
> +        * amdgpu_vm_handle_moved and the fence will be updated by
> +        * amdgpu_vm_fence_imports in
> amdgpu_amdkfd_gpuvm_restore_process_bos.
> +        *
> +        * Nested locking below for the case that a GEM object is opened in
> +        * kfd_mem_export_dmabuf. Since the lock below is only taken for
> imports,
> +        * but not for export, this is a different lock class that cannot lead to
> +        * circular lock dependencies.
> +        */
> +       if (!vm->is_compute_context || !vm->process_info)
> +               return 0;
> +       if (!obj->import_attach ||
> +           !dma_buf_is_dynamic(obj->import_attach->dmabuf))
> +               return 0;
> +       mutex_lock_nested(&vm->process_info->lock, 1);
> +       if (!WARN_ON(!vm->process_info->eviction_fence)) {
> +               r = amdgpu_amdkfd_bo_validate_and_fence(abo,
> AMDGPU_GEM_DOMAIN_GTT,
> +                                                       &vm->process_info->eviction_fence->base);
> +               if (r)
> +                       dev_warn(adev->dev, "%d: validate_and_fence failed: %d\n",
> +                                vm->task_info.pid, r);
> +       }
> +       mutex_unlock(&vm->process_info->lock);
> +
>         return 0;
>  }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 0d685577243c..b2c7449ab561 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1307,6 +1307,7 @@ int amdgpu_vm_clear_freed(struct
> amdgpu_device *adev,
>   * @adev: amdgpu_device pointer
>   * @vm: requested vm
>   * @ticket: optional reservation ticket used to reserve the VM
> + * @valitate: whether to auto-validate invalid DMABuf imports

valitate -> validate

>   *
>   * Make sure all BOs which are moved are updated in the PTs.
>   *
> @@ -1317,7 +1318,8 @@ int amdgpu_vm_clear_freed(struct
> amdgpu_device *adev,
>   */
>  int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
>                            struct amdgpu_vm *vm,
> -                          struct ww_acquire_ctx *ticket)
> +                          struct ww_acquire_ctx *ticket,
> +                          bool validate)
>  {
>         struct amdgpu_bo_va *bo_va;
>         struct dma_resv *resv;
> @@ -1337,6 +1339,12 @@ int amdgpu_vm_handle_moved(struct
> amdgpu_device *adev,
>                 spin_lock(&vm->status_lock);
>         }
>
> +       /* If we're validating user BOs, splice all evicted user BOs into
> +        * the list of invalid BOs for revalidation
> +        */
> +       if (validate)
> +               list_splice_init(&vm->evicted_user, &vm->invalidated);
> +
>         while (!list_empty(&vm->invalidated)) {
>                 bo_va = list_first_entry(&vm->invalidated, struct amdgpu_bo_va,
>                                          base.vm_status); @@ -1357,17 +1365,120 @@ int
> amdgpu_vm_handle_moved(struct amdgpu_device *adev,
>                         unlock = false;
>                 }
>
> +               /* Automatically validate DMABuf imports in compute VMs, if we
> +                * have a reservation, or remember them for later validation.
> +                */
> +               if (vm->is_compute_context && bo_va->base.bo &&
> +                   bo_va->base.bo->tbo.base.import_attach &&
> +                   (!bo_va->base.bo->tbo.resource ||
> +                    bo_va->base.bo->tbo.resource->mem_type == TTM_PL_SYSTEM))
> {
> +                       struct ttm_operation_ctx ctx = { true, false };
> +                       struct amdgpu_bo *bo = bo_va->base.bo;
> +
> +                       if (!validate) {
> +                               r = amdgpu_vm_bo_update(adev, bo_va, clear);
> +                               if (!r)
> +                                       amdgpu_vm_bo_evicted_user(&bo_va->base);
> +                               goto unlock;
> +                       }
> +
> +                       if (clear) {
> +                               pr_warn_ratelimited("Invalid DMABuf import is busy in pid
> %d\n", vm->task_info.pid);
> +                               r = -EBUSY;
> +                               goto unlock;
> +                       }
> +
> +                       amdgpu_bo_placement_from_domain(bo,
> +                                                       bo->preferred_domains);
> +                       r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
> +                       if (r)
> +                               goto unlock;
> +                       r = amdgpu_bo_sync_wait(bo, AMDGPU_FENCE_OWNER_KFD,
> +                                               true);
> +                       if (r)
> +                               goto unlock;
> +               }
> +
>                 r = amdgpu_vm_bo_update(adev, bo_va, clear);
> +unlock:
> +               if (unlock)
> +                       dma_resv_unlock(resv);
>                 if (r)
>                         return r;
> +               spin_lock(&vm->status_lock);
> +       }
> +       spin_unlock(&vm->status_lock);
> +
> +       return 0;
> +}
> +
> +/**
> + * amdgpu_vm_fence_imports - add fence to valid DMABuf imports
> + *
> + * @vm: requested vm
> + * @ticket: optional reservation ticket used to reserve the VM
> + * @fence: fence to add
> + *
> + * Add the specified fence to all dymanic DMABuf imports that are valid.
> + *
> + * Returns:
> + * 0 for success.
> + */
> +int amdgpu_vm_fence_imports(struct amdgpu_vm *vm,
> +                           struct ww_acquire_ctx *ticket,
> +                           struct dma_fence *fence) {
> +       struct amdgpu_bo_va *bo_va, *tmp;
> +       struct dma_resv *resv;
> +       LIST_HEAD(imports);
> +       bool unlock;
> +       int ret = 0;
> +
> +       if (!vm->is_compute_context)
> +               return 0;
> +
> +       /* Move all the DMABuf imports to a private list so I can reserve
> +        * them while not holding te status_lock.
> +        */
> +       spin_lock(&vm->status_lock);
> +       list_for_each_entry_safe(bo_va, tmp, &vm->idle, base.vm_status) {
> +               if (bo_va->base.bo && bo_va->base.bo->tbo.base.import_attach &&
> +                   dma_buf_is_dynamic(bo_va->base.bo->tbo.base.import_attach-
> >dmabuf))
> +                       list_move(&bo_va->base.vm_status, &imports);
> +       }
> +       spin_unlock(&vm->status_lock);
> +
> +       list_for_each_entry(bo_va, &imports, base.vm_status) {
> +               resv = bo_va->base.bo->tbo.base.resv;
> +
> +               /* Try to reserve the BO */
> +               if (dma_resv_trylock(resv)) {
> +                       unlock = true;
> +               /* The caller is already holding the reservation lock */
> +               } else if (ticket && dma_resv_locking_ctx(resv) == ticket) {
> +                       unlock = false;
> +               } else {
> +                       WARN_ONCE(1, "Failed to reserve DMABuf import");
> +                       ret = -EBUSY;
> +                       break;
> +               }
> +
> +               ret = dma_resv_reserve_fences(resv, 1);
> +               if (!ret)
> +                       dma_resv_add_fence(resv, fence,
> +                                          DMA_RESV_USAGE_BOOKKEEP);
>
>                 if (unlock)
>                         dma_resv_unlock(resv);
> -               spin_lock(&vm->status_lock);
> +               if (ret)
> +                       break;
>         }
> +
> +       spin_lock(&vm->status_lock);
> +       list_splice(&imports, &vm->idle);
>         spin_unlock(&vm->status_lock);
>
> -       return 0;
> +       return ret;
>  }
>
>  /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 939d0c2219c0..2db04b8fef97 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -443,7 +443,11 @@ int amdgpu_vm_clear_freed(struct amdgpu_device
> *adev,
>                           struct dma_fence **fence);  int
> amdgpu_vm_handle_moved(struct amdgpu_device *adev,
>                            struct amdgpu_vm *vm,
> -                          struct ww_acquire_ctx *ticket);
> +                          struct ww_acquire_ctx *ticket,
> +                          bool validate); int
> +amdgpu_vm_fence_imports(struct amdgpu_vm *vm,
> +                           struct ww_acquire_ctx *ticket,
> +                           struct dma_fence *fence);
>  int amdgpu_vm_flush_compute_tlb(struct amdgpu_device *adev,
>                                 struct amdgpu_vm *vm,
>                                 uint32_t flush_type,
> --
> 2.34.1

[PATCH 4/6] drm/amdkfd: Export DMABufs from KFD using GEM handles

[Felix Kuehling]

Create GEM handles for exporting DMABufs using GEM-Prime APIs. The GEM
handles are created in a drm_client_dev context to avoid exposing them
in user mode contexts through a DMABuf import.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c    | 11 +++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  5 +++
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 33 +++++++++++++++----
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      |  4 +--
 4 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
index 6ab17330a6ed..b0a67f16540a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
@@ -142,6 +142,7 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
 {
 	int i;
 	int last_valid_bit;
+	int ret;
 
 	amdgpu_amdkfd_gpuvm_init_mem_limits();
 
@@ -160,6 +161,12 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
 			.enable_mes = adev->enable_mes,
 		};
 
+		ret = drm_client_init(&adev->ddev, &adev->kfd.client, "kfd", NULL);
+		if (ret) {
+			dev_err(adev->dev, "Failed to init DRM client: %d\n", ret);
+			return;
+		}
+
 		/* this is going to have a few of the MSBs set that we need to
 		 * clear
 		 */
@@ -198,6 +205,10 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
 
 		adev->kfd.init_complete = kgd2kfd_device_init(adev->kfd.dev,
 							&gpu_resources);
+		if (adev->kfd.init_complete)
+			drm_client_register(&adev->kfd.client);
+		else
+			drm_client_release(&adev->kfd.client);
 
 		amdgpu_amdkfd_total_mem_size += adev->gmc.real_vram_size;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index 68d534a89942..4caf8cece028 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -32,6 +32,7 @@
 #include <linux/mmu_notifier.h>
 #include <linux/memremap.h>
 #include <kgd_kfd_interface.h>
+#include <drm/drm_client.h>
 #include <drm/ttm/ttm_execbuf_util.h>
 #include "amdgpu_sync.h"
 #include "amdgpu_vm.h"
@@ -84,6 +85,7 @@ struct kgd_mem {
 
 	struct amdgpu_sync sync;
 
+	uint32_t gem_handle;
 	bool aql_queue;
 	bool is_imported;
 };
@@ -106,6 +108,9 @@ struct amdgpu_kfd_dev {
 
 	/* HMM page migration MEMORY_DEVICE_PRIVATE mapping */
 	struct dev_pagemap pgmap;
+
+	/* Client for KFD BO GEM handle allocations */
+	struct drm_client_dev client;
 };
 
 enum kgd_engine_type {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 0c1cb6048259..4bb8b5fd7598 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -25,6 +25,7 @@
 #include <linux/pagemap.h>
 #include <linux/sched/mm.h>
 #include <linux/sched/task.h>
+#include <linux/fdtable.h>
 #include <drm/ttm/ttm_tt.h>
 
 #include "amdgpu_object.h"
@@ -804,13 +805,22 @@ kfd_mem_dmaunmap_attachment(struct kgd_mem *mem,
 static int kfd_mem_export_dmabuf(struct kgd_mem *mem)
 {
 	if (!mem->dmabuf) {
-		struct dma_buf *ret = amdgpu_gem_prime_export(
-			&mem->bo->tbo.base,
+		struct amdgpu_device *bo_adev;
+		struct dma_buf *dmabuf;
+		int r, fd;
+
+		bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev);
+		r = drm_gem_prime_handle_to_fd(&bo_adev->ddev, bo_adev->kfd.client.file,
+					       mem->gem_handle,
 			mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
-				DRM_RDWR : 0);
-		if (IS_ERR(ret))
-			return PTR_ERR(ret);
-		mem->dmabuf = ret;
+					       DRM_RDWR : 0, &fd);
+		if (r)
+			return r;
+		dmabuf = dma_buf_get(fd);
+		close_fd(fd);
+		if (WARN_ON_ONCE(IS_ERR(dmabuf)))
+			return PTR_ERR(dmabuf);
+		mem->dmabuf = dmabuf;
 	}
 
 	return 0;
@@ -1826,6 +1836,9 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
 		pr_debug("Failed to allow vma node access. ret %d\n", ret);
 		goto err_node_allow;
 	}
+	ret = drm_gem_handle_create(adev->kfd.client.file, gobj, &(*mem)->gem_handle);
+	if (ret)
+		goto err_gem_handle_create;
 	bo = gem_to_amdgpu_bo(gobj);
 	if (bo_type == ttm_bo_type_sg) {
 		bo->tbo.sg = sg;
@@ -1877,6 +1890,8 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
 err_pin_bo:
 err_validate_bo:
 	remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
+	drm_gem_handle_delete(adev->kfd.client.file, (*mem)->gem_handle);
+err_gem_handle_create:
 	drm_vma_node_revoke(&gobj->vma_node, drm_priv);
 err_node_allow:
 	/* Don't unreserve system mem limit twice */
@@ -1991,8 +2006,12 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
 
 	/* Free the BO*/
 	drm_vma_node_revoke(&mem->bo->tbo.base.vma_node, drm_priv);
-	if (mem->dmabuf)
+	if (!mem->is_imported)
+		drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
+	if (mem->dmabuf) {
 		dma_buf_put(mem->dmabuf);
+		mem->dmabuf = NULL;
+	}
 	mutex_destroy(&mem->lock);
 
 	/* If this releases the last reference, it will end up calling
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 06988cf1db51..4417a9863cd0 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -1855,8 +1855,8 @@ static uint32_t get_process_num_bos(struct kfd_process *p)
 	return num_of_bos;
 }
 
-static int criu_get_prime_handle(struct kgd_mem *mem, int flags,
-				      u32 *shared_fd)
+static int criu_get_prime_handle(struct kgd_mem *mem,
+				 int flags, u32 *shared_fd)
 {
 	struct dma_buf *dmabuf;
 	int ret;
-- 
2.34.1

RE: [PATCH 4/6] drm/amdkfd: Export DMABufs from KFD using GEM handles

[Errabolu, Ramesh]

[AMD Official Use Only - General]

My queries inline.

Regards,
Ramesh

-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling@amd.com>
Sent: Tuesday, November 7, 2023 10:28 PM
To: amd-gfx@lists.freedesktop.org
Cc: Koenig, Christian <Christian.Koenig@amd.com>; Chen, Xiaogang <Xiaogang.Chen@amd.com>; Errabolu, Ramesh <Ramesh.Errabolu@amd.com>
Subject: [PATCH 4/6] drm/amdkfd: Export DMABufs from KFD using GEM handles

Create GEM handles for exporting DMABufs using GEM-Prime APIs. The GEM handles are created in a drm_client_dev context to avoid exposing them in user mode contexts through a DMABuf import.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c    | 11 +++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  5 +++
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 33 +++++++++++++++----
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      |  4 +--
 4 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
index 6ab17330a6ed..b0a67f16540a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
@@ -142,6 +142,7 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)  {
        int i;
        int last_valid_bit;
+       int ret;

        amdgpu_amdkfd_gpuvm_init_mem_limits();

@@ -160,6 +161,12 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
                        .enable_mes = adev->enable_mes,
                };

+               ret = drm_client_init(&adev->ddev, &adev->kfd.client, "kfd", NULL);
+               if (ret) {
+                       dev_err(adev->dev, "Failed to init DRM client: %d\n", ret);
+                       return;
+               }
+
                /* this is going to have a few of the MSBs set that we need to
                 * clear
                 */
@@ -198,6 +205,10 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)

                adev->kfd.init_complete = kgd2kfd_device_init(adev->kfd.dev,
                                                        &gpu_resources);
+               if (adev->kfd.init_complete)
+                       drm_client_register(&adev->kfd.client);
+               else
+                       drm_client_release(&adev->kfd.client);

                amdgpu_amdkfd_total_mem_size += adev->gmc.real_vram_size;

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index 68d534a89942..4caf8cece028 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -32,6 +32,7 @@
 #include <linux/mmu_notifier.h>
 #include <linux/memremap.h>
 #include <kgd_kfd_interface.h>
+#include <drm/drm_client.h>
 #include <drm/ttm/ttm_execbuf_util.h>
 #include "amdgpu_sync.h"
 #include "amdgpu_vm.h"
@@ -84,6 +85,7 @@ struct kgd_mem {

        struct amdgpu_sync sync;

+       uint32_t gem_handle;
        bool aql_queue;
        bool is_imported;
 };
@@ -106,6 +108,9 @@ struct amdgpu_kfd_dev {

        /* HMM page migration MEMORY_DEVICE_PRIVATE mapping */
        struct dev_pagemap pgmap;
+
+       /* Client for KFD BO GEM handle allocations */
+       struct drm_client_dev client;
 };

 enum kgd_engine_type {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 0c1cb6048259..4bb8b5fd7598 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -25,6 +25,7 @@
 #include <linux/pagemap.h>
 #include <linux/sched/mm.h>
 #include <linux/sched/task.h>
+#include <linux/fdtable.h>
 #include <drm/ttm/ttm_tt.h>

 #include "amdgpu_object.h"
@@ -804,13 +805,22 @@ kfd_mem_dmaunmap_attachment(struct kgd_mem *mem,  static int kfd_mem_export_dmabuf(struct kgd_mem *mem)  {
        if (!mem->dmabuf) {
-               struct dma_buf *ret = amdgpu_gem_prime_export(
-                       &mem->bo->tbo.base,
+               struct amdgpu_device *bo_adev;
+               struct dma_buf *dmabuf;
+               int r, fd;
+
+               bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev);
+               r = drm_gem_prime_handle_to_fd(&bo_adev->ddev, bo_adev->kfd.client.file,
+                                              mem->gem_handle,
                        mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
-                               DRM_RDWR : 0);
-               if (IS_ERR(ret))
-                       return PTR_ERR(ret);
-               mem->dmabuf = ret;
+                                              DRM_RDWR : 0, &fd);
+               if (r)
+                       return r;
+               dmabuf = dma_buf_get(fd);
+               close_fd(fd);
+               if (WARN_ON_ONCE(IS_ERR(dmabuf)))
+                       return PTR_ERR(dmabuf);
+               mem->dmabuf = dmabuf;
        }

        return 0;
@@ -1826,6 +1836,9 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
                pr_debug("Failed to allow vma node access. ret %d\n", ret);
                goto err_node_allow;
        }
+       ret = drm_gem_handle_create(adev->kfd.client.file, gobj, &(*mem)->gem_handle);
+       if (ret)
+               goto err_gem_handle_create;
        bo = gem_to_amdgpu_bo(gobj);
        if (bo_type == ttm_bo_type_sg) {
                bo->tbo.sg = sg;
@@ -1877,6 +1890,8 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
 err_pin_bo:
 err_validate_bo:
        remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
+       drm_gem_handle_delete(adev->kfd.client.file, (*mem)->gem_handle);
+err_gem_handle_create:
        drm_vma_node_revoke(&gobj->vma_node, drm_priv);
 err_node_allow:
        /* Don't unreserve system mem limit twice */ @@ -1991,8 +2006,12 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(

        /* Free the BO*/
        drm_vma_node_revoke(&mem->bo->tbo.base.vma_node, drm_priv);
-       if (mem->dmabuf)
+       if (!mem->is_imported)
+               drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
+       if (mem->dmabuf) {
                dma_buf_put(mem->dmabuf);
+               mem->dmabuf = NULL;
+       }
        mutex_destroy(&mem->lock);

Ramesh: What happens if user invokes "free" using KFD IOCTL while the BO is imported and mapped on the device using DRM IOCTLs. Could it lead to inconsistent state?. For example the call drm_gem_handle_delete() will remove Dmabuf, close the GEM object. If user invokes free() using KFD Apis then the underlying object could be removed. It is not clear if this impacts devices that have just imported.

        /* If this releases the last reference, it will end up calling diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 06988cf1db51..4417a9863cd0 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -1855,8 +1855,8 @@ static uint32_t get_process_num_bos(struct kfd_process *p)
        return num_of_bos;
 }

-static int criu_get_prime_handle(struct kgd_mem *mem, int flags,
-                                     u32 *shared_fd)
+static int criu_get_prime_handle(struct kgd_mem *mem,
+                                int flags, u32 *shared_fd)
 {
        struct dma_buf *dmabuf;
        int ret;
--
2.34.1

Re: [PATCH 4/6] drm/amdkfd: Export DMABufs from KFD using GEM handles

[Felix Kuehling]

On 2023-11-07 14:44, Errabolu, Ramesh wrote:
> [AMD Official Use Only - General]
>
> My queries inline.
>
> Regards,
> Ramesh
>
> -----Original Message-----
> From: Kuehling, Felix <Felix.Kuehling@amd.com>
> Sent: Tuesday, November 7, 2023 10:28 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Koenig, Christian <Christian.Koenig@amd.com>; Chen, Xiaogang <Xiaogang.Chen@amd.com>; Errabolu, Ramesh <Ramesh.Errabolu@amd.com>
> Subject: [PATCH 4/6] drm/amdkfd: Export DMABufs from KFD using GEM handles
>
> Create GEM handles for exporting DMABufs using GEM-Prime APIs. The GEM handles are created in a drm_client_dev context to avoid exposing them in user mode contexts through a DMABuf import.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c    | 11 +++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  5 +++
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 33 +++++++++++++++----
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      |  4 +--
>   4 files changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index 6ab17330a6ed..b0a67f16540a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -142,6 +142,7 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)  {
>          int i;
>          int last_valid_bit;
> +       int ret;
>
>          amdgpu_amdkfd_gpuvm_init_mem_limits();
>
> @@ -160,6 +161,12 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
>                          .enable_mes = adev->enable_mes,
>                  };
>
> +               ret = drm_client_init(&adev->ddev, &adev->kfd.client, "kfd", NULL);
> +               if (ret) {
> +                       dev_err(adev->dev, "Failed to init DRM client: %d\n", ret);
> +                       return;
> +               }
> +
>                  /* this is going to have a few of the MSBs set that we need to
>                   * clear
>                   */
> @@ -198,6 +205,10 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
>
>                  adev->kfd.init_complete = kgd2kfd_device_init(adev->kfd.dev,
>                                                          &gpu_resources);
> +               if (adev->kfd.init_complete)
> +                       drm_client_register(&adev->kfd.client);
> +               else
> +                       drm_client_release(&adev->kfd.client);
>
>                  amdgpu_amdkfd_total_mem_size += adev->gmc.real_vram_size;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index 68d534a89942..4caf8cece028 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -32,6 +32,7 @@
>   #include <linux/mmu_notifier.h>
>   #include <linux/memremap.h>
>   #include <kgd_kfd_interface.h>
> +#include <drm/drm_client.h>
>   #include <drm/ttm/ttm_execbuf_util.h>
>   #include "amdgpu_sync.h"
>   #include "amdgpu_vm.h"
> @@ -84,6 +85,7 @@ struct kgd_mem {
>
>          struct amdgpu_sync sync;
>
> +       uint32_t gem_handle;
>          bool aql_queue;
>          bool is_imported;
>   };
> @@ -106,6 +108,9 @@ struct amdgpu_kfd_dev {
>
>          /* HMM page migration MEMORY_DEVICE_PRIVATE mapping */
>          struct dev_pagemap pgmap;
> +
> +       /* Client for KFD BO GEM handle allocations */
> +       struct drm_client_dev client;
>   };
>
>   enum kgd_engine_type {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 0c1cb6048259..4bb8b5fd7598 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -25,6 +25,7 @@
>   #include <linux/pagemap.h>
>   #include <linux/sched/mm.h>
>   #include <linux/sched/task.h>
> +#include <linux/fdtable.h>
>   #include <drm/ttm/ttm_tt.h>
>
>   #include "amdgpu_object.h"
> @@ -804,13 +805,22 @@ kfd_mem_dmaunmap_attachment(struct kgd_mem *mem,  static int kfd_mem_export_dmabuf(struct kgd_mem *mem)  {
>          if (!mem->dmabuf) {
> -               struct dma_buf *ret = amdgpu_gem_prime_export(
> -                       &mem->bo->tbo.base,
> +               struct amdgpu_device *bo_adev;
> +               struct dma_buf *dmabuf;
> +               int r, fd;
> +
> +               bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev);
> +               r = drm_gem_prime_handle_to_fd(&bo_adev->ddev, bo_adev->kfd.client.file,
> +                                              mem->gem_handle,
>                          mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
> -                               DRM_RDWR : 0);
> -               if (IS_ERR(ret))
> -                       return PTR_ERR(ret);
> -               mem->dmabuf = ret;
> +                                              DRM_RDWR : 0, &fd);
> +               if (r)
> +                       return r;
> +               dmabuf = dma_buf_get(fd);
> +               close_fd(fd);
> +               if (WARN_ON_ONCE(IS_ERR(dmabuf)))
> +                       return PTR_ERR(dmabuf);
> +               mem->dmabuf = dmabuf;
>          }
>
>          return 0;
> @@ -1826,6 +1836,9 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>                  pr_debug("Failed to allow vma node access. ret %d\n", ret);
>                  goto err_node_allow;
>          }
> +       ret = drm_gem_handle_create(adev->kfd.client.file, gobj, &(*mem)->gem_handle);
> +       if (ret)
> +               goto err_gem_handle_create;
>          bo = gem_to_amdgpu_bo(gobj);
>          if (bo_type == ttm_bo_type_sg) {
>                  bo->tbo.sg = sg;
> @@ -1877,6 +1890,8 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>   err_pin_bo:
>   err_validate_bo:
>          remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
> +       drm_gem_handle_delete(adev->kfd.client.file, (*mem)->gem_handle);
> +err_gem_handle_create:
>          drm_vma_node_revoke(&gobj->vma_node, drm_priv);
>   err_node_allow:
>          /* Don't unreserve system mem limit twice */ @@ -1991,8 +2006,12 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
>
>          /* Free the BO*/
>          drm_vma_node_revoke(&mem->bo->tbo.base.vma_node, drm_priv);
> -       if (mem->dmabuf)
> +       if (!mem->is_imported)
> +               drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
> +       if (mem->dmabuf) {
>                  dma_buf_put(mem->dmabuf);
> +               mem->dmabuf = NULL;
> +       }
>          mutex_destroy(&mem->lock);
>
> Ramesh: What happens if user invokes "free" using KFD IOCTL while the BO is imported and mapped on the device using DRM IOCTLs. Could it lead to inconsistent state?. For example the call drm_gem_handle_delete() will remove Dmabuf, close the GEM object. If user invokes free() using KFD Apis then the underlying object could be removed. It is not clear if this impacts devices that have just imported.

Reference counting should prevent any such problems. Both the dmabuf 
itself or the import in a DRM device hold a reference to the underlying 
GEM object. Using the KFD free ioctl only drops the KFD reference to the 
GEM object and cleans up the handle and kgd_mem object in KFD.

When the BO gets imported in a DRM render node, that creates a new GEM 
handle in that context.

Regards,
   Felix


>
>          /* If this releases the last reference, it will end up calling diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 06988cf1db51..4417a9863cd0 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1855,8 +1855,8 @@ static uint32_t get_process_num_bos(struct kfd_process *p)
>          return num_of_bos;
>   }
>
> -static int criu_get_prime_handle(struct kgd_mem *mem, int flags,
> -                                     u32 *shared_fd)
> +static int criu_get_prime_handle(struct kgd_mem *mem,
> +                                int flags, u32 *shared_fd)
>   {
>          struct dma_buf *dmabuf;
>          int ret;
> --
> 2.34.1
>

RE: [PATCH 4/6] drm/amdkfd: Export DMABufs from KFD using GEM handles

[Errabolu, Ramesh]

[AMD Official Use Only - General]

Reviewed-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>

-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling@amd.com>
Sent: Wednesday, November 8, 2023 1:27 AM
To: Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Koenig, Christian <Christian.Koenig@amd.com>; Chen, Xiaogang <Xiaogang.Chen@amd.com>
Subject: Re: [PATCH 4/6] drm/amdkfd: Export DMABufs from KFD using GEM handles


On 2023-11-07 14:44, Errabolu, Ramesh wrote:
> [AMD Official Use Only - General]
>
> My queries inline.
>
> Regards,
> Ramesh
>
> -----Original Message-----
> From: Kuehling, Felix <Felix.Kuehling@amd.com>
> Sent: Tuesday, November 7, 2023 10:28 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Koenig, Christian <Christian.Koenig@amd.com>; Chen, Xiaogang
> <Xiaogang.Chen@amd.com>; Errabolu, Ramesh <Ramesh.Errabolu@amd.com>
> Subject: [PATCH 4/6] drm/amdkfd: Export DMABufs from KFD using GEM
> handles
>
> Create GEM handles for exporting DMABufs using GEM-Prime APIs. The GEM handles are created in a drm_client_dev context to avoid exposing them in user mode contexts through a DMABuf import.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c    | 11 +++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  5 +++
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 33 +++++++++++++++----
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      |  4 +--
>   4 files changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index 6ab17330a6ed..b0a67f16540a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -142,6 +142,7 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)  {
>          int i;
>          int last_valid_bit;
> +       int ret;
>
>          amdgpu_amdkfd_gpuvm_init_mem_limits();
>
> @@ -160,6 +161,12 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
>                          .enable_mes = adev->enable_mes,
>                  };
>
> +               ret = drm_client_init(&adev->ddev, &adev->kfd.client, "kfd", NULL);
> +               if (ret) {
> +                       dev_err(adev->dev, "Failed to init DRM client: %d\n", ret);
> +                       return;
> +               }
> +
>                  /* this is going to have a few of the MSBs set that we need to
>                   * clear
>                   */
> @@ -198,6 +205,10 @@ void amdgpu_amdkfd_device_init(struct
> amdgpu_device *adev)
>
>                  adev->kfd.init_complete = kgd2kfd_device_init(adev->kfd.dev,
>
> &gpu_resources);
> +               if (adev->kfd.init_complete)
> +                       drm_client_register(&adev->kfd.client);
> +               else
> +                       drm_client_release(&adev->kfd.client);
>
>                  amdgpu_amdkfd_total_mem_size +=
> adev->gmc.real_vram_size;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index 68d534a89942..4caf8cece028 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -32,6 +32,7 @@
>   #include <linux/mmu_notifier.h>
>   #include <linux/memremap.h>
>   #include <kgd_kfd_interface.h>
> +#include <drm/drm_client.h>
>   #include <drm/ttm/ttm_execbuf_util.h>
>   #include "amdgpu_sync.h"
>   #include "amdgpu_vm.h"
> @@ -84,6 +85,7 @@ struct kgd_mem {
>
>          struct amdgpu_sync sync;
>
> +       uint32_t gem_handle;
>          bool aql_queue;
>          bool is_imported;
>   };
> @@ -106,6 +108,9 @@ struct amdgpu_kfd_dev {
>
>          /* HMM page migration MEMORY_DEVICE_PRIVATE mapping */
>          struct dev_pagemap pgmap;
> +
> +       /* Client for KFD BO GEM handle allocations */
> +       struct drm_client_dev client;
>   };
>
>   enum kgd_engine_type {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 0c1cb6048259..4bb8b5fd7598 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -25,6 +25,7 @@
>   #include <linux/pagemap.h>
>   #include <linux/sched/mm.h>
>   #include <linux/sched/task.h>
> +#include <linux/fdtable.h>
>   #include <drm/ttm/ttm_tt.h>
>
>   #include "amdgpu_object.h"
> @@ -804,13 +805,22 @@ kfd_mem_dmaunmap_attachment(struct kgd_mem *mem,  static int kfd_mem_export_dmabuf(struct kgd_mem *mem)  {
>          if (!mem->dmabuf) {
> -               struct dma_buf *ret = amdgpu_gem_prime_export(
> -                       &mem->bo->tbo.base,
> +               struct amdgpu_device *bo_adev;
> +               struct dma_buf *dmabuf;
> +               int r, fd;
> +
> +               bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev);
> +               r = drm_gem_prime_handle_to_fd(&bo_adev->ddev, bo_adev->kfd.client.file,
> +                                              mem->gem_handle,
>                          mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
> -                               DRM_RDWR : 0);
> -               if (IS_ERR(ret))
> -                       return PTR_ERR(ret);
> -               mem->dmabuf = ret;
> +                                              DRM_RDWR : 0, &fd);
> +               if (r)
> +                       return r;
> +               dmabuf = dma_buf_get(fd);
> +               close_fd(fd);
> +               if (WARN_ON_ONCE(IS_ERR(dmabuf)))
> +                       return PTR_ERR(dmabuf);
> +               mem->dmabuf = dmabuf;
>          }
>
>          return 0;
> @@ -1826,6 +1836,9 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>                  pr_debug("Failed to allow vma node access. ret %d\n", ret);
>                  goto err_node_allow;
>          }
> +       ret = drm_gem_handle_create(adev->kfd.client.file, gobj, &(*mem)->gem_handle);
> +       if (ret)
> +               goto err_gem_handle_create;
>          bo = gem_to_amdgpu_bo(gobj);
>          if (bo_type == ttm_bo_type_sg) {
>                  bo->tbo.sg = sg;
> @@ -1877,6 +1890,8 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>   err_pin_bo:
>   err_validate_bo:
>          remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
> +       drm_gem_handle_delete(adev->kfd.client.file,
> +(*mem)->gem_handle);
> +err_gem_handle_create:
>          drm_vma_node_revoke(&gobj->vma_node, drm_priv);
>   err_node_allow:
>          /* Don't unreserve system mem limit twice */ @@ -1991,8
> +2006,12 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
>
>          /* Free the BO*/
>          drm_vma_node_revoke(&mem->bo->tbo.base.vma_node, drm_priv);
> -       if (mem->dmabuf)
> +       if (!mem->is_imported)
> +               drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
> +       if (mem->dmabuf) {
>                  dma_buf_put(mem->dmabuf);
> +               mem->dmabuf = NULL;
> +       }
>          mutex_destroy(&mem->lock);
>
> Ramesh: What happens if user invokes "free" using KFD IOCTL while the BO is imported and mapped on the device using DRM IOCTLs. Could it lead to inconsistent state?. For example the call drm_gem_handle_delete() will remove Dmabuf, close the GEM object. If user invokes free() using KFD Apis then the underlying object could be removed. It is not clear if this impacts devices that have just imported.

Reference counting should prevent any such problems. Both the dmabuf itself or the import in a DRM device hold a reference to the underlying GEM object. Using the KFD free ioctl only drops the KFD reference to the GEM object and cleans up the handle and kgd_mem object in KFD.

When the BO gets imported in a DRM render node, that creates a new GEM handle in that context.

Regards,
   Felix


>
>          /* If this releases the last reference, it will end up
> calling diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 06988cf1db51..4417a9863cd0 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1855,8 +1855,8 @@ static uint32_t get_process_num_bos(struct kfd_process *p)
>          return num_of_bos;
>   }
>
> -static int criu_get_prime_handle(struct kgd_mem *mem, int flags,
> -                                     u32 *shared_fd)
> +static int criu_get_prime_handle(struct kgd_mem *mem,
> +                                int flags, u32 *shared_fd)
>   {
>          struct dma_buf *dmabuf;
>          int ret;
> --
> 2.34.1
>

Re: [PATCH 4/6] drm/amdkfd: Export DMABufs from KFD using GEM handles

[Felix Kuehling]

On 2023-11-07 11:58, Felix Kuehling wrote:
> Create GEM handles for exporting DMABufs using GEM-Prime APIs. The GEM
> handles are created in a drm_client_dev context to avoid exposing them
> in user mode contexts through a DMABuf import.
This patch (and the next one) won't apply upstream because Thomas 
Zimmerman just made drm_gem_prime_fd_to_handle and 
drm_gem_prime_handle_to_fd private because nobody was using them. 
(drm/prime: Unexport helpers for fd/handle conversion)

Is it OK to export those APIs again? Or is there a better way for 
drivers to export/import DMABufs without using the GEM ioctls?

Regards,
   Felix


>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c    | 11 +++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  5 +++
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 33 +++++++++++++++----
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      |  4 +--
>   4 files changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index 6ab17330a6ed..b0a67f16540a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -142,6 +142,7 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
>   {
>   	int i;
>   	int last_valid_bit;
> +	int ret;
>   
>   	amdgpu_amdkfd_gpuvm_init_mem_limits();
>   
> @@ -160,6 +161,12 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
>   			.enable_mes = adev->enable_mes,
>   		};
>   
> +		ret = drm_client_init(&adev->ddev, &adev->kfd.client, "kfd", NULL);
> +		if (ret) {
> +			dev_err(adev->dev, "Failed to init DRM client: %d\n", ret);
> +			return;
> +		}
> +
>   		/* this is going to have a few of the MSBs set that we need to
>   		 * clear
>   		 */
> @@ -198,6 +205,10 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
>   
>   		adev->kfd.init_complete = kgd2kfd_device_init(adev->kfd.dev,
>   							&gpu_resources);
> +		if (adev->kfd.init_complete)
> +			drm_client_register(&adev->kfd.client);
> +		else
> +			drm_client_release(&adev->kfd.client);
>   
>   		amdgpu_amdkfd_total_mem_size += adev->gmc.real_vram_size;
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index 68d534a89942..4caf8cece028 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -32,6 +32,7 @@
>   #include <linux/mmu_notifier.h>
>   #include <linux/memremap.h>
>   #include <kgd_kfd_interface.h>
> +#include <drm/drm_client.h>
>   #include <drm/ttm/ttm_execbuf_util.h>
>   #include "amdgpu_sync.h"
>   #include "amdgpu_vm.h"
> @@ -84,6 +85,7 @@ struct kgd_mem {
>   
>   	struct amdgpu_sync sync;
>   
> +	uint32_t gem_handle;
>   	bool aql_queue;
>   	bool is_imported;
>   };
> @@ -106,6 +108,9 @@ struct amdgpu_kfd_dev {
>   
>   	/* HMM page migration MEMORY_DEVICE_PRIVATE mapping */
>   	struct dev_pagemap pgmap;
> +
> +	/* Client for KFD BO GEM handle allocations */
> +	struct drm_client_dev client;
>   };
>   
>   enum kgd_engine_type {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 0c1cb6048259..4bb8b5fd7598 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -25,6 +25,7 @@
>   #include <linux/pagemap.h>
>   #include <linux/sched/mm.h>
>   #include <linux/sched/task.h>
> +#include <linux/fdtable.h>
>   #include <drm/ttm/ttm_tt.h>
>   
>   #include "amdgpu_object.h"
> @@ -804,13 +805,22 @@ kfd_mem_dmaunmap_attachment(struct kgd_mem *mem,
>   static int kfd_mem_export_dmabuf(struct kgd_mem *mem)
>   {
>   	if (!mem->dmabuf) {
> -		struct dma_buf *ret = amdgpu_gem_prime_export(
> -			&mem->bo->tbo.base,
> +		struct amdgpu_device *bo_adev;
> +		struct dma_buf *dmabuf;
> +		int r, fd;
> +
> +		bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev);
> +		r = drm_gem_prime_handle_to_fd(&bo_adev->ddev, bo_adev->kfd.client.file,
> +					       mem->gem_handle,
>   			mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
> -				DRM_RDWR : 0);
> -		if (IS_ERR(ret))
> -			return PTR_ERR(ret);
> -		mem->dmabuf = ret;
> +					       DRM_RDWR : 0, &fd);
> +		if (r)
> +			return r;
> +		dmabuf = dma_buf_get(fd);
> +		close_fd(fd);
> +		if (WARN_ON_ONCE(IS_ERR(dmabuf)))
> +			return PTR_ERR(dmabuf);
> +		mem->dmabuf = dmabuf;
>   	}
>   
>   	return 0;
> @@ -1826,6 +1836,9 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>   		pr_debug("Failed to allow vma node access. ret %d\n", ret);
>   		goto err_node_allow;
>   	}
> +	ret = drm_gem_handle_create(adev->kfd.client.file, gobj, &(*mem)->gem_handle);
> +	if (ret)
> +		goto err_gem_handle_create;
>   	bo = gem_to_amdgpu_bo(gobj);
>   	if (bo_type == ttm_bo_type_sg) {
>   		bo->tbo.sg = sg;
> @@ -1877,6 +1890,8 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>   err_pin_bo:
>   err_validate_bo:
>   	remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
> +	drm_gem_handle_delete(adev->kfd.client.file, (*mem)->gem_handle);
> +err_gem_handle_create:
>   	drm_vma_node_revoke(&gobj->vma_node, drm_priv);
>   err_node_allow:
>   	/* Don't unreserve system mem limit twice */
> @@ -1991,8 +2006,12 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
>   
>   	/* Free the BO*/
>   	drm_vma_node_revoke(&mem->bo->tbo.base.vma_node, drm_priv);
> -	if (mem->dmabuf)
> +	if (!mem->is_imported)
> +		drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
> +	if (mem->dmabuf) {
>   		dma_buf_put(mem->dmabuf);
> +		mem->dmabuf = NULL;
> +	}
>   	mutex_destroy(&mem->lock);
>   
>   	/* If this releases the last reference, it will end up calling
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 06988cf1db51..4417a9863cd0 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1855,8 +1855,8 @@ static uint32_t get_process_num_bos(struct kfd_process *p)
>   	return num_of_bos;
>   }
>   
> -static int criu_get_prime_handle(struct kgd_mem *mem, int flags,
> -				      u32 *shared_fd)
> +static int criu_get_prime_handle(struct kgd_mem *mem,
> +				 int flags, u32 *shared_fd)
>   {
>   	struct dma_buf *dmabuf;
>   	int ret;

Re: [PATCH 4/6] drm/amdkfd: Export DMABufs from KFD using GEM handles

[Christian König]

Am 16.11.23 um 22:53 schrieb Felix Kuehling:
>
> On 2023-11-07 11:58, Felix Kuehling wrote:
>> Create GEM handles for exporting DMABufs using GEM-Prime APIs. The GEM
>> handles are created in a drm_client_dev context to avoid exposing them
>> in user mode contexts through a DMABuf import.
> This patch (and the next one) won't apply upstream because Thomas 
> Zimmerman just made drm_gem_prime_fd_to_handle and 
> drm_gem_prime_handle_to_fd private because nobody was using them. 
> (drm/prime: Unexport helpers for fd/handle conversion)
>
> Is it OK to export those APIs again? Or is there a better way for 
> drivers to export/import DMABufs without using the GEM ioctls?

Well let me say so: I think it's the least evil approach :)

So yeah, propose a patch to export them again.

Regards,
Christian.

>
> Regards,
>   Felix
>
>
>>
>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c    | 11 +++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  5 +++
>>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 33 +++++++++++++++----
>>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      |  4 +--
>>   4 files changed, 44 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
>> index 6ab17330a6ed..b0a67f16540a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
>> @@ -142,6 +142,7 @@ void amdgpu_amdkfd_device_init(struct 
>> amdgpu_device *adev)
>>   {
>>       int i;
>>       int last_valid_bit;
>> +    int ret;
>>         amdgpu_amdkfd_gpuvm_init_mem_limits();
>>   @@ -160,6 +161,12 @@ void amdgpu_amdkfd_device_init(struct 
>> amdgpu_device *adev)
>>               .enable_mes = adev->enable_mes,
>>           };
>>   +        ret = drm_client_init(&adev->ddev, &adev->kfd.client, 
>> "kfd", NULL);
>> +        if (ret) {
>> +            dev_err(adev->dev, "Failed to init DRM client: %d\n", ret);
>> +            return;
>> +        }
>> +
>>           /* this is going to have a few of the MSBs set that we need to
>>            * clear
>>            */
>> @@ -198,6 +205,10 @@ void amdgpu_amdkfd_device_init(struct 
>> amdgpu_device *adev)
>>             adev->kfd.init_complete = kgd2kfd_device_init(adev->kfd.dev,
>>                               &gpu_resources);
>> +        if (adev->kfd.init_complete)
>> +            drm_client_register(&adev->kfd.client);
>> +        else
>> +            drm_client_release(&adev->kfd.client);
>>             amdgpu_amdkfd_total_mem_size += adev->gmc.real_vram_size;
>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> index 68d534a89942..4caf8cece028 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> @@ -32,6 +32,7 @@
>>   #include <linux/mmu_notifier.h>
>>   #include <linux/memremap.h>
>>   #include <kgd_kfd_interface.h>
>> +#include <drm/drm_client.h>
>>   #include <drm/ttm/ttm_execbuf_util.h>
>>   #include "amdgpu_sync.h"
>>   #include "amdgpu_vm.h"
>> @@ -84,6 +85,7 @@ struct kgd_mem {
>>         struct amdgpu_sync sync;
>>   +    uint32_t gem_handle;
>>       bool aql_queue;
>>       bool is_imported;
>>   };
>> @@ -106,6 +108,9 @@ struct amdgpu_kfd_dev {
>>         /* HMM page migration MEMORY_DEVICE_PRIVATE mapping */
>>       struct dev_pagemap pgmap;
>> +
>> +    /* Client for KFD BO GEM handle allocations */
>> +    struct drm_client_dev client;
>>   };
>>     enum kgd_engine_type {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index 0c1cb6048259..4bb8b5fd7598 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -25,6 +25,7 @@
>>   #include <linux/pagemap.h>
>>   #include <linux/sched/mm.h>
>>   #include <linux/sched/task.h>
>> +#include <linux/fdtable.h>
>>   #include <drm/ttm/ttm_tt.h>
>>     #include "amdgpu_object.h"
>> @@ -804,13 +805,22 @@ kfd_mem_dmaunmap_attachment(struct kgd_mem *mem,
>>   static int kfd_mem_export_dmabuf(struct kgd_mem *mem)
>>   {
>>       if (!mem->dmabuf) {
>> -        struct dma_buf *ret = amdgpu_gem_prime_export(
>> -            &mem->bo->tbo.base,
>> +        struct amdgpu_device *bo_adev;
>> +        struct dma_buf *dmabuf;
>> +        int r, fd;
>> +
>> +        bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev);
>> +        r = drm_gem_prime_handle_to_fd(&bo_adev->ddev, 
>> bo_adev->kfd.client.file,
>> +                           mem->gem_handle,
>>               mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
>> -                DRM_RDWR : 0);
>> -        if (IS_ERR(ret))
>> -            return PTR_ERR(ret);
>> -        mem->dmabuf = ret;
>> +                           DRM_RDWR : 0, &fd);
>> +        if (r)
>> +            return r;
>> +        dmabuf = dma_buf_get(fd);
>> +        close_fd(fd);
>> +        if (WARN_ON_ONCE(IS_ERR(dmabuf)))
>> +            return PTR_ERR(dmabuf);
>> +        mem->dmabuf = dmabuf;
>>       }
>>         return 0;
>> @@ -1826,6 +1836,9 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>>           pr_debug("Failed to allow vma node access. ret %d\n", ret);
>>           goto err_node_allow;
>>       }
>> +    ret = drm_gem_handle_create(adev->kfd.client.file, gobj, 
>> &(*mem)->gem_handle);
>> +    if (ret)
>> +        goto err_gem_handle_create;
>>       bo = gem_to_amdgpu_bo(gobj);
>>       if (bo_type == ttm_bo_type_sg) {
>>           bo->tbo.sg = sg;
>> @@ -1877,6 +1890,8 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>>   err_pin_bo:
>>   err_validate_bo:
>>       remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
>> +    drm_gem_handle_delete(adev->kfd.client.file, (*mem)->gem_handle);
>> +err_gem_handle_create:
>>       drm_vma_node_revoke(&gobj->vma_node, drm_priv);
>>   err_node_allow:
>>       /* Don't unreserve system mem limit twice */
>> @@ -1991,8 +2006,12 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
>>         /* Free the BO*/
>>       drm_vma_node_revoke(&mem->bo->tbo.base.vma_node, drm_priv);
>> -    if (mem->dmabuf)
>> +    if (!mem->is_imported)
>> +        drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
>> +    if (mem->dmabuf) {
>>           dma_buf_put(mem->dmabuf);
>> +        mem->dmabuf = NULL;
>> +    }
>>       mutex_destroy(&mem->lock);
>>         /* If this releases the last reference, it will end up calling
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> index 06988cf1db51..4417a9863cd0 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> @@ -1855,8 +1855,8 @@ static uint32_t get_process_num_bos(struct 
>> kfd_process *p)
>>       return num_of_bos;
>>   }
>>   -static int criu_get_prime_handle(struct kgd_mem *mem, int flags,
>> -                      u32 *shared_fd)
>> +static int criu_get_prime_handle(struct kgd_mem *mem,
>> +                 int flags, u32 *shared_fd)
>>   {
>>       struct dma_buf *dmabuf;
>>       int ret;

[PATCH 5/6] drm/amdkfd: Import DMABufs for interop through DRM

[Felix Kuehling]

Use drm_gem_prime_fd_to_handle to import DMABufs for interop. This
ensures that a GEM handle is created on import and that obj->dma_buf
will be set and remain set as long as the object is imported into KFD.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  9 ++-
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 64 +++++++++++++------
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      | 15 ++---
 3 files changed, 52 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index 4caf8cece028..88a0e0734270 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -318,11 +318,10 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *process_info,
 					    struct dma_fence **ef);
 int amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
 					      struct kfd_vm_fault_info *info);
-int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
-				      struct dma_buf *dmabuf,
-				      uint64_t va, void *drm_priv,
-				      struct kgd_mem **mem, uint64_t *size,
-				      uint64_t *mmap_offset);
+int amdgpu_amdkfd_gpuvm_import_dmabuf_fd(struct amdgpu_device *adev, int fd,
+					 uint64_t va, void *drm_priv,
+					 struct kgd_mem **mem, uint64_t *size,
+					 uint64_t *mmap_offset);
 int amdgpu_amdkfd_gpuvm_export_dmabuf(struct kgd_mem *mem,
 				      struct dma_buf **dmabuf);
 void amdgpu_amdkfd_debug_mem_fence(struct amdgpu_device *adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 4bb8b5fd7598..1077de8bced2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -2006,8 +2006,7 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
 
 	/* Free the BO*/
 	drm_vma_node_revoke(&mem->bo->tbo.base.vma_node, drm_priv);
-	if (!mem->is_imported)
-		drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
+	drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
 	if (mem->dmabuf) {
 		dma_buf_put(mem->dmabuf);
 		mem->dmabuf = NULL;
@@ -2363,34 +2362,26 @@ int amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
 	return 0;
 }
 
-int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
-				      struct dma_buf *dma_buf,
-				      uint64_t va, void *drm_priv,
-				      struct kgd_mem **mem, uint64_t *size,
-				      uint64_t *mmap_offset)
+static int import_obj_create(struct amdgpu_device *adev,
+			     struct dma_buf *dma_buf,
+			     struct drm_gem_object *obj,
+			     uint64_t va, void *drm_priv,
+			     struct kgd_mem **mem, uint64_t *size,
+			     uint64_t *mmap_offset)
 {
 	struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv);
-	struct drm_gem_object *obj;
 	struct amdgpu_bo *bo;
 	int ret;
 
-	obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
-	if (IS_ERR(obj))
-		return PTR_ERR(obj);
-
 	bo = gem_to_amdgpu_bo(obj);
 	if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
-				    AMDGPU_GEM_DOMAIN_GTT))) {
+				    AMDGPU_GEM_DOMAIN_GTT)))
 		/* Only VRAM and GTT BOs are supported */
-		ret = -EINVAL;
-		goto err_put_obj;
-	}
+		return -EINVAL;
 
 	*mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
-	if (!*mem) {
-		ret = -ENOMEM;
-		goto err_put_obj;
-	}
+	if (!*mem)
+		return -ENOMEM;
 
 	ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
 	if (ret)
@@ -2440,8 +2431,41 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
 	drm_vma_node_revoke(&obj->vma_node, drm_priv);
 err_free_mem:
 	kfree(*mem);
+	return ret;
+}
+
+int amdgpu_amdkfd_gpuvm_import_dmabuf_fd(struct amdgpu_device *adev, int fd,
+					 uint64_t va, void *drm_priv,
+					 struct kgd_mem **mem, uint64_t *size,
+					 uint64_t *mmap_offset)
+{
+	struct drm_gem_object *obj;
+	uint32_t handle;
+	int ret;
+
+	ret = drm_gem_prime_fd_to_handle(&adev->ddev, adev->kfd.client.file, fd,
+					 &handle);
+	if (ret)
+		return ret;
+	obj = drm_gem_object_lookup(adev->kfd.client.file, handle);
+	if (!obj) {
+		ret = -EINVAL;
+		goto err_release_handle;
+	}
+
+	ret = import_obj_create(adev, obj->dma_buf, obj, va, drm_priv, mem, size,
+				mmap_offset);
+	if (ret)
+		goto err_put_obj;
+
+	(*mem)->gem_handle = handle;
+
+	return 0;
+
 err_put_obj:
 	drm_gem_object_put(obj);
+err_release_handle:
+	drm_gem_handle_delete(adev->kfd.client.file, handle);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 4417a9863cd0..1a2e9f564b7f 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -1564,16 +1564,11 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
 {
 	struct kfd_ioctl_import_dmabuf_args *args = data;
 	struct kfd_process_device *pdd;
-	struct dma_buf *dmabuf;
 	int idr_handle;
 	uint64_t size;
 	void *mem;
 	int r;
 
-	dmabuf = dma_buf_get(args->dmabuf_fd);
-	if (IS_ERR(dmabuf))
-		return PTR_ERR(dmabuf);
-
 	mutex_lock(&p->mutex);
 	pdd = kfd_process_device_data_by_id(p, args->gpu_id);
 	if (!pdd) {
@@ -1587,10 +1582,10 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
 		goto err_unlock;
 	}
 
-	r = amdgpu_amdkfd_gpuvm_import_dmabuf(pdd->dev->adev, dmabuf,
-					      args->va_addr, pdd->drm_priv,
-					      (struct kgd_mem **)&mem, &size,
-					      NULL);
+	r = amdgpu_amdkfd_gpuvm_import_dmabuf_fd(pdd->dev->adev, args->dmabuf_fd,
+						 args->va_addr, pdd->drm_priv,
+						 (struct kgd_mem **)&mem, &size,
+						 NULL);
 	if (r)
 		goto err_unlock;
 
@@ -1601,7 +1596,6 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
 	}
 
 	mutex_unlock(&p->mutex);
-	dma_buf_put(dmabuf);
 
 	args->handle = MAKE_HANDLE(args->gpu_id, idr_handle);
 
@@ -1612,7 +1606,6 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
 					       pdd->drm_priv, NULL);
 err_unlock:
 	mutex_unlock(&p->mutex);
-	dma_buf_put(dmabuf);
 	return r;
 }
 
-- 
2.34.1

RE: [PATCH 5/6] drm/amdkfd: Import DMABufs for interop through DRM

[Errabolu, Ramesh]

[AMD Official Use Only - General]

Reviewed-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>

-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling@amd.com>
Sent: Tuesday, November 7, 2023 10:28 PM
To: amd-gfx@lists.freedesktop.org
Cc: Koenig, Christian <Christian.Koenig@amd.com>; Chen, Xiaogang <Xiaogang.Chen@amd.com>; Errabolu, Ramesh <Ramesh.Errabolu@amd.com>
Subject: [PATCH 5/6] drm/amdkfd: Import DMABufs for interop through DRM

Use drm_gem_prime_fd_to_handle to import DMABufs for interop. This ensures that a GEM handle is created on import and that obj->dma_buf will be set and remain set as long as the object is imported into KFD.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  9 ++-
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 64 +++++++++++++------
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      | 15 ++---
 3 files changed, 52 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index 4caf8cece028..88a0e0734270 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -318,11 +318,10 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *process_info,
                                            struct dma_fence **ef);
 int amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
                                              struct kfd_vm_fault_info *info); -int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
-                                     struct dma_buf *dmabuf,
-                                     uint64_t va, void *drm_priv,
-                                     struct kgd_mem **mem, uint64_t *size,
-                                     uint64_t *mmap_offset);
+int amdgpu_amdkfd_gpuvm_import_dmabuf_fd(struct amdgpu_device *adev, int fd,
+                                        uint64_t va, void *drm_priv,
+                                        struct kgd_mem **mem, uint64_t *size,
+                                        uint64_t *mmap_offset);
 int amdgpu_amdkfd_gpuvm_export_dmabuf(struct kgd_mem *mem,
                                      struct dma_buf **dmabuf);
 void amdgpu_amdkfd_debug_mem_fence(struct amdgpu_device *adev); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 4bb8b5fd7598..1077de8bced2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -2006,8 +2006,7 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(

        /* Free the BO*/
        drm_vma_node_revoke(&mem->bo->tbo.base.vma_node, drm_priv);
-       if (!mem->is_imported)
-               drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
+       drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
        if (mem->dmabuf) {
                dma_buf_put(mem->dmabuf);
                mem->dmabuf = NULL;
@@ -2363,34 +2362,26 @@ int amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
        return 0;
 }

-int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
-                                     struct dma_buf *dma_buf,
-                                     uint64_t va, void *drm_priv,
-                                     struct kgd_mem **mem, uint64_t *size,
-                                     uint64_t *mmap_offset)
+static int import_obj_create(struct amdgpu_device *adev,
+                            struct dma_buf *dma_buf,
+                            struct drm_gem_object *obj,
+                            uint64_t va, void *drm_priv,
+                            struct kgd_mem **mem, uint64_t *size,
+                            uint64_t *mmap_offset)
 {
        struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv);
-       struct drm_gem_object *obj;
        struct amdgpu_bo *bo;
        int ret;

-       obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
-       if (IS_ERR(obj))
-               return PTR_ERR(obj);
-
        bo = gem_to_amdgpu_bo(obj);
        if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
-                                   AMDGPU_GEM_DOMAIN_GTT))) {
+                                   AMDGPU_GEM_DOMAIN_GTT)))
                /* Only VRAM and GTT BOs are supported */
-               ret = -EINVAL;
-               goto err_put_obj;
-       }
+               return -EINVAL;

        *mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
-       if (!*mem) {
-               ret = -ENOMEM;
-               goto err_put_obj;
-       }
+       if (!*mem)
+               return -ENOMEM;

        ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
        if (ret)
@@ -2440,8 +2431,41 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
        drm_vma_node_revoke(&obj->vma_node, drm_priv);
 err_free_mem:
        kfree(*mem);
+       return ret;
+}
+
+int amdgpu_amdkfd_gpuvm_import_dmabuf_fd(struct amdgpu_device *adev, int fd,
+                                        uint64_t va, void *drm_priv,
+                                        struct kgd_mem **mem, uint64_t *size,
+                                        uint64_t *mmap_offset)
+{
+       struct drm_gem_object *obj;
+       uint32_t handle;
+       int ret;
+
+       ret = drm_gem_prime_fd_to_handle(&adev->ddev, adev->kfd.client.file, fd,
+                                        &handle);
+       if (ret)
+               return ret;
+       obj = drm_gem_object_lookup(adev->kfd.client.file, handle);
+       if (!obj) {
+               ret = -EINVAL;
+               goto err_release_handle;
+       }
+
+       ret = import_obj_create(adev, obj->dma_buf, obj, va, drm_priv, mem, size,
+                               mmap_offset);
+       if (ret)
+               goto err_put_obj;
+
+       (*mem)->gem_handle = handle;
+
+       return 0;
+
 err_put_obj:
        drm_gem_object_put(obj);
+err_release_handle:
+       drm_gem_handle_delete(adev->kfd.client.file, handle);
        return ret;
 }

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 4417a9863cd0..1a2e9f564b7f 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -1564,16 +1564,11 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,  {
        struct kfd_ioctl_import_dmabuf_args *args = data;
        struct kfd_process_device *pdd;
-       struct dma_buf *dmabuf;
        int idr_handle;
        uint64_t size;
        void *mem;
        int r;

-       dmabuf = dma_buf_get(args->dmabuf_fd);
-       if (IS_ERR(dmabuf))
-               return PTR_ERR(dmabuf);
-
        mutex_lock(&p->mutex);
        pdd = kfd_process_device_data_by_id(p, args->gpu_id);
        if (!pdd) {
@@ -1587,10 +1582,10 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
                goto err_unlock;
        }

-       r = amdgpu_amdkfd_gpuvm_import_dmabuf(pdd->dev->adev, dmabuf,
-                                             args->va_addr, pdd->drm_priv,
-                                             (struct kgd_mem **)&mem, &size,
-                                             NULL);
+       r = amdgpu_amdkfd_gpuvm_import_dmabuf_fd(pdd->dev->adev, args->dmabuf_fd,
+                                                args->va_addr, pdd->drm_priv,
+                                                (struct kgd_mem **)&mem, &size,
+                                                NULL);
        if (r)
                goto err_unlock;

@@ -1601,7 +1596,6 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
        }

        mutex_unlock(&p->mutex);
-       dma_buf_put(dmabuf);

        args->handle = MAKE_HANDLE(args->gpu_id, idr_handle);

@@ -1612,7 +1606,6 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
                                               pdd->drm_priv, NULL);
 err_unlock:
        mutex_unlock(&p->mutex);
-       dma_buf_put(dmabuf);
        return r;
 }

--
2.34.1

Re: [PATCH 5/6] drm/amdkfd: Import DMABufs for interop through DRM

[Chen, Xiaogang]

On 11/7/2023 10:58 AM, Felix Kuehling wrote:
> Use drm_gem_prime_fd_to_handle to import DMABufs for interop. This
> ensures that a GEM handle is created on import and that obj->dma_buf
> will be set and remain set as long as the object is imported into KFD.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  9 ++-
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 64 +++++++++++++------
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      | 15 ++---
>   3 files changed, 52 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index 4caf8cece028..88a0e0734270 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -318,11 +318,10 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *process_info,
>   					    struct dma_fence **ef);
>   int amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
>   					      struct kfd_vm_fault_info *info);
> -int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
> -				      struct dma_buf *dmabuf,
> -				      uint64_t va, void *drm_priv,
> -				      struct kgd_mem **mem, uint64_t *size,
> -				      uint64_t *mmap_offset);
> +int amdgpu_amdkfd_gpuvm_import_dmabuf_fd(struct amdgpu_device *adev, int fd,
> +					 uint64_t va, void *drm_priv,
> +					 struct kgd_mem **mem, uint64_t *size,
> +					 uint64_t *mmap_offset);
>   int amdgpu_amdkfd_gpuvm_export_dmabuf(struct kgd_mem *mem,
>   				      struct dma_buf **dmabuf);
>   void amdgpu_amdkfd_debug_mem_fence(struct amdgpu_device *adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 4bb8b5fd7598..1077de8bced2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -2006,8 +2006,7 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
>   
>   	/* Free the BO*/
>   	drm_vma_node_revoke(&mem->bo->tbo.base.vma_node, drm_priv);
> -	if (!mem->is_imported)
> -		drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
> +	drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);

A minor thing for this patch: I think this is a correction for last 
patch " Export DMABufs from KFD using GEM handles". mem->gem_handle is 
created unconditionally at amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu.  
drm_gem_handle_delete should be put at the lat patch.

Regards

Xiaogang

>   	if (mem->dmabuf) {
>   		dma_buf_put(mem->dmabuf);
>   		mem->dmabuf = NULL;
> @@ -2363,34 +2362,26 @@ int amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
>   	return 0;
>   }
>   
> -int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
> -				      struct dma_buf *dma_buf,
> -				      uint64_t va, void *drm_priv,
> -				      struct kgd_mem **mem, uint64_t *size,
> -				      uint64_t *mmap_offset)
> +static int import_obj_create(struct amdgpu_device *adev,
> +			     struct dma_buf *dma_buf,
> +			     struct drm_gem_object *obj,
> +			     uint64_t va, void *drm_priv,
> +			     struct kgd_mem **mem, uint64_t *size,
> +			     uint64_t *mmap_offset)
>   {
>   	struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv);
> -	struct drm_gem_object *obj;
>   	struct amdgpu_bo *bo;
>   	int ret;
>   
> -	obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
> -	if (IS_ERR(obj))
> -		return PTR_ERR(obj);
> -
>   	bo = gem_to_amdgpu_bo(obj);
>   	if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
> -				    AMDGPU_GEM_DOMAIN_GTT))) {
> +				    AMDGPU_GEM_DOMAIN_GTT)))
>   		/* Only VRAM and GTT BOs are supported */
> -		ret = -EINVAL;
> -		goto err_put_obj;
> -	}
> +		return -EINVAL;
>   
>   	*mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
> -	if (!*mem) {
> -		ret = -ENOMEM;
> -		goto err_put_obj;
> -	}
> +	if (!*mem)
> +		return -ENOMEM;
>   
>   	ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
>   	if (ret)
> @@ -2440,8 +2431,41 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>   	drm_vma_node_revoke(&obj->vma_node, drm_priv);
>   err_free_mem:
>   	kfree(*mem);
> +	return ret;
> +}
> +
> +int amdgpu_amdkfd_gpuvm_import_dmabuf_fd(struct amdgpu_device *adev, int fd,
> +					 uint64_t va, void *drm_priv,
> +					 struct kgd_mem **mem, uint64_t *size,
> +					 uint64_t *mmap_offset)
> +{
> +	struct drm_gem_object *obj;
> +	uint32_t handle;
> +	int ret;
> +
> +	ret = drm_gem_prime_fd_to_handle(&adev->ddev, adev->kfd.client.file, fd,
> +					 &handle);
> +	if (ret)
> +		return ret;
> +	obj = drm_gem_object_lookup(adev->kfd.client.file, handle);
> +	if (!obj) {
> +		ret = -EINVAL;
> +		goto err_release_handle;
> +	}
> +
> +	ret = import_obj_create(adev, obj->dma_buf, obj, va, drm_priv, mem, size,
> +				mmap_offset);
> +	if (ret)
> +		goto err_put_obj;
> +
> +	(*mem)->gem_handle = handle;
> +
> +	return 0;
> +
>   err_put_obj:
>   	drm_gem_object_put(obj);
> +err_release_handle:
> +	drm_gem_handle_delete(adev->kfd.client.file, handle);
>   	return ret;
>   }
>   
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 4417a9863cd0..1a2e9f564b7f 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1564,16 +1564,11 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
>   {
>   	struct kfd_ioctl_import_dmabuf_args *args = data;
>   	struct kfd_process_device *pdd;
> -	struct dma_buf *dmabuf;
>   	int idr_handle;
>   	uint64_t size;
>   	void *mem;
>   	int r;
>   
> -	dmabuf = dma_buf_get(args->dmabuf_fd);
> -	if (IS_ERR(dmabuf))
> -		return PTR_ERR(dmabuf);
> -
>   	mutex_lock(&p->mutex);
>   	pdd = kfd_process_device_data_by_id(p, args->gpu_id);
>   	if (!pdd) {
> @@ -1587,10 +1582,10 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
>   		goto err_unlock;
>   	}
>   
> -	r = amdgpu_amdkfd_gpuvm_import_dmabuf(pdd->dev->adev, dmabuf,
> -					      args->va_addr, pdd->drm_priv,
> -					      (struct kgd_mem **)&mem, &size,
> -					      NULL);
> +	r = amdgpu_amdkfd_gpuvm_import_dmabuf_fd(pdd->dev->adev, args->dmabuf_fd,
> +						 args->va_addr, pdd->drm_priv,
> +						 (struct kgd_mem **)&mem, &size,
> +						 NULL);
>   	if (r)
>   		goto err_unlock;
>   
> @@ -1601,7 +1596,6 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
>   	}
>   
>   	mutex_unlock(&p->mutex);
> -	dma_buf_put(dmabuf);
>   
>   	args->handle = MAKE_HANDLE(args->gpu_id, idr_handle);
>   
> @@ -1612,7 +1606,6 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
>   					       pdd->drm_priv, NULL);
>   err_unlock:
>   	mutex_unlock(&p->mutex);
> -	dma_buf_put(dmabuf);
>   	return r;
>   }
>   

Re: [PATCH 5/6] drm/amdkfd: Import DMABufs for interop through DRM

[Felix Kuehling]

On 2023-11-08 18:20, Chen, Xiaogang wrote:
>
> On 11/7/2023 10:58 AM, Felix Kuehling wrote:
>> Use drm_gem_prime_fd_to_handle to import DMABufs for interop. This
>> ensures that a GEM handle is created on import and that obj->dma_buf
>> will be set and remain set as long as the object is imported into KFD.
>>
>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  9 ++-
>>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 64 +++++++++++++------
>>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      | 15 ++---
>>   3 files changed, 52 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> index 4caf8cece028..88a0e0734270 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> @@ -318,11 +318,10 @@ int 
>> amdgpu_amdkfd_gpuvm_restore_process_bos(void *process_info,
>>                           struct dma_fence **ef);
>>   int amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
>>                             struct kfd_vm_fault_info *info);
>> -int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>> -                      struct dma_buf *dmabuf,
>> -                      uint64_t va, void *drm_priv,
>> -                      struct kgd_mem **mem, uint64_t *size,
>> -                      uint64_t *mmap_offset);
>> +int amdgpu_amdkfd_gpuvm_import_dmabuf_fd(struct amdgpu_device *adev, 
>> int fd,
>> +                     uint64_t va, void *drm_priv,
>> +                     struct kgd_mem **mem, uint64_t *size,
>> +                     uint64_t *mmap_offset);
>>   int amdgpu_amdkfd_gpuvm_export_dmabuf(struct kgd_mem *mem,
>>                         struct dma_buf **dmabuf);
>>   void amdgpu_amdkfd_debug_mem_fence(struct amdgpu_device *adev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index 4bb8b5fd7598..1077de8bced2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -2006,8 +2006,7 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
>>         /* Free the BO*/
>>       drm_vma_node_revoke(&mem->bo->tbo.base.vma_node, drm_priv);
>> -    if (!mem->is_imported)
>> -        drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
>> +    drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
>
> A minor thing for this patch: I think this is a correction for last 
> patch " Export DMABufs from KFD using GEM handles". mem->gem_handle is 
> created unconditionally at amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu.  
> drm_gem_handle_delete should be put at the lat patch.

This change was intentional. Without this patch, imported DMABufs didn't 
get a GEM handle, so I didn't need to delete one. With this patch, I now 
have a GEM handle for imported BOs, so I delete the GEM handle 
unconditionally.

Regards,
   Felix


>
> Regards
>
> Xiaogang
>
>>       if (mem->dmabuf) {
>>           dma_buf_put(mem->dmabuf);
>>           mem->dmabuf = NULL;
>> @@ -2363,34 +2362,26 @@ int 
>> amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
>>       return 0;
>>   }
>>   -int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>> -                      struct dma_buf *dma_buf,
>> -                      uint64_t va, void *drm_priv,
>> -                      struct kgd_mem **mem, uint64_t *size,
>> -                      uint64_t *mmap_offset)
>> +static int import_obj_create(struct amdgpu_device *adev,
>> +                 struct dma_buf *dma_buf,
>> +                 struct drm_gem_object *obj,
>> +                 uint64_t va, void *drm_priv,
>> +                 struct kgd_mem **mem, uint64_t *size,
>> +                 uint64_t *mmap_offset)
>>   {
>>       struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv);
>> -    struct drm_gem_object *obj;
>>       struct amdgpu_bo *bo;
>>       int ret;
>>   -    obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
>> -    if (IS_ERR(obj))
>> -        return PTR_ERR(obj);
>> -
>>       bo = gem_to_amdgpu_bo(obj);
>>       if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
>> -                    AMDGPU_GEM_DOMAIN_GTT))) {
>> +                    AMDGPU_GEM_DOMAIN_GTT)))
>>           /* Only VRAM and GTT BOs are supported */
>> -        ret = -EINVAL;
>> -        goto err_put_obj;
>> -    }
>> +        return -EINVAL;
>>         *mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
>> -    if (!*mem) {
>> -        ret = -ENOMEM;
>> -        goto err_put_obj;
>> -    }
>> +    if (!*mem)
>> +        return -ENOMEM;
>>         ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
>>       if (ret)
>> @@ -2440,8 +2431,41 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct 
>> amdgpu_device *adev,
>>       drm_vma_node_revoke(&obj->vma_node, drm_priv);
>>   err_free_mem:
>>       kfree(*mem);
>> +    return ret;
>> +}
>> +
>> +int amdgpu_amdkfd_gpuvm_import_dmabuf_fd(struct amdgpu_device *adev, 
>> int fd,
>> +                     uint64_t va, void *drm_priv,
>> +                     struct kgd_mem **mem, uint64_t *size,
>> +                     uint64_t *mmap_offset)
>> +{
>> +    struct drm_gem_object *obj;
>> +    uint32_t handle;
>> +    int ret;
>> +
>> +    ret = drm_gem_prime_fd_to_handle(&adev->ddev, 
>> adev->kfd.client.file, fd,
>> +                     &handle);
>> +    if (ret)
>> +        return ret;
>> +    obj = drm_gem_object_lookup(adev->kfd.client.file, handle);
>> +    if (!obj) {
>> +        ret = -EINVAL;
>> +        goto err_release_handle;
>> +    }
>> +
>> +    ret = import_obj_create(adev, obj->dma_buf, obj, va, drm_priv, 
>> mem, size,
>> +                mmap_offset);
>> +    if (ret)
>> +        goto err_put_obj;
>> +
>> +    (*mem)->gem_handle = handle;
>> +
>> +    return 0;
>> +
>>   err_put_obj:
>>       drm_gem_object_put(obj);
>> +err_release_handle:
>> +    drm_gem_handle_delete(adev->kfd.client.file, handle);
>>       return ret;
>>   }
>>   diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> index 4417a9863cd0..1a2e9f564b7f 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> @@ -1564,16 +1564,11 @@ static int kfd_ioctl_import_dmabuf(struct 
>> file *filep,
>>   {
>>       struct kfd_ioctl_import_dmabuf_args *args = data;
>>       struct kfd_process_device *pdd;
>> -    struct dma_buf *dmabuf;
>>       int idr_handle;
>>       uint64_t size;
>>       void *mem;
>>       int r;
>>   -    dmabuf = dma_buf_get(args->dmabuf_fd);
>> -    if (IS_ERR(dmabuf))
>> -        return PTR_ERR(dmabuf);
>> -
>>       mutex_lock(&p->mutex);
>>       pdd = kfd_process_device_data_by_id(p, args->gpu_id);
>>       if (!pdd) {
>> @@ -1587,10 +1582,10 @@ static int kfd_ioctl_import_dmabuf(struct 
>> file *filep,
>>           goto err_unlock;
>>       }
>>   -    r = amdgpu_amdkfd_gpuvm_import_dmabuf(pdd->dev->adev, dmabuf,
>> -                          args->va_addr, pdd->drm_priv,
>> -                          (struct kgd_mem **)&mem, &size,
>> -                          NULL);
>> +    r = amdgpu_amdkfd_gpuvm_import_dmabuf_fd(pdd->dev->adev, 
>> args->dmabuf_fd,
>> +                         args->va_addr, pdd->drm_priv,
>> +                         (struct kgd_mem **)&mem, &size,
>> +                         NULL);
>>       if (r)
>>           goto err_unlock;
>>   @@ -1601,7 +1596,6 @@ static int kfd_ioctl_import_dmabuf(struct 
>> file *filep,
>>       }
>>         mutex_unlock(&p->mutex);
>> -    dma_buf_put(dmabuf);
>>         args->handle = MAKE_HANDLE(args->gpu_id, idr_handle);
>>   @@ -1612,7 +1606,6 @@ static int kfd_ioctl_import_dmabuf(struct 
>> file *filep,
>>                              pdd->drm_priv, NULL);
>>   err_unlock:
>>       mutex_unlock(&p->mutex);
>> -    dma_buf_put(dmabuf);
>>       return r;
>>   }

Re: [PATCH 5/6] drm/amdkfd: Import DMABufs for interop through DRM

[Chen, Xiaogang]

On 11/8/2023 5:26 PM, Felix Kuehling wrote:
> On 2023-11-08 18:20, Chen, Xiaogang wrote:
>>
>> On 11/7/2023 10:58 AM, Felix Kuehling wrote:
>>> Use drm_gem_prime_fd_to_handle to import DMABufs for interop. This
>>> ensures that a GEM handle is created on import and that obj->dma_buf
>>> will be set and remain set as long as the object is imported into KFD.
>>>
>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  9 ++-
>>>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 64 
>>> +++++++++++++------
>>>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      | 15 ++---
>>>   3 files changed, 52 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>>> index 4caf8cece028..88a0e0734270 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>>> @@ -318,11 +318,10 @@ int 
>>> amdgpu_amdkfd_gpuvm_restore_process_bos(void *process_info,
>>>                           struct dma_fence **ef);
>>>   int amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
>>>                             struct kfd_vm_fault_info *info);
>>> -int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>>> -                      struct dma_buf *dmabuf,
>>> -                      uint64_t va, void *drm_priv,
>>> -                      struct kgd_mem **mem, uint64_t *size,
>>> -                      uint64_t *mmap_offset);
>>> +int amdgpu_amdkfd_gpuvm_import_dmabuf_fd(struct amdgpu_device 
>>> *adev, int fd,
>>> +                     uint64_t va, void *drm_priv,
>>> +                     struct kgd_mem **mem, uint64_t *size,
>>> +                     uint64_t *mmap_offset);
>>>   int amdgpu_amdkfd_gpuvm_export_dmabuf(struct kgd_mem *mem,
>>>                         struct dma_buf **dmabuf);
>>>   void amdgpu_amdkfd_debug_mem_fence(struct amdgpu_device *adev);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> index 4bb8b5fd7598..1077de8bced2 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> @@ -2006,8 +2006,7 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
>>>         /* Free the BO*/
>>> drm_vma_node_revoke(&mem->bo->tbo.base.vma_node, drm_priv);
>>> -    if (!mem->is_imported)
>>> -        drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
>>> +    drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
>>
>> A minor thing for this patch: I think this is a correction for last 
>> patch " Export DMABufs from KFD using GEM handles". mem->gem_handle 
>> is created unconditionally at 
>> amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu.  drm_gem_handle_delete 
>> should be put at the lat patch.
>
> This change was intentional. Without this patch, imported DMABufs 
> didn't get a GEM handle, so I didn't need to delete one. With this 
> patch, I now have a GEM handle for imported BOs, so I delete the GEM 
> handle unconditionally.
>
oh, imported mem->gem_handle is generated at 
amdgpu_amdkfd_gpuvm_import_dmabuf_fd. Need refresh my memory after a 
while.  This patch is:

Reviwed-by: Xiaogang.Chen <Xiaogang.Chen@amd.com>

> Regards,
>   Felix
>
>
>>
>> Regards
>>
>> Xiaogang
>>
>>>       if (mem->dmabuf) {
>>>           dma_buf_put(mem->dmabuf);
>>>           mem->dmabuf = NULL;
>>> @@ -2363,34 +2362,26 @@ int 
>>> amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
>>>       return 0;
>>>   }
>>>   -int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>>> -                      struct dma_buf *dma_buf,
>>> -                      uint64_t va, void *drm_priv,
>>> -                      struct kgd_mem **mem, uint64_t *size,
>>> -                      uint64_t *mmap_offset)
>>> +static int import_obj_create(struct amdgpu_device *adev,
>>> +                 struct dma_buf *dma_buf,
>>> +                 struct drm_gem_object *obj,
>>> +                 uint64_t va, void *drm_priv,
>>> +                 struct kgd_mem **mem, uint64_t *size,
>>> +                 uint64_t *mmap_offset)
>>>   {
>>>       struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv);
>>> -    struct drm_gem_object *obj;
>>>       struct amdgpu_bo *bo;
>>>       int ret;
>>>   -    obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
>>> -    if (IS_ERR(obj))
>>> -        return PTR_ERR(obj);
>>> -
>>>       bo = gem_to_amdgpu_bo(obj);
>>>       if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
>>> -                    AMDGPU_GEM_DOMAIN_GTT))) {
>>> +                    AMDGPU_GEM_DOMAIN_GTT)))
>>>           /* Only VRAM and GTT BOs are supported */
>>> -        ret = -EINVAL;
>>> -        goto err_put_obj;
>>> -    }
>>> +        return -EINVAL;
>>>         *mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
>>> -    if (!*mem) {
>>> -        ret = -ENOMEM;
>>> -        goto err_put_obj;
>>> -    }
>>> +    if (!*mem)
>>> +        return -ENOMEM;
>>>         ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
>>>       if (ret)
>>> @@ -2440,8 +2431,41 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct 
>>> amdgpu_device *adev,
>>>       drm_vma_node_revoke(&obj->vma_node, drm_priv);
>>>   err_free_mem:
>>>       kfree(*mem);
>>> +    return ret;
>>> +}
>>> +
>>> +int amdgpu_amdkfd_gpuvm_import_dmabuf_fd(struct amdgpu_device 
>>> *adev, int fd,
>>> +                     uint64_t va, void *drm_priv,
>>> +                     struct kgd_mem **mem, uint64_t *size,
>>> +                     uint64_t *mmap_offset)
>>> +{
>>> +    struct drm_gem_object *obj;
>>> +    uint32_t handle;
>>> +    int ret;
>>> +
>>> +    ret = drm_gem_prime_fd_to_handle(&adev->ddev, 
>>> adev->kfd.client.file, fd,
>>> +                     &handle);
>>> +    if (ret)
>>> +        return ret;
>>> +    obj = drm_gem_object_lookup(adev->kfd.client.file, handle);
>>> +    if (!obj) {
>>> +        ret = -EINVAL;
>>> +        goto err_release_handle;
>>> +    }
>>> +
>>> +    ret = import_obj_create(adev, obj->dma_buf, obj, va, drm_priv, 
>>> mem, size,
>>> +                mmap_offset);
>>> +    if (ret)
>>> +        goto err_put_obj;
>>> +
>>> +    (*mem)->gem_handle = handle;
>>> +
>>> +    return 0;
>>> +
>>>   err_put_obj:
>>>       drm_gem_object_put(obj);
>>> +err_release_handle:
>>> +    drm_gem_handle_delete(adev->kfd.client.file, handle);
>>>       return ret;
>>>   }
>>>   diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> index 4417a9863cd0..1a2e9f564b7f 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> @@ -1564,16 +1564,11 @@ static int kfd_ioctl_import_dmabuf(struct 
>>> file *filep,
>>>   {
>>>       struct kfd_ioctl_import_dmabuf_args *args = data;
>>>       struct kfd_process_device *pdd;
>>> -    struct dma_buf *dmabuf;
>>>       int idr_handle;
>>>       uint64_t size;
>>>       void *mem;
>>>       int r;
>>>   -    dmabuf = dma_buf_get(args->dmabuf_fd);
>>> -    if (IS_ERR(dmabuf))
>>> -        return PTR_ERR(dmabuf);
>>> -
>>>       mutex_lock(&p->mutex);
>>>       pdd = kfd_process_device_data_by_id(p, args->gpu_id);
>>>       if (!pdd) {
>>> @@ -1587,10 +1582,10 @@ static int kfd_ioctl_import_dmabuf(struct 
>>> file *filep,
>>>           goto err_unlock;
>>>       }
>>>   -    r = amdgpu_amdkfd_gpuvm_import_dmabuf(pdd->dev->adev, dmabuf,
>>> -                          args->va_addr, pdd->drm_priv,
>>> -                          (struct kgd_mem **)&mem, &size,
>>> -                          NULL);
>>> +    r = amdgpu_amdkfd_gpuvm_import_dmabuf_fd(pdd->dev->adev, 
>>> args->dmabuf_fd,
>>> +                         args->va_addr, pdd->drm_priv,
>>> +                         (struct kgd_mem **)&mem, &size,
>>> +                         NULL);
>>>       if (r)
>>>           goto err_unlock;
>>>   @@ -1601,7 +1596,6 @@ static int kfd_ioctl_import_dmabuf(struct 
>>> file *filep,
>>>       }
>>>         mutex_unlock(&p->mutex);
>>> -    dma_buf_put(dmabuf);
>>>         args->handle = MAKE_HANDLE(args->gpu_id, idr_handle);
>>>   @@ -1612,7 +1606,6 @@ static int kfd_ioctl_import_dmabuf(struct 
>>> file *filep,
>>>                              pdd->drm_priv, NULL);
>>>   err_unlock:
>>>       mutex_unlock(&p->mutex);
>>> -    dma_buf_put(dmabuf);
>>>       return r;
>>>   }

Re: [PATCH 5/6] drm/amdkfd: Import DMABufs for interop through DRM

[Christian König]

Am 07.11.23 um 17:58 schrieb Felix Kuehling:
> Use drm_gem_prime_fd_to_handle to import DMABufs for interop. This
> ensures that a GEM handle is created on import and that obj->dma_buf
> will be set and remain set as long as the object is imported into KFD.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>

Acked-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  9 ++-
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 64 +++++++++++++------
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      | 15 ++---
>   3 files changed, 52 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index 4caf8cece028..88a0e0734270 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -318,11 +318,10 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *process_info,
>   					    struct dma_fence **ef);
>   int amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
>   					      struct kfd_vm_fault_info *info);
> -int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
> -				      struct dma_buf *dmabuf,
> -				      uint64_t va, void *drm_priv,
> -				      struct kgd_mem **mem, uint64_t *size,
> -				      uint64_t *mmap_offset);
> +int amdgpu_amdkfd_gpuvm_import_dmabuf_fd(struct amdgpu_device *adev, int fd,
> +					 uint64_t va, void *drm_priv,
> +					 struct kgd_mem **mem, uint64_t *size,
> +					 uint64_t *mmap_offset);
>   int amdgpu_amdkfd_gpuvm_export_dmabuf(struct kgd_mem *mem,
>   				      struct dma_buf **dmabuf);
>   void amdgpu_amdkfd_debug_mem_fence(struct amdgpu_device *adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 4bb8b5fd7598..1077de8bced2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -2006,8 +2006,7 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
>   
>   	/* Free the BO*/
>   	drm_vma_node_revoke(&mem->bo->tbo.base.vma_node, drm_priv);
> -	if (!mem->is_imported)
> -		drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
> +	drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
>   	if (mem->dmabuf) {
>   		dma_buf_put(mem->dmabuf);
>   		mem->dmabuf = NULL;
> @@ -2363,34 +2362,26 @@ int amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
>   	return 0;
>   }
>   
> -int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
> -				      struct dma_buf *dma_buf,
> -				      uint64_t va, void *drm_priv,
> -				      struct kgd_mem **mem, uint64_t *size,
> -				      uint64_t *mmap_offset)
> +static int import_obj_create(struct amdgpu_device *adev,
> +			     struct dma_buf *dma_buf,
> +			     struct drm_gem_object *obj,
> +			     uint64_t va, void *drm_priv,
> +			     struct kgd_mem **mem, uint64_t *size,
> +			     uint64_t *mmap_offset)
>   {
>   	struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv);
> -	struct drm_gem_object *obj;
>   	struct amdgpu_bo *bo;
>   	int ret;
>   
> -	obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
> -	if (IS_ERR(obj))
> -		return PTR_ERR(obj);
> -
>   	bo = gem_to_amdgpu_bo(obj);
>   	if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
> -				    AMDGPU_GEM_DOMAIN_GTT))) {
> +				    AMDGPU_GEM_DOMAIN_GTT)))
>   		/* Only VRAM and GTT BOs are supported */
> -		ret = -EINVAL;
> -		goto err_put_obj;
> -	}
> +		return -EINVAL;
>   
>   	*mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
> -	if (!*mem) {
> -		ret = -ENOMEM;
> -		goto err_put_obj;
> -	}
> +	if (!*mem)
> +		return -ENOMEM;
>   
>   	ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
>   	if (ret)
> @@ -2440,8 +2431,41 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>   	drm_vma_node_revoke(&obj->vma_node, drm_priv);
>   err_free_mem:
>   	kfree(*mem);
> +	return ret;
> +}
> +
> +int amdgpu_amdkfd_gpuvm_import_dmabuf_fd(struct amdgpu_device *adev, int fd,
> +					 uint64_t va, void *drm_priv,
> +					 struct kgd_mem **mem, uint64_t *size,
> +					 uint64_t *mmap_offset)
> +{
> +	struct drm_gem_object *obj;
> +	uint32_t handle;
> +	int ret;
> +
> +	ret = drm_gem_prime_fd_to_handle(&adev->ddev, adev->kfd.client.file, fd,
> +					 &handle);
> +	if (ret)
> +		return ret;
> +	obj = drm_gem_object_lookup(adev->kfd.client.file, handle);
> +	if (!obj) {
> +		ret = -EINVAL;
> +		goto err_release_handle;
> +	}
> +
> +	ret = import_obj_create(adev, obj->dma_buf, obj, va, drm_priv, mem, size,
> +				mmap_offset);
> +	if (ret)
> +		goto err_put_obj;
> +
> +	(*mem)->gem_handle = handle;
> +
> +	return 0;
> +
>   err_put_obj:
>   	drm_gem_object_put(obj);
> +err_release_handle:
> +	drm_gem_handle_delete(adev->kfd.client.file, handle);
>   	return ret;
>   }
>   
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 4417a9863cd0..1a2e9f564b7f 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1564,16 +1564,11 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
>   {
>   	struct kfd_ioctl_import_dmabuf_args *args = data;
>   	struct kfd_process_device *pdd;
> -	struct dma_buf *dmabuf;
>   	int idr_handle;
>   	uint64_t size;
>   	void *mem;
>   	int r;
>   
> -	dmabuf = dma_buf_get(args->dmabuf_fd);
> -	if (IS_ERR(dmabuf))
> -		return PTR_ERR(dmabuf);
> -
>   	mutex_lock(&p->mutex);
>   	pdd = kfd_process_device_data_by_id(p, args->gpu_id);
>   	if (!pdd) {
> @@ -1587,10 +1582,10 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
>   		goto err_unlock;
>   	}
>   
> -	r = amdgpu_amdkfd_gpuvm_import_dmabuf(pdd->dev->adev, dmabuf,
> -					      args->va_addr, pdd->drm_priv,
> -					      (struct kgd_mem **)&mem, &size,
> -					      NULL);
> +	r = amdgpu_amdkfd_gpuvm_import_dmabuf_fd(pdd->dev->adev, args->dmabuf_fd,
> +						 args->va_addr, pdd->drm_priv,
> +						 (struct kgd_mem **)&mem, &size,
> +						 NULL);
>   	if (r)
>   		goto err_unlock;
>   
> @@ -1601,7 +1596,6 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
>   	}
>   
>   	mutex_unlock(&p->mutex);
> -	dma_buf_put(dmabuf);
>   
>   	args->handle = MAKE_HANDLE(args->gpu_id, idr_handle);
>   
> @@ -1612,7 +1606,6 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
>   					       pdd->drm_priv, NULL);
>   err_unlock:
>   	mutex_unlock(&p->mutex);
> -	dma_buf_put(dmabuf);
>   	return r;
>   }
>   

[PATCH 6/6] drm/amdkfd: Bump KFD ioctl version

[Felix Kuehling]

This is not strictly a change in the IOCTL API. This version bump is meant
to indicate to user mode the presence of a number of changes and fixes
that enable the management of VA mappings in compute VMs using the GEM_VA
ioctl for DMABufs exported from KFD.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 include/uapi/linux/kfd_ioctl.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
index f0ed68974c54..9ce46edc62a5 100644
--- a/include/uapi/linux/kfd_ioctl.h
+++ b/include/uapi/linux/kfd_ioctl.h
@@ -40,9 +40,10 @@
  * - 1.12 - Add DMA buf export ioctl
  * - 1.13 - Add debugger API
  * - 1.14 - Update kfd_event_data
+ * - 1.15 - Enable managing mappings in compute VMs with GEM_VA ioctl
  */
 #define KFD_IOCTL_MAJOR_VERSION 1
-#define KFD_IOCTL_MINOR_VERSION 14
+#define KFD_IOCTL_MINOR_VERSION 15
 
 struct kfd_ioctl_get_version_args {
 	__u32 major_version;	/* from KFD */
-- 
2.34.1

Re: [PATCH 1/6] drm/amdgpu: Fix possible null pointer dereference

[Christian König]

Am 07.11.23 um 17:58 schrieb Felix Kuehling:
> mem = bo->tbo.resource may be NULL in amdgpu_vm_bo_update.
>
> Fixes: 180253782038 ("drm/ttm: stop allocating dummy resources during BO creation")
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 7b2a7c9156f0..1442d97ddd0f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1015,8 +1015,8 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
>   				bo = gem_to_amdgpu_bo(gobj);
>   		}
>   		mem = bo->tbo.resource;
> -		if (mem->mem_type == TTM_PL_TT ||
> -		    mem->mem_type == AMDGPU_PL_PREEMPT)
> +		if (mem && (mem->mem_type == TTM_PL_TT ||
> +			    mem->mem_type == AMDGPU_PL_PREEMPT))
>   			pages_addr = bo->tbo.ttm->dma_address;
>   	}
>