[PATCH v6] drm/ioctl: Add a ioctl to set and get a label on GEM objects

[Eric Anholt]

On Thu, May 28, 2020 at 10:06 AM Rohan Garg <rohan.garg at collabora.com> 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 at collabora.com>

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.