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

[Emil Velikov]

On Thu, 21 May 2020 at 01:07, Rohan Garg <rohan.garg at collabora.com> wrote:
>
> Hey Emil
> I've applied all the suggestions except the ones I discuss below.
>
> >
> > 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.
> >
>
> Yep, the intention is to replace the VC4 specific labeling with a more generic
> framework that all drivers can use.
>
>From a quick look the VC4 labeling combines user-space labels + in-kernel ones.
Seems like msm also has labeling - although in-kernel only.

So this series will help quite a bit, but in-kernel bits will remain.
Pretty sure we can live with that.

> > A handful of ideas and suggestions below:
> >
> > On Thu, 14 May 2020 at 16:05, Rohan Garg <rohan.garg at collabora.com> wrote:
> > > Signed-off-by: Rohan Garg <rohan.garg at collabora.com>
> > > Reported-by: kbuild test robot <lkp at intel.com>
> > > Reported-by: Dan Carpenter <dan.carpenter at oracle.com>
> >
> > New functionality usually has suggested-by tags. Reported-by tags are
> > used when the feature isn't behaving as expected.
> >
>
> This was suggested as part of the previous review process [1].
>
The tag is used for bugfixes, not new features.
See the relevant section in Documentation/process/5.Posting.rst


> > > +
> > > +       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.
> >
>
> Great catch. I'll protect this from concurrent access.
>
> >
> > > +
> > > +       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.
> >
>
> This is intentional since vmgfx uses TTM and the DRM helpers would not work.
> Sure, we could simply add a patch to the series that hooks up the relevant
> code to vmgfx and then calls the DRM label helper for all other drivers, but
> I'd rather have driver developers explicitly opt into this functionality.
>
How about we add a simple drm_core_check_feature(dev, DRIVER_GEM)
check + return appropriate errno.
Grep ^^ for examples.

The check will trigger on vmwgfx and some UMS drivers.

> > > +               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.
> >
>
> Hm, I'm not entirely sure what documentation I should add here since we
> already document the drm_handle_label struct in the relevant header.
>
Hmm brain fart - the comment should be for the getter. Will elaborate below.

> >
> > > +
> > > +       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.
> >
>
> I wouldn't be opposed to this if such a need arises in the future.
>
This cannot be changed in the future I'm afraid. The change is pretty
trivial although I haven't seen many ioctls do this.
Perhaps it's not worth it. Here's a quick example, esp for the
DRIVER_GEM thingy.

{
  ...

  if (dev->driver->get_label)
    label = dev->driver->get_label(...);
  else if (drm_core_check_feature(dev, DRIVER_GEM)
    label = generic_gem_impl(...);
  else
    return -EOPNOTSUPP;

  if (!label)
    return -EFAULT;

  args->len = strlen(label) + 1;

  if (args->label)
    return copy_to_user(u64_to_user_ptr(args->label), label, args->len);

  return 0;
}

> > 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 :-\
> >
>
> From a quick read of the IO{W,R} documentation, I suppose we should be marking
> SET_LABEL as DRM_IOW and GET_LABEL as DRM_IOR.
>
Are you sure GET_LABEL is unidirectional? The ioctl reads data from
userspace _and_ writes the string length back to userspace.

-Emil