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@xxxxxxxxxxxxx>
> > > ---
> > >
> > > 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
