Re: [PATCH v5 1/2] drm/ioctl: Add a ioctl to set and get a label on GEM objects

[Emil Velikov <emil.l.velikov@gmail.com>]

Hi Rohan,

As a high-level question: how does this compare to VC4_LABEL_BO?
Is it possible to implement to replace or partially implement the vc4
one with this infra?

IMHO this is something to aim for.

A handful of ideas and suggestions below:

On Thu, 14 May 2020 at 16:05, Rohan Garg <rohan.garg@collabora.com> wrote:

> Signed-off-by: Rohan Garg <rohan.garg@collabora.com>
> Reported-by: kbuild test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
New functionality usually has suggested-by tags. Reported-by tags are
used when the feature isn't behaving as expected.

> +int drm_gem_set_label(struct drm_device *dev,
> +                      struct drm_file *file_priv,
> +                          uint32_t handle,
> +                          const char *label)
Nit: re-wrap to use more of the 80 (ish) columns (applies for the whole patch)

> +{
> +       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 out;
> +       }
> +       drm_gem_adopt_label(gem_obj, label);
> +
> +out:
> +       drm_gem_object_put_unlocked(gem_obj);
I've just renamed this - s/_unlocked//g (applies for the whole patch)

> +       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);
Hmm the caller already creates a copy. Why do we create yet another one?
Personally I would drop this one + the free in the caller.

> +
> +       kfree(gem_obj->label);
> +
> +       gem_obj->label = adopted_label;
Do we have any protection of ->label wrt concurrent access? Say two
writers, attempting to both set the label.

> +}
> +EXPORT_SYMBOL(drm_gem_adopt_label);
> +
> +char *drm_gem_get_label(struct drm_device *dev,
> +                      struct drm_file *file_priv,
> +                          uint32_t handle)
> +{
> +       struct drm_gem_object *gem_obj;
> +
> +       gem_obj = drm_gem_object_lookup(file_priv, handle);
> +       if (!gem_obj) {
> +               DRM_DEBUG("Failed to look up GEM BO %d\n", handle);
> +               return NULL;
> +       }
> +
> +       return gem_obj->label;
Missing drm_gem_object_put? I suggest writing a few IGT tests, to flex
the issues raised.

> +}
> +EXPORT_SYMBOL(drm_gem_get_label);
> +


> +/**
> + * drm_handle_set_label_ioctl - Assign a string label with a handle
Nit: s/with a/to a/ reads better IMHO.

> + * @data: user argument
> + * @file_priv: drm file-private structure
> + *
> + * This ioctl can be used by whoever decides the purpose of a buffer to
> + * label the buffer object associated with the handle.
> + *
I'd drop the "whoever" section, leaving only the user-space part.

> + * This is typically targeted at user space drivers to label buffer objects
> + * with relevant information to provide human readable information about the
> + * contents of a buffer (for eg: a UBO, command buffer, shader, etc).
> + *
> + * Label length *must* not be larger than PAGE_SIZE.
> + *
> + * Returns:
> + * 0 if setting a label succeeded, negative errno otherwise.
> + */
> +
> +int drm_handle_set_label_ioctl(struct drm_device *dev,
This and the getter could be static functions. They're used only
within this file.

> +                               void *data, struct drm_file *file_priv)
> +{
> +       char *label;
> +       struct drm_handle_label *args = data;
> +       int ret = 0;
Nit: there's no need to initialize ret.

> +
> +       if (!dev->driver->set_label || args->len > PAGE_SIZE)
AFAICT the PAGE_SIZE check should be a EINVAL.

Additionally, It would be better, to use the default implementation
when the function pointer is not explicitly set.
That should allow for more consistent and easier use.

Think about the time gap (esp. for some distributions) between the
kernel feature landing and being generally accessible to userspace.

> +               return -EOPNOTSUPP;
> +
> +       if (!args->len)
> +               label = NULL;
> +       else if (args->len && args->label)
> +               label = strndup_user(u64_to_user_ptr(args->label), args->len);
> +       else
Might be worth throwing EINVAL for !len && label... or perhaps not. In
either case please document it.

> +               return -EINVAL;
> +
> +       if (IS_ERR(label)) {
> +               ret = PTR_ERR(label);
> +               return ret;
> +       }
> +
> +       ret = dev->driver->set_label(dev, file_priv, args->handle, label);
> +
> +       kfree(label);
> +       return ret;
> +}
> +

Missing documentation?
> +int drm_handle_get_label_ioctl(struct drm_device *dev,
> +                               void *data, struct drm_file *file_priv)
> +{
> +       struct drm_handle_label *args = data;
> +       int ret = 0;
Nit: drop the initialization

> +       char *label;
> +
> +       if (!dev->driver->get_label)
> +               return -EOPNOTSUPP;
Same logic as the setter applies.

> +
> +       label = dev->driver->get_label(dev, file_priv, args->handle);
> +       args->len = strlen(label) + 1;
> +
> +       if (!label)
> +               return -EFAULT;
The label is explicitly cleared or empty, why is this an error?
A more indicative feedback is to return success with len being zero.

> +
> +       if (args->label)
> +               ret = copy_to_user(u64_to_user_ptr(args->label),
> +                                  label,
> +                                  args->len);
> +
Consider the following - userspace allocates less memory than needed
for the whole string.
Be that size concerns or simply because it's interested only in the
first X bytes.

If we're interested in supporting that, a simple min(args->len, len)
could be used.

> +       return ret;
> +}


> +       /**
> +        * @set_label:
> +        *
> +        * This label's a buffer object.
EPARSE


> +       /**
> +        * @get_label:
> +        *
> +        * This reads's the label of a buffer object.
Nit: This reads the label of a buffer object.


> +struct drm_handle_label {
> +       /** Handle for the object being labelled. */
> +       __u32 handle;
> +
> +       /** Label and label length (len includes the trailing NUL). */
Nit: NULL + mention the PAGE_SIZE limitation.

> +       __u32 len;
> +       __u64 label;
> +
> +       /** Flags */
> +       int flags;
s/int/__u32/ + comment, currently no flags are defined.


> +#define DRM_IOCTL_HANDLE_SET_LABEL      DRM_IOWR(0xCF, struct drm_handle_label)
Pretty sure that WR is wrong here, although I don't recall we should
be using read or write only.
Unfortunately many drivers/ioctls get this wrong :-\

HTH
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel