| Message ID | 20231018181727.772-2-gurchetansingh@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] drm/virtio: use uint64_t more in virtio_gpu_context_init_ioctl | expand |
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>
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?
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 > >
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 >> >>
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.
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 :)
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 > >
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;