[PATCH v5 0/2] Introducing IOCTL's to set/get label's for a buffer object

[PATCH v5 0/2] Introducing IOCTL's to set/get label's for a buffer object

[Rohan Garg]

Hi
I've been reworking these label'ing patches in conjunction
with their userspace usage that can be found here [1].

The intention is that these patches will be useful for driver
developers and application developers alike in conjuction with
something like the OpenGL Label'ing extension [2].

Cheers
Rohan Garg

[1] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/2426/
[2] https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_debug_label.txt

Rohan Garg (2):
  drm/ioctl: Add a ioctl to set and get a label on GEM objects
  panfrost: Set default labeling helpers

 drivers/gpu/drm/drm_gem.c               | 54 ++++++++++++++++++
 drivers/gpu/drm/drm_internal.h          | 14 +++++
 drivers/gpu/drm/drm_ioctl.c             | 74 +++++++++++++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_drv.c |  3 +
 include/drm/drm_drv.h                   | 32 +++++++++++
 include/drm/drm_gem.h                   | 16 +++++-
 include/uapi/drm/drm.h                  | 21 ++++++-
 7 files changed, 212 insertions(+), 2 deletions(-)

-- 
2.17.1

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

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

[Rohan Garg]

DRM_IOCTL_HANDLE_SET_LABEL lets you label buffers associated
with a handle, making it easier to debug issues in userspace
applications.

DRM_IOCTL_HANDLE_GET_LABEL lets you read the label associated
with a buffer.

Changes in v2:
  - Hoist the IOCTL up into the drm_driver framework

Changes in v3:
  - Introduce a drm_gem_set_label for drivers to use internally
    in order to label a GEM object
  - Hoist string copying up into the IOCTL
  - Fix documentation
  - Move actual gem labeling into drm_gem_adopt_label

Changes in v4:
  - Refactor IOCTL call to only perform string duplication and move
    all gem lookup logic into GEM specific call

Changes in v5:
  - Fix issues pointed out by kbuild test robot
  - Cleanup minor issues around kfree and out/err labels
  - Fixed API documentation issues
  - Rename to DRM_IOCTL_HANDLE_SET_LABEL
  - Introduce a DRM_IOCTL_HANDLE_GET_LABEL to read labels
  - Added some documentation for consumers of this IOCTL
  - Ensure that label's cannot be longer than PAGE_SIZE
  - Set a default label value
  - Drop useless warning
  - Properly return length of label to userspace even if
    userspace did not allocate memory for label.

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>
---
 drivers/gpu/drm/drm_gem.c      | 54 +++++++++++++++++++++++++
 drivers/gpu/drm/drm_internal.h | 14 +++++++
 drivers/gpu/drm/drm_ioctl.c    | 74 ++++++++++++++++++++++++++++++++++
 include/drm/drm_drv.h          | 32 +++++++++++++++
 include/drm/drm_gem.h          | 16 +++++++-
 include/uapi/drm/drm.h         | 21 +++++++++-
 6 files changed, 209 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 7bf628e13023..3b2645906281 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -154,6 +154,7 @@ void drm_gem_private_object_init(struct drm_device *dev,
 
 	obj->dev = dev;
 	obj->filp = NULL;
+	obj->label = NULL;
 
 	kref_init(&obj->refcount);
 	obj->handle_count = 0;
@@ -940,6 +941,57 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private)
 	idr_destroy(&file_private->object_idr);
 }
 
+int drm_gem_set_label(struct drm_device *dev,
+		       struct drm_file *file_priv,
+			   uint32_t handle,
+			   const char *label)
+{
+	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);
+	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);
+
+	kfree(gem_obj->label);
+
+	gem_obj->label = adopted_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;
+}
+EXPORT_SYMBOL(drm_gem_get_label);
+
 /**
  * drm_gem_object_release - release GEM buffer object resources
  * @obj: GEM buffer object
@@ -957,6 +1009,8 @@ drm_gem_object_release(struct drm_gem_object *obj)
 
 	dma_resv_fini(&obj->_resv);
 	drm_gem_free_mmap_offset(obj);
+
+	kfree(obj->label);
 }
 EXPORT_SYMBOL(drm_gem_object_release);
 
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 2470a352730b..c1fc65097e14 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -161,6 +161,20 @@ int drm_gem_pin(struct drm_gem_object *obj);
 void drm_gem_unpin(struct drm_gem_object *obj);
 void *drm_gem_vmap(struct drm_gem_object *obj);
 void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr);
+int drm_handle_set_label_ioctl(struct drm_device *dev,
+			void *data,
+			struct drm_file *file_priv);
+int drm_handle_get_label_ioctl(struct drm_device *dev,
+			void *data,
+			struct drm_file *file_priv);
+int drm_gem_set_label(struct drm_device *dev,
+			struct drm_file *file_priv,
+			uint32_t handle,
+			const char *label);
+void drm_gem_adopt_label(struct drm_gem_object *gem_obj, const char *label);
+char *drm_gem_get_label(struct drm_device *dev,
+			struct drm_file *file_priv,
+			uint32_t handle);
 
 /* drm_debugfs.c drm_debugfs_crc.c */
 #if defined(CONFIG_DEBUG_FS)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 73e31dd4e442..b2f628fdf026 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -715,6 +715,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER),
+	DRM_IOCTL_DEF(DRM_IOCTL_HANDLE_SET_LABEL, drm_handle_set_label_ioctl,
+		      DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_HANDLE_GET_LABEL, drm_handle_get_label_ioctl,
+		      DRM_RENDER_ALLOW),
 };
 
 #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
@@ -928,3 +932,73 @@ bool drm_ioctl_flags(unsigned int nr, unsigned int *flags)
 	return true;
 }
 EXPORT_SYMBOL(drm_ioctl_flags);
+
+/**
+ * drm_handle_set_label_ioctl - Assign a string label with a handle
+ * @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.
+ *
+ * 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,
+				void *data, struct drm_file *file_priv)
+{
+	char *label;
+	struct drm_handle_label *args = data;
+	int ret = 0;
+
+	if (!dev->driver->set_label || args->len > PAGE_SIZE)
+		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
+		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;
+}
+
+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;
+	char *label;
+
+	if (!dev->driver->get_label)
+		return -EOPNOTSUPP;
+
+	label = dev->driver->get_label(dev, file_priv, args->handle);
+	args->len = strlen(label) + 1;
+
+	if (!label)
+		return -EFAULT;
+
+	if (args->label)
+		ret = copy_to_user(u64_to_user_ptr(args->label),
+				   label,
+				   args->len);
+
+	return ret;
+}
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 6d457652f199..7da95a3157cb 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -550,6 +550,38 @@ struct drm_driver {
 			    struct drm_device *dev,
 			    uint32_t handle);
 
+	/**
+	 * @set_label:
+	 *
+	 * This label's a buffer object.
+	 *
+	 * Called by the user via ioctl.
+	 *
+	 * Returns:
+	 *
+	 * Zero on success, negative errno on failure.
+	 */
+	int (*set_label)(struct drm_device *dev,
+				struct drm_file *file_priv,
+				uint32_t handle,
+				const char *label);
+
+
+	/**
+	 * @get_label:
+	 *
+	 * This reads's the label of a buffer object.
+	 *
+	 * Called by the user via ioctl.
+	 *
+	 * Returns:
+	 *
+	 * Zero on success, negative errno on failiure.
+	 */
+	char *(*get_label)(struct drm_device *dev,
+				struct drm_file *file_priv,
+				uint32_t handle);
+
 	/**
 	 * @gem_vm_ops: Driver private ops for this object
 	 *
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 0b375069cd48..f3e2feae1be3 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -250,6 +250,13 @@ struct drm_gem_object {
 	 */
 	int name;
 
+	/**
+	 * @label:
+	 *
+	 * Label for this object, should be a human readable string.
+	 */
+	char *label;
+
 	/**
 	 * @dma_buf:
 	 *
@@ -419,5 +426,12 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
 int drm_gem_dumb_destroy(struct drm_file *file,
 			 struct drm_device *dev,
 			 uint32_t handle);
-
+int drm_gem_set_label(struct drm_device *dev,
+		       struct drm_file *file_priv,
+			   uint32_t handle,
+			   const char *label);
+void drm_gem_adopt_label(struct drm_gem_object *gem_obj, const char *label);
+char *drm_gem_get_label(struct drm_device *dev,
+		       struct drm_file *file_priv,
+			   uint32_t handle);
 #endif /* __DRM_GEM_H__ */
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 808b48a93330..905bb8960753 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -626,6 +626,23 @@ struct drm_gem_open {
 	__u64 size;
 };
 
+/** struct drm_handle_label - ioctl argument for labelling BOs.
+ *
+ * This label's a BO with a userspace label
+ *
+ */
+struct drm_handle_label {
+	/** Handle for the object being labelled. */
+	__u32 handle;
+
+	/** Label and label length (len includes the trailing NUL). */
+	__u32 len;
+	__u64 label;
+
+	/** Flags */
+	int flags;
+};
+
 #define DRM_CAP_DUMB_BUFFER		0x1
 #define DRM_CAP_VBLANK_HIGH_CRTC	0x2
 #define DRM_CAP_DUMB_PREFERRED_DEPTH	0x3
@@ -947,8 +964,10 @@ extern "C" {
 #define DRM_IOCTL_SYNCOBJ_QUERY		DRM_IOWR(0xCB, struct drm_syncobj_timeline_array)
 #define DRM_IOCTL_SYNCOBJ_TRANSFER	DRM_IOWR(0xCC, struct drm_syncobj_transfer)
 #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL	DRM_IOWR(0xCD, struct drm_syncobj_timeline_array)
-
 #define DRM_IOCTL_MODE_GETFB2		DRM_IOWR(0xCE, struct drm_mode_fb_cmd2)
+#define DRM_IOCTL_HANDLE_SET_LABEL      DRM_IOWR(0xCF, struct drm_handle_label)
+#define DRM_IOCTL_HANDLE_GET_LABEL      DRM_IOWR(0xD0, struct drm_handle_label)
+
 
 /**
  * Device specific ioctls should only be in their respective headers
-- 
2.17.1

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

[PATCH v5 2/2] panfrost: Set default labeling helpers

[Rohan Garg]

Set the default labeling helpers in order to be able
to label the buffers.

Signed-off-by: Rohan Garg <rohan.garg@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 882fecc33fdb..335768236e3b 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -12,6 +12,7 @@
 #include <drm/drm_ioctl.h>
 #include <drm/drm_syncobj.h>
 #include <drm/drm_utils.h>
+#include <drm/drm_gem.h>
 
 #include "panfrost_device.h"
 #include "panfrost_devfreq.h"
@@ -567,6 +568,8 @@ static struct drm_driver panfrost_drm_driver = {
 	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
 	.gem_prime_import_sg_table = panfrost_gem_prime_import_sg_table,
 	.gem_prime_mmap		= drm_gem_prime_mmap,
+	.set_label          = drm_gem_set_label,
+	.get_label          = drm_gem_get_label,
 };
 
 static int panfrost_probe(struct platform_device *pdev)
-- 
2.17.1

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

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

[Emil Velikov]

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

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

[Rohan Garg]

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

Re: [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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel