[PATCH v4 0/6] DRM_SET_CLIENT_NAME ioctl

[PATCH v4 0/6] DRM_SET_CLIENT_NAME ioctl

[Pierre-Eric Pelloux-Prayer]

v4 changelog:
* DRM_SET_NAME -> DRM_SET_CLIENT_NAME (Dmitry)
* reject names that would mess up with formatting (Sima),
  and use a stricter filter (isgraph allowed extended ASCII
  which weren't looking great)
* documentation edits, minor fixups (Dmitry, Trvtko)
* clarified commit message of commit 3/6 (Trvtko)
* reworked amdgpu_vm_set_task_info a bit in 4/6 (Trvtko)

v3: https://lists.freedesktop.org/archives/dri-devel/2024-September/470488.html

Pierre-Eric Pelloux-Prayer (6):
  drm: add DRM_SET_CLIENT_NAME ioctl
  drm: use drm_file client_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        | 62 +++++++++++++------
 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                 | 14 +++--
 drivers/gpu/drm/drm_file.c                    | 10 +++
 drivers/gpu/drm/drm_ioctl.c                   | 55 ++++++++++++++++
 include/drm/drm_file.h                        |  9 +++
 include/uapi/drm/drm.h                        | 17 +++++
 23 files changed, 171 insertions(+), 42 deletions(-)

-- 
2.40.1

[PATCH v4 1/6] drm: add DRM_SET_CLIENT_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 client_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

If the string passed by userspace contains chars that would mess up output
when it's going to be printed (in dmesg, fdinfo, etc), -EINVAL is returned.

A 0-length string is a valid use, and clears the existing name.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 drivers/gpu/drm/drm_debugfs.c | 14 ++++++---
 drivers/gpu/drm/drm_file.c    |  5 ++++
 drivers/gpu/drm/drm_ioctl.c   | 55 +++++++++++++++++++++++++++++++++++
 include/drm/drm_file.h        |  9 ++++++
 include/uapi/drm/drm.h        | 17 +++++++++++
 5 files changed, 96 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 6b239a24f1df..5c99322a4c6f 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -78,12 +78,14 @@ 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 %*s\n",
 		   "command",
 		   "tgid",
 		   "dev",
 		   "uid",
-		   "magic");
+		   "magic",
+		   DRM_CLIENT_NAME_MAX_LEN,
+		   "name");
 
 	/* dev->filelist is sorted youngest first, but we want to present
 	 * oldest first (i.e. kernel, servers, clients), so walk backwardss.
@@ -94,19 +96,23 @@ static int drm_clients_info(struct seq_file *m, void *data)
 		struct task_struct *task;
 		struct pid *pid;
 
+		mutex_lock(&priv->client_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 %*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);
+			   priv->magic,
+			   DRM_CLIENT_NAME_MAX_LEN,
+			   priv->client_name ? priv->client_name : "<unset>");
 		rcu_read_unlock();
+		mutex_unlock(&priv->client_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..64f5e15304e7 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->client_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->client_name_lock);
+	kfree(file->client_name);
+
 	kfree(file);
 }
 
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 51f39912866f..df8d59bd5241 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -540,6 +540,59 @@ int drm_version(struct drm_device *dev, void *data,
 	return err;
 }
 
+/*
+ * Check if the passed string contains control char or spaces or
+ * anything that would mess up a formatted output.
+ */
+static int drm_validate_value_string(const char *value, size_t len)
+{
+	int i;
+
+	for (i = 0; i < len; i++) {
+		if (value[i] <= 32 || value[i] >= 127)
+			return -EINVAL;
+	}
+	return 0;
+}
+
+static int drm_set_client_name(struct drm_device *dev, void *data,
+			       struct drm_file *file_priv)
+{
+	struct drm_set_client_name *name = data;
+	void __user *user_ptr;
+	char *new_name;
+	size_t len;
+
+	if (name->name_len > DRM_CLIENT_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 ||
+	    drm_validate_value_string(new_name, len) < 0) {
+		kfree(new_name);
+		return -EINVAL;
+	}
+
+	mutex_lock(&file_priv->client_name_lock);
+	kfree(file_priv->client_name);
+	if (len > 0) {
+		file_priv->client_name = new_name;
+	} else {
+		kfree(new_name);
+		file_priv->client_name = NULL;
+	}
+	mutex_unlock(&file_priv->client_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 +663,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_CLIENT_NAME, drm_set_client_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..d4f1c115ea0f 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;
+
+	/**
+	 * @client_name:
+	 *
+	 * Userspace-provided name; useful for accounting and debugging.
+	 */
+	const char *client_name;
+	/** @name_lock: Protects @client_name. */
+	struct mutex client_name_lock;
 };
 
 /**
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 16122819edfe..7fba37b94401 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_CLIENT_NAME_MAX_LEN		64
+struct drm_set_client_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_CLIENT_NAME - Attach a name to a drm_file
+ *
+ * Having a name allows for easier tracking and debugging.
+ * The length of the name (without null ending char) must be
+ * <= DRM_CLIENT_NAME_MAX_LEN.
+ * The call will fail if the name contains whitespaces or non-printable chars.
+ */
+#define DRM_IOCTL_SET_CLIENT_NAME	DRM_IOWR(0xD1, struct drm_set_client_name)
+
 /*
  * Device specific ioctls should only be in their respective headers
  * The device specific ioctl range is from 0x40 to 0x9f.
-- 
2.40.1

Re: [PATCH v4 1/6] drm: add DRM_SET_CLIENT_NAME ioctl

[Dmitry Osipenko]

On 9/27/24 11:48, 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 client_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
> 
> If the string passed by userspace contains chars that would mess up output
> when it's going to be printed (in dmesg, fdinfo, etc), -EINVAL is returned.
> 
> A 0-length string is a valid use, and clears the existing name.
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> ---
>  drivers/gpu/drm/drm_debugfs.c | 14 ++++++---
>  drivers/gpu/drm/drm_file.c    |  5 ++++
>  drivers/gpu/drm/drm_ioctl.c   | 55 +++++++++++++++++++++++++++++++++++
>  include/drm/drm_file.h        |  9 ++++++
>  include/uapi/drm/drm.h        | 17 +++++++++++
>  5 files changed, 96 insertions(+), 4 deletions(-)

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry

Re: [PATCH v4 1/6] drm: add DRM_SET_CLIENT_NAME ioctl

[Tvrtko Ursulin]

On 27/09/2024 09:48, 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 client_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
> 
> If the string passed by userspace contains chars that would mess up output
> when it's going to be printed (in dmesg, fdinfo, etc), -EINVAL is returned.
> 
> A 0-length string is a valid use, and clears the existing name.
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> ---
>   drivers/gpu/drm/drm_debugfs.c | 14 ++++++---
>   drivers/gpu/drm/drm_file.c    |  5 ++++
>   drivers/gpu/drm/drm_ioctl.c   | 55 +++++++++++++++++++++++++++++++++++
>   include/drm/drm_file.h        |  9 ++++++
>   include/uapi/drm/drm.h        | 17 +++++++++++
>   5 files changed, 96 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 6b239a24f1df..5c99322a4c6f 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -78,12 +78,14 @@ 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 %*s\n",
>   		   "command",
>   		   "tgid",
>   		   "dev",
>   		   "uid",
> -		   "magic");
> +		   "magic",
> +		   DRM_CLIENT_NAME_MAX_LEN,
> +		   "name");
>   
>   	/* dev->filelist is sorted youngest first, but we want to present
>   	 * oldest first (i.e. kernel, servers, clients), so walk backwardss.
> @@ -94,19 +96,23 @@ static int drm_clients_info(struct seq_file *m, void *data)
>   		struct task_struct *task;
>   		struct pid *pid;
>   
> +		mutex_lock(&priv->client_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 %*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);
> +			   priv->magic,
> +			   DRM_CLIENT_NAME_MAX_LEN,
> +			   priv->client_name ? priv->client_name : "<unset>");
>   		rcu_read_unlock();
> +		mutex_unlock(&priv->client_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..64f5e15304e7 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->client_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->client_name_lock);
> +	kfree(file->client_name);
> +
>   	kfree(file);
>   }
>   
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 51f39912866f..df8d59bd5241 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -540,6 +540,59 @@ int drm_version(struct drm_device *dev, void *data,
>   	return err;
>   }
>   
> +/*
> + * Check if the passed string contains control char or spaces or
> + * anything that would mess up a formatted output.
> + */
> +static int drm_validate_value_string(const char *value, size_t len)
> +{
> +	int i;
> +
> +	for (i = 0; i < len; i++) {
> +		if (value[i] <= 32 || value[i] >= 127)

Would !isascii() || isgraph() work for what you have in mind here, 
considering the comment from the cover letter about the extended ASCII?

> +			return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int drm_set_client_name(struct drm_device *dev, void *data,
> +			       struct drm_file *file_priv)
> +{
> +	struct drm_set_client_name *name = data;
> +	void __user *user_ptr;
> +	char *new_name;
> +	size_t len;
> +
> +	if (name->name_len > DRM_CLIENT_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 ||
> +	    drm_validate_value_string(new_name, len) < 0) {
> +		kfree(new_name);
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&file_priv->client_name_lock);
> +	kfree(file_priv->client_name);
> +	if (len > 0) {
> +		file_priv->client_name = new_name;
> +	} else {
> +		kfree(new_name);
> +		file_priv->client_name = NULL;
> +	}
> +	mutex_unlock(&file_priv->client_name_lock);

FWIW I still find it hard to look at needlessly allocating a string when 
userspace has passed name->name_len == 0.

I would have done it something like this:

{
	struct drm_set_client_name *name = data;
	size_t len = name->len;
	char *new_name;

	if (len > DRM_CLIENT_NAME_MAX_LEN) {
		return -EINVAL;
	} else if (len) {
		new_name = memdup_user_nul(u64_to_user_ptr(name->name), len);
		if (IS_ERR(new_name))
			return PTR_ERR(new_name);

		if (strlen(new_name) != len ||
		    drm_validate_value_string(new_name, len) < 0) {
			kfree(new_name);
			return -EINVAL;
		}
	} else {
		new_name = NULL;
	}

	mutex_lock(&file_priv->client_name_lock);
	kfree(file_priv->client_name);
	file_priv->client_name = new_name;
	mutex_unlock(&file_priv->client_name_lock);

But whatever, you can keep the r-b regardless.

Regards,

Tvrtko

> +
> +	return 0;
> +}
> +
>   static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
>   {
>   	/* ROOT_ONLY is only for CAP_SYS_ADMIN */
> @@ -610,6 +663,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_CLIENT_NAME, drm_set_client_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..d4f1c115ea0f 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;
> +
> +	/**
> +	 * @client_name:
> +	 *
> +	 * Userspace-provided name; useful for accounting and debugging.
> +	 */
> +	const char *client_name;
> +	/** @name_lock: Protects @client_name. */
> +	struct mutex client_name_lock;
>   };
>   
>   /**
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 16122819edfe..7fba37b94401 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_CLIENT_NAME_MAX_LEN		64
> +struct drm_set_client_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_CLIENT_NAME - Attach a name to a drm_file
> + *
> + * Having a name allows for easier tracking and debugging.
> + * The length of the name (without null ending char) must be
> + * <= DRM_CLIENT_NAME_MAX_LEN.
> + * The call will fail if the name contains whitespaces or non-printable chars.
> + */
> +#define DRM_IOCTL_SET_CLIENT_NAME	DRM_IOWR(0xD1, struct drm_set_client_name)
> +
>   /*
>    * Device specific ioctls should only be in their respective headers
>    * The device specific ioctl range is from 0x40 to 0x9f.

[PATCH v4 2/6] drm: use drm_file client_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..566e122e6e60 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_CLIENT_NAME.
+
+
 Utilization
 ^^^^^^^^^^^
 
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 64f5e15304e7..1c9e03477a24 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->client_name_lock);
+	if (file->client_name)
+		drm_printf(&p, "drm-client-name:\t%s\n", file->client_name);
+	mutex_unlock(&file->client_name_lock);
+
 	if (dev->driver->show_fdinfo)
 		dev->driver->show_fdinfo(&p, file);
 }
-- 
2.40.1

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

[Pierre-Eric Pelloux-Prayer]

At the point the VM is locked (through the root PDs dma_resv object), so
it's safer to call amdgpu_vm_set_task_info.

The original place was not protected against concurrent access, but the
risk was limited to mangled process/task name.

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 v4 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 | 29 ++++++++++++--------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index e20d19ae01b2..6cd5bd5362d4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2331,7 +2331,7 @@ amdgpu_vm_get_task_info_vm(struct amdgpu_vm *vm)
 {
 	struct amdgpu_task_info *ti = NULL;
 
-	if (vm) {
+	if (vm && vm->task_info) {
 		ti = vm->task_info;
 		kref_get(&vm->task_info->refcount);
 	}
@@ -2361,6 +2361,10 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm)
 	if (!vm->task_info)
 		return -ENOMEM;
 
+	/* Set process attributes now. */
+	vm->task_info->tgid = current->group_leader->pid;
+	get_task_comm(vm->task_info->process_name, current->group_leader);
+
 	kref_init(&vm->task_info->refcount);
 	return 0;
 }
@@ -2372,20 +2376,16 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm)
  */
 void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
 {
-	if (!vm->task_info)
-		return;
-
-	if (vm->task_info->pid == current->pid)
+	if (!vm->task_info) {
+		if (amdgpu_vm_create_task_info(vm))
+			return;
+	} else if (vm->task_info->pid == current->pid) {
 		return;
+	}
 
+	/* Update task attributes. */
 	vm->task_info->pid = current->pid;
 	get_task_comm(vm->task_info->task_name, current);
-
-	if (current->group_leader->mm != current->mm)
-		return;
-
-	vm->task_info->tgid = current->group_leader->pid;
-	get_task_comm(vm->task_info->process_name, current->group_leader);
 }
 
 /**
@@ -2482,10 +2482,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	if (r)
 		goto error_free_root;
 
-	r = amdgpu_vm_create_task_info(vm);
-	if (r)
-		DRM_DEBUG("Failed to create task info for VM\n");
-
 	amdgpu_bo_unreserve(vm->root.bo);
 	amdgpu_bo_unref(&root_bo);
 
@@ -2608,7 +2604,8 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 
 	root = amdgpu_bo_ref(vm->root.bo);
 	amdgpu_bo_reserve(root, true);
-	amdgpu_vm_put_task_info(vm->task_info);
+	if (vm->task_info)
+		amdgpu_vm_put_task_info(vm->task_info);
 	amdgpu_vm_set_pasid(adev, vm, 0);
 	dma_fence_wait(vm->last_unlocked, false);
 	dma_fence_put(vm->last_unlocked);
-- 
2.40.1

[PATCH v4 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           | 13 +++++++++++--
 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, 24 insertions(+), 15 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 6cd5bd5362d4..cec0a5cffcc8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2357,13 +2357,22 @@ 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;
 
 	/* Set process attributes now. */
 	vm->task_info->tgid = current->group_leader->pid;
-	get_task_comm(vm->task_info->process_name, current->group_leader);
+	strscpy(vm->task_info->process_desc, process_name, desc_len + 1);
 
 	kref_init(&vm->task_info->refcount);
 	return 0;
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 v4 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, 29 insertions(+), 8 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 cec0a5cffcc8..f6e2be6d4e9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2355,25 +2355,40 @@ 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);
 
+	mutex_lock(&filp->client_name_lock);
+	if (filp->client_name)
+		desc_len += 1 + strlen(filp->client_name);
+
 	vm->task_info = kzalloc(
 		struct_size(vm->task_info, process_desc, desc_len + 1),
 		GFP_KERNEL);
 
-	if (!vm->task_info)
+	if (!vm->task_info) {
+		mutex_unlock(&filp->client_name_lock);
 		return -ENOMEM;
+	}
 
 	/* Set process attributes now. */
 	vm->task_info->tgid = current->group_leader->pid;
 	strscpy(vm->task_info->process_desc, process_name, desc_len + 1);
 
+	if (filp->client_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->client_name, (desc_len + 1) - (p_len + 1));
+	}
+	mutex_unlock(&filp->client_name_lock);
+
 	kref_init(&vm->task_info->refcount);
 	return 0;
 }
@@ -2382,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;
 	} else if (vm->task_info->pid == current->pid) {
 		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 v4 6/6] drm/amdgpu: use drm_file::name in task_info::process_desc

[Tvrtko Ursulin]

On 27/09/2024 09:48, Pierre-Eric Pelloux-Prayer wrote:
> 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, 29 insertions(+), 8 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 cec0a5cffcc8..f6e2be6d4e9e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2355,25 +2355,40 @@ 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;

Nit - would be nicer to avoid the churn from patch to patch by starting 
with the correct type in the previous patch.

>   
>   	get_task_comm(process_name, current->group_leader);
>   	desc_len = strlen(process_name);
>   
> +	mutex_lock(&filp->client_name_lock);
> +	if (filp->client_name)
> +		desc_len += 1 + strlen(filp->client_name);
> +
>   	vm->task_info = kzalloc(
>   		struct_size(vm->task_info, process_desc, desc_len + 1),
>   		GFP_KERNEL);
>   
> -	if (!vm->task_info)
> +	if (!vm->task_info) {
> +		mutex_unlock(&filp->client_name_lock);
>   		return -ENOMEM;
> +	}
>   
>   	/* Set process attributes now. */
>   	vm->task_info->tgid = current->group_leader->pid;
>   	strscpy(vm->task_info->process_desc, process_name, desc_len + 1);
>   
> +	if (filp->client_name) {
> +		size_t p_len = strlen(process_name);

Another nit is that you are taking this strlen twice. Maybe cache it in 
a top level local so it looks cleaner.

But those are just nits to make the series look more polished. 
Fundamentals look fine to me so up to you if you want to respin or not.

Regards,

Tvrtko

> +
> +		vm->task_info->process_desc[p_len] = '/';
> +		strscpy(&vm->task_info->process_desc[p_len + 1],
> +			filp->client_name, (desc_len + 1) - (p_len + 1));
> +	}
> +	mutex_unlock(&filp->client_name_lock);
> +
>   	kref_init(&vm->task_info->refcount);
>   	return 0;
>   }
> @@ -2382,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;
>   	} else if (vm->task_info->pid == current->pid) {
>   		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 v4 0/6] DRM_SET_CLIENT_NAME ioctl

[Christian König]

Am 27.09.24 um 10:48 schrieb Pierre-Eric Pelloux-Prayer:
> v4 changelog:
> * DRM_SET_NAME -> DRM_SET_CLIENT_NAME (Dmitry)
> * reject names that would mess up with formatting (Sima),
>    and use a stricter filter (isgraph allowed extended ASCII
>    which weren't looking great)
> * documentation edits, minor fixups (Dmitry, Trvtko)
> * clarified commit message of commit 3/6 (Trvtko)
> * reworked amdgpu_vm_set_task_info a bit in 4/6 (Trvtko)

If nobody has any more additional comments on this I'm going to pick it 
up and merge it through drm-misc-next by the end of today.

Regards,
Christian

>
> v3: https://lists.freedesktop.org/archives/dri-devel/2024-September/470488.html
>
> Pierre-Eric Pelloux-Prayer (6):
>    drm: add DRM_SET_CLIENT_NAME ioctl
>    drm: use drm_file client_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        | 62 +++++++++++++------
>   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                 | 14 +++--
>   drivers/gpu/drm/drm_file.c                    | 10 +++
>   drivers/gpu/drm/drm_ioctl.c                   | 55 ++++++++++++++++
>   include/drm/drm_file.h                        |  9 +++
>   include/uapi/drm/drm.h                        | 17 +++++
>   23 files changed, 171 insertions(+), 42 deletions(-)
>

Re: [PATCH v4 0/6] DRM_SET_CLIENT_NAME ioctl

[Jani Nikula]

On Fri, 27 Sep 2024, Christian König <christian.koenig@amd.com> wrote:
> Am 27.09.24 um 10:48 schrieb Pierre-Eric Pelloux-Prayer:
>> v4 changelog:
>> * DRM_SET_NAME -> DRM_SET_CLIENT_NAME (Dmitry)
>> * reject names that would mess up with formatting (Sima),
>>    and use a stricter filter (isgraph allowed extended ASCII
>>    which weren't looking great)
>> * documentation edits, minor fixups (Dmitry, Trvtko)
>> * clarified commit message of commit 3/6 (Trvtko)
>> * reworked amdgpu_vm_set_task_info a bit in 4/6 (Trvtko)
>
> If nobody has any more additional comments on this I'm going to pick it 
> up and merge it through drm-misc-next by the end of today.

AFAICT the userspace is not reviewed and ready for merging [1].

BR,
Jani.


[1] https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1428


-- 
Jani Nikula, Intel

Re: [PATCH v4 0/6] DRM_SET_CLIENT_NAME ioctl

[Dmitry Osipenko]

On 9/27/24 12:16, Jani Nikula wrote:
> On Fri, 27 Sep 2024, Christian König <christian.koenig@amd.com> wrote:
>> Am 27.09.24 um 10:48 schrieb Pierre-Eric Pelloux-Prayer:
>>> v4 changelog:
>>> * DRM_SET_NAME -> DRM_SET_CLIENT_NAME (Dmitry)
>>> * reject names that would mess up with formatting (Sima),
>>>    and use a stricter filter (isgraph allowed extended ASCII
>>>    which weren't looking great)
>>> * documentation edits, minor fixups (Dmitry, Trvtko)
>>> * clarified commit message of commit 3/6 (Trvtko)
>>> * reworked amdgpu_vm_set_task_info a bit in 4/6 (Trvtko)
>>
>> If nobody has any more additional comments on this I'm going to pick it 
>> up and merge it through drm-misc-next by the end of today.
> 
> AFAICT the userspace is not reviewed and ready for merging [1].
> 
> BR,
> Jani.
> 
> 
> [1] https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1428

Userspace has been discussed in the past. I'll merge virglrenderer MR
once Pierre will update it based on this v4 and after the kernel patch
will land to misc-next.

-- 
Best regards,
Dmitry