On Thu, 21 May 2020 at 01:07, Rohan Garg <rohan.garg@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> wrote:
> > > Signed-off-by: Rohan Garg <rohan.garg@xxxxxxxxxxxxx>
> > > Reported-by: kbuild test robot <lkp@xxxxxxxxx>
> > > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> >
> > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
