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

[Christian König <christian.koenig@xxxxxxx>]

Am 03.10.24 um 14:43 schrieb Pierre-Eric Pelloux-Prayer:
> 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.
>
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 29 ++++++++++++--------------
>   1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index e20d19ae01b2..6cd5bd5362d4 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);
>   	}
> @@ -2361,6 +2361,10 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm)
>   	if (!vm->task_info)
>   		return -ENOMEM;
>   
> +	/* Set process attributes now. */
> +	vm->task_info->tgid = current->group_leader->pid;
> +	get_task_comm(vm->task_info->process_name, current->group_leader);
> +
>   	kref_init(&vm->task_info->refcount);
>   	return 0;
>   }
> @@ -2372,20 +2376,16 @@ 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->pid == current->pid)
> +	if (!vm->task_info) {
> +		if (amdgpu_vm_create_task_info(vm))
> +			return;
> +	} else if (vm->task_info->pid == current->pid) {
>   		return;
> +	}
>   
> +	/* Update task attributes. */
>   	vm->task_info->pid = current->pid;
>   	get_task_comm(vm->task_info->task_name, current);
> -
> -	if (current->group_leader->mm != current->mm)
> -		return;
> -
> -	vm->task_info->tgid = current->group_leader->pid;
> -	get_task_comm(vm->task_info->process_name, current->group_leader);
>   }
>   
>   /**
> @@ -2482,10 +2482,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");
> -
>   	amdgpu_bo_unreserve(vm->root.bo);
>   	amdgpu_bo_unref(&root_bo);
>   
> @@ -2608,7 +2604,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);

That NULL check should probably be moved into amdgpu_vm_put_task_info().

Apart from that the patch is Reviewed-by: Christian König 
<christian.koenig@xxxxxxx>.

Regards,
Christian.

>   	amdgpu_vm_set_pasid(adev, vm, 0);
>   	dma_fence_wait(vm->last_unlocked, false);
>   	dma_fence_put(vm->last_unlocked);