[PATCH v4] drm/ioctl: Add a ioctl to label GEM objects
Rohan Garg
rohan.garg at collabora.com
Tue Oct 22 08:58:02 UTC 2019
Hey Daniel
On lunes, 14 de octubre de 2019 10:59:38 (CEST) Daniel Vetter wrote:
> On Fri, Oct 11, 2019 at 04:30:09PM +0200, Rohan Garg wrote:
> > DRM_IOCTL_DUMB_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
> >
> > Changes in v4:
> > - Refactor IOCTL call to only perform string duplication and move
> >
> > all gem lookup logic into GEM specific call
>
> I still think we should just make this GEM-only and avoid the indirection.
> Except if your userspace actually does run on vmwgfx, and you absolutely
> want/need it to run there. Everything else is GEM-only.
> -Daniel
>
The plan for TTM drivers is to call drm_gem_adopt_label internally. So in
practice, there really won't be too much TTM specific code.
This approach also future proof's us to be able to label any handles, not just
GEM handle.
For the reasons above, I'd like to stick with my approach.
Cheers
Rohan Garg
> > Signed-off-by: Rohan Garg <rohan.garg at collabora.com>
> > ---
> >
> > drivers/gpu/drm/drm_gem.c | 70 ++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/drm_internal.h | 8 ++++
> > drivers/gpu/drm/drm_ioctl.c | 1 +
> > include/drm/drm_drv.h | 16 ++++++++
> > include/drm/drm_gem.h | 7 ++++
> > include/uapi/drm/drm.h | 20 ++++++++++
> > 6 files changed, 122 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 6854f5867d51..0fa4609b2817 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -941,6 +941,71 @@ drm_gem_release(struct drm_device *dev, struct
> > drm_file *file_private)>
> > idr_destroy(&file_private->object_idr);
> >
> > }
> >
> > +int drm_dumb_set_label_ioctl(struct drm_device *dev,
> > + void *data, struct drm_file
*file_priv)
> > +{
> > + char *label;
> > + struct drm_dumb_set_label_object *args = data;
> > + int ret = 0;
> > +
> > + if (!args->len || !args->name)
> > + return -EINVAL;
> > +
> > + if (!dev->driver->label)
> > + return -EOPNOTSUPP;
> > +
> > + label = strndup_user(u64_to_user_ptr(args->name), args->len);
> > +
> > + if (IS_ERR(label)) {
> > + ret = PTR_ERR(label);
> > + goto err;
> > + }
> > +
> > + ret = dev->driver->label(dev, file_priv, args->handle, label);
> > +
> > +err:
> > + kfree(label);
> > + return ret;
> > +}
> > +
> > +int drm_gem_set_label(struct drm_device *dev,
> > + struct drm_file *file_priv,
> > + uint32_t handle,
> > + const char *label)
> > +{
> > + struct drm_gem_object *gem_obj;
> > + int ret = 0;
> > +
> > + gem_obj = drm_gem_object_lookup(file_priv, handle);
> > + if (!gem_obj) {
> > + DRM_DEBUG("Failed to look up GEM BO %d\n", handle);
> > + ret = -ENOENT;
> > + goto err;
> > + }
> > + drm_gem_adopt_label(gem_obj, label);
> > +
> > +err:
> > + drm_gem_object_put_unlocked(gem_obj);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(drm_gem_set_label);
> > +
> > +void drm_gem_adopt_label(struct drm_gem_object *gem_obj, const char
> > *label) +{
> > + char *adopted_label = NULL;
> > +
> > + if (label)
> > + adopted_label = kstrdup(label, GFP_KERNEL);
> > +
> > + if (gem_obj->label) {
> > + kfree(gem_obj->label);
> > + gem_obj->label = NULL;
> > + }
> > +
> > + gem_obj->label = adopted_label;
> > +}
> > +EXPORT_SYMBOL(drm_gem_adopt_label);
> > +
> >
> > /**
> >
> > * drm_gem_object_release - release GEM buffer object resources
> > * @obj: GEM buffer object
> >
> > @@ -958,6 +1023,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..cbc3f7b7fb9b 100644
> > --- a/drivers/gpu/drm/drm_internal.h
> > +++ b/drivers/gpu/drm/drm_internal.h
> > @@ -137,6 +137,14 @@ 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_dumb_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_file *file_priv,
> > + uint32_t handle,
> > + const char *label);
> > +void drm_gem_adopt_label(struct drm_gem_object *gem_obj, const char
> > *label);>
> > /* 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 fcd728d7cf72..f34e1571d70f 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -714,6 +714,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_DUMB_SET_LABEL, drm_dumb_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 cf13470810a5..501a3924354a 100644
> > --- a/include/drm/drm_drv.h
> > +++ b/include/drm/drm_drv.h
> > @@ -715,6 +715,22 @@ 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_device *dev,
> > + struct drm_file *file_priv,
> > + uint32_t handle,
> > + char *label);
> > +
> >
> > /**
> >
> > * @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..309c3c091385 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_dumb_set_label_object - ioctl argument for labelling BOs.
> > + *
> > + * This label's a BO with a userspace label
> > + *
> > + */
> > +struct drm_dumb_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_DUMB_SET_LABEL DRM_IOWR(0xCE, struct
> > drm_dumb_set_label_object)>
> > /**
> >
> > * Device specific ioctls should only be in their respective headers
More information about the dri-devel
mailing list