[PATCH v3 4/6] drm/amdgpu: alloc and init vm::task_info from first submit

[Pierre-Eric Pelloux-Prayer]

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);