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