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

[Tvrtko Ursulin]

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?

> 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?

>   		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?

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.

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