[PATCH v3 0/6] DRM_SET_NAME ioctl

[PATCH v3 0/6] DRM_SET_NAME ioctl

[Pierre-Eric Pelloux-Prayer]

v3 changelog:
* Added DRM_NAME_MAX_LEN instead of using NAME_MAX
* Fixed Tvrtko & Christian comments on patch 1
* Added R-b tags to patch 2
* Following Christian' suggestion patch 3 became 4 patches:
   - amdgpu_task_info::process_name is now a flexible array
   - amdgpu_vm::task_info is allocated on first submit
   - amdgpu_task_info::process_name was renamed process_desc,
     and includes drm_file::name when set. Like task_info
     allocation this is done only once.

Pierre-Eric Pelloux-Prayer (6):
  drm: add DRM_SET_NAME ioctl
  drm: use drm_file name in fdinfo
  drm/amdgpu: delay the use of amdgpu_vm_set_task_info
  drm/amdgpu: alloc and init vm::task_info from first submit
  drm/amdgpu: make process_name a flexible array
  drm/amdgpu: use drm_file::name in task_info::process_desc

 Documentation/gpu/drm-usage-stats.rst         |  5 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  1 +
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  3 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        |  6 +--
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c   |  2 +-
 .../gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c  |  4 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c       |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 48 +++++++++++++++----
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |  4 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c        |  2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c        |  2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c        |  2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c         |  2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c         |  2 +-
 drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c        |  2 +-
 drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c      |  2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_events.c       |  2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_process.c      |  3 ++
 drivers/gpu/drm/drm_debugfs.c                 | 12 +++--
 drivers/gpu/drm/drm_file.c                    | 10 ++++
 drivers/gpu/drm/drm_ioctl.c                   | 48 +++++++++++++++++++
 include/drm/drm_file.h                        |  9 ++++
 include/uapi/drm/drm.h                        | 17 +++++++
 23 files changed, 159 insertions(+), 31 deletions(-)

-- 
2.40.1

[PATCH v3 1/6] drm: add DRM_SET_NAME ioctl

[Pierre-Eric Pelloux-Prayer]

Giving the opportunity to userspace to associate a free-form
name with a drm_file struct is helpful for tracking and debugging.

This is similar to the existing DMA_BUF_SET_NAME ioctl.

Access to name is protected by a mutex, and the 'clients' debugfs
file has been updated to print it.

Userspace MR to use this ioctl:
   https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1428

The string passed by userspace is filtered a bit, to avoid messing
output when it's going to be printed (in dmesg, fdinfo, etc):
  * all chars failing isgraph() are replaced by '-'
  * if a 0-length string is passed the name is cleared

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 drivers/gpu/drm/drm_debugfs.c | 12 ++++++---
 drivers/gpu/drm/drm_file.c    |  5 ++++
 drivers/gpu/drm/drm_ioctl.c   | 48 +++++++++++++++++++++++++++++++++++
 include/drm/drm_file.h        |  9 +++++++
 include/uapi/drm/drm.h        | 17 +++++++++++++
 5 files changed, 87 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 6b239a24f1df..482e71160544 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -78,12 +78,13 @@ static int drm_clients_info(struct seq_file *m, void *data)
 	kuid_t uid;
 
 	seq_printf(m,
-		   "%20s %5s %3s master a %5s %10s\n",
+		   "%20s %5s %3s master a %5s %10s %20s\n",
 		   "command",
 		   "tgid",
 		   "dev",
 		   "uid",
-		   "magic");
+		   "magic",
+		   "name");
 
 	/* dev->filelist is sorted youngest first, but we want to present
 	 * oldest first (i.e. kernel, servers, clients), so walk backwardss.
@@ -94,19 +95,22 @@ static int drm_clients_info(struct seq_file *m, void *data)
 		struct task_struct *task;
 		struct pid *pid;
 
+		mutex_lock(&priv->name_lock);
 		rcu_read_lock(); /* Locks priv->pid and pid_task()->comm! */
 		pid = rcu_dereference(priv->pid);
 		task = pid_task(pid, PIDTYPE_TGID);
 		uid = task ? __task_cred(task)->euid : GLOBAL_ROOT_UID;
-		seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u\n",
+		seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u %20s\n",
 			   task ? task->comm : "<unknown>",
 			   pid_vnr(pid),
 			   priv->minor->index,
 			   is_current_master ? 'y' : 'n',
 			   priv->authenticated ? 'y' : 'n',
 			   from_kuid_munged(seq_user_ns(m), uid),
-			   priv->magic);
+			   priv->magic,
+			   priv->name ?: "");
 		rcu_read_unlock();
+		mutex_unlock(&priv->name_lock);
 	}
 	mutex_unlock(&dev->filelist_mutex);
 	return 0;
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 01fde94fe2a9..e9dd0e90a1f9 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -158,6 +158,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
 
 	spin_lock_init(&file->master_lookup_lock);
 	mutex_init(&file->event_read_lock);
+	mutex_init(&file->name_lock);
 
 	if (drm_core_check_feature(dev, DRIVER_GEM))
 		drm_gem_open(dev, file);
@@ -259,6 +260,10 @@ void drm_file_free(struct drm_file *file)
 	WARN_ON(!list_empty(&file->event_list));
 
 	put_pid(rcu_access_pointer(file->pid));
+
+	mutex_destroy(&file->name_lock);
+	kfree(file->name);
+
 	kfree(file);
 }
 
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 51f39912866f..46dadbd1bb35 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -540,6 +540,52 @@ int drm_version(struct drm_device *dev, void *data,
 	return err;
 }
 
+static int drm_set_name(struct drm_device *dev, void *data,
+			struct drm_file *file_priv)
+{
+	struct drm_set_name *name = data;
+	void __user *user_ptr;
+	char *new_name;
+	size_t i, len;
+
+	if (name->name_len > DRM_NAME_MAX_LEN)
+		return -EINVAL;
+
+	user_ptr = u64_to_user_ptr(name->name);
+
+	new_name = memdup_user_nul(user_ptr, name->name_len);
+	if (IS_ERR(new_name))
+		return PTR_ERR(new_name);
+
+	len = strlen(new_name);
+
+	if (len != name->name_len) {
+		kfree(new_name);
+		return -EINVAL;
+	}
+
+	/*
+	 * Filter out control char / spaces / new lines etc in the name
+	 * since it's going to be used in dmesg or fdinfo's output.
+	 */
+	for (i = 0; i < len; i++) {
+		if (!isgraph(new_name[i]))
+			new_name[i] = '-';
+	}
+
+	mutex_lock(&file_priv->name_lock);
+	kfree(file_priv->name);
+	if (len > 0) {
+		file_priv->name = new_name;
+	} else {
+		kfree(new_name);
+		file_priv->name = NULL;
+	}
+	mutex_unlock(&file_priv->name_lock);
+
+	return 0;
+}
+
 static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
 {
 	/* ROOT_ONLY is only for CAP_SYS_ADMIN */
@@ -610,6 +656,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
 
+	DRM_IOCTL_DEF(DRM_IOCTL_SET_NAME, drm_set_name, DRM_RENDER_ALLOW),
+
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETCRTC, drm_mode_setcrtc, DRM_MASTER),
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 8c0030c77308..df26eee8f79c 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -388,6 +388,15 @@ struct drm_file {
 	 * Per-file buffer caches used by the PRIME buffer sharing code.
 	 */
 	struct drm_prime_file_private prime;
+
+	/**
+	 * @name:
+	 *
+	 * Userspace-provided name; useful for accounting and debugging.
+	 */
+	const char *name;
+	/** @name_lock: Protects @name. */
+	struct mutex name_lock;
 };
 
 /**
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 16122819edfe..f5e92e4f909b 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -1024,6 +1024,13 @@ struct drm_crtc_queue_sequence {
 	__u64 user_data;	/* user data passed to event */
 };
 
+#define DRM_NAME_MAX_LEN	64
+struct drm_set_name {
+	__u64 name_len;
+	__u64 name;
+};
+
+
 #if defined(__cplusplus)
 }
 #endif
@@ -1288,6 +1295,16 @@ extern "C" {
  */
 #define DRM_IOCTL_MODE_CLOSEFB		DRM_IOWR(0xD0, struct drm_mode_closefb)
 
+/**
+ * DRM_IOCTL_SET_NAME - Attach a name to a drm_file
+ *
+ * This ioctl is similar to DMA_BUF_SET_NAME - it allows for easier tracking
+ * and debugging.
+ * The length of the name must <= DRM_NAME_MAX_LEN. All characters that are
+ * non-printable or whitespaces will be replaced by -.
+ */
+#define DRM_IOCTL_SET_NAME		DRM_IOWR(0xD1, struct drm_set_name)
+
 /*
  * Device specific ioctls should only be in their respective headers
  * The device specific ioctl range is from 0x40 to 0x9f.
-- 
2.40.1

[PATCH v3 2/6] drm: use drm_file name in fdinfo

[Pierre-Eric Pelloux-Prayer]

Add an optional drm-client-name field to drm fdinfo's output.

Reviewed-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 Documentation/gpu/drm-usage-stats.rst | 5 +++++
 drivers/gpu/drm/drm_file.c            | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
index a80f95ca1b2f..ed1d7edbbc5f 100644
--- a/Documentation/gpu/drm-usage-stats.rst
+++ b/Documentation/gpu/drm-usage-stats.rst
@@ -73,6 +73,11 @@ scope of each device, in which case `drm-pdev` shall be present as well.
 Userspace should make sure to not double account any usage statistics by using
 the above described criteria in order to associate data to individual clients.
 
+- drm-client-name: <valstr>
+
+String optionally set by userspace using DRM_IOCTL_SET_NAME.
+
+
 Utilization
 ^^^^^^^^^^^
 
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index e9dd0e90a1f9..6a3621f50784 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -955,6 +955,11 @@ void drm_show_fdinfo(struct seq_file *m, struct file *f)
 			   PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
 	}
 
+	mutex_lock(&file->name_lock);
+	if (file->name)
+		drm_printf(&p, "drm-client-name:\t%s\n", file->name);
+	mutex_unlock(&file->name_lock);
+
 	if (dev->driver->show_fdinfo)
 		dev->driver->show_fdinfo(&p, file);
 }
-- 
2.40.1

[PATCH v3 3/6] drm/amdgpu: delay the use of amdgpu_vm_set_task_info

[Pierre-Eric Pelloux-Prayer]

At this point the vm is locked so we safely modify it without risk of
concurrent access.

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 1e475eb01417..891128ecee6d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -309,9 +309,6 @@ static int amdgpu_cs_pass1(struct amdgpu_cs_parser *p,
 		p->gang_leader->uf_addr = uf_offset;
 	kvfree(chunk_array);
 
-	/* Use this opportunity to fill in task info for the vm */
-	amdgpu_vm_set_task_info(vm);
-
 	return 0;
 
 free_all_kdata:
@@ -1180,6 +1177,9 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 		job->vm_pd_addr = amdgpu_gmc_pd_addr(vm->root.bo);
 	}
 
+	/* Use this opportunity to fill in task info for the vm */
+	amdgpu_vm_set_task_info(vm);
+
 	if (adev->debug_vm) {
 		/* Invalidate all BOs to test for userspace bugs */
 		amdgpu_bo_list_for_each_entry(e, p->bo_list) {
-- 
2.40.1

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

[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@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)
 		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);
 }
 
 /**
@@ -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");
 
@@ -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);
-- 
2.40.1

[PATCH v3 5/6] drm/amdgpu: make process_name a flexible array

[Pierre-Eric Pelloux-Prayer]

And rename it process_desc, since it will soon contain more than
just the process_name.

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c      |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c |  4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c          |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c           | 15 ++++++++++++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h           |  2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c           |  2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c           |  2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c           |  2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c            |  2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c            |  2 +-
 drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c           |  2 +-
 drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c         |  2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_events.c          |  2 +-
 13 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index cbef720de779..c2185e43e38d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1786,7 +1786,7 @@ static int amdgpu_debugfs_vm_info_show(struct seq_file *m, void *unused)
 
 		ti = amdgpu_vm_get_task_info_vm(vm);
 		if (ti) {
-			seq_printf(m, "pid:%d\tProcess:%s ----------\n", ti->pid, ti->process_name);
+			seq_printf(m, "pid:%d\tProcess:%s ----------\n", ti->pid, ti->process_desc);
 			amdgpu_vm_put_task_info(ti);
 		}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c
index 5ac59b62020c..4ca0a372984b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c
@@ -220,8 +220,8 @@ amdgpu_devcoredump_read(char *buffer, loff_t offset, size_t count,
 		   coredump->reset_time.tv_nsec);
 
 	if (coredump->reset_task_info.pid)
-		drm_printf(&p, "process_name: %s PID: %d\n",
-			   coredump->reset_task_info.process_name,
+		drm_printf(&p, "process: %s PID: %d\n",
+			   coredump->reset_task_info.process_desc,
 			   coredump->reset_task_info.pid);
 
 	/* SOC Information */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index ad6bf5d4e0a9..d1678eebbff3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -125,7 +125,7 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct drm_sched_job *s_job)
 	if (ti) {
 		dev_err(adev->dev,
 			"Process information: process %s pid %d thread %s pid %d\n",
-			ti->process_name, ti->tgid, ti->task_name, ti->pid);
+			ti->process_desc, ti->tgid, ti->task_name, ti->pid);
 		amdgpu_vm_put_task_info(ti);
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 690676cab022..7f852029d6e1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2357,10 +2357,21 @@ amdgpu_vm_get_task_info_pasid(struct amdgpu_device *adev, u32 pasid)
 
 static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm)
 {
-	vm->task_info = kzalloc(sizeof(struct amdgpu_task_info), GFP_KERNEL);
+	char process_name[TASK_COMM_LEN];
+	int desc_len;
+
+	get_task_comm(process_name, current->group_leader);
+	desc_len = strlen(process_name);
+
+	vm->task_info = kzalloc(
+		struct_size(vm->task_info, process_desc, desc_len + 1),
+		GFP_KERNEL);
+
 	if (!vm->task_info)
 		return -ENOMEM;
 
+	strscpy(vm->task_info->process_desc, process_name, desc_len + 1);
+
 	kref_init(&vm->task_info->refcount);
 	return 0;
 }
@@ -2375,8 +2386,6 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
 	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)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index d12d66dca8e9..44da250217be 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -232,11 +232,11 @@ struct amdgpu_vm_pte_funcs {
 };
 
 struct amdgpu_task_info {
-	char		process_name[TASK_COMM_LEN];
 	char		task_name[TASK_COMM_LEN];
 	pid_t		pid;
 	pid_t		tgid;
 	struct kref	refcount;
+	char		process_desc[];
 };
 
 /**
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
index 9784a2892185..c82364e43a15 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
@@ -166,7 +166,7 @@ static int gmc_v10_0_process_interrupt(struct amdgpu_device *adev,
 	if (task_info) {
 		dev_err(adev->dev,
 			" in process %s pid %d thread %s pid %d\n",
-			task_info->process_name, task_info->tgid,
+			task_info->process_desc, task_info->tgid,
 			task_info->task_name, task_info->pid);
 		amdgpu_vm_put_task_info(task_info);
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
index 2797fd84432b..4dace3de1def 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
@@ -136,7 +136,7 @@ static int gmc_v11_0_process_interrupt(struct amdgpu_device *adev,
 		if (task_info) {
 			dev_err(adev->dev,
 				" in process %s pid %d thread %s pid %d)\n",
-				task_info->process_name, task_info->tgid,
+				task_info->process_desc, task_info->tgid,
 				task_info->task_name, task_info->pid);
 			amdgpu_vm_put_task_info(task_info);
 		}
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c
index edcb5351f8cc..e56d702fbfed 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c
@@ -129,7 +129,7 @@ static int gmc_v12_0_process_interrupt(struct amdgpu_device *adev,
 		if (task_info) {
 			dev_err(adev->dev,
 				" in process %s pid %d thread %s pid %d)\n",
-				task_info->process_name, task_info->tgid,
+				task_info->process_desc, task_info->tgid,
 				task_info->task_name, task_info->pid);
 			amdgpu_vm_put_task_info(task_info);
 		}
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
index 86488c052f82..5fca64e71ada 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
@@ -1451,7 +1451,7 @@ static int gmc_v8_0_process_interrupt(struct amdgpu_device *adev,
 		task_info = amdgpu_vm_get_task_info_pasid(adev, entry->pasid);
 		if (task_info) {
 			dev_err(adev->dev, " for process %s pid %d thread %s pid %d\n",
-				task_info->process_name, task_info->tgid,
+				task_info->process_desc, task_info->tgid,
 				task_info->task_name, task_info->pid);
 			amdgpu_vm_put_task_info(task_info);
 		}
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index c76ac0dfe572..c54c86dac14f 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -635,7 +635,7 @@ static int gmc_v9_0_process_interrupt(struct amdgpu_device *adev,
 	if (task_info) {
 		dev_err(adev->dev,
 			" for process %s pid %d thread %s pid %d)\n",
-			task_info->process_name, task_info->tgid,
+			task_info->process_desc, task_info->tgid,
 			task_info->task_name, task_info->pid);
 		amdgpu_vm_put_task_info(task_info);
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
index 23ef4eb36b40..ea1990c19803 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
@@ -2186,7 +2186,7 @@ static int sdma_v4_0_print_iv_entry(struct amdgpu_device *adev,
 	if (task_info) {
 		dev_dbg_ratelimited(adev->dev,
 				    " for process %s pid %d thread %s pid %d\n",
-				    task_info->process_name, task_info->tgid,
+				    task_info->process_desc, task_info->tgid,
 				    task_info->task_name, task_info->pid);
 		amdgpu_vm_put_task_info(task_info);
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
index c77889040760..df00bf480dcf 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
@@ -1701,7 +1701,7 @@ static int sdma_v4_4_2_print_iv_entry(struct amdgpu_device *adev,
 	task_info = amdgpu_vm_get_task_info_pasid(adev, entry->pasid);
 	if (task_info) {
 		dev_dbg_ratelimited(adev->dev, " for process %s pid %d thread %s pid %d\n",
-				    task_info->process_name, task_info->tgid,
+				    task_info->process_desc, task_info->tgid,
 				    task_info->task_name, task_info->pid);
 		amdgpu_vm_put_task_info(task_info);
 	}
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index ea3792249209..95ba07ae3b89 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -1267,7 +1267,7 @@ void kfd_signal_reset_event(struct kfd_node *dev)
 			if (ti) {
 				dev_err(dev->adev->dev,
 					"Queues reset on process %s tid %d thread %s pid %d\n",
-					ti->process_name, ti->tgid, ti->task_name, ti->pid);
+					ti->process_desc, ti->tgid, ti->task_name, ti->pid);
 				amdgpu_vm_put_task_info(ti);
 			}
 		}
-- 
2.40.1

[PATCH v3 6/6] drm/amdgpu: use drm_file::name in task_info::process_desc

[Pierre-Eric Pelloux-Prayer]

If a drm_file name is set append it to the process name.

This information is useful with the virtio/native-context driver: this
allows the guest applications identifier to visible in amdgpu's output.

The output in amdgpu_vm_info/amdgpu_gem_info looks like this:
   pid:12255	Process:glxgears/test-set-fd-name ----------

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  1 +
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  3 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 26 ++++++++++++++++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |  2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_process.c      |  3 +++
 6 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index f9d119448442..ad909173e419 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -299,6 +299,7 @@ int amdgpu_amdkfd_gpuvm_set_vm_pasid(struct amdgpu_device *adev,
 				     struct amdgpu_vm *avm, u32 pasid);
 int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct amdgpu_device *adev,
 					struct amdgpu_vm *avm,
+					struct drm_file *filp,
 					void **process_info,
 					struct dma_fence **ef);
 void amdgpu_amdkfd_gpuvm_release_process_vm(struct amdgpu_device *adev,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 6d5fd371d5ce..172882af6705 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1558,6 +1558,7 @@ int amdgpu_amdkfd_gpuvm_set_vm_pasid(struct amdgpu_device *adev,
 
 int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct amdgpu_device *adev,
 					   struct amdgpu_vm *avm,
+					   struct drm_file *filp,
 					   void **process_info,
 					   struct dma_fence **ef)
 {
@@ -1577,7 +1578,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct amdgpu_device *adev,
 	if (ret)
 		return ret;
 
-	amdgpu_vm_set_task_info(avm);
+	amdgpu_vm_set_task_info(avm, filp);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 891128ecee6d..5d43e24906d2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1178,7 +1178,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 	}
 
 	/* Use this opportunity to fill in task info for the vm */
-	amdgpu_vm_set_task_info(vm);
+	amdgpu_vm_set_task_info(vm, p->filp);
 
 	if (adev->debug_vm) {
 		/* Invalidate all BOs to test for userspace bugs */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 7f852029d6e1..a2b12f0c3253 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2355,14 +2355,20 @@ 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)
+static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm, struct drm_file *filp)
 {
 	char process_name[TASK_COMM_LEN];
-	int desc_len;
+	size_t desc_len;
 
 	get_task_comm(process_name, current->group_leader);
 	desc_len = strlen(process_name);
 
+	if (filp) {
+		mutex_lock(&filp->name_lock);
+		if (filp->name)
+			desc_len += 1 + strlen(filp->name);
+	}
+
 	vm->task_info = kzalloc(
 		struct_size(vm->task_info, process_desc, desc_len + 1),
 		GFP_KERNEL);
@@ -2371,6 +2377,17 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm)
 		return -ENOMEM;
 
 	strscpy(vm->task_info->process_desc, process_name, desc_len + 1);
+	if (filp) {
+		if (filp->name) {
+			size_t p_len = strlen(process_name);
+
+			vm->task_info->process_desc[p_len] = '/';
+			strscpy(&vm->task_info->process_desc[p_len + 1],
+				filp->name, (desc_len + 1) - (p_len + 1));
+		}
+		mutex_unlock(&filp->name_lock);
+	}
+
 
 	kref_init(&vm->task_info->refcount);
 	return 0;
@@ -2380,11 +2397,12 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm)
  * amdgpu_vm_set_task_info - Sets VMs task info.
  *
  * @vm: vm for which to set the info
+ * @filp: drm_file instance
  */
-void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
+void amdgpu_vm_set_task_info(struct amdgpu_vm *vm, struct drm_file *filp)
 {
 	if (!vm->task_info) {
-		if (amdgpu_vm_create_task_info(vm))
+		if (amdgpu_vm_create_task_info(vm, filp))
 			return;
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 44da250217be..8df3dece54c2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -561,7 +561,7 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
 			    u32 vmid, u32 node_id, uint64_t addr, uint64_t ts,
 			    bool write_fault);
 
-void amdgpu_vm_set_task_info(struct amdgpu_vm *vm);
+void amdgpu_vm_set_task_info(struct amdgpu_vm *vm, struct drm_file *filp);
 
 void amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev,
 				struct amdgpu_vm *vm);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index a902950cc060..e473fe433d3f 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -1654,6 +1654,7 @@ int kfd_process_device_init_vm(struct kfd_process_device *pdd,
 			       struct file *drm_file)
 {
 	struct amdgpu_fpriv *drv_priv;
+	struct drm_file *filp;
 	struct amdgpu_vm *avm;
 	struct kfd_process *p;
 	struct dma_fence *ef;
@@ -1673,8 +1674,10 @@ int kfd_process_device_init_vm(struct kfd_process_device *pdd,
 
 	p = pdd->process;
 	dev = pdd->dev;
+	filp = drm_file->private_data;
 
 	ret = amdgpu_amdkfd_gpuvm_acquire_process_vm(dev->adev, avm,
+						     filp,
 						     &p->kgd_process_info,
 						     &ef);
 	if (ret) {
-- 
2.40.1

Re: [PATCH v3 6/6] drm/amdgpu: use drm_file::name in task_info::process_desc

[Christian König]

Am 20.09.24 um 11:06 schrieb Pierre-Eric Pelloux-Prayer:
> If a drm_file name is set append it to the process name.
>
> This information is useful with the virtio/native-context driver: this
> allows the guest applications identifier to visible in amdgpu's output.
>
> The output in amdgpu_vm_info/amdgpu_gem_info looks like this:
>     pid:12255	Process:glxgears/test-set-fd-name ----------
>
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  1 +
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  3 ++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 26 ++++++++++++++++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |  2 +-
>   drivers/gpu/drm/amd/amdkfd/kfd_process.c      |  3 +++
>   6 files changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index f9d119448442..ad909173e419 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -299,6 +299,7 @@ int amdgpu_amdkfd_gpuvm_set_vm_pasid(struct amdgpu_device *adev,
>   				     struct amdgpu_vm *avm, u32 pasid);
>   int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct amdgpu_device *adev,
>   					struct amdgpu_vm *avm,
> +					struct drm_file *filp,
>   					void **process_info,
>   					struct dma_fence **ef);
>   void amdgpu_amdkfd_gpuvm_release_process_vm(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 6d5fd371d5ce..172882af6705 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1558,6 +1558,7 @@ int amdgpu_amdkfd_gpuvm_set_vm_pasid(struct amdgpu_device *adev,
>   
>   int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct amdgpu_device *adev,
>   					   struct amdgpu_vm *avm,
> +					   struct drm_file *filp,
>   					   void **process_info,
>   					   struct dma_fence **ef)
>   {
> @@ -1577,7 +1578,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct amdgpu_device *adev,
>   	if (ret)
>   		return ret;
>   
> -	amdgpu_vm_set_task_info(avm);
> +	amdgpu_vm_set_task_info(avm, filp);
>   
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 891128ecee6d..5d43e24906d2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1178,7 +1178,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>   	}
>   
>   	/* Use this opportunity to fill in task info for the vm */
> -	amdgpu_vm_set_task_info(vm);
> +	amdgpu_vm_set_task_info(vm, p->filp);
>   
>   	if (adev->debug_vm) {
>   		/* Invalidate all BOs to test for userspace bugs */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 7f852029d6e1..a2b12f0c3253 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2355,14 +2355,20 @@ 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)
> +static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm, struct drm_file *filp)
>   {
>   	char process_name[TASK_COMM_LEN];
> -	int desc_len;
> +	size_t desc_len;
>   
>   	get_task_comm(process_name, current->group_leader);
>   	desc_len = strlen(process_name);
>   
> +	if (filp) {
> +		mutex_lock(&filp->name_lock);
> +		if (filp->name)
> +			desc_len += 1 + strlen(filp->name);
> +	}
> +
>   	vm->task_info = kzalloc(
>   		struct_size(vm->task_info, process_desc, desc_len + 1),
>   		GFP_KERNEL);
> @@ -2371,6 +2377,17 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm)
>   		return -ENOMEM;
>   
>   	strscpy(vm->task_info->process_desc, process_name, desc_len + 1);
> +	if (filp) {

I think we can drop those checks now, filp is now a mandatory argument.

Apart from that the series looks good to me.

Regards,
Christian.

> +		if (filp->name) {
> +			size_t p_len = strlen(process_name);
> +
> +			vm->task_info->process_desc[p_len] = '/';
> +			strscpy(&vm->task_info->process_desc[p_len + 1],
> +				filp->name, (desc_len + 1) - (p_len + 1));
> +		}
> +		mutex_unlock(&filp->name_lock);
> +	}
> +
>   
>   	kref_init(&vm->task_info->refcount);
>   	return 0;
> @@ -2380,11 +2397,12 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm)
>    * amdgpu_vm_set_task_info - Sets VMs task info.
>    *
>    * @vm: vm for which to set the info
> + * @filp: drm_file instance
>    */
> -void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
> +void amdgpu_vm_set_task_info(struct amdgpu_vm *vm, struct drm_file *filp)
>   {
>   	if (!vm->task_info) {
> -		if (amdgpu_vm_create_task_info(vm))
> +		if (amdgpu_vm_create_task_info(vm, filp))
>   			return;
>   	}
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 44da250217be..8df3dece54c2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -561,7 +561,7 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
>   			    u32 vmid, u32 node_id, uint64_t addr, uint64_t ts,
>   			    bool write_fault);
>   
> -void amdgpu_vm_set_task_info(struct amdgpu_vm *vm);
> +void amdgpu_vm_set_task_info(struct amdgpu_vm *vm, struct drm_file *filp);
>   
>   void amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev,
>   				struct amdgpu_vm *vm);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index a902950cc060..e473fe433d3f 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -1654,6 +1654,7 @@ int kfd_process_device_init_vm(struct kfd_process_device *pdd,
>   			       struct file *drm_file)
>   {
>   	struct amdgpu_fpriv *drv_priv;
> +	struct drm_file *filp;
>   	struct amdgpu_vm *avm;
>   	struct kfd_process *p;
>   	struct dma_fence *ef;
> @@ -1673,8 +1674,10 @@ int kfd_process_device_init_vm(struct kfd_process_device *pdd,
>   
>   	p = pdd->process;
>   	dev = pdd->dev;
> +	filp = drm_file->private_data;
>   
>   	ret = amdgpu_amdkfd_gpuvm_acquire_process_vm(dev->adev, avm,
> +						     filp,
>   						     &p->kgd_process_info,
>   						     &ef);
>   	if (ret) {

Re: [PATCH v3 1/6] drm: add DRM_SET_NAME ioctl

[Tvrtko Ursulin]

On 20/09/2024 10:06, Pierre-Eric Pelloux-Prayer wrote:
> Giving the opportunity to userspace to associate a free-form
> name with a drm_file struct is helpful for tracking and debugging.
> 
> This is similar to the existing DMA_BUF_SET_NAME ioctl.
> 
> Access to name is protected by a mutex, and the 'clients' debugfs
> file has been updated to print it.
> 
> Userspace MR to use this ioctl:
>     https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1428
> 
> The string passed by userspace is filtered a bit, to avoid messing
> output when it's going to be printed (in dmesg, fdinfo, etc):
>    * all chars failing isgraph() are replaced by '-'
>    * if a 0-length string is passed the name is cleared
> 
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> ---
>   drivers/gpu/drm/drm_debugfs.c | 12 ++++++---
>   drivers/gpu/drm/drm_file.c    |  5 ++++
>   drivers/gpu/drm/drm_ioctl.c   | 48 +++++++++++++++++++++++++++++++++++
>   include/drm/drm_file.h        |  9 +++++++
>   include/uapi/drm/drm.h        | 17 +++++++++++++
>   5 files changed, 87 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 6b239a24f1df..482e71160544 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -78,12 +78,13 @@ static int drm_clients_info(struct seq_file *m, void *data)
>   	kuid_t uid;
>   
>   	seq_printf(m,
> -		   "%20s %5s %3s master a %5s %10s\n",
> +		   "%20s %5s %3s master a %5s %10s %20s\n",

Allow full DRM_NAME_MAX_LEN? Not sure, feels not very consequential 
either way.

>   		   "command",
>   		   "tgid",
>   		   "dev",
>   		   "uid",
> -		   "magic");
> +		   "magic",
> +		   "name");
>   
>   	/* dev->filelist is sorted youngest first, but we want to present
>   	 * oldest first (i.e. kernel, servers, clients), so walk backwardss.
> @@ -94,19 +95,22 @@ static int drm_clients_info(struct seq_file *m, void *data)
>   		struct task_struct *task;
>   		struct pid *pid;
>   
> +		mutex_lock(&priv->name_lock);
>   		rcu_read_lock(); /* Locks priv->pid and pid_task()->comm! */
>   		pid = rcu_dereference(priv->pid);
>   		task = pid_task(pid, PIDTYPE_TGID);
>   		uid = task ? __task_cred(task)->euid : GLOBAL_ROOT_UID;
> -		seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u\n",
> +		seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u %20s\n",
>   			   task ? task->comm : "<unknown>",
>   			   pid_vnr(pid),
>   			   priv->minor->index,
>   			   is_current_master ? 'y' : 'n',
>   			   priv->authenticated ? 'y' : 'n',
>   			   from_kuid_munged(seq_user_ns(m), uid),
> -			   priv->magic);
> +			   priv->magic,
> +			   priv->name ?: "");
>   		rcu_read_unlock();
> +		mutex_unlock(&priv->name_lock);
>   	}
>   	mutex_unlock(&dev->filelist_mutex);
>   	return 0;
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 01fde94fe2a9..e9dd0e90a1f9 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -158,6 +158,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>   
>   	spin_lock_init(&file->master_lookup_lock);
>   	mutex_init(&file->event_read_lock);
> +	mutex_init(&file->name_lock);
>   
>   	if (drm_core_check_feature(dev, DRIVER_GEM))
>   		drm_gem_open(dev, file);
> @@ -259,6 +260,10 @@ void drm_file_free(struct drm_file *file)
>   	WARN_ON(!list_empty(&file->event_list));
>   
>   	put_pid(rcu_access_pointer(file->pid));
> +
> +	mutex_destroy(&file->name_lock);
> +	kfree(file->name);
> +
>   	kfree(file);
>   }
>   
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 51f39912866f..46dadbd1bb35 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -540,6 +540,52 @@ int drm_version(struct drm_device *dev, void *data,
>   	return err;
>   }
>   
> +static int drm_set_name(struct drm_device *dev, void *data,
> +			struct drm_file *file_priv)
> +{
> +	struct drm_set_name *name = data;
> +	void __user *user_ptr;
> +	char *new_name;
> +	size_t i, len;
> +
> +	if (name->name_len > DRM_NAME_MAX_LEN)
> +		return -EINVAL;
> +
> +	user_ptr = u64_to_user_ptr(name->name);
> +
> +	new_name = memdup_user_nul(user_ptr, name->name_len);
> +	if (IS_ERR(new_name))
> +		return PTR_ERR(new_name);
> +
> +	len = strlen(new_name);
> +
> +	if (len != name->name_len) {
> +		kfree(new_name);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Filter out control char / spaces / new lines etc in the name
> +	 * since it's going to be used in dmesg or fdinfo's output.
> +	 */
> +	for (i = 0; i < len; i++) {
> +		if (!isgraph(new_name[i]))
> +			new_name[i] = '-';
> +	}
> +
> +	mutex_lock(&file_priv->name_lock);
> +	kfree(file_priv->name);
> +	if (len > 0) {
> +		file_priv->name = new_name;
> +	} else {
> +		kfree(new_name);
> +		file_priv->name = NULL;
> +	}
> +	mutex_unlock(&file_priv->name_lock);
> +
> +	return 0;
> +}
> +
>   static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
>   {
>   	/* ROOT_ONLY is only for CAP_SYS_ADMIN */
> @@ -610,6 +656,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>   	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
>   	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
>   
> +	DRM_IOCTL_DEF(DRM_IOCTL_SET_NAME, drm_set_name, DRM_RENDER_ALLOW),
> +
>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0),
>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETCRTC, drm_mode_setcrtc, DRM_MASTER),
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 8c0030c77308..df26eee8f79c 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -388,6 +388,15 @@ struct drm_file {
>   	 * Per-file buffer caches used by the PRIME buffer sharing code.
>   	 */
>   	struct drm_prime_file_private prime;
> +
> +	/**
> +	 * @name:
> +	 *
> +	 * Userspace-provided name; useful for accounting and debugging.
> +	 */
> +	const char *name;
> +	/** @name_lock: Protects @name. */
> +	struct mutex name_lock;
>   };
>   
>   /**
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 16122819edfe..f5e92e4f909b 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -1024,6 +1024,13 @@ struct drm_crtc_queue_sequence {
>   	__u64 user_data;	/* user data passed to event */
>   };
>   
> +#define DRM_NAME_MAX_LEN	64
> +struct drm_set_name {
> +	__u64 name_len;
> +	__u64 name;
> +};
> +
> +
>   #if defined(__cplusplus)
>   }
>   #endif
> @@ -1288,6 +1295,16 @@ extern "C" {
>    */
>   #define DRM_IOCTL_MODE_CLOSEFB		DRM_IOWR(0xD0, struct drm_mode_closefb)
>   
> +/**
> + * DRM_IOCTL_SET_NAME - Attach a name to a drm_file
> + *
> + * This ioctl is similar to DMA_BUF_SET_NAME - it allows for easier tracking
> + * and debugging.
> + * The length of the name must <= DRM_NAME_MAX_LEN. All characters that are
> + * non-printable or whitespaces will be replaced by -.
> + */
> +#define DRM_IOCTL_SET_NAME		DRM_IOWR(0xD1, struct drm_set_name)
> +

A comment, nice! :) Overal looks good to me.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

I do however wish for more opinions (before merging) on whether strings 
with invalid characters should perhaps instead be rejected. I don't 
currently have a solid argument either way.

Perhaps the only argument against silent transformation is if someone 
sets some wild string, then greps for it somewhere, which would be a 
false negative without the understanding of what kind of remapping 
kernel does. It is weak but it is uapi so worth discussing every crazy 
possibility I think.

On the other hand it would create another annoying source of EINVAL. :shrug:

Also, how are with with testing the DRM core features? Add something for 
the uapi in IGT/tests/drm_client_name, or some such?

Regards,

Tvrtko

>   /*
>    * Device specific ioctls should only be in their respective headers
>    * The device specific ioctl range is from 0x40 to 0x9f.

Re: [PATCH v3 3/6] drm/amdgpu: delay the use of amdgpu_vm_set_task_info

[Tvrtko Ursulin]

On 20/09/2024 10:06, Pierre-Eric Pelloux-Prayer wrote:
> At this point the vm is locked so we safely modify it without risk of
> concurrent access.

To which particular lock this is referring to and does this imply 
previous placement was unsafe?

Regards,

Tvrtko

> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 1e475eb01417..891128ecee6d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -309,9 +309,6 @@ static int amdgpu_cs_pass1(struct amdgpu_cs_parser *p,
>   		p->gang_leader->uf_addr = uf_offset;
>   	kvfree(chunk_array);
>   
> -	/* Use this opportunity to fill in task info for the vm */
> -	amdgpu_vm_set_task_info(vm);
> -
>   	return 0;
>   
>   free_all_kdata:
> @@ -1180,6 +1177,9 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>   		job->vm_pd_addr = amdgpu_gmc_pd_addr(vm->root.bo);
>   	}
>   
> +	/* Use this opportunity to fill in task info for the vm */
> +	amdgpu_vm_set_task_info(vm);
> +
>   	if (adev->debug_vm) {
>   		/* Invalidate all BOs to test for userspace bugs */
>   		amdgpu_bo_list_for_each_entry(e, p->bo_list) {

Re: [PATCH v3 1/6] drm: add DRM_SET_NAME ioctl

[Dmitry Osipenko]

On 9/20/24 12:06, Pierre-Eric Pelloux-Prayer wrote:
> Giving the opportunity to userspace to associate a free-form
> name with a drm_file struct is helpful for tracking and debugging.
> 
> This is similar to the existing DMA_BUF_SET_NAME ioctl.
> 
> Access to name is protected by a mutex, and the 'clients' debugfs
> file has been updated to print it.
> 
> Userspace MR to use this ioctl:
>    https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1428
> 
> The string passed by userspace is filtered a bit, to avoid messing
> output when it's going to be printed (in dmesg, fdinfo, etc):
>   * all chars failing isgraph() are replaced by '-'
>   * if a 0-length string is passed the name is cleared
> 
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> ---
>  drivers/gpu/drm/drm_debugfs.c | 12 ++++++---
>  drivers/gpu/drm/drm_file.c    |  5 ++++
>  drivers/gpu/drm/drm_ioctl.c   | 48 +++++++++++++++++++++++++++++++++++
>  include/drm/drm_file.h        |  9 +++++++
>  include/uapi/drm/drm.h        | 17 +++++++++++++
>  5 files changed, 87 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 6b239a24f1df..482e71160544 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -78,12 +78,13 @@ static int drm_clients_info(struct seq_file *m, void *data)
>  	kuid_t uid;
>  
>  	seq_printf(m,
> -		   "%20s %5s %3s master a %5s %10s\n",
> +		   "%20s %5s %3s master a %5s %10s %20s\n",
>  		   "command",
>  		   "tgid",
>  		   "dev",
>  		   "uid",
> -		   "magic");
> +		   "magic",
> +		   "name");
>  
>  	/* dev->filelist is sorted youngest first, but we want to present
>  	 * oldest first (i.e. kernel, servers, clients), so walk backwardss.
> @@ -94,19 +95,22 @@ static int drm_clients_info(struct seq_file *m, void *data)
>  		struct task_struct *task;
>  		struct pid *pid;
>  
> +		mutex_lock(&priv->name_lock);
>  		rcu_read_lock(); /* Locks priv->pid and pid_task()->comm! */
>  		pid = rcu_dereference(priv->pid);
>  		task = pid_task(pid, PIDTYPE_TGID);
>  		uid = task ? __task_cred(task)->euid : GLOBAL_ROOT_UID;
> -		seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u\n",
> +		seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u %20s\n",
>  			   task ? task->comm : "<unknown>",
>  			   pid_vnr(pid),
>  			   priv->minor->index,
>  			   is_current_master ? 'y' : 'n',
>  			   priv->authenticated ? 'y' : 'n',
>  			   from_kuid_munged(seq_user_ns(m), uid),
> -			   priv->magic);
> +			   priv->magic,
> +			   priv->name ?: "");

There should be a default name similar to task->comm, like "<undefined>"
when not set. Perhaps also set name to task->comm by default.

>  		rcu_read_unlock();
> +		mutex_unlock(&priv->name_lock);
>  	}
>  	mutex_unlock(&dev->filelist_mutex);
>  	return 0;
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 01fde94fe2a9..e9dd0e90a1f9 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -158,6 +158,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>  
>  	spin_lock_init(&file->master_lookup_lock);
>  	mutex_init(&file->event_read_lock);
> +	mutex_init(&file->name_lock);
>  
>  	if (drm_core_check_feature(dev, DRIVER_GEM))
>  		drm_gem_open(dev, file);
> @@ -259,6 +260,10 @@ void drm_file_free(struct drm_file *file)
>  	WARN_ON(!list_empty(&file->event_list));
>  
>  	put_pid(rcu_access_pointer(file->pid));
> +
> +	mutex_destroy(&file->name_lock);
> +	kfree(file->name);
> +
>  	kfree(file);
>  }
>  
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 51f39912866f..46dadbd1bb35 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -540,6 +540,52 @@ int drm_version(struct drm_device *dev, void *data,
>  	return err;
>  }
>  
> +static int drm_set_name(struct drm_device *dev, void *data,
> +			struct drm_file *file_priv)
> +{
> +	struct drm_set_name *name = data;
> +	void __user *user_ptr;
> +	char *new_name;
> +	size_t i, len;
> +
> +	if (name->name_len > DRM_NAME_MAX_LEN)
> +		return -EINVAL;
> +
> +	user_ptr = u64_to_user_ptr(name->name);
> +
> +	new_name = memdup_user_nul(user_ptr, name->name_len);
> +	if (IS_ERR(new_name))
> +		return PTR_ERR(new_name);
> +
> +	len = strlen(new_name);

strnlen

> +	if (len != name->name_len) {
> +		kfree(new_name);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Filter out control char / spaces / new lines etc in the name
> +	 * since it's going to be used in dmesg or fdinfo's output.
> +	 */
> +	for (i = 0; i < len; i++) {
> +		if (!isgraph(new_name[i]))
> +			new_name[i] = '-';
> +	}
> +
> +	mutex_lock(&file_priv->name_lock);
> +	kfree(file_priv->name);
> +	if (len > 0) {
> +		file_priv->name = new_name;
> +	} else {
> +		kfree(new_name);
> +		file_priv->name = NULL;
> +	}
> +	mutex_unlock(&file_priv->name_lock);
> +
> +	return 0;
> +}
> +
>  static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
>  {
>  	/* ROOT_ONLY is only for CAP_SYS_ADMIN */
> @@ -610,6 +656,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>  	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
>  
> +	DRM_IOCTL_DEF(DRM_IOCTL_SET_NAME, drm_set_name, DRM_RENDER_ALLOW),
> +
>  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0),
>  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
>  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETCRTC, drm_mode_setcrtc, DRM_MASTER),
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 8c0030c77308..df26eee8f79c 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -388,6 +388,15 @@ struct drm_file {
>  	 * Per-file buffer caches used by the PRIME buffer sharing code.
>  	 */
>  	struct drm_prime_file_private prime;
> +
> +	/**
> +	 * @name:
> +	 *
> +	 * Userspace-provided name; useful for accounting and debugging.
> +	 */
> +	const char *name;

I'd make the "name" string static, i.e. char name[DRM_NAME_MAX_LEN + 1].
That will prevent pointer deref troubles and no additional MM code
bloating will be needed.

> +	/** @name_lock: Protects @name. */
> +	struct mutex name_lock;

And then this lock isn't strictly needed anymore and can be removed if
"name" string is static.

>  };
>  
>  /**
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 16122819edfe..f5e92e4f909b 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -1024,6 +1024,13 @@ struct drm_crtc_queue_sequence {
>  	__u64 user_data;	/* user data passed to event */
>  };
>  
> +#define DRM_NAME_MAX_LEN	64

What about 63, to align data size to 64 bytes including the NULL byte.

> +struct drm_set_name {

drm_set_name sounds very generic, IMO. Maybe drm_context_set_name?

> +	__u64 name_len;
> +	__u64 name;
> +};
> +
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> @@ -1288,6 +1295,16 @@ extern "C" {
>   */
>  #define DRM_IOCTL_MODE_CLOSEFB		DRM_IOWR(0xD0, struct drm_mode_closefb)
>  
> +/**
> + * DRM_IOCTL_SET_NAME - Attach a name to a drm_file
> + *
> + * This ioctl is similar to DMA_BUF_SET_NAME - it allows for easier tracking
> + * and debugging.

Don't refer to DMA_BUF_SET_NAME, explain what DRM_IOCTL_SET_NAME
actually do. Tell that it sets the DRM context name and that chars are
filtered.

-- 
Best regards,
Dmitry

Re: [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@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);

Re: [PATCH v3 1/6] drm: add DRM_SET_NAME ioctl

[Dmitry Osipenko]

On 9/23/24 13:28, Dmitry Osipenko wrote:
...
>>  /**
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index 16122819edfe..f5e92e4f909b 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -1024,6 +1024,13 @@ struct drm_crtc_queue_sequence {
>>  	__u64 user_data;	/* user data passed to event */
>>  };
>>  
>> +#define DRM_NAME_MAX_LEN	64
> 
> What about 63, to align data size to 64 bytes including the NULL byte.

On the other hand, the string is copied without NULL byte, so it doesn't
matter.

This leads to another question, why not using strndup_user like it's
done for dma_buf_set_name?

-- 
Best regards,
Dmitry

Re: [PATCH v3 1/6] drm: add DRM_SET_NAME ioctl

[Pierre-Eric Pelloux-Prayer]

Hi Dmitry,

Le 23/09/2024 à 12:28, Dmitry Osipenko a écrit :
> On 9/20/24 12:06, Pierre-Eric Pelloux-Prayer wrote:
>> Giving the opportunity to userspace to associate a free-form
>> name with a drm_file struct is helpful for tracking and debugging.
>>
>> This is similar to the existing DMA_BUF_SET_NAME ioctl.
>>
>> Access to name is protected by a mutex, and the 'clients' debugfs
>> file has been updated to print it.
>>
>> Userspace MR to use this ioctl:
>>     https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1428
>>
>> The string passed by userspace is filtered a bit, to avoid messing
>> output when it's going to be printed (in dmesg, fdinfo, etc):
>>    * all chars failing isgraph() are replaced by '-'
>>    * if a 0-length string is passed the name is cleared
>>
>> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
>> ---
>>   drivers/gpu/drm/drm_debugfs.c | 12 ++++++---
>>   drivers/gpu/drm/drm_file.c    |  5 ++++
>>   drivers/gpu/drm/drm_ioctl.c   | 48 +++++++++++++++++++++++++++++++++++
>>   include/drm/drm_file.h        |  9 +++++++
>>   include/uapi/drm/drm.h        | 17 +++++++++++++
>>   5 files changed, 87 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
>> index 6b239a24f1df..482e71160544 100644
>> --- a/drivers/gpu/drm/drm_debugfs.c
>> +++ b/drivers/gpu/drm/drm_debugfs.c
>> @@ -78,12 +78,13 @@ static int drm_clients_info(struct seq_file *m, void *data)
>>   	kuid_t uid;
>>   
>>   	seq_printf(m,
>> -		   "%20s %5s %3s master a %5s %10s\n",
>> +		   "%20s %5s %3s master a %5s %10s %20s\n",
>>   		   "command",
>>   		   "tgid",
>>   		   "dev",
>>   		   "uid",
>> -		   "magic");
>> +		   "magic",
>> +		   "name");
>>   
>>   	/* dev->filelist is sorted youngest first, but we want to present
>>   	 * oldest first (i.e. kernel, servers, clients), so walk backwardss.
>> @@ -94,19 +95,22 @@ static int drm_clients_info(struct seq_file *m, void *data)
>>   		struct task_struct *task;
>>   		struct pid *pid;
>>   
>> +		mutex_lock(&priv->name_lock);
>>   		rcu_read_lock(); /* Locks priv->pid and pid_task()->comm! */
>>   		pid = rcu_dereference(priv->pid);
>>   		task = pid_task(pid, PIDTYPE_TGID);
>>   		uid = task ? __task_cred(task)->euid : GLOBAL_ROOT_UID;
>> -		seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u\n",
>> +		seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u %20s\n",
>>   			   task ? task->comm : "<unknown>",
>>   			   pid_vnr(pid),
>>   			   priv->minor->index,
>>   			   is_current_master ? 'y' : 'n',
>>   			   priv->authenticated ? 'y' : 'n',
>>   			   from_kuid_munged(seq_user_ns(m), uid),
>> -			   priv->magic);
>> +			   priv->magic,
>> +			   priv->name ?: "");
> 
> There should be a default name similar to task->comm, like "<undefined>"
> when not set. Perhaps also set name to task->comm by default.

Honestly I don't see much value in printing "<undefined>" or any other default value (+ task->comm 
is already printed above).

> 
>>   		rcu_read_unlock();
>> +		mutex_unlock(&priv->name_lock);
>>   	}
>>   	mutex_unlock(&dev->filelist_mutex);
>>   	return 0;
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index 01fde94fe2a9..e9dd0e90a1f9 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -158,6 +158,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>   
>>   	spin_lock_init(&file->master_lookup_lock);
>>   	mutex_init(&file->event_read_lock);
>> +	mutex_init(&file->name_lock);
>>   
>>   	if (drm_core_check_feature(dev, DRIVER_GEM))
>>   		drm_gem_open(dev, file);
>> @@ -259,6 +260,10 @@ void drm_file_free(struct drm_file *file)
>>   	WARN_ON(!list_empty(&file->event_list));
>>   
>>   	put_pid(rcu_access_pointer(file->pid));
>> +
>> +	mutex_destroy(&file->name_lock);
>> +	kfree(file->name);
>> +
>>   	kfree(file);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index 51f39912866f..46dadbd1bb35 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -540,6 +540,52 @@ int drm_version(struct drm_device *dev, void *data,
>>   	return err;
>>   }
>>   
>> +static int drm_set_name(struct drm_device *dev, void *data,
>> +			struct drm_file *file_priv)
>> +{
>> +	struct drm_set_name *name = data;
>> +	void __user *user_ptr;
>> +	char *new_name;
>> +	size_t i, len;
>> +
>> +	if (name->name_len > DRM_NAME_MAX_LEN)
>> +		return -EINVAL;
>> +
>> +	user_ptr = u64_to_user_ptr(name->name);
>> +
>> +	new_name = memdup_user_nul(user_ptr, name->name_len);
>> +	if (IS_ERR(new_name))
>> +		return PTR_ERR(new_name);
>> +
>> +	len = strlen(new_name);
> 
> strnlen

memdup_user_nul returns a NUL-terminated string so I don't see much need for using strnlen.

> 
>> +	if (len != name->name_len) {
>> +		kfree(new_name);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * Filter out control char / spaces / new lines etc in the name
>> +	 * since it's going to be used in dmesg or fdinfo's output.
>> +	 */
>> +	for (i = 0; i < len; i++) {
>> +		if (!isgraph(new_name[i]))
>> +			new_name[i] = '-';
>> +	}
>> +
>> +	mutex_lock(&file_priv->name_lock);
>> +	kfree(file_priv->name);
>> +	if (len > 0) {
>> +		file_priv->name = new_name;
>> +	} else {
>> +		kfree(new_name);
>> +		file_priv->name = NULL;
>> +	}
>> +	mutex_unlock(&file_priv->name_lock);
>> +
>> +	return 0;
>> +}
>> +
>>   static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
>>   {
>>   	/* ROOT_ONLY is only for CAP_SYS_ADMIN */
>> @@ -610,6 +656,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>>   	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
>>   	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
>>   
>> +	DRM_IOCTL_DEF(DRM_IOCTL_SET_NAME, drm_set_name, DRM_RENDER_ALLOW),
>> +
>>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0),
>>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
>>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETCRTC, drm_mode_setcrtc, DRM_MASTER),
>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>> index 8c0030c77308..df26eee8f79c 100644
>> --- a/include/drm/drm_file.h
>> +++ b/include/drm/drm_file.h
>> @@ -388,6 +388,15 @@ struct drm_file {
>>   	 * Per-file buffer caches used by the PRIME buffer sharing code.
>>   	 */
>>   	struct drm_prime_file_private prime;
>> +
>> +	/**
>> +	 * @name:
>> +	 *
>> +	 * Userspace-provided name; useful for accounting and debugging.
>> +	 */
>> +	const char *name;
> 
> I'd make the "name" string static, i.e. char name[DRM_NAME_MAX_LEN + 1].
> That will prevent pointer deref troubles and no additional MM code
> bloating will be needed.
> 

Sure, I can do that if others prefer this way too.


>> +	/** @name_lock: Protects @name. */
>> +	struct mutex name_lock;
> 
> And then this lock isn't strictly needed anymore and can be removed if
> "name" string is static.

The locking also prevents concurrent modification.

> 
>>   };
>>   
>>   /**
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index 16122819edfe..f5e92e4f909b 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -1024,6 +1024,13 @@ struct drm_crtc_queue_sequence {
>>   	__u64 user_data;	/* user data passed to event */
>>   };
>>   
>> +#define DRM_NAME_MAX_LEN	64
> 
> What about 63, to align data size to 64 bytes including the NULL byte.

Same as "const char *name;" vs "char name[...]": I don't mind updating the code as long as there's a 
consensus.

> 
>> +struct drm_set_name {
> 
> drm_set_name sounds very generic, IMO. Maybe drm_context_set_name?

drm_client_set_name?
(since other places use client, like drm_clients_info())


> 
>> +	__u64 name_len;
>> +	__u64 name;
>> +};
>> +
>> +
>>   #if defined(__cplusplus)
>>   }
>>   #endif
>> @@ -1288,6 +1295,16 @@ extern "C" {
>>    */
>>   #define DRM_IOCTL_MODE_CLOSEFB		DRM_IOWR(0xD0, struct drm_mode_closefb)
>>   
>> +/**
>> + * DRM_IOCTL_SET_NAME - Attach a name to a drm_file
>> + *
>> + * This ioctl is similar to DMA_BUF_SET_NAME - it allows for easier tracking
>> + * and debugging.
> 
> Don't refer to DMA_BUF_SET_NAME, explain what DRM_IOCTL_SET_NAME
> actually do. Tell that it sets the DRM context name and that chars are
> filtered.
> 

OK, I'll update based on your suggestion.

Thanks,
Pierre-Eric

Re: [PATCH v3 1/6] drm: add DRM_SET_NAME ioctl

[Dmitry Osipenko]

On 9/23/24 19:29, Pierre-Eric Pelloux-Prayer wrote:
...
>>> @@ -78,12 +78,13 @@ static int drm_clients_info(struct seq_file *m,
>>> void *data)
>>>       kuid_t uid;
>>>         seq_printf(m,
>>> -           "%20s %5s %3s master a %5s %10s\n",
>>> +           "%20s %5s %3s master a %5s %10s %20s\n",
>>>              "command",
>>>              "tgid",
>>>              "dev",
>>>              "uid",
>>> -           "magic");
>>> +           "magic",
>>> +           "name");
>>>         /* dev->filelist is sorted youngest first, but we want to
>>> present
>>>        * oldest first (i.e. kernel, servers, clients), so walk
>>> backwardss.
>>> @@ -94,19 +95,22 @@ static int drm_clients_info(struct seq_file *m,
>>> void *data)
>>>           struct task_struct *task;
>>>           struct pid *pid;
>>>   +        mutex_lock(&priv->name_lock);
>>>           rcu_read_lock(); /* Locks priv->pid and pid_task()->comm! */
>>>           pid = rcu_dereference(priv->pid);
>>>           task = pid_task(pid, PIDTYPE_TGID);
>>>           uid = task ? __task_cred(task)->euid : GLOBAL_ROOT_UID;
>>> -        seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u\n",
>>> +        seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u %20s\n",
>>>                  task ? task->comm : "<unknown>",
>>>                  pid_vnr(pid),
>>>                  priv->minor->index,
>>>                  is_current_master ? 'y' : 'n',
>>>                  priv->authenticated ? 'y' : 'n',
>>>                  from_kuid_munged(seq_user_ns(m), uid),
>>> -               priv->magic);
>>> +               priv->magic,
>>> +               priv->name ?: "");
>>
>> There should be a default name similar to task->comm, like "<undefined>"
>> when not set. Perhaps also set name to task->comm by default.
> 
> Honestly I don't see much value in printing "<undefined>" or any other
> default value (+ task->comm is already printed above).

For a machine-parsed string in userspace there should be a value,
otherwise it won't be parseable if you'll add another parameter after
the name, AFAICT.

...
>>>   +static int drm_set_name(struct drm_device *dev, void *data,
>>> +            struct drm_file *file_priv)
>>> +{
>>> +    struct drm_set_name *name = data;
>>> +    void __user *user_ptr;
>>> +    char *new_name;
>>> +    size_t i, len;
>>> +
>>> +    if (name->name_len > DRM_NAME_MAX_LEN)
>>> +        return -EINVAL;
>>> +
>>> +    user_ptr = u64_to_user_ptr(name->name);
>>> +
>>> +    new_name = memdup_user_nul(user_ptr, name->name_len);
>>> +    if (IS_ERR(new_name))
>>> +        return PTR_ERR(new_name);
>>> +
>>> +    len = strlen(new_name);
>>
>> strnlen
> 
> memdup_user_nul returns a NUL-terminated string so I don't see much need
> for using strnlen.

Indeed

...
>>>   static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
>>>   {
>>>       /* ROOT_ONLY is only for CAP_SYS_ADMIN */
>>> @@ -610,6 +656,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>>>       DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD,
>>> drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
>>>       DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE,
>>> drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
>>>   +    DRM_IOCTL_DEF(DRM_IOCTL_SET_NAME, drm_set_name,
>>> DRM_RENDER_ALLOW),
>>> +
>>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES,
>>> drm_mode_getplane_res, 0),
>>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
>>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETCRTC, drm_mode_setcrtc,
>>> DRM_MASTER),
>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>> index 8c0030c77308..df26eee8f79c 100644
>>> --- a/include/drm/drm_file.h
>>> +++ b/include/drm/drm_file.h
>>> @@ -388,6 +388,15 @@ struct drm_file {
>>>        * Per-file buffer caches used by the PRIME buffer sharing code.
>>>        */
>>>       struct drm_prime_file_private prime;
>>> +
>>> +    /**
>>> +     * @name:
>>> +     *
>>> +     * Userspace-provided name; useful for accounting and debugging.
>>> +     */
>>> +    const char *name;
>>
>> I'd make the "name" string static, i.e. char name[DRM_NAME_MAX_LEN + 1].
>> That will prevent pointer deref troubles and no additional MM code
>> bloating will be needed.
>>
> 
> Sure, I can do that if others prefer this way too.

Note that in the other email I suggested to use strndup_user(), that
will remove the name-length limitation, but then the name var will
remain to be a string pointer. To me best option would be to replicate
how dma_buf_set_name works.

>>> +    /** @name_lock: Protects @name. */
>>> +    struct mutex name_lock;
>>
>> And then this lock isn't strictly needed anymore and can be removed if
>> "name" string is static.
> 
> The locking also prevents concurrent modification.

Right, locking still will be needed

>>>   };
>>>     /**
>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>>> index 16122819edfe..f5e92e4f909b 100644
>>> --- a/include/uapi/drm/drm.h
>>> +++ b/include/uapi/drm/drm.h
>>> @@ -1024,6 +1024,13 @@ struct drm_crtc_queue_sequence {
>>>       __u64 user_data;    /* user data passed to event */
>>>   };
>>>   +#define DRM_NAME_MAX_LEN    64
>>
>> What about 63, to align data size to 64 bytes including the NULL byte.
> 
> Same as "const char *name;" vs "char name[...]": I don't mind updating
> the code as long as there's a consensus.
> 
>>
>>> +struct drm_set_name {
>>
>> drm_set_name sounds very generic, IMO. Maybe drm_context_set_name?
> 
> drm_client_set_name?
> (since other places use client, like drm_clients_info())

drm_client_set_name sounds good

-- 
Best regards,
Dmitry

Re: [PATCH v3 1/6] drm: add DRM_SET_NAME ioctl

[Dmitry Osipenko]

On 9/23/24 21:09, Dmitry Osipenko wrote:
>> Sure, I can do that if others prefer this way too.
> Note that in the other email I suggested to use strndup_user(), that
> will remove the name-length limitation, but then the name var will
> remain to be a string pointer. To me best option would be to replicate
> how dma_buf_set_name works.

My bad, strndup_user() is also size-limited. Then the point about static
string remains.

-- 
Best regards,
Dmitry

Re: [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@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);

Re: [PATCH v3 1/6] drm: add DRM_SET_NAME ioctl

[Pierre-Eric Pelloux-Prayer]

Le 23/09/2024 à 12:06, Tvrtko Ursulin a écrit :
> 
> On 20/09/2024 10:06, Pierre-Eric Pelloux-Prayer wrote:
>> Giving the opportunity to userspace to associate a free-form
>> name with a drm_file struct is helpful for tracking and debugging.
>>
>> This is similar to the existing DMA_BUF_SET_NAME ioctl.
>>
>> Access to name is protected by a mutex, and the 'clients' debugfs
>> file has been updated to print it.
>>
>> Userspace MR to use this ioctl:
>>     https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1428
>>
>> The string passed by userspace is filtered a bit, to avoid messing
>> output when it's going to be printed (in dmesg, fdinfo, etc):
>>    * all chars failing isgraph() are replaced by '-'
>>    * if a 0-length string is passed the name is cleared
>>
>> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
>> ---
>>   drivers/gpu/drm/drm_debugfs.c | 12 ++++++---
>>   drivers/gpu/drm/drm_file.c    |  5 ++++
>>   drivers/gpu/drm/drm_ioctl.c   | 48 +++++++++++++++++++++++++++++++++++
>>   include/drm/drm_file.h        |  9 +++++++
>>   include/uapi/drm/drm.h        | 17 +++++++++++++
>>   5 files changed, 87 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
>> index 6b239a24f1df..482e71160544 100644
>> --- a/drivers/gpu/drm/drm_debugfs.c
>> +++ b/drivers/gpu/drm/drm_debugfs.c
>> @@ -78,12 +78,13 @@ static int drm_clients_info(struct seq_file *m, void *data)
>>       kuid_t uid;
>>       seq_printf(m,
>> -           "%20s %5s %3s master a %5s %10s\n",
>> +           "%20s %5s %3s master a %5s %10s %20s\n",
> 
> Allow full DRM_NAME_MAX_LEN? Not sure, feels not very consequential either way.

I'll switch to:

         seq_printf(m,
                    "%20s %5s %3s master a %5s %10s %*s\n",
                    "command",
                    "tgid",
                    "dev",
                    "uid",
                    "magic",
                    DRM_CLIENT_NAME_MAX_LEN,
                    "name");

And:

                 seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u %*s\n",
                            task ? task->comm : "<unknown>",
                            pid_vnr(pid),
                            priv->minor->index,
                            is_current_master ? 'y' : 'n',
                            priv->authenticated ? 'y' : 'n',
                            from_kuid_munged(seq_user_ns(m), uid),
                            priv->magic,
                            DRM_CLIENT_NAME_MAX_LEN,
                            priv->client_name ? priv->client_name : "<unset>");


> 
>>              "command",
>>              "tgid",
>>              "dev",
>>              "uid",
>> -           "magic");
>> +           "magic",
>> +           "name");
>>       /* dev->filelist is sorted youngest first, but we want to present
>>        * oldest first (i.e. kernel, servers, clients), so walk backwardss.
>> @@ -94,19 +95,22 @@ static int drm_clients_info(struct seq_file *m, void *data)
>>           struct task_struct *task;
>>           struct pid *pid;
>> +        mutex_lock(&priv->name_lock);
>>           rcu_read_lock(); /* Locks priv->pid and pid_task()->comm! */
>>           pid = rcu_dereference(priv->pid);
>>           task = pid_task(pid, PIDTYPE_TGID);
>>           uid = task ? __task_cred(task)->euid : GLOBAL_ROOT_UID;
>> -        seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u\n",
>> +        seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u %20s\n",
>>                  task ? task->comm : "<unknown>",
>>                  pid_vnr(pid),
>>                  priv->minor->index,
>>                  is_current_master ? 'y' : 'n',
>>                  priv->authenticated ? 'y' : 'n',
>>                  from_kuid_munged(seq_user_ns(m), uid),
>> -               priv->magic);
>> +               priv->magic,
>> +               priv->name ?: "");
>>           rcu_read_unlock();
>> +        mutex_unlock(&priv->name_lock);
>>       }
>>       mutex_unlock(&dev->filelist_mutex);
>>       return 0;
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index 01fde94fe2a9..e9dd0e90a1f9 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -158,6 +158,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>       spin_lock_init(&file->master_lookup_lock);
>>       mutex_init(&file->event_read_lock);
>> +    mutex_init(&file->name_lock);
>>       if (drm_core_check_feature(dev, DRIVER_GEM))
>>           drm_gem_open(dev, file);
>> @@ -259,6 +260,10 @@ void drm_file_free(struct drm_file *file)
>>       WARN_ON(!list_empty(&file->event_list));
>>       put_pid(rcu_access_pointer(file->pid));
>> +
>> +    mutex_destroy(&file->name_lock);
>> +    kfree(file->name);
>> +
>>       kfree(file);
>>   }
>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index 51f39912866f..46dadbd1bb35 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -540,6 +540,52 @@ int drm_version(struct drm_device *dev, void *data,
>>       return err;
>>   }
>> +static int drm_set_name(struct drm_device *dev, void *data,
>> +            struct drm_file *file_priv)
>> +{
>> +    struct drm_set_name *name = data;
>> +    void __user *user_ptr;
>> +    char *new_name;
>> +    size_t i, len;
>> +
>> +    if (name->name_len > DRM_NAME_MAX_LEN)
>> +        return -EINVAL;
>> +
>> +    user_ptr = u64_to_user_ptr(name->name);
>> +
>> +    new_name = memdup_user_nul(user_ptr, name->name_len);
>> +    if (IS_ERR(new_name))
>> +        return PTR_ERR(new_name);
>> +
>> +    len = strlen(new_name);
>> +
>> +    if (len != name->name_len) {
>> +        kfree(new_name);
>> +        return -EINVAL;
>> +    }
>> +
>> +    /*
>> +     * Filter out control char / spaces / new lines etc in the name
>> +     * since it's going to be used in dmesg or fdinfo's output.
>> +     */
>> +    for (i = 0; i < len; i++) {
>> +        if (!isgraph(new_name[i]))
>> +            new_name[i] = '-';
>> +    }
>> +
>> +    mutex_lock(&file_priv->name_lock);
>> +    kfree(file_priv->name);
>> +    if (len > 0) {
>> +        file_priv->name = new_name;
>> +    } else {
>> +        kfree(new_name);
>> +        file_priv->name = NULL;
>> +    }
>> +    mutex_unlock(&file_priv->name_lock);
>> +
>> +    return 0;
>> +}
>> +
>>   static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
>>   {
>>       /* ROOT_ONLY is only for CAP_SYS_ADMIN */
>> @@ -610,6 +656,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>>       DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
>>       DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
>> +    DRM_IOCTL_DEF(DRM_IOCTL_SET_NAME, drm_set_name, DRM_RENDER_ALLOW),
>> +
>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0),
>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETCRTC, drm_mode_setcrtc, DRM_MASTER),
>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>> index 8c0030c77308..df26eee8f79c 100644
>> --- a/include/drm/drm_file.h
>> +++ b/include/drm/drm_file.h
>> @@ -388,6 +388,15 @@ struct drm_file {
>>        * Per-file buffer caches used by the PRIME buffer sharing code.
>>        */
>>       struct drm_prime_file_private prime;
>> +
>> +    /**
>> +     * @name:
>> +     *
>> +     * Userspace-provided name; useful for accounting and debugging.
>> +     */
>> +    const char *name;
>> +    /** @name_lock: Protects @name. */
>> +    struct mutex name_lock;
>>   };
>>   /**
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index 16122819edfe..f5e92e4f909b 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -1024,6 +1024,13 @@ struct drm_crtc_queue_sequence {
>>       __u64 user_data;    /* user data passed to event */
>>   };
>> +#define DRM_NAME_MAX_LEN    64
>> +struct drm_set_name {
>> +    __u64 name_len;
>> +    __u64 name;
>> +};
>> +
>> +
>>   #if defined(__cplusplus)
>>   }
>>   #endif
>> @@ -1288,6 +1295,16 @@ extern "C" {
>>    */
>>   #define DRM_IOCTL_MODE_CLOSEFB        DRM_IOWR(0xD0, struct drm_mode_closefb)
>> +/**
>> + * DRM_IOCTL_SET_NAME - Attach a name to a drm_file
>> + *
>> + * This ioctl is similar to DMA_BUF_SET_NAME - it allows for easier tracking
>> + * and debugging.
>> + * The length of the name must <= DRM_NAME_MAX_LEN. All characters that are
>> + * non-printable or whitespaces will be replaced by -.
>> + */
>> +#define DRM_IOCTL_SET_NAME        DRM_IOWR(0xD1, struct drm_set_name)
>> +
> 
> A comment, nice! :) Overal looks good to me.
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> 
> I do however wish for more opinions (before merging) on whether strings with invalid characters 
> should perhaps instead be rejected. I don't currently have a solid argument either way.
> 
> Perhaps the only argument against silent transformation is if someone sets some wild string, then 
> greps for it somewhere, which would be a false negative without the understanding of what kind of 
> remapping kernel does. It is weak but it is uapi so worth discussing every crazy possibility I think.
> 
> On the other hand it would create another annoying source of EINVAL. :shrug:

No strong feelings either, but I agree that gathering more opinions before merging would be useful.

> 
> Also, how are with with testing the DRM core features? Add something for the uapi in 
> IGT/tests/drm_client_name, or some such?

Something like using the ioctl and then reading back the name using /sys/kernel/debug/dri/0/clients?
I can do that if it's required / useful.

Thanks,
Pierre-Eric

> 
> Regards,
> 
> Tvrtko
> 
>>   /*
>>    * Device specific ioctls should only be in their respective headers
>>    * The device specific ioctl range is from 0x40 to 0x9f.

Re: [PATCH v3 3/6] drm/amdgpu: delay the use of amdgpu_vm_set_task_info

[Christian König]

Am 23.09.24 um 12:25 schrieb Tvrtko Ursulin:
>
> On 20/09/2024 10:06, Pierre-Eric Pelloux-Prayer wrote:
>> At this point the vm is locked so we safely modify it without risk of
>> concurrent access.
>
> To which particular lock this is referring to and does this imply 
> previous placement was unsafe?

We use the root PDs dma_resv object as VM lock to protect most field 
inside the VM structure, only a few are protected by an additional spinlock.

And yes, previously it was possible that you got a mangled process/task 
name because no lock was protecting the task_info structure.

Regards,
Christian.

>
> Regards,
>
> Tvrtko
>
>> Signed-off-by: Pierre-Eric Pelloux-Prayer 
>> <pierre-eric.pelloux-prayer@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index 1e475eb01417..891128ecee6d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -309,9 +309,6 @@ static int amdgpu_cs_pass1(struct 
>> amdgpu_cs_parser *p,
>>           p->gang_leader->uf_addr = uf_offset;
>>       kvfree(chunk_array);
>>   -    /* Use this opportunity to fill in task info for the vm */
>> -    amdgpu_vm_set_task_info(vm);
>> -
>>       return 0;
>>     free_all_kdata:
>> @@ -1180,6 +1177,9 @@ static int amdgpu_cs_vm_handling(struct 
>> amdgpu_cs_parser *p)
>>           job->vm_pd_addr = amdgpu_gmc_pd_addr(vm->root.bo);
>>       }
>>   +    /* Use this opportunity to fill in task info for the vm */
>> +    amdgpu_vm_set_task_info(vm);
>> +
>>       if (adev->debug_vm) {
>>           /* Invalidate all BOs to test for userspace bugs */
>>           amdgpu_bo_list_for_each_entry(e, p->bo_list) {

Re: [PATCH v3 1/6] drm: add DRM_SET_NAME ioctl

[Tvrtko Ursulin]

On 24/09/2024 09:22, Pierre-Eric Pelloux-Prayer wrote:
> 
> 
> Le 23/09/2024 à 12:06, Tvrtko Ursulin a écrit :
>>
>> On 20/09/2024 10:06, Pierre-Eric Pelloux-Prayer wrote:
>>> Giving the opportunity to userspace to associate a free-form
>>> name with a drm_file struct is helpful for tracking and debugging.
>>>
>>> This is similar to the existing DMA_BUF_SET_NAME ioctl.
>>>
>>> Access to name is protected by a mutex, and the 'clients' debugfs
>>> file has been updated to print it.
>>>
>>> Userspace MR to use this ioctl:
>>>     
>>> https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1428
>>>
>>> The string passed by userspace is filtered a bit, to avoid messing
>>> output when it's going to be printed (in dmesg, fdinfo, etc):
>>>    * all chars failing isgraph() are replaced by '-'
>>>    * if a 0-length string is passed the name is cleared
>>>
>>> Signed-off-by: Pierre-Eric Pelloux-Prayer 
>>> <pierre-eric.pelloux-prayer@amd.com>
>>> ---
>>>   drivers/gpu/drm/drm_debugfs.c | 12 ++++++---
>>>   drivers/gpu/drm/drm_file.c    |  5 ++++
>>>   drivers/gpu/drm/drm_ioctl.c   | 48 +++++++++++++++++++++++++++++++++++
>>>   include/drm/drm_file.h        |  9 +++++++
>>>   include/uapi/drm/drm.h        | 17 +++++++++++++
>>>   5 files changed, 87 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_debugfs.c 
>>> b/drivers/gpu/drm/drm_debugfs.c
>>> index 6b239a24f1df..482e71160544 100644
>>> --- a/drivers/gpu/drm/drm_debugfs.c
>>> +++ b/drivers/gpu/drm/drm_debugfs.c
>>> @@ -78,12 +78,13 @@ static int drm_clients_info(struct seq_file *m, 
>>> void *data)
>>>       kuid_t uid;
>>>       seq_printf(m,
>>> -           "%20s %5s %3s master a %5s %10s\n",
>>> +           "%20s %5s %3s master a %5s %10s %20s\n",
>>
>> Allow full DRM_NAME_MAX_LEN? Not sure, feels not very consequential 
>> either way.
> 
> I'll switch to:
> 
>          seq_printf(m,
>                     "%20s %5s %3s master a %5s %10s %*s\n",
>                     "command",
>                     "tgid",
>                     "dev",
>                     "uid",
>                     "magic",
>                     DRM_CLIENT_NAME_MAX_LEN,
>                     "name");
> 

That works.

> And:
> 
>                  seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u %*s\n",
>                             task ? task->comm : "<unknown>",
>                             pid_vnr(pid),
>                             priv->minor->index,
>                             is_current_master ? 'y' : 'n',
>                             priv->authenticated ? 'y' : 'n',
>                             from_kuid_munged(seq_user_ns(m), uid),
>                             priv->magic,
>                             DRM_CLIENT_NAME_MAX_LEN,
>                             priv->client_name ? priv->client_name : 
> "<unset>");

Also works for me although it will look a bit busy by default since 
every line will contain it.

I don't immediately see "parseability" is a concern (what Dmitry raised) 
because new code can detect if there is something there or not. For old 
code, or future changes, we do not care in debugfs.

Equally we don't care that much if it looks busy, hence why I said 
"<unset>" works for me.

I'd also be okay with repeating task->comm, but that perhaps complicates 
things too much when task is not available.

>>
>>>              "command",
>>>              "tgid",
>>>              "dev",
>>>              "uid",
>>> -           "magic");
>>> +           "magic",
>>> +           "name");
>>>       /* dev->filelist is sorted youngest first, but we want to present
>>>        * oldest first (i.e. kernel, servers, clients), so walk 
>>> backwardss.
>>> @@ -94,19 +95,22 @@ static int drm_clients_info(struct seq_file *m, 
>>> void *data)
>>>           struct task_struct *task;
>>>           struct pid *pid;
>>> +        mutex_lock(&priv->name_lock);
>>>           rcu_read_lock(); /* Locks priv->pid and pid_task()->comm! */
>>>           pid = rcu_dereference(priv->pid);
>>>           task = pid_task(pid, PIDTYPE_TGID);
>>>           uid = task ? __task_cred(task)->euid : GLOBAL_ROOT_UID;
>>> -        seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u\n",
>>> +        seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u %20s\n",
>>>                  task ? task->comm : "<unknown>",
>>>                  pid_vnr(pid),
>>>                  priv->minor->index,
>>>                  is_current_master ? 'y' : 'n',
>>>                  priv->authenticated ? 'y' : 'n',
>>>                  from_kuid_munged(seq_user_ns(m), uid),
>>> -               priv->magic);
>>> +               priv->magic,
>>> +               priv->name ?: "");
>>>           rcu_read_unlock();
>>> +        mutex_unlock(&priv->name_lock);
>>>       }
>>>       mutex_unlock(&dev->filelist_mutex);
>>>       return 0;
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index 01fde94fe2a9..e9dd0e90a1f9 100644
>>> --- a/drivers/gpu/drm/drm_file.c
>>> +++ b/drivers/gpu/drm/drm_file.c
>>> @@ -158,6 +158,7 @@ struct drm_file *drm_file_alloc(struct drm_minor 
>>> *minor)
>>>       spin_lock_init(&file->master_lookup_lock);
>>>       mutex_init(&file->event_read_lock);
>>> +    mutex_init(&file->name_lock);
>>>       if (drm_core_check_feature(dev, DRIVER_GEM))
>>>           drm_gem_open(dev, file);
>>> @@ -259,6 +260,10 @@ void drm_file_free(struct drm_file *file)
>>>       WARN_ON(!list_empty(&file->event_list));
>>>       put_pid(rcu_access_pointer(file->pid));
>>> +
>>> +    mutex_destroy(&file->name_lock);
>>> +    kfree(file->name);
>>> +
>>>       kfree(file);
>>>   }
>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>>> index 51f39912866f..46dadbd1bb35 100644
>>> --- a/drivers/gpu/drm/drm_ioctl.c
>>> +++ b/drivers/gpu/drm/drm_ioctl.c
>>> @@ -540,6 +540,52 @@ int drm_version(struct drm_device *dev, void *data,
>>>       return err;
>>>   }
>>> +static int drm_set_name(struct drm_device *dev, void *data,
>>> +            struct drm_file *file_priv)
>>> +{
>>> +    struct drm_set_name *name = data;
>>> +    void __user *user_ptr;
>>> +    char *new_name;
>>> +    size_t i, len;
>>> +
>>> +    if (name->name_len > DRM_NAME_MAX_LEN)
>>> +        return -EINVAL;
>>> +
>>> +    user_ptr = u64_to_user_ptr(name->name);
>>> +
>>> +    new_name = memdup_user_nul(user_ptr, name->name_len);
>>> +    if (IS_ERR(new_name))
>>> +        return PTR_ERR(new_name);
>>> +
>>> +    len = strlen(new_name);
>>> +
>>> +    if (len != name->name_len) {
>>> +        kfree(new_name);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    /*
>>> +     * Filter out control char / spaces / new lines etc in the name
>>> +     * since it's going to be used in dmesg or fdinfo's output.
>>> +     */
>>> +    for (i = 0; i < len; i++) {
>>> +        if (!isgraph(new_name[i]))
>>> +            new_name[i] = '-';
>>> +    }
>>> +
>>> +    mutex_lock(&file_priv->name_lock);
>>> +    kfree(file_priv->name);
>>> +    if (len > 0) {
>>> +        file_priv->name = new_name;
>>> +    } else {
>>> +        kfree(new_name);
>>> +        file_priv->name = NULL;
>>> +    }
>>> +    mutex_unlock(&file_priv->name_lock);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
>>>   {
>>>       /* ROOT_ONLY is only for CAP_SYS_ADMIN */
>>> @@ -610,6 +656,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>>>       DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, 
>>> drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
>>>       DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, 
>>> drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
>>> +    DRM_IOCTL_DEF(DRM_IOCTL_SET_NAME, drm_set_name, DRM_RENDER_ALLOW),
>>> +
>>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, 
>>> drm_mode_getplane_res, 0),
>>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
>>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETCRTC, drm_mode_setcrtc, 
>>> DRM_MASTER),
>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>> index 8c0030c77308..df26eee8f79c 100644
>>> --- a/include/drm/drm_file.h
>>> +++ b/include/drm/drm_file.h
>>> @@ -388,6 +388,15 @@ struct drm_file {
>>>        * Per-file buffer caches used by the PRIME buffer sharing code.
>>>        */
>>>       struct drm_prime_file_private prime;
>>> +
>>> +    /**
>>> +     * @name:
>>> +     *
>>> +     * Userspace-provided name; useful for accounting and debugging.
>>> +     */
>>> +    const char *name;
>>> +    /** @name_lock: Protects @name. */
>>> +    struct mutex name_lock;
>>>   };
>>>   /**
>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>>> index 16122819edfe..f5e92e4f909b 100644
>>> --- a/include/uapi/drm/drm.h
>>> +++ b/include/uapi/drm/drm.h
>>> @@ -1024,6 +1024,13 @@ struct drm_crtc_queue_sequence {
>>>       __u64 user_data;    /* user data passed to event */
>>>   };
>>> +#define DRM_NAME_MAX_LEN    64
>>> +struct drm_set_name {
>>> +    __u64 name_len;
>>> +    __u64 name;
>>> +};
>>> +
>>> +
>>>   #if defined(__cplusplus)
>>>   }
>>>   #endif
>>> @@ -1288,6 +1295,16 @@ extern "C" {
>>>    */
>>>   #define DRM_IOCTL_MODE_CLOSEFB        DRM_IOWR(0xD0, struct 
>>> drm_mode_closefb)
>>> +/**
>>> + * DRM_IOCTL_SET_NAME - Attach a name to a drm_file
>>> + *
>>> + * This ioctl is similar to DMA_BUF_SET_NAME - it allows for easier 
>>> tracking
>>> + * and debugging.
>>> + * The length of the name must <= DRM_NAME_MAX_LEN. All characters 
>>> that are
>>> + * non-printable or whitespaces will be replaced by -.
>>> + */
>>> +#define DRM_IOCTL_SET_NAME        DRM_IOWR(0xD1, struct drm_set_name)
>>> +
>>
>> A comment, nice! :) Overal looks good to me.
>>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>
>> I do however wish for more opinions (before merging) on whether 
>> strings with invalid characters should perhaps instead be rejected. I 
>> don't currently have a solid argument either way.
>>
>> Perhaps the only argument against silent transformation is if someone 
>> sets some wild string, then greps for it somewhere, which would be a 
>> false negative without the understanding of what kind of remapping 
>> kernel does. It is weak but it is uapi so worth discussing every crazy 
>> possibility I think.
>>
>> On the other hand it would create another annoying source of EINVAL. 
>> :shrug:
> 
> No strong feelings either, but I agree that gathering more opinions 
> before merging would be useful.

Right.

>>
>> Also, how are with with testing the DRM core features? Add something 
>> for the uapi in IGT/tests/drm_client_name, or some such?
> 
> Something like using the ioctl and then reading back the name using 
> /sys/kernel/debug/dri/0/clients?
> I can do that if it's required / useful.

That and making sure various invalid and evil inputs are 
rejected/handled. Would be nice, but looking at what's currently in IGT 
I don't see that it is a requirement.

Regards,

Tvrtko

> 
> Thanks,
> Pierre-Eric
> 
>>
>> Regards,
>>
>> Tvrtko
>>
>>>   /*
>>>    * Device specific ioctls should only be in their respective headers
>>>    * The device specific ioctl range is from 0x40 to 0x9f.

Re: [PATCH v3 3/6] drm/amdgpu: delay the use of amdgpu_vm_set_task_info

[Tvrtko Ursulin]

On 24/09/2024 09:23, Christian König wrote:
> Am 23.09.24 um 12:25 schrieb Tvrtko Ursulin:
>>
>> On 20/09/2024 10:06, Pierre-Eric Pelloux-Prayer wrote:
>>> At this point the vm is locked so we safely modify it without risk of
>>> concurrent access.
>>
>> To which particular lock this is referring to and does this imply 
>> previous placement was unsafe?
> 
> We use the root PDs dma_resv object as VM lock to protect most field 
> inside the VM structure, only a few are protected by an additional 
> spinlock.
> 
> And yes, previously it was possible that you got a mangled process/task 
> name because no lock was protecting the task_info structure.

Got it, thanks Christian!

In this case I only suggest to be more explicit in the commit message 
and clearly say it is fixing an existing bug. Like it stands I wasn't 
sure if it was that, or the movement was just enabling the changes which 
come later in the series.

Regards,

Tvrtko

>>> Signed-off-by: Pierre-Eric Pelloux-Prayer 
>>> <pierre-eric.pelloux-prayer@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index 1e475eb01417..891128ecee6d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -309,9 +309,6 @@ static int amdgpu_cs_pass1(struct 
>>> amdgpu_cs_parser *p,
>>>           p->gang_leader->uf_addr = uf_offset;
>>>       kvfree(chunk_array);
>>>   -    /* Use this opportunity to fill in task info for the vm */
>>> -    amdgpu_vm_set_task_info(vm);
>>> -
>>>       return 0;
>>>     free_all_kdata:
>>> @@ -1180,6 +1177,9 @@ static int amdgpu_cs_vm_handling(struct 
>>> amdgpu_cs_parser *p)
>>>           job->vm_pd_addr = amdgpu_gmc_pd_addr(vm->root.bo);
>>>       }
>>>   +    /* Use this opportunity to fill in task info for the vm */
>>> +    amdgpu_vm_set_task_info(vm);
>>> +
>>>       if (adev->debug_vm) {
>>>           /* Invalidate all BOs to test for userspace bugs */
>>>           amdgpu_bo_list_for_each_entry(e, p->bo_list) {
> 

Re: [PATCH v3 3/6] drm/amdgpu: delay the use of amdgpu_vm_set_task_info

[Pierre-Eric Pelloux-Prayer]

Le 24/09/2024 à 10:43, Tvrtko Ursulin a écrit :
> 
> On 24/09/2024 09:23, Christian König wrote:
>> Am 23.09.24 um 12:25 schrieb Tvrtko Ursulin:
>>>
>>> On 20/09/2024 10:06, Pierre-Eric Pelloux-Prayer wrote:
>>>> At this point the vm is locked so we safely modify it without risk of
>>>> concurrent access.
>>>
>>> To which particular lock this is referring to and does this imply previous placement was unsafe?
>>
>> We use the root PDs dma_resv object as VM lock to protect most field inside the VM structure, only 
>> a few are protected by an additional spinlock.
>>
>> And yes, previously it was possible that you got a mangled process/task name because no lock was 
>> protecting the task_info structure.
> 
> Got it, thanks Christian!
> 
> In this case I only suggest to be more explicit in the commit message and clearly say it is fixing 
> an existing bug. Like it stands I wasn't sure if it was that, or the movement was just enabling the 
> changes which come later in the series.

Good idea, will do.

Pierre-Eric

> 
> Regards,
> 
> Tvrtko
> 
>>>> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 6 +++---
>>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> index 1e475eb01417..891128ecee6d 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> @@ -309,9 +309,6 @@ static int amdgpu_cs_pass1(struct amdgpu_cs_parser *p,
>>>>           p->gang_leader->uf_addr = uf_offset;
>>>>       kvfree(chunk_array);
>>>>   -    /* Use this opportunity to fill in task info for the vm */
>>>> -    amdgpu_vm_set_task_info(vm);
>>>> -
>>>>       return 0;
>>>>     free_all_kdata:
>>>> @@ -1180,6 +1177,9 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>>>>           job->vm_pd_addr = amdgpu_gmc_pd_addr(vm->root.bo);
>>>>       }
>>>>   +    /* Use this opportunity to fill in task info for the vm */
>>>> +    amdgpu_vm_set_task_info(vm);
>>>> +
>>>>       if (adev->debug_vm) {
>>>>           /* Invalidate all BOs to test for userspace bugs */
>>>>           amdgpu_bo_list_for_each_entry(e, p->bo_list) {
>>

Re: [PATCH v3 1/6] drm: add DRM_SET_NAME ioctl

[Dmitry Osipenko]

On 9/23/24 21:18, Dmitry Osipenko wrote:
> On 9/23/24 21:09, Dmitry Osipenko wrote:
>>> Sure, I can do that if others prefer this way too.
>> Note that in the other email I suggested to use strndup_user(), that
>> will remove the name-length limitation, but then the name var will
>> remain to be a string pointer. To me best option would be to replicate
>> how dma_buf_set_name works.
> 
> My bad, strndup_user() is also size-limited. Then the point about static
> string remains.

To clarify a bit further, I'm fine with both variants. Having a
consistent solution across kernel is also good if it's good enough. I.e.
replicating the whole dma_buf_set_name using the name pointer is okay to
me, though not having string pointers is more robust in general. Choose
what you like more.

-- 
Best regards,
Dmitry