On Thu, 21 May 2020 at 01:07, Rohan Garg rohan.garg@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@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].
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