| Message ID | 20190926153640.19817-1-rohan.garg@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/ioctl: Add a ioctl to label GEM objects | expand |
Hi Rohan First of all, I'm not in a position to merge new ioctls, so take my review with a grain of salt. You should get a review from one of the DRM maintainers. If no one shows up, ping danvet on IRC. Am 26.09.19 um 17:36 schrieb Rohan Garg: > DRM_IOCTL_BO_SET_LABEL lets you label GEM objects, making it The code says DRM_IOCTL_GEM_SET_LABEL. > easier to debug issues in userspace applications. I'd appreciate pointers to an actual user-space application that makes use of this functionality; or at least a prototype. We should not merge ioctls that have no users. You should also wire-up at least one driver, so that the code has an internal user. > > 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 > > Signed-off-by: Rohan Garg <rohan.garg@collabora.com> > --- > drivers/gpu/drm/drm_gem.c | 69 ++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_internal.h | 3 ++ > drivers/gpu/drm/drm_ioctl.c | 1 + > include/drm/drm_drv.h | 13 +++++++ > include/drm/drm_gem.h | 7 ++++ > include/uapi/drm/drm.h | 20 ++++++++++ > 6 files changed, 113 insertions(+) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 6854f5867d51..24ab463e3a33 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -941,6 +941,70 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private) > idr_destroy(&file_private->object_idr); > } There's no generic ioctl interface for creating BOs in DRM. The closes thing is dumb buffers, which return plain, unaccelerated framebuffers. Drivers that want more have to provide their own ioctls. Therefore, I don't think that this ioctl should be specific to GEM buffer objects. Maybe make it a dump-buffer interface that defaults to the GEM implementation, see drm_mode_mmap_dumb_ioctl [1] for an example. Drivers with more sophisticated buffer objects can add their own ioctls for labeling. > +int drm_gem_set_label_ioctl(struct drm_device *dev, void *data, > + struct drm_file *file_priv) This function should be the default for dev->driver->label; similar to [1]. > +{ > + struct drm_gem_set_label_object *args = data; > + struct drm_gem_object *gem_obj; > + int ret = 0; > + char *label = NULL; > + > + if (!args->len || !args->name) > + return -EINVAL; > + > + if (!dev->driver->label) > + return -EOPNOTSUPP; > + > + if (args->len > 0) > + label = strndup_user(u64_to_user_ptr(args->name), args->len); > + > + if (IS_ERR(label)) { > + ret = PTR_ERR(label); > + goto err; > + } > + > + 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); > + ret = -ENOENT; > + goto err; > + } > + > + dev->driver->label(gem_obj, label, args->len); Call drm_gem_adopt_label() directly here. Using a callback from struct drm_driver is not the right level of abstraction at this point. > + drm_gem_object_put_unlocked(gem_obj); > + > + return 0; > +err: > + kfree(label); > + return ret; > +} > + > +static void drm_gem_adopt_label(struct drm_gem_object *gem_obj, char *label, int len) The adopt function doesn't really need len, so please leave it out. The value of label is NULL or the new label string. That should be all that this function needs. > +{ > + if ((len == 0 && label == NULL) || gem_obj->label) { > + kfree(gem_obj->label); > + gem_obj->label = NULL; > + } Please split this branch into two. If gem_obj->label is set, you always want to kfree the memory. Afterwards, assign the label's new value. How can a caller clear the label. > + > + gem_obj->label = label; > +} > +EXPORT_SYMBOL(drm_gem_adopt_label); You're exporting a static function. > + > +int drm_gem_set_label(struct drm_gem_object *gem_obj, const char *label, int len) > +{ > + char *new_label = NULL; > + > + if (label) { > + new_label = kstrndup(label, len, GFP_KERNEL); Please also don't use len and simply call kstrdup(). > + if (IS_ERR(new_label)) > + return PTR_ERR(new_label); > + } > + > + drm_gem_adopt_label(gem_obj, new_label, len); > + return 0; > +} > +EXPORT_SYMBOL(drm_gem_set_label); > + > /** > * drm_gem_object_release - release GEM buffer object resources > * @obj: GEM buffer object > @@ -958,6 +1022,11 @@ drm_gem_object_release(struct drm_gem_object *obj) > > dma_resv_fini(&obj->_resv); > drm_gem_free_mmap_offset(obj); > + > + if (obj->label) { > + kfree(obj->label); > + obj->label = NULL; > + } > } > EXPORT_SYMBOL(drm_gem_object_release); > > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h > index 51a2055c8f18..c4d6e8bcd34f 100644 > --- a/drivers/gpu/drm/drm_internal.h > +++ b/drivers/gpu/drm/drm_internal.h > @@ -137,6 +137,9 @@ int drm_gem_pin(struct drm_gem_object *obj); > void drm_gem_unpin(struct drm_gem_object *obj); > void *drm_gem_vmap(struct drm_gem_object *obj); > void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr); > +int drm_gem_set_label_ioctl(struct drm_device *dev, void *data, > + struct drm_file *file_priv); > +int drm_gem_set_label(struct drm_gem_object *gem_obj, const char *label, int len); > > /* drm_debugfs.c drm_debugfs_crc.c */ > #if defined(CONFIG_DEBUG_FS) > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index f675a3bb2c88..3316e6a12a26 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -709,6 +709,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { > DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER), > + DRM_IOCTL_DEF(DRM_IOCTL_GEM_SET_LABEL, drm_gem_set_label_ioctl, DRM_RENDER_ALLOW), As mentioned, I'd rather use DRM_IOCTL_MODE_LABEL_DUMP that labels a dumb buffer. Best regards Thomas [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_dumb_buffers.c?h=v5.3#n117 > }; > > #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls ) > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h > index 8976afe48c1c..5150ddf5031c 100644 > --- a/include/drm/drm_drv.h > +++ b/include/drm/drm_drv.h > @@ -715,6 +715,19 @@ struct drm_driver { > struct drm_device *dev, > uint32_t handle); > > + /** > + * @label: > + * > + * This label's a buffer object. > + * > + * Called by the user via ioctl. > + * > + * Returns: > + * > + * Zero on success, negative errno on failure. > + */ > + int (*label)(struct drm_gem_object *gem_obj, const char *label, int len); > + > /** > * @gem_vm_ops: Driver private ops for this object > * > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > index 6aaba14f5972..f801c054e972 100644 > --- a/include/drm/drm_gem.h > +++ b/include/drm/drm_gem.h > @@ -237,6 +237,13 @@ struct drm_gem_object { > */ > int name; > > + /** > + * @label: > + * > + * Label for this object, should be a human readable string. > + */ > + char *label; > + > /** > * @dma_buf: > * > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index 8a5b2f8f8eb9..56fa009e9be6 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -626,6 +626,25 @@ struct drm_gem_open { > __u64 size; > }; > > +/** struct drm_gem_set_label_object - ioctl argument for labelling BOs. > + * > + * This label's a BO with a userspace label > + * > + */ > +struct drm_gem_set_label_object { > + /** Handle for the object being labelled. */ > + __u32 handle; > + > + /** Label and label length. > + * len includes the trailing NUL. > + */ > + __u32 len; > + __u64 name; > + > + /** Flags */ > + int flags; > +}; > + > #define DRM_CAP_DUMB_BUFFER 0x1 > #define DRM_CAP_VBLANK_HIGH_CRTC 0x2 > #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3 > @@ -946,6 +965,7 @@ extern "C" { > #define DRM_IOCTL_SYNCOBJ_QUERY DRM_IOWR(0xCB, struct drm_syncobj_timeline_array) > #define DRM_IOCTL_SYNCOBJ_TRANSFER DRM_IOWR(0xCC, struct drm_syncobj_transfer) > #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL DRM_IOWR(0xCD, struct drm_syncobj_timeline_array) > +#define DRM_IOCTL_GEM_SET_LABEL DRM_IOWR(0xCE, struct drm_gem_set_label_object) > > /** > * Device specific ioctls should only be in their respective headers >
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 6854f5867d51..24ab463e3a33 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -941,6 +941,70 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private) idr_destroy(&file_private->object_idr); } +int drm_gem_set_label_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_priv) +{ + struct drm_gem_set_label_object *args = data; + struct drm_gem_object *gem_obj; + int ret = 0; + char *label = NULL; + + if (!args->len || !args->name) + return -EINVAL; + + if (!dev->driver->label) + return -EOPNOTSUPP; + + if (args->len > 0) + label = strndup_user(u64_to_user_ptr(args->name), args->len); + + if (IS_ERR(label)) { + ret = PTR_ERR(label); + goto err; + } + + 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); + ret = -ENOENT; + goto err; + } + + dev->driver->label(gem_obj, label, args->len); + drm_gem_object_put_unlocked(gem_obj); + + return 0; +err: + kfree(label); + return ret; +} + +static void drm_gem_adopt_label(struct drm_gem_object *gem_obj, char *label, int len) +{ + if ((len == 0 && label == NULL) || gem_obj->label) { + kfree(gem_obj->label); + gem_obj->label = NULL; + } + + gem_obj->label = label; +} +EXPORT_SYMBOL(drm_gem_adopt_label); + +int drm_gem_set_label(struct drm_gem_object *gem_obj, const char *label, int len) +{ + char *new_label = NULL; + + if (label) { + new_label = kstrndup(label, len, GFP_KERNEL); + if (IS_ERR(new_label)) + return PTR_ERR(new_label); + } + + drm_gem_adopt_label(gem_obj, new_label, len); + return 0; +} +EXPORT_SYMBOL(drm_gem_set_label); + /** * drm_gem_object_release - release GEM buffer object resources * @obj: GEM buffer object @@ -958,6 +1022,11 @@ drm_gem_object_release(struct drm_gem_object *obj) dma_resv_fini(&obj->_resv); drm_gem_free_mmap_offset(obj); + + if (obj->label) { + kfree(obj->label); + obj->label = NULL; + } } EXPORT_SYMBOL(drm_gem_object_release); diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 51a2055c8f18..c4d6e8bcd34f 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -137,6 +137,9 @@ int drm_gem_pin(struct drm_gem_object *obj); void drm_gem_unpin(struct drm_gem_object *obj); void *drm_gem_vmap(struct drm_gem_object *obj); void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr); +int drm_gem_set_label_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_priv); +int drm_gem_set_label(struct drm_gem_object *gem_obj, const char *label, int len); /* drm_debugfs.c drm_debugfs_crc.c */ #if defined(CONFIG_DEBUG_FS) diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index f675a3bb2c88..3316e6a12a26 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -709,6 +709,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER), DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER), + DRM_IOCTL_DEF(DRM_IOCTL_GEM_SET_LABEL, drm_gem_set_label_ioctl, DRM_RENDER_ALLOW), }; #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls ) diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 8976afe48c1c..5150ddf5031c 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -715,6 +715,19 @@ struct drm_driver { struct drm_device *dev, uint32_t handle); + /** + * @label: + * + * This label's a buffer object. + * + * Called by the user via ioctl. + * + * Returns: + * + * Zero on success, negative errno on failure. + */ + int (*label)(struct drm_gem_object *gem_obj, const char *label, int len); + /** * @gem_vm_ops: Driver private ops for this object * diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 6aaba14f5972..f801c054e972 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -237,6 +237,13 @@ struct drm_gem_object { */ int name; + /** + * @label: + * + * Label for this object, should be a human readable string. + */ + char *label; + /** * @dma_buf: * diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 8a5b2f8f8eb9..56fa009e9be6 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -626,6 +626,25 @@ struct drm_gem_open { __u64 size; }; +/** struct drm_gem_set_label_object - ioctl argument for labelling BOs. + * + * This label's a BO with a userspace label + * + */ +struct drm_gem_set_label_object { + /** Handle for the object being labelled. */ + __u32 handle; + + /** Label and label length. + * len includes the trailing NUL. + */ + __u32 len; + __u64 name; + + /** Flags */ + int flags; +}; + #define DRM_CAP_DUMB_BUFFER 0x1 #define DRM_CAP_VBLANK_HIGH_CRTC 0x2 #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3 @@ -946,6 +965,7 @@ extern "C" { #define DRM_IOCTL_SYNCOBJ_QUERY DRM_IOWR(0xCB, struct drm_syncobj_timeline_array) #define DRM_IOCTL_SYNCOBJ_TRANSFER DRM_IOWR(0xCC, struct drm_syncobj_transfer) #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL DRM_IOWR(0xCD, struct drm_syncobj_timeline_array) +#define DRM_IOCTL_GEM_SET_LABEL DRM_IOWR(0xCE, struct drm_gem_set_label_object) /** * Device specific ioctls should only be in their respective headers
DRM_IOCTL_BO_SET_LABEL lets you label GEM objects, making it easier to debug issues in userspace applications. 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 Signed-off-by: Rohan Garg <rohan.garg@collabora.com> --- drivers/gpu/drm/drm_gem.c | 69 ++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_internal.h | 3 ++ drivers/gpu/drm/drm_ioctl.c | 1 + include/drm/drm_drv.h | 13 +++++++ include/drm/drm_gem.h | 7 ++++ include/uapi/drm/drm.h | 20 ++++++++++ 6 files changed, 113 insertions(+)