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

[Rohan Garg <rohan.garg@collabora.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3527 bytes --]

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.

> 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.
> 

This was suggested as part of the previous review process [1].

> > +
> > +       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.

> > +               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.

> 
> > +
> > +       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.

> 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.

Thanks!
Rohan Garg

[1] https://patchwork.freedesktop.org/patch/335508/?
series=66752&rev=4#comment_621167

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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