| Message ID | 20190919125321.22880-1-rohan.garg@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/ioctl: Add a ioctl to label GEM objects | expand |
Hi Am 19.09.19 um 14:53 schrieb Rohan Garg: > 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 > > Signed-off-by: Rohan Garg <rohan.garg@collabora.com> > --- > drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_internal.h | 4 +++ > drivers/gpu/drm/drm_ioctl.c | 1 + > include/drm/drm_drv.h | 18 ++++++++++ > include/drm/drm_gem.h | 7 ++++ > include/uapi/drm/drm.h | 20 +++++++++++ > 6 files changed, 114 insertions(+) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 6854f5867d51..785ac561619a 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -941,6 +941,65 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private) > idr_destroy(&file_private->object_idr); > } > > +int drm_bo_set_label_ioctl(struct drm_device *dev, void *data, > + struct drm_file *file_priv) > +{ > + struct drm_label_object *args = data; > + > + if (!args->len || !args->name) > + return -EINVAL; > + > + if (dev->driver->label) > + return dev->driver->label(dev, args, file_priv); > + > + return -EOPNOTSUPP; > +} > + > +/** > + * drm_gem_label - label a given GEM object > + * @dev: drm_device for the associated GEM object > + * @data: drm_label_bo struct with a label, label length and any relevant flags > + * @file_private: drm file-private structure to clean up > + */ > + > +int drm_gem_set_label(struct drm_device *dev, struct drm_label_object *args, struct drm_file *file_priv) I'd like to set labels for internal GEM objects. Could you split off the object update code into something that is easily callable from within drivers? Something like // called by ioctl int drm_gem_object_adopt_label(struct drm_gem_object *gem, char *label) { // your object update code goes here } // called by drivers int drm_gem_object_set_label(struct drm_gem_object *gem, const char *label) { char* new_label = strdup(label) return drm_gem_object_adopt_label(gem, new_label); } We have debugfs support for TTM-based GEM objects at [1]. I think the label would be a welcome addition to the output. Best regards Thomas [1] https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/drm_gem_ttm_helper.c#n23 > +{ > + struct drm_gem_object *gem_obj; > + int err = 0; > + char *label; > + > + if (args->len > 0) > + label = strndup_user(u64_to_user_ptr(args->name), args->len); > + > + gem_obj = drm_gem_object_lookup(file_priv, args->handle); > + if (!gem_obj) { > + DRM_ERROR("Failed to look up GEM BO %d\n", args->handle); > + err = -ENOENT; > + goto err; > + } > + > + if ((args->len == 0 && args->name == NULL) || gem_obj->label) { > + kfree(gem_obj->label); > + gem_obj->label = NULL; > + } > + > + gem_obj->label = label; > + > + drm_gem_object_put_unlocked(gem_obj); > + > + if (IS_ERR(gem_obj->label)) { > + err = PTR_ERR(gem_obj->label); > + goto err; > + } > + > +err: > + if (err != 0) > + args->flags = err; > + > + return err; > +} > + > + > /** > * drm_gem_object_release - release GEM buffer object resources > * @obj: GEM buffer object > @@ -958,6 +1017,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..8259622f9ab6 100644 > --- a/drivers/gpu/drm/drm_internal.h > +++ b/drivers/gpu/drm/drm_internal.h > @@ -137,6 +137,10 @@ 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_bo_set_label_ioctl(struct drm_device *dev, void *data, > + struct drm_file *file_priv); > +int drm_gem_set_label(struct drm_device *dev, struct drm_label_object *args, > + struct drm_file *file_priv); > > /* 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..079d1422f9c5 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_BO_SET_LABEL, drm_bo_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..f736ba1f42a6 100644 > --- a/include/drm/drm_drv.h > +++ b/include/drm/drm_drv.h > @@ -715,6 +715,24 @@ struct drm_driver { > struct drm_device *dev, > uint32_t handle); > > + > + /** > + * @label: > + * > + * This label's a buffer object. > + * > + * Called by the user via ioctl. > + * > + * The default implementation is drm_gem_label(). GEM based drivers > + * must not overwrite this. > + > + * Returns: > + * > + * Zero on success, negative errno on failure. > + */ > + int (*label)(struct drm_device *dev, struct drm_label_object *args, > + struct drm_file *file_priv); > + > /** > * @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..23b507f5c571 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_label_object - ioctl argument for labelling BOs. > + * > + * This label's a BO with a userspace label > + * > + */ > +struct drm_label_object { > + /** Handle for the object being labelled. */ > + __u32 handle; > + > + /** Label and label length. > + * length includes the trailing NULL. > + */ > + __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_BO_SET_LABEL DRM_IOWR(0xCE, struct drm_label_object) > > /** > * Device specific ioctls should only be in their respective headers >
On Thu, Sep 19, 2019 at 02:53:21PM +0200, Rohan Garg wrote: > 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 > > Signed-off-by: Rohan Garg <rohan.garg@collabora.com> > --- > drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_internal.h | 4 +++ > drivers/gpu/drm/drm_ioctl.c | 1 + > include/drm/drm_drv.h | 18 ++++++++++ > include/drm/drm_gem.h | 7 ++++ > include/uapi/drm/drm.h | 20 +++++++++++ > 6 files changed, 114 insertions(+) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 6854f5867d51..785ac561619a 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -941,6 +941,65 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private) > idr_destroy(&file_private->object_idr); > } > > +int drm_bo_set_label_ioctl(struct drm_device *dev, void *data, > + struct drm_file *file_priv) Perhaps call this drm_gem_set_label_ioctl() so that it's consistent with the other GEM related IOCTLs? > +{ > + struct drm_label_object *args = data; Similarly, perhaps make this struct drm_gem_set_label for consistency. > + > + if (!args->len || !args->name) > + return -EINVAL; > + > + if (dev->driver->label) > + return dev->driver->label(dev, args, file_priv); > + > + return -EOPNOTSUPP; > +} > + > +/** > + * drm_gem_label - label a given GEM object > + * @dev: drm_device for the associated GEM object > + * @data: drm_label_bo struct with a label, label length and any relevant flags > + * @file_private: drm file-private structure to clean up > + */ > + > +int drm_gem_set_label(struct drm_device *dev, struct drm_label_object *args, struct drm_file *file_priv) Function name doesn't match the kerneldoc. > +{ > + struct drm_gem_object *gem_obj; > + int err = 0; > + char *label; > + > + if (args->len > 0) > + label = strndup_user(u64_to_user_ptr(args->name), args->len); > + > + gem_obj = drm_gem_object_lookup(file_priv, args->handle); > + if (!gem_obj) { > + DRM_ERROR("Failed to look up GEM BO %d\n", args->handle); People could abuse this to spam the system logs with these error messages. Better make this DRM_DEBUG() or leave it out completely. > + err = -ENOENT; > + goto err; I think you're leaking the label string here. > + } > + > + if ((args->len == 0 && args->name == NULL) || gem_obj->label) { > + kfree(gem_obj->label); > + gem_obj->label = NULL; > + } > + > + gem_obj->label = label; > + > + drm_gem_object_put_unlocked(gem_obj); > + > + if (IS_ERR(gem_obj->label)) { > + err = PTR_ERR(gem_obj->label); > + goto err; > + } Why don't you check for this earlier? That seems suboptimal because now you've got some error value in gem_obj->label that you're going to have to special case at every step of the way. > + > +err: > + if (err != 0) > + args->flags = err; Why the flags? We typically just return the error... > + > + return err; ... like here. > +} Do we want to export this so that drivers can use it? > + > + > /** > * drm_gem_object_release - release GEM buffer object resources > * @obj: GEM buffer object > @@ -958,6 +1017,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..8259622f9ab6 100644 > --- a/drivers/gpu/drm/drm_internal.h > +++ b/drivers/gpu/drm/drm_internal.h > @@ -137,6 +137,10 @@ 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_bo_set_label_ioctl(struct drm_device *dev, void *data, > + struct drm_file *file_priv); > +int drm_gem_set_label(struct drm_device *dev, struct drm_label_object *args, > + struct drm_file *file_priv); This one seems to be unused now. > > /* 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..079d1422f9c5 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_BO_SET_LABEL, drm_bo_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..f736ba1f42a6 100644 > --- a/include/drm/drm_drv.h > +++ b/include/drm/drm_drv.h > @@ -715,6 +715,24 @@ struct drm_driver { > struct drm_device *dev, > uint32_t handle); > > + > + /** > + * @label: > + * > + * This label's a buffer object. > + * > + * Called by the user via ioctl. > + * > + * The default implementation is drm_gem_label(). GEM based drivers > + * must not overwrite this. > + Spurious blank line. > + * Returns: > + * > + * Zero on success, negative errno on failure. > + */ > + int (*label)(struct drm_device *dev, struct drm_label_object *args, > + struct drm_file *file_priv); If I understand correctly, you use this so that non-GEM drivers can use this IOCTL to label their non-GEM objects. Do you think that's really useful? I mean they've already got quite a bit of special code to deal with their objects, so singling out this IOCTL seems a bit odd. Then again, I guess it doesn't really hurt since GEM-based drivers will always use the standard implementation. > + > /** > * @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..23b507f5c571 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_label_object - ioctl argument for labelling BOs. > + * > + * This label's a BO with a userspace label > + * > + */ > +struct drm_label_object { > + /** Handle for the object being labelled. */ > + __u32 handle; > + > + /** Label and label length. > + * length includes the trailing NULL. Nit: I think you mean "trailing NUL". Also, the parameter is called len below, so the comment here should match. Thierry > + */ > + __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_BO_SET_LABEL DRM_IOWR(0xCE, struct drm_label_object) > > /** > * Device specific ioctls should only be in their respective headers > -- > 2.17.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On viernes, 20 de septiembre de 2019 17:25:10 (CEST) Thierry Reding wrote: > On Thu, Sep 19, 2019 at 02:53:21PM +0200, Rohan Garg wrote: > > 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 > > > > Signed-off-by: Rohan Garg <rohan.garg@collabora.com> > > --- > > > > drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/drm_internal.h | 4 +++ > > drivers/gpu/drm/drm_ioctl.c | 1 + > > include/drm/drm_drv.h | 18 ++++++++++ > > include/drm/drm_gem.h | 7 ++++ > > include/uapi/drm/drm.h | 20 +++++++++++ > > 6 files changed, 114 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > index 6854f5867d51..785ac561619a 100644 > > --- a/drivers/gpu/drm/drm_gem.c > > +++ b/drivers/gpu/drm/drm_gem.c > > @@ -941,6 +941,65 @@ drm_gem_release(struct drm_device *dev, struct > > drm_file *file_private)> > > idr_destroy(&file_private->object_idr); > > > > } > > > > +int drm_bo_set_label_ioctl(struct drm_device *dev, void *data, > > + struct drm_file *file_priv) > > Perhaps call this drm_gem_set_label_ioctl() so that it's consistent with > the other GEM related IOCTLs? > > > +{ > > + struct drm_label_object *args = data; > > Similarly, perhaps make this struct drm_gem_set_label for consistency. > > > + > > + if (!args->len || !args->name) > > + return -EINVAL; > > + > > + if (dev->driver->label) > > + return dev->driver->label(dev, args, file_priv); > > + > > + return -EOPNOTSUPP; > > +} > > + > > +/** > > + * drm_gem_label - label a given GEM object > > + * @dev: drm_device for the associated GEM object > > + * @data: drm_label_bo struct with a label, label length and any relevant > > flags + * @file_private: drm file-private structure to clean up > > + */ > > + > > +int drm_gem_set_label(struct drm_device *dev, struct drm_label_object > > *args, struct drm_file *file_priv) > Function name doesn't match the kerneldoc. > > > +{ > > + struct drm_gem_object *gem_obj; > > + int err = 0; > > + char *label; > > + > > + if (args->len > 0) > > + label = strndup_user(u64_to_user_ptr(args->name), args- >len); > > + > > + gem_obj = drm_gem_object_lookup(file_priv, args->handle); > > + if (!gem_obj) { > > + DRM_ERROR("Failed to look up GEM BO %d\n", args- >handle); > > People could abuse this to spam the system logs with these error > messages. Better make this DRM_DEBUG() or leave it out completely. > > > + err = -ENOENT; > > + goto err; > > I think you're leaking the label string here. > > > + } > > + > > + if ((args->len == 0 && args->name == NULL) || gem_obj->label) { > > + kfree(gem_obj->label); > > + gem_obj->label = NULL; > > + } > > + > > + gem_obj->label = label; > > + > > + drm_gem_object_put_unlocked(gem_obj); > > + > > + if (IS_ERR(gem_obj->label)) { > > + err = PTR_ERR(gem_obj->label); > > + goto err; > > + } > > Why don't you check for this earlier? That seems suboptimal because now > you've got some error value in gem_obj->label that you're going to have > to special case at every step of the way. > > > + > > +err: > > + if (err != 0) > > + args->flags = err; > > Why the flags? We typically just return the error... > > > + > > + return err; > > ... like here. > > > +} > > Do we want to export this so that drivers can use it? > > > + > > + > > > > /** > > > > * drm_gem_object_release - release GEM buffer object resources > > * @obj: GEM buffer object > > > > @@ -958,6 +1017,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..8259622f9ab6 100644 > > --- a/drivers/gpu/drm/drm_internal.h > > +++ b/drivers/gpu/drm/drm_internal.h > > @@ -137,6 +137,10 @@ 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_bo_set_label_ioctl(struct drm_device *dev, void *data, > > + struct drm_file *file_priv); > > +int drm_gem_set_label(struct drm_device *dev, struct drm_label_object > > *args, + struct drm_file *file_priv); > > This one seems to be unused now. > > > /* 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..079d1422f9c5 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_BO_SET_LABEL, drm_bo_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..f736ba1f42a6 100644 > > --- a/include/drm/drm_drv.h > > +++ b/include/drm/drm_drv.h > > @@ -715,6 +715,24 @@ struct drm_driver { > > > > struct drm_device *dev, > > uint32_t handle); > > > > + > > + /** > > + * @label: > > + * > > + * This label's a buffer object. > > + * > > + * Called by the user via ioctl. > > + * > > + * The default implementation is drm_gem_label(). GEM based drivers > > + * must not overwrite this. > > + > > Spurious blank line. > Ack to all the above. I'll address these in v3! > > + * Returns: > > + * > > + * Zero on success, negative errno on failure. > > + */ > > + int (*label)(struct drm_device *dev, struct drm_label_object *args, > > + struct drm_file *file_priv); > > If I understand correctly, you use this so that non-GEM drivers can use > this IOCTL to label their non-GEM objects. Do you think that's really > useful? I mean they've already got quite a bit of special code to deal > with their objects, so singling out this IOCTL seems a bit odd. > > Then again, I guess it doesn't really hurt since GEM-based drivers will > always use the standard implementation. > Please refer to Thomas Zimmermann's reply earlier in this thread. This was also specifically requested as part of the v1 review of this patch. > > + > > > > /** > > > > * @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..23b507f5c571 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_label_object - ioctl argument for labelling BOs. > > + * > > + * This label's a BO with a userspace label > > + * > > + */ > > +struct drm_label_object { > > + /** Handle for the object being labelled. */ > > + __u32 handle; > > + > > + /** Label and label length. > > + * length includes the trailing NULL. > > Nit: I think you mean "trailing NUL". Also, the parameter is called len > below, so the comment here should match. > Ack. Thanks for the review! Cheers Rohan Garg
Hi On jueves, 19 de septiembre de 2019 16:02:57 (CEST) Thomas Zimmermann wrote: > Hi > > Am 19.09.19 um 14:53 schrieb Rohan Garg: > > 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 > > > > Signed-off-by: Rohan Garg <rohan.garg@collabora.com> > > --- > > > > drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/drm_internal.h | 4 +++ > > drivers/gpu/drm/drm_ioctl.c | 1 + > > include/drm/drm_drv.h | 18 ++++++++++ > > include/drm/drm_gem.h | 7 ++++ > > include/uapi/drm/drm.h | 20 +++++++++++ > > 6 files changed, 114 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > index 6854f5867d51..785ac561619a 100644 > > --- a/drivers/gpu/drm/drm_gem.c > > +++ b/drivers/gpu/drm/drm_gem.c > > @@ -941,6 +941,65 @@ drm_gem_release(struct drm_device *dev, struct > > drm_file *file_private)> > > idr_destroy(&file_private->object_idr); > > > > } > > > > +int drm_bo_set_label_ioctl(struct drm_device *dev, void *data, > > + struct drm_file *file_priv) > > +{ > > + struct drm_label_object *args = data; > > + > > + if (!args->len || !args->name) > > + return -EINVAL; > > + > > + if (dev->driver->label) > > + return dev->driver->label(dev, args, file_priv); > > + > > + return -EOPNOTSUPP; > > +} > > + > > +/** > > + * drm_gem_label - label a given GEM object > > + * @dev: drm_device for the associated GEM object > > + * @data: drm_label_bo struct with a label, label length and any relevant > > flags + * @file_private: drm file-private structure to clean up > > + */ > > + > > +int drm_gem_set_label(struct drm_device *dev, struct drm_label_object > > *args, struct drm_file *file_priv) > I'd like to set labels for internal GEM objects. Could you split off the > object update code into something that is easily callable from within > drivers? Something like > > // called by ioctl > int drm_gem_object_adopt_label(struct drm_gem_object *gem, > char *label) > { > // your object update code goes here > } > > // called by drivers > int drm_gem_object_set_label(struct drm_gem_object *gem, > const char *label) > { > char* new_label = strdup(label) > return drm_gem_object_adopt_label(gem, new_label); > } > > > We have debugfs support for TTM-based GEM objects at [1]. I think the > label would be a welcome addition to the output. > Ack, I'll address this in v3. Cheers Rohan Garg
On Thu, Sep 26, 2019 at 10:47:35AM +0200, Rohan Garg wrote: > On viernes, 20 de septiembre de 2019 17:25:10 (CEST) Thierry Reding wrote: > > On Thu, Sep 19, 2019 at 02:53:21PM +0200, Rohan Garg wrote: > > > 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 > > > > > > Signed-off-by: Rohan Garg <rohan.garg@collabora.com> > > > --- > > > > > > drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++ > > > drivers/gpu/drm/drm_internal.h | 4 +++ > > > drivers/gpu/drm/drm_ioctl.c | 1 + > > > include/drm/drm_drv.h | 18 ++++++++++ > > > include/drm/drm_gem.h | 7 ++++ > > > include/uapi/drm/drm.h | 20 +++++++++++ > > > 6 files changed, 114 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > > index 6854f5867d51..785ac561619a 100644 > > > --- a/drivers/gpu/drm/drm_gem.c > > > +++ b/drivers/gpu/drm/drm_gem.c > > > @@ -941,6 +941,65 @@ drm_gem_release(struct drm_device *dev, struct > > > drm_file *file_private)> > > > idr_destroy(&file_private->object_idr); > > > > > > } > > > > > > +int drm_bo_set_label_ioctl(struct drm_device *dev, void *data, > > > + struct drm_file *file_priv) > > > > Perhaps call this drm_gem_set_label_ioctl() so that it's consistent with > > the other GEM related IOCTLs? > > > > > +{ > > > + struct drm_label_object *args = data; > > > > Similarly, perhaps make this struct drm_gem_set_label for consistency. > > > > > + > > > + if (!args->len || !args->name) > > > + return -EINVAL; > > > + > > > + if (dev->driver->label) > > > + return dev->driver->label(dev, args, file_priv); > > > + > > > + return -EOPNOTSUPP; > > > +} > > > + > > > +/** > > > + * drm_gem_label - label a given GEM object > > > + * @dev: drm_device for the associated GEM object > > > + * @data: drm_label_bo struct with a label, label length and any relevant > > > flags + * @file_private: drm file-private structure to clean up > > > + */ > > > + > > > +int drm_gem_set_label(struct drm_device *dev, struct drm_label_object > > > *args, struct drm_file *file_priv) > > Function name doesn't match the kerneldoc. > > > > > +{ > > > + struct drm_gem_object *gem_obj; > > > + int err = 0; > > > + char *label; > > > + > > > + if (args->len > 0) > > > + label = strndup_user(u64_to_user_ptr(args->name), args- > >len); > > > + > > > + gem_obj = drm_gem_object_lookup(file_priv, args->handle); > > > + if (!gem_obj) { > > > + DRM_ERROR("Failed to look up GEM BO %d\n", args- > >handle); > > > > People could abuse this to spam the system logs with these error > > messages. Better make this DRM_DEBUG() or leave it out completely. > > > > > + err = -ENOENT; > > > + goto err; > > > > I think you're leaking the label string here. > > > > > + } > > > + > > > + if ((args->len == 0 && args->name == NULL) || gem_obj->label) { > > > + kfree(gem_obj->label); > > > + gem_obj->label = NULL; > > > + } > > > + > > > + gem_obj->label = label; > > > + > > > + drm_gem_object_put_unlocked(gem_obj); > > > + > > > + if (IS_ERR(gem_obj->label)) { > > > + err = PTR_ERR(gem_obj->label); > > > + goto err; > > > + } > > > > Why don't you check for this earlier? That seems suboptimal because now > > you've got some error value in gem_obj->label that you're going to have > > to special case at every step of the way. > > > > > + > > > +err: > > > + if (err != 0) > > > + args->flags = err; > > > > Why the flags? We typically just return the error... > > > > > + > > > + return err; > > > > ... like here. > > > > > +} > > > > Do we want to export this so that drivers can use it? > > > > > + > > > + > > > > > > /** > > > > > > * drm_gem_object_release - release GEM buffer object resources > > > * @obj: GEM buffer object > > > > > > @@ -958,6 +1017,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..8259622f9ab6 100644 > > > --- a/drivers/gpu/drm/drm_internal.h > > > +++ b/drivers/gpu/drm/drm_internal.h > > > @@ -137,6 +137,10 @@ 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_bo_set_label_ioctl(struct drm_device *dev, void *data, > > > + struct drm_file *file_priv); > > > +int drm_gem_set_label(struct drm_device *dev, struct drm_label_object > > > *args, + struct drm_file *file_priv); > > > > This one seems to be unused now. > > > > > /* 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..079d1422f9c5 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_BO_SET_LABEL, drm_bo_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..f736ba1f42a6 100644 > > > --- a/include/drm/drm_drv.h > > > +++ b/include/drm/drm_drv.h > > > @@ -715,6 +715,24 @@ struct drm_driver { > > > > > > struct drm_device *dev, > > > uint32_t handle); > > > > > > + > > > + /** > > > + * @label: > > > + * > > > + * This label's a buffer object. > > > + * > > > + * Called by the user via ioctl. > > > + * > > > + * The default implementation is drm_gem_label(). GEM based > drivers > > > + * must not overwrite this. > > > + > > > > Spurious blank line. > > > > Ack to all the above. I'll address these in v3! > > > > + * Returns: > > > + * > > > + * Zero on success, negative errno on failure. > > > + */ > > > + int (*label)(struct drm_device *dev, struct drm_label_object > *args, > > > + struct drm_file *file_priv); > > > > If I understand correctly, you use this so that non-GEM drivers can use > > this IOCTL to label their non-GEM objects. Do you think that's really > > useful? I mean they've already got quite a bit of special code to deal > > with their objects, so singling out this IOCTL seems a bit odd. > > > > Then again, I guess it doesn't really hurt since GEM-based drivers will > > always use the standard implementation. > > > > Please refer to Thomas Zimmermann's reply earlier in this thread. This was > also specifically requested as part of the v1 review of this patch. Imo ditch this. There's a grant total of 1 non-gem driver, and that one uses 0 of the other gem related sharing infrastructure. I don't expect any new non-gem driver to get merged, ever (we're merging ttm and gem internally, which was really the only reason). And if vmwgfx wants to label objects, they can do that through their own ioctl. -Daniel > > > > + > > > > > > /** > > > > > > * @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..23b507f5c571 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_label_object - ioctl argument for labelling BOs. > > > + * > > > + * This label's a BO with a userspace label > > > + * > > > + */ > > > +struct drm_label_object { > > > + /** Handle for the object being labelled. */ > > > + __u32 handle; > > > + > > > + /** Label and label length. > > > + * length includes the trailing NULL. > > > > Nit: I think you mean "trailing NUL". Also, the parameter is called len > > below, so the comment here should match. > > > > Ack. > > Thanks for the review! > > Cheers > Rohan Garg > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 6854f5867d51..785ac561619a 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -941,6 +941,65 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private) idr_destroy(&file_private->object_idr); } +int drm_bo_set_label_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_priv) +{ + struct drm_label_object *args = data; + + if (!args->len || !args->name) + return -EINVAL; + + if (dev->driver->label) + return dev->driver->label(dev, args, file_priv); + + return -EOPNOTSUPP; +} + +/** + * drm_gem_label - label a given GEM object + * @dev: drm_device for the associated GEM object + * @data: drm_label_bo struct with a label, label length and any relevant flags + * @file_private: drm file-private structure to clean up + */ + +int drm_gem_set_label(struct drm_device *dev, struct drm_label_object *args, struct drm_file *file_priv) +{ + struct drm_gem_object *gem_obj; + int err = 0; + char *label; + + if (args->len > 0) + label = strndup_user(u64_to_user_ptr(args->name), args->len); + + gem_obj = drm_gem_object_lookup(file_priv, args->handle); + if (!gem_obj) { + DRM_ERROR("Failed to look up GEM BO %d\n", args->handle); + err = -ENOENT; + goto err; + } + + if ((args->len == 0 && args->name == NULL) || gem_obj->label) { + kfree(gem_obj->label); + gem_obj->label = NULL; + } + + gem_obj->label = label; + + drm_gem_object_put_unlocked(gem_obj); + + if (IS_ERR(gem_obj->label)) { + err = PTR_ERR(gem_obj->label); + goto err; + } + +err: + if (err != 0) + args->flags = err; + + return err; +} + + /** * drm_gem_object_release - release GEM buffer object resources * @obj: GEM buffer object @@ -958,6 +1017,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..8259622f9ab6 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -137,6 +137,10 @@ 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_bo_set_label_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_priv); +int drm_gem_set_label(struct drm_device *dev, struct drm_label_object *args, + struct drm_file *file_priv); /* 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..079d1422f9c5 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_BO_SET_LABEL, drm_bo_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..f736ba1f42a6 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -715,6 +715,24 @@ struct drm_driver { struct drm_device *dev, uint32_t handle); + + /** + * @label: + * + * This label's a buffer object. + * + * Called by the user via ioctl. + * + * The default implementation is drm_gem_label(). GEM based drivers + * must not overwrite this. + + * Returns: + * + * Zero on success, negative errno on failure. + */ + int (*label)(struct drm_device *dev, struct drm_label_object *args, + struct drm_file *file_priv); + /** * @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..23b507f5c571 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_label_object - ioctl argument for labelling BOs. + * + * This label's a BO with a userspace label + * + */ +struct drm_label_object { + /** Handle for the object being labelled. */ + __u32 handle; + + /** Label and label length. + * length includes the trailing NULL. + */ + __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_BO_SET_LABEL DRM_IOWR(0xCE, struct drm_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 Signed-off-by: Rohan Garg <rohan.garg@collabora.com> --- drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_internal.h | 4 +++ drivers/gpu/drm/drm_ioctl.c | 1 + include/drm/drm_drv.h | 18 ++++++++++ include/drm/drm_gem.h | 7 ++++ include/uapi/drm/drm.h | 20 +++++++++++ 6 files changed, 114 insertions(+)