On Thu, May 28, 2020 at 10:06 AM Rohan Garg <rohan.garg@xxxxxxxxxxxxx> wrote:
>
> DRM_IOCTL_HANDLE_SET_LABEL lets you label buffers associated
> with a handle, making it easier to debug issues in userspace
> applications.
>
> DRM_IOCTL_HANDLE_GET_LABEL lets you read the label associated
> with a buffer.
>
> Changes in v2:
> - Hoist the IOCTL up into the drm_driver framework
>
> Changes in v3:
> - Introduce a drm_gem_set_label for drivers to use internally
> in order to label a GEM object
> - Hoist string copying up into the IOCTL
> - Fix documentation
> - Move actual gem labeling into drm_gem_adopt_label
>
> Changes in v4:
> - Refactor IOCTL call to only perform string duplication and move
> all gem lookup logic into GEM specific call
>
> Changes in v5:
> - Fix issues pointed out by kbuild test robot
> - Cleanup minor issues around kfree and out/err labels
> - Fixed API documentation issues
> - Rename to DRM_IOCTL_HANDLE_SET_LABEL
> - Introduce a DRM_IOCTL_HANDLE_GET_LABEL to read labels
> - Added some documentation for consumers of this IOCTL
> - Ensure that label's cannot be longer than PAGE_SIZE
> - Set a default label value
> - Drop useless warning
> - Properly return length of label to userspace even if
> userspace did not allocate memory for label.
>
> Changes in v6:
> - Wrap code to make better use of 80 char limit
> - Drop redundant copies of the label
> - Protect concurrent access to labels
> - Improved documentation
> - Refactor setter/getter ioctl's to be static
> - Return EINVAL when label length > PAGE_SIZE
> - Simplify code by calling the default GEM label'ing
> function for all drivers using GEM
> - Do not error out when fetching empty labels
> - Refactor flags to the u32 type and add documentation
> - Refactor ioctls to use correct DRM_IOCTL{R,W,WR} macros
> - Return length of copied label to userspace
>
> Signed-off-by: Rohan Garg <rohan.garg@xxxxxxxxxxxxx>
I don't think we should land this until it actually does something
with the label, that feels out of the spirit of our uapi merge rules.
I would suggest looking at dma_buf_set_name(), which would produce
useful output in debugfs's /dma_buf/buf_info. But also presumably you
have something in panfrost using this?
> +int drm_gem_get_label(struct drm_device *dev, struct drm_file *file_priv,
> + struct drm_handle_label *args)
> +{
> + struct drm_gem_object *gem_obj;
> + int len, ret;
> +
> + gem_obj = drm_gem_object_lookup(file_priv, args->handle);
> + if (!gem_obj) {
> + DRM_DEBUG("Failed to look up GEM BO %d\n", args->handle);
> + return -ENOENT;
> + }
> +
> + if (!gem_obj->label) {
> + args->label = NULL;
> + args->len = 0;
> + return 0;
> + }
> +
> + mutex_lock(&gem_obj->bo_lock);
> + len = strlen(gem_obj->label);
> + ret = copy_to_user(u64_to_user_ptr(args->label), gem_obj->label,
> + min(args->len, len));
> + mutex_unlock(&gem_obj->bo_lock);
> + args->len = len;
> + drm_gem_object_put(gem_obj);
> + return ret;
> +}
I think the !gem_obj->label code path also needs to be under the lock,
otherwise you could be racing to copy_to_user from a NULL pointer,
right?
> #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index bb924cddc09c..6fcb7b9ff322 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -540,6 +540,36 @@ struct drm_driver {
> struct drm_device *dev,
> uint32_t handle);
>
> + /**
> + * @set_label:
> + *
> + * Label a buffer object
> + *
> + * Called by the user via ioctl.
> + *
> + * Returns:
> + *
> + * Length of label on success, negative errno on failure.
> + */
> + int (*set_label)(struct drm_device *dev,
> + struct drm_file *file_priv,
> + struct drm_handle_label *args);
> +
> + /**
> + * @get_label:
> + *
> + * Read the label of a buffer object.
> + *
> + * Called by the user via ioctl.
> + *
> + * Returns:
> + *
> + * Length of label on success, negative errno on failiure.
> + */
> + char *(*get_label)(struct drm_device *dev,
> + struct drm_file *file_priv,
> + struct drm_handle_label *args);
> +
I think the documentation should note that these are optional.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
