[PATCH v3 4/6] drm/amdgpu: alloc and init vm::task_info from first submit
Pierre-Eric Pelloux-Prayer
pierre-eric at damsy.net
Tue Sep 24 07:21:56 UTC 2024
Le 23/09/2024 à 12:58, Tvrtko Ursulin a écrit :
>
> On 20/09/2024 10:06, Pierre-Eric Pelloux-Prayer wrote:
>> This will allow to use flexible array to store the process name and
>> other information.
>>
>> This also means that process name will be determined once and for all,
>> instead of at each submit.
>
> But the pid and others can still change? By design?
pid and task_name can change, yes.
tgid could be set once and for all I think.
>
>> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer at amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 15 +++++++++------
>> 1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index e20d19ae01b2..690676cab022 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -2331,7 +2331,7 @@ amdgpu_vm_get_task_info_vm(struct amdgpu_vm *vm)
>> {
>> struct amdgpu_task_info *ti = NULL;
>> - if (vm) {
>> + if (vm && vm->task_info) {
>> ti = vm->task_info;
>> kref_get(&vm->task_info->refcount);
>> }
>> @@ -2372,8 +2372,12 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm)
>> */
>> void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
>> {
>> - if (!vm->task_info)
>> - return;
>> + if (!vm->task_info) {
>> + if (amdgpu_vm_create_task_info(vm))
>> + return;
>> +
>> + get_task_comm(vm->task_info->process_name, current->group_leader);
>> + }
>> if (vm->task_info->pid == current->pid)
>
> This ends up relying on vm->task_info->pid being zero due kzalloc right?
Yes.
>
>> return;
>> @@ -2385,7 +2389,6 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
>> return;
>> vm->task_info->tgid = current->group_leader->pid;
>> - get_task_comm(vm->task_info->process_name, current->group_leader);
>> }
>
> I wonder how many of the task_info fields you want to set once instead of per submission. Like a
> fully one shot like the below be what you want?
As written above, process name, drm client name and pid (tgid) can be set once.
Task name + tid are updated on submit.
I've updated slightly this part, so v4 should hopefully be clearer.
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index a060c28f0877..da492223a8b5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2349,16 +2349,6 @@ amdgpu_vm_get_task_info_pasid(struct amdgpu_device *adev, u32 pasid)
> amdgpu_vm_get_vm_from_pasid(adev, pasid));
> }
>
> -static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm)
> -{
> - vm->task_info = kzalloc(sizeof(struct amdgpu_task_info), GFP_KERNEL);
> - if (!vm->task_info)
> - return -ENOMEM;
> -
> - kref_init(&vm->task_info->refcount);
> - return 0;
> -}
> -
> /**
> * amdgpu_vm_set_task_info - Sets VMs task info.
> *
> @@ -2366,20 +2356,28 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm)
> */
> void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
> {
> - if (!vm->task_info)
> - return;
> + struct amdgpu_task_info *task_info = vm->task_info;
> +
> + if (!task_info) {
> + task_info = kzalloc(sizeof(struct amdgpu_task_info),
> + GFP_KERNEL);
> + if (!task_info)
> + return;
>
> - if (vm->task_info->pid == current->pid)
> + kref_init(&task_info->refcount);
> + } else {
> return;
> + }
>
> - vm->task_info->pid = current->pid;
> - get_task_comm(vm->task_info->task_name, current);
> + task_info->pid = current->pid;
> + get_task_comm(task_info->task_name, current);
>
> - if (current->group_leader->mm != current->mm)
> - return;
> + if (current->group_leader->mm == current->mm) {
> + task_info->tgid = current->group_leader->pid;
> + get_task_comm(task_info->process_name, current->group_leader);
> + }
>
> - vm->task_info->tgid = current->group_leader->pid;
> - get_task_comm(vm->task_info->process_name, current->group_leader);
> + vm->task_info = task_info;
> }
>
> /**
>
> End result is code like this:
>
> void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
> {
> struct amdgpu_task_info *task_info = vm->task_info;
>
> if (!task_info) {
> task_info = kzalloc(sizeof(struct amdgpu_task_info),
> GFP_KERNEL);
> if (!task_info)
> return;
>
> kref_init(&task_info->refcount);
> } else {
> return;
> }
>
> task_info->pid = current->pid;
> get_task_comm(task_info->task_name, current);
>
> if (current->group_leader->mm == current->mm) {
> task_info->tgid = current->group_leader->pid;
> get_task_comm(task_info->process_name, current->group_leader);
> }
>
> vm->task_info = task_info;
> }
>
> ?
>
>> /**
>> @@ -2482,7 +2485,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>> if (r)
>> goto error_free_root;
>> - r = amdgpu_vm_create_task_info(vm);
>> if (r)
>> DRM_DEBUG("Failed to create task info for VM\n");
>
> Two more lines to delete here.
Done, thanks.
Pierre-Eric
>
> Regards,
>
> Tvrtko
>
>> @@ -2608,7 +2610,8 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>> root = amdgpu_bo_ref(vm->root.bo);
>> amdgpu_bo_reserve(root, true);
>> - amdgpu_vm_put_task_info(vm->task_info);
>> + if (vm->task_info)
>> + amdgpu_vm_put_task_info(vm->task_info);
>> amdgpu_vm_set_pasid(adev, vm, 0);
>> dma_fence_wait(vm->last_unlocked, false);
>> dma_fence_put(vm->last_unlocked);
More information about the amd-gfx
mailing list