On 01.07.25 15:12, Liang, Prike wrote:
> [Public]
>
> Regards,
> Prike
>
>> -----Original Message-----
>> From: Koenig, Christian <christian.koe...@amd.com>
>> Sent: Wednesday, June 25, 2025 3:52 PM
>> To: Liang, Prike <prike.li...@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <alexander.deuc...@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.li...@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;
>>>
>
