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@xxxxxxxxxxxx> > Reviewed-by: Josh Simonot <josh.simonot@xxxxxxxxx> > --- > 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@xxxxxxxxxxxxx> -- Best regards, Dmitry
