Re: [PATCH v2] drm/ioctl: Add a ioctl to label GEM objects
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 > > > --- > > > > > > 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
Re: [PATCH v2] drm/ioctl: Add a ioctl to label GEM objects
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 > > --- > > > > 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 signature.asc Description: This is a digitally signed message part. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/ioctl: Add a ioctl to label GEM objects
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 > > --- > > > > 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 f675
Re: [PATCH v2] drm/ioctl: Add a ioctl to label GEM objects
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 > --- > 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_
Re: [PATCH v2] drm/ioctl: Add a ioctl to label GEM objects
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 > --- > 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_
[PATCH v2] drm/ioctl: Add a ioctl to label GEM objects
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 --- 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) +{ + 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_l