[PATCH v3 1/2] drm/virtio: use uint64_t more in virtio_gpu_context_init_ioctl

[PATCH v3 1/2] drm/virtio: use uint64_t more in virtio_gpu_context_init_ioctl

[Gurchetan Singh]

drm_virtgpu_context_set_param defines both param and
value to be u64s.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
Reviewed-by: Josh Simonot <josh.simonot@gmail.com>
Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index b24b11f25197..8d13b17c215b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -565,8 +565,8 @@ static int virtio_gpu_context_init_ioctl(struct drm_device *dev,
 					 void *data, struct drm_file *file)
 {
 	int ret = 0;
-	uint32_t num_params, i, param, value;
-	uint64_t valid_ring_mask;
+	uint32_t num_params, i;
+	uint64_t valid_ring_mask, param, value;
 	size_t len;
 	struct drm_virtgpu_context_set_param *ctx_set_params = NULL;
 	struct virtio_gpu_device *vgdev = dev->dev_private;
-- 
2.42.0.655.g421f12c284-goog

[PATCH v3 2/2] drm/uapi: add explicit virtgpu context debug name

[Gurchetan Singh]

There are two problems with the current method of determining the
virtio-gpu debug name.

1) TASK_COMM_LEN is defined to be 16 bytes only, and this is a
   Linux kernel idiom (see PR_SET_NAME + PR_GET_NAME). Though,
   Android/FreeBSD get around this via setprogname(..)/getprogname(..)
   in libc.

   On Android, names longer than 16 bytes are common.  For example,
   one often encounters a program like "com.android.systemui".

   The virtio-gpu spec allows the debug name to be up to 64 bytes, so
   ideally userspace should be able to set debug names up to 64 bytes.

2) The current implementation determines the debug name using whatever
   task initiated virtgpu.  This is could be a "RenderThread" of a
   larger program, when we actually want to propagate the debug name
   of the program.

To fix these issues, add a new CONTEXT_INIT param that allows userspace
to set the debug name when creating a context.

It takes a null-terminated C-string as the param value. The length of the
string (excluding the terminator) **should** be <= 64 bytes.  Otherwise,
the debug_name will be truncated to 64 bytes.

Link to open-source userspace:
https://android-review.googlesource.com/c/platform/hardware/google/gfxstream/+/2787176

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
Reviewed-by: Josh Simonot <josh.simonot@gmail.com>
---
Fixes suggested by Dmitry Osipenko
v2:
    - Squash implementation and UAPI change into one commit
    - Avoid unnecessary casts
    - Use bool when necessary
v3:
    - Use DEBUG_NAME_MAX_LEN - 1 when copying string

 drivers/gpu/drm/virtio/virtgpu_drv.h   |  5 ++++
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 39 ++++++++++++++++++++++----
 include/uapi/drm/virtgpu_drm.h         |  2 ++
 3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 96365a772f77..bb7d86a0c6a1 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -58,6 +58,9 @@
 #define MAX_CAPSET_ID 63
 #define MAX_RINGS 64
 
+/* See virtio_gpu_ctx_create. One additional character for NULL terminator. */
+#define DEBUG_NAME_MAX_LEN 65
+
 struct virtio_gpu_object_params {
 	unsigned long size;
 	bool dumb;
@@ -274,6 +277,8 @@ struct virtio_gpu_fpriv {
 	uint64_t base_fence_ctx;
 	uint64_t ring_idx_mask;
 	struct mutex context_lock;
+	char debug_name[DEBUG_NAME_MAX_LEN];
+	bool explicit_debug_name;
 };
 
 /* virtgpu_ioctl.c */
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 8d13b17c215b..65811e818925 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -42,12 +42,19 @@
 static void virtio_gpu_create_context_locked(struct virtio_gpu_device *vgdev,
 					     struct virtio_gpu_fpriv *vfpriv)
 {
-	char dbgname[TASK_COMM_LEN];
+	if (vfpriv->explicit_debug_name) {
+		virtio_gpu_cmd_context_create(vgdev, vfpriv->ctx_id,
+					      vfpriv->context_init,
+					      strlen(vfpriv->debug_name),
+					      vfpriv->debug_name);
+	} else {
+		char dbgname[TASK_COMM_LEN];
 
-	get_task_comm(dbgname, current);
-	virtio_gpu_cmd_context_create(vgdev, vfpriv->ctx_id,
-				      vfpriv->context_init, strlen(dbgname),
-				      dbgname);
+		get_task_comm(dbgname, current);
+		virtio_gpu_cmd_context_create(vgdev, vfpriv->ctx_id,
+					      vfpriv->context_init, strlen(dbgname),
+					      dbgname);
+	}
 
 	vfpriv->context_created = true;
 }
@@ -107,6 +114,9 @@ static int virtio_gpu_getparam_ioctl(struct drm_device *dev, void *data,
 	case VIRTGPU_PARAM_SUPPORTED_CAPSET_IDs:
 		value = vgdev->capset_id_mask;
 		break;
+	case VIRTGPU_PARAM_EXPLICIT_DEBUG_NAME:
+		value = vgdev->has_context_init ? 1 : 0;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -580,7 +590,7 @@ static int virtio_gpu_context_init_ioctl(struct drm_device *dev,
 		return -EINVAL;
 
 	/* Number of unique parameters supported at this time. */
-	if (num_params > 3)
+	if (num_params > 4)
 		return -EINVAL;
 
 	ctx_set_params = memdup_user(u64_to_user_ptr(args->ctx_set_params),
@@ -642,6 +652,23 @@ static int virtio_gpu_context_init_ioctl(struct drm_device *dev,
 
 			vfpriv->ring_idx_mask = value;
 			break;
+		case VIRTGPU_CONTEXT_PARAM_DEBUG_NAME:
+			if (vfpriv->explicit_debug_name) {
+				ret = -EINVAL;
+				goto out_unlock;
+			}
+
+			ret = strncpy_from_user(vfpriv->debug_name,
+						u64_to_user_ptr(value),
+						DEBUG_NAME_MAX_LEN - 1);
+
+			if (ret < 0) {
+				ret = -EFAULT;
+				goto out_unlock;
+			}
+
+			vfpriv->explicit_debug_name = true;
+			break;
 		default:
 			ret = -EINVAL;
 			goto out_unlock;
diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
index b1d0e56565bc..c2ce71987e9b 100644
--- a/include/uapi/drm/virtgpu_drm.h
+++ b/include/uapi/drm/virtgpu_drm.h
@@ -97,6 +97,7 @@ struct drm_virtgpu_execbuffer {
 #define VIRTGPU_PARAM_CROSS_DEVICE 5 /* Cross virtio-device resource sharing  */
 #define VIRTGPU_PARAM_CONTEXT_INIT 6 /* DRM_VIRTGPU_CONTEXT_INIT */
 #define VIRTGPU_PARAM_SUPPORTED_CAPSET_IDs 7 /* Bitmask of supported capability set ids */
+#define VIRTGPU_PARAM_EXPLICIT_DEBUG_NAME 8 /* Ability to set debug name from userspace */
 
 struct drm_virtgpu_getparam {
 	__u64 param;
@@ -198,6 +199,7 @@ struct drm_virtgpu_resource_create_blob {
 #define VIRTGPU_CONTEXT_PARAM_CAPSET_ID       0x0001
 #define VIRTGPU_CONTEXT_PARAM_NUM_RINGS       0x0002
 #define VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK 0x0003
+#define VIRTGPU_CONTEXT_PARAM_DEBUG_NAME      0x0004
 struct drm_virtgpu_context_set_param {
 	__u64 param;
 	__u64 value;
-- 
2.42.0.655.g421f12c284-goog

Re: [PATCH v3 2/2] drm/uapi: add explicit virtgpu context debug name

[Dmitry Osipenko]

On 10/18/23 21:17, Gurchetan Singh wrote:
> There are two problems with the current method of determining the
> virtio-gpu debug name.
> 
> 1) TASK_COMM_LEN is defined to be 16 bytes only, and this is a
>    Linux kernel idiom (see PR_SET_NAME + PR_GET_NAME). Though,
>    Android/FreeBSD get around this via setprogname(..)/getprogname(..)
>    in libc.
> 
>    On Android, names longer than 16 bytes are common.  For example,
>    one often encounters a program like "com.android.systemui".
> 
>    The virtio-gpu spec allows the debug name to be up to 64 bytes, so
>    ideally userspace should be able to set debug names up to 64 bytes.
> 
> 2) The current implementation determines the debug name using whatever
>    task initiated virtgpu.  This is could be a "RenderThread" of a
>    larger program, when we actually want to propagate the debug name
>    of the program.
> 
> To fix these issues, add a new CONTEXT_INIT param that allows userspace
> to set the debug name when creating a context.
> 
> It takes a null-terminated C-string as the param value. The length of the
> string (excluding the terminator) **should** be <= 64 bytes.  Otherwise,
> the debug_name will be truncated to 64 bytes.
> 
> Link to open-source userspace:
> https://android-review.googlesource.com/c/platform/hardware/google/gfxstream/+/2787176
> 
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> Reviewed-by: Josh Simonot <josh.simonot@gmail.com>
> ---
> Fixes suggested by Dmitry Osipenko
> v2:
>     - Squash implementation and UAPI change into one commit
>     - Avoid unnecessary casts
>     - Use bool when necessary
> v3:
>     - Use DEBUG_NAME_MAX_LEN - 1 when copying string
> 
>  drivers/gpu/drm/virtio/virtgpu_drv.h   |  5 ++++
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c | 39 ++++++++++++++++++++++----
>  include/uapi/drm/virtgpu_drm.h         |  2 ++
>  3 files changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 96365a772f77..bb7d86a0c6a1 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -58,6 +58,9 @@
>  #define MAX_CAPSET_ID 63
>  #define MAX_RINGS 64
>  
> +/* See virtio_gpu_ctx_create. One additional character for NULL terminator. */
> +#define DEBUG_NAME_MAX_LEN 65
> +
>  struct virtio_gpu_object_params {
>  	unsigned long size;
>  	bool dumb;
> @@ -274,6 +277,8 @@ struct virtio_gpu_fpriv {
>  	uint64_t base_fence_ctx;
>  	uint64_t ring_idx_mask;
>  	struct mutex context_lock;
> +	char debug_name[DEBUG_NAME_MAX_LEN];
> +	bool explicit_debug_name;
>  };
>  
>  /* virtgpu_ioctl.c */
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index 8d13b17c215b..65811e818925 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -42,12 +42,19 @@
>  static void virtio_gpu_create_context_locked(struct virtio_gpu_device *vgdev,
>  					     struct virtio_gpu_fpriv *vfpriv)
>  {
> -	char dbgname[TASK_COMM_LEN];
> +	if (vfpriv->explicit_debug_name) {
> +		virtio_gpu_cmd_context_create(vgdev, vfpriv->ctx_id,
> +					      vfpriv->context_init,
> +					      strlen(vfpriv->debug_name),
> +					      vfpriv->debug_name);
> +	} else {
> +		char dbgname[TASK_COMM_LEN];
>  
> -	get_task_comm(dbgname, current);
> -	virtio_gpu_cmd_context_create(vgdev, vfpriv->ctx_id,
> -				      vfpriv->context_init, strlen(dbgname),
> -				      dbgname);
> +		get_task_comm(dbgname, current);
> +		virtio_gpu_cmd_context_create(vgdev, vfpriv->ctx_id,
> +					      vfpriv->context_init, strlen(dbgname),
> +					      dbgname);
> +	}
>  
>  	vfpriv->context_created = true;
>  }
> @@ -107,6 +114,9 @@ static int virtio_gpu_getparam_ioctl(struct drm_device *dev, void *data,
>  	case VIRTGPU_PARAM_SUPPORTED_CAPSET_IDs:
>  		value = vgdev->capset_id_mask;
>  		break;
> +	case VIRTGPU_PARAM_EXPLICIT_DEBUG_NAME:
> +		value = vgdev->has_context_init ? 1 : 0;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -580,7 +590,7 @@ static int virtio_gpu_context_init_ioctl(struct drm_device *dev,
>  		return -EINVAL;
>  
>  	/* Number of unique parameters supported at this time. */
> -	if (num_params > 3)
> +	if (num_params > 4)
>  		return -EINVAL;
>  
>  	ctx_set_params = memdup_user(u64_to_user_ptr(args->ctx_set_params),
> @@ -642,6 +652,23 @@ static int virtio_gpu_context_init_ioctl(struct drm_device *dev,
>  
>  			vfpriv->ring_idx_mask = value;
>  			break;
> +		case VIRTGPU_CONTEXT_PARAM_DEBUG_NAME:
> +			if (vfpriv->explicit_debug_name) {
> +				ret = -EINVAL;
> +				goto out_unlock;
> +			}
> +
> +			ret = strncpy_from_user(vfpriv->debug_name,
> +						u64_to_user_ptr(value),
> +						DEBUG_NAME_MAX_LEN - 1);
> +
> +			if (ret < 0) {
> +				ret = -EFAULT;
> +				goto out_unlock;
> +			}
> +
> +			vfpriv->explicit_debug_name = true;
> +			break;
>  		default:
>  			ret = -EINVAL;
>  			goto out_unlock;
> diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
> index b1d0e56565bc..c2ce71987e9b 100644
> --- a/include/uapi/drm/virtgpu_drm.h
> +++ b/include/uapi/drm/virtgpu_drm.h
> @@ -97,6 +97,7 @@ struct drm_virtgpu_execbuffer {
>  #define VIRTGPU_PARAM_CROSS_DEVICE 5 /* Cross virtio-device resource sharing  */
>  #define VIRTGPU_PARAM_CONTEXT_INIT 6 /* DRM_VIRTGPU_CONTEXT_INIT */
>  #define VIRTGPU_PARAM_SUPPORTED_CAPSET_IDs 7 /* Bitmask of supported capability set ids */
> +#define VIRTGPU_PARAM_EXPLICIT_DEBUG_NAME 8 /* Ability to set debug name from userspace */
>  
>  struct drm_virtgpu_getparam {
>  	__u64 param;
> @@ -198,6 +199,7 @@ struct drm_virtgpu_resource_create_blob {
>  #define VIRTGPU_CONTEXT_PARAM_CAPSET_ID       0x0001
>  #define VIRTGPU_CONTEXT_PARAM_NUM_RINGS       0x0002
>  #define VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK 0x0003
> +#define VIRTGPU_CONTEXT_PARAM_DEBUG_NAME      0x0004
>  struct drm_virtgpu_context_set_param {
>  	__u64 param;
>  	__u64 value;

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

-- 
Best regards,
Dmitry

Re: [PATCH v3 2/2] drm/uapi: add explicit virtgpu context debug name

[Dmitry Osipenko]

On 10/18/23 21:17, Gurchetan Singh wrote:
> There are two problems with the current method of determining the
> virtio-gpu debug name.
> 
> 1) TASK_COMM_LEN is defined to be 16 bytes only, and this is a
>    Linux kernel idiom (see PR_SET_NAME + PR_GET_NAME). Though,
>    Android/FreeBSD get around this via setprogname(..)/getprogname(..)
>    in libc.
> 
>    On Android, names longer than 16 bytes are common.  For example,
>    one often encounters a program like "com.android.systemui".
> 
>    The virtio-gpu spec allows the debug name to be up to 64 bytes, so
>    ideally userspace should be able to set debug names up to 64 bytes.
> 
> 2) The current implementation determines the debug name using whatever
>    task initiated virtgpu.  This is could be a "RenderThread" of a
>    larger program, when we actually want to propagate the debug name
>    of the program.
> 
> To fix these issues, add a new CONTEXT_INIT param that allows userspace
> to set the debug name when creating a context.
> 
> It takes a null-terminated C-string as the param value. The length of the
> string (excluding the terminator) **should** be <= 64 bytes.  Otherwise,
> the debug_name will be truncated to 64 bytes.
> 
> Link to open-source userspace:
> https://android-review.googlesource.com/c/platform/hardware/google/gfxstream/+/2787176
> 
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> Reviewed-by: Josh Simonot <josh.simonot@gmail.com>
> ---
> Fixes suggested by Dmitry Osipenko
> v2:
>     - Squash implementation and UAPI change into one commit
>     - Avoid unnecessary casts
>     - Use bool when necessary
> v3:
>     - Use DEBUG_NAME_MAX_LEN - 1 when copying string
> 
>  drivers/gpu/drm/virtio/virtgpu_drv.h   |  5 ++++
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c | 39 ++++++++++++++++++++++----
>  include/uapi/drm/virtgpu_drm.h         |  2 ++
>  3 files changed, 40 insertions(+), 6 deletions(-)

Gerd, do you have objections to this UAPI change?

-- 
Best regards,
Dmitry

Re: [PATCH v3 2/2] drm/uapi: add explicit virtgpu context debug name

[Gurchetan Singh]

[-- Attachment #1: Type: text/plain, Size: 2530 bytes --]

On Sun, Oct 22, 2023 at 4:50 PM Dmitry Osipenko <
dmitry.osipenko@collabora.com> wrote:

> On 10/18/23 21:17, Gurchetan Singh wrote:
> > There are two problems with the current method of determining the
> > virtio-gpu debug name.
> >
> > 1) TASK_COMM_LEN is defined to be 16 bytes only, and this is a
> >    Linux kernel idiom (see PR_SET_NAME + PR_GET_NAME). Though,
> >    Android/FreeBSD get around this via setprogname(..)/getprogname(..)
> >    in libc.
> >
> >    On Android, names longer than 16 bytes are common.  For example,
> >    one often encounters a program like "com.android.systemui".
> >
> >    The virtio-gpu spec allows the debug name to be up to 64 bytes, so
> >    ideally userspace should be able to set debug names up to 64 bytes.
> >
> > 2) The current implementation determines the debug name using whatever
> >    task initiated virtgpu.  This is could be a "RenderThread" of a
> >    larger program, when we actually want to propagate the debug name
> >    of the program.
> >
> > To fix these issues, add a new CONTEXT_INIT param that allows userspace
> > to set the debug name when creating a context.
> >
> > It takes a null-terminated C-string as the param value. The length of the
> > string (excluding the terminator) **should** be <= 64 bytes.  Otherwise,
> > the debug_name will be truncated to 64 bytes.
> >
> > Link to open-source userspace:
> >
> https://android-review.googlesource.com/c/platform/hardware/google/gfxstream/+/2787176
> >
> > Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> > Reviewed-by: Josh Simonot <josh.simonot@gmail.com>
> > ---
> > Fixes suggested by Dmitry Osipenko
> > v2:
> >     - Squash implementation and UAPI change into one commit
> >     - Avoid unnecessary casts
> >     - Use bool when necessary
> > v3:
> >     - Use DEBUG_NAME_MAX_LEN - 1 when copying string
> >
> >  drivers/gpu/drm/virtio/virtgpu_drv.h   |  5 ++++
> >  drivers/gpu/drm/virtio/virtgpu_ioctl.c | 39 ++++++++++++++++++++++----
> >  include/uapi/drm/virtgpu_drm.h         |  2 ++
> >  3 files changed, 40 insertions(+), 6 deletions(-)
>
> Gerd, do you have objections to this UAPI change?
>

Bump.  I say we wait another week and see if anyone cares [I suspect nobody
does].


https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-misc.html#merge-criteria

As per DRM guidelines, if there are no open comments and the change is
reviewed, it is mergeable.


>
> --
> Best regards,
> Dmitry
>
>

[-- Attachment #2: Type: text/html, Size: 3736 bytes --]

Re: [PATCH v3 2/2] drm/uapi: add explicit virtgpu context debug name

[Gurchetan Singh]

[-- Attachment #1: Type: text/plain, Size: 2757 bytes --]

On Tue, Oct 31, 2023 at 8:55 AM Gurchetan Singh <gurchetansingh@chromium.org>
wrote:

>
>
> On Sun, Oct 22, 2023 at 4:50 PM Dmitry Osipenko <
> dmitry.osipenko@collabora.com> wrote:
>
>> On 10/18/23 21:17, Gurchetan Singh wrote:
>> > There are two problems with the current method of determining the
>> > virtio-gpu debug name.
>> >
>> > 1) TASK_COMM_LEN is defined to be 16 bytes only, and this is a
>> >    Linux kernel idiom (see PR_SET_NAME + PR_GET_NAME). Though,
>> >    Android/FreeBSD get around this via setprogname(..)/getprogname(..)
>> >    in libc.
>> >
>> >    On Android, names longer than 16 bytes are common.  For example,
>> >    one often encounters a program like "com.android.systemui".
>> >
>> >    The virtio-gpu spec allows the debug name to be up to 64 bytes, so
>> >    ideally userspace should be able to set debug names up to 64 bytes.
>> >
>> > 2) The current implementation determines the debug name using whatever
>> >    task initiated virtgpu.  This is could be a "RenderThread" of a
>> >    larger program, when we actually want to propagate the debug name
>> >    of the program.
>> >
>> > To fix these issues, add a new CONTEXT_INIT param that allows userspace
>> > to set the debug name when creating a context.
>> >
>> > It takes a null-terminated C-string as the param value. The length of
>> the
>> > string (excluding the terminator) **should** be <= 64 bytes.  Otherwise,
>> > the debug_name will be truncated to 64 bytes.
>> >
>> > Link to open-source userspace:
>> >
>> https://android-review.googlesource.com/c/platform/hardware/google/gfxstream/+/2787176
>> >
>> > Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
>> > Reviewed-by: Josh Simonot <josh.simonot@gmail.com>
>> > ---
>> > Fixes suggested by Dmitry Osipenko
>> > v2:
>> >     - Squash implementation and UAPI change into one commit
>> >     - Avoid unnecessary casts
>> >     - Use bool when necessary
>> > v3:
>> >     - Use DEBUG_NAME_MAX_LEN - 1 when copying string
>> >
>> >  drivers/gpu/drm/virtio/virtgpu_drv.h   |  5 ++++
>> >  drivers/gpu/drm/virtio/virtgpu_ioctl.c | 39 ++++++++++++++++++++++----
>> >  include/uapi/drm/virtgpu_drm.h         |  2 ++
>> >  3 files changed, 40 insertions(+), 6 deletions(-)
>>
>> Gerd, do you have objections to this UAPI change?
>>
>
> Bump.  I say we wait another week and see if anyone cares [I suspect
> nobody does].
>
>
> https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-misc.html#merge-criteria
>
> As per DRM guidelines, if there are no open comments and the change is
> reviewed, it is mergeable.
>

*hears crickets*

Can we merge this now?


>
>>
>> --
>> Best regards,
>> Dmitry
>>
>>

[-- Attachment #2: Type: text/html, Size: 4477 bytes --]

Re: [PATCH v3 2/2] drm/uapi: add explicit virtgpu context debug name

[Dmitry Osipenko]

On 10/18/23 21:17, Gurchetan Singh wrote:
> There are two problems with the current method of determining the
> virtio-gpu debug name.
> 
> 1) TASK_COMM_LEN is defined to be 16 bytes only, and this is a
>    Linux kernel idiom (see PR_SET_NAME + PR_GET_NAME). Though,
>    Android/FreeBSD get around this via setprogname(..)/getprogname(..)
>    in libc.
> 
>    On Android, names longer than 16 bytes are common.  For example,
>    one often encounters a program like "com.android.systemui".
> 
>    The virtio-gpu spec allows the debug name to be up to 64 bytes, so
>    ideally userspace should be able to set debug names up to 64 bytes.
> 
> 2) The current implementation determines the debug name using whatever
>    task initiated virtgpu.  This is could be a "RenderThread" of a
>    larger program, when we actually want to propagate the debug name
>    of the program.
> 
> To fix these issues, add a new CONTEXT_INIT param that allows userspace
> to set the debug name when creating a context.
> 
> It takes a null-terminated C-string as the param value. The length of the
> string (excluding the terminator) **should** be <= 64 bytes.  Otherwise,
> the debug_name will be truncated to 64 bytes.
> 
> Link to open-source userspace:
> https://android-review.googlesource.com/c/platform/hardware/google/gfxstream/+/2787176
> 
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> Reviewed-by: Josh Simonot <josh.simonot@gmail.com>
> ---
> Fixes suggested by Dmitry Osipenko
> v2:
>     - Squash implementation and UAPI change into one commit
>     - Avoid unnecessary casts
>     - Use bool when necessary
> v3:
>     - Use DEBUG_NAME_MAX_LEN - 1 when copying string
> 
>  drivers/gpu/drm/virtio/virtgpu_drv.h   |  5 ++++
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c | 39 ++++++++++++++++++++++----
>  include/uapi/drm/virtgpu_drm.h         |  2 ++
>  3 files changed, 40 insertions(+), 6 deletions(-)
...
> +			ret = strncpy_from_user(vfpriv->debug_name,
> +						u64_to_user_ptr(value),
> +						DEBUG_NAME_MAX_LEN - 1);
> +
> +			if (ret < 0) {
> +				ret = -EFAULT;
> +				goto out_unlock;
> +			}

The strncpy_from_user() itself returns -EFAULT. I changed code to return
the "ret" directly and applied to misc-next.

Gerd, let us know if have any objections to this patch.

-- 
Best regards,
Dmitry

Re: [PATCH v3 2/2] drm/uapi: add explicit virtgpu context debug name

[Dmitry Osipenko]

On 10/18/23 21:17, Gurchetan Singh wrote:
> +		case VIRTGPU_CONTEXT_PARAM_DEBUG_NAME:
> +			if (vfpriv->explicit_debug_name) {
> +				ret = -EINVAL;
> +				goto out_unlock;
> +			}
> +
> +			ret = strncpy_from_user(vfpriv->debug_name,
> +						u64_to_user_ptr(value),
> +						DEBUG_NAME_MAX_LEN - 1);
> +
> +			if (ret < 0) {
> +				ret = -EFAULT;
> +				goto out_unlock;
> +			}
> +
> +			vfpriv->explicit_debug_name = true;
> +			break;

Spotted a problem here. The ret needs to be set to zero on success. I'll
send the fix shortly. Gurchetan you should've been getting the
DRM_IOCTL_VIRTGPU_CONTEXT_INIT failure from gfxstream when you tested
this patch, haven't you?

Also noticed that the patch title says "drm/uapi" instead of
"drm/virtio". My bad for not noticing it earlier. Please be more careful
next time too :)

-- 
Best regards,
Dmitry

Re: [PATCH v3 2/2] drm/uapi: add explicit virtgpu context debug name

[Gurchetan Singh]

[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]

On Sat, Nov 11, 2023 at 2:37 PM Dmitry Osipenko <
dmitry.osipenko@collabora.com> wrote:

> On 10/18/23 21:17, Gurchetan Singh wrote:
> > +             case VIRTGPU_CONTEXT_PARAM_DEBUG_NAME:
> > +                     if (vfpriv->explicit_debug_name) {
> > +                             ret = -EINVAL;
> > +                             goto out_unlock;
> > +                     }
> > +
> > +                     ret = strncpy_from_user(vfpriv->debug_name,
> > +                                             u64_to_user_ptr(value),
> > +                                             DEBUG_NAME_MAX_LEN - 1);
> > +
> > +                     if (ret < 0) {
> > +                             ret = -EFAULT;
> > +                             goto out_unlock;
> > +                     }
> > +
> > +                     vfpriv->explicit_debug_name = true;
> > +                     break;
>
> Spotted a problem here. The ret needs to be set to zero on success. I'll
> send the fix shortly. Gurchetan you should've been getting the
> DRM_IOCTL_VIRTGPU_CONTEXT_INIT failure from gfxstream when you tested
> this patch, haven't you?
>

To accommodate older kernels/QEMU, gfxstream doesn't fail if CONTEXT_INIT
fails.  So the guest thought it failed and didn't react, but the value was
propagated to the host.


>
> Also noticed that the patch title says "drm/uapi" instead of
> "drm/virtio". My bad for not noticing it earlier. Please be more careful
> next time too :)
>
> --
> Best regards,
> Dmitry
>
>

[-- Attachment #2: Type: text/html, Size: 2371 bytes --]