[PATCH v4 07/11] drm/amdgpu: add user queue vm identifier

[Christian König]

On 01.07.25 15:12, Liang, Prike wrote:
> [Public]
> 
> Regards,
>       Prike
> 
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig at amd.com>
>> Sent: Wednesday, June 25, 2025 3:52 PM
>> To: Liang, Prike <Prike.Liang at amd.com>; amd-gfx at lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
>> Subject: Re: [PATCH v4 07/11] drm/amdgpu: add user queue vm identifier
>>
>> On 24.06.25 10:45, Prike Liang wrote:
>>> Add a user queue vm identifier for each userqueue kms opt accessing.
>>>
>>> Signed-off-by: Prike Liang <Prike.Liang at amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 4 ++++
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c    | 2 +-
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h    | 2 +-
>>>  3 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> index 79b263c18eb1..0208c6b8a8e0 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> @@ -501,6 +501,8 @@ amdgpu_userq_destroy(struct drm_file *filp, int queue_id)
>>>     pm_runtime_mark_last_busy(adev_to_drm(adev)->dev);
>>>     pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
>>>
>>> +   fpriv->vm.is_userq_context = false;
>>> +
>>
>> I really don't like that approach. The VM should not have any special handling
>> dependign on if user queues exists or not.
>>
>> Why do you want that?
> This change is using the is_userq_context flag for identifying the user queue task to see whether it has been scheduled, and then
> further used for handling the sequence of attaching the eviction fence . The eviction fence is only available when the user queue
> process is scheduled, but in the amdgpu_gem_object_open(), it always attaches the eviction fence to the BOs regardless of whether
> the eviction fence is valid or not.

That is intentionally done this way.

> Based on this flag, we can simplify the eviction fence attaching sequence and only attach a valid
> eviction fence to the BO.

That won't work and is over all unecessary. The eviction fence is a no-op as long as no user queues are created, but should always be attached.

What you do here sounds like it is racy as hell. Please completely drop that approach.

Regards,
Christian.

> 
>>
>> Regards,
>> Christian.
>>
>>>     return r;
>>>  }
>>>
>>> @@ -535,6 +537,8 @@ amdgpu_userq_create(struct drm_file *filp, union
>> drm_amdgpu_userq *args)
>>>             (args->in.flags &
>> AMDGPU_USERQ_CREATE_FLAGS_QUEUE_PRIORITY_MASK) >>
>>>             AMDGPU_USERQ_CREATE_FLAGS_QUEUE_PRIORITY_SHIFT;
>>>
>>> +   fpriv->vm.is_userq_context = true;
>>> +
>>>     r = amdgpu_userq_priority_permit(filp, priority);
>>>     if (r)
>>>             return r;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 3911c78f8282..5e075e8f0ca3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -2560,7 +2560,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev,
>> struct amdgpu_vm *vm,
>>>     ttm_lru_bulk_move_init(&vm->lru_bulk_move);
>>>
>>>     vm->is_compute_context = false;
>>> -
>>> +   vm->is_userq_context = false;
>>>     vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode &
>>>                                 AMDGPU_VM_USE_CPU_FOR_GFX);
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> index f3ad687125ad..1e3c0f85c98e 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> @@ -422,7 +422,7 @@ struct amdgpu_vm {
>>>     struct ttm_lru_bulk_move lru_bulk_move;
>>>     /* Flag to indicate if VM is used for compute */
>>>     bool                    is_compute_context;
>>> -
>>> +   bool                    is_userq_context;
>>>     /* Memory partition number, -1 means any partition */
>>>     int8_t                  mem_id;
>>>
>