* [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects
@ 2019-10-11 14:30 Rohan Garg
2019-10-11 17:09 ` Daniel Stone
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Rohan Garg @ 2019-10-11 14:30 UTC (permalink / raw)
To: dri-devel; +Cc: kernel
DRM_IOCTL_DUMB_SET_LABEL lets you label GEM objects, making it
easier to debug issues in userspace applications.
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
Signed-off-by: Rohan Garg <rohan.garg@collabora.com>
---
drivers/gpu/drm/drm_gem.c | 70 ++++++++++++++++++++++++++++++++++
drivers/gpu/drm/drm_internal.h | 8 ++++
drivers/gpu/drm/drm_ioctl.c | 1 +
include/drm/drm_drv.h | 16 ++++++++
include/drm/drm_gem.h | 7 ++++
include/uapi/drm/drm.h | 20 ++++++++++
6 files changed, 122 insertions(+)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 6854f5867d51..0fa4609b2817 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -941,6 +941,71 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private)
idr_destroy(&file_private->object_idr);
}
+int drm_dumb_set_label_ioctl(struct drm_device *dev,
+ void *data, struct drm_file *file_priv)
+{
+ char *label;
+ struct drm_dumb_set_label_object *args = data;
+ int ret = 0;
+
+ if (!args->len || !args->name)
+ return -EINVAL;
+
+ if (!dev->driver->label)
+ return -EOPNOTSUPP;
+
+ label = strndup_user(u64_to_user_ptr(args->name), args->len);
+
+ if (IS_ERR(label)) {
+ ret = PTR_ERR(label);
+ goto err;
+ }
+
+ ret = dev->driver->label(dev, file_priv, args->handle, label);
+
+err:
+ kfree(label);
+ return ret;
+}
+
+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 err;
+ }
+ drm_gem_adopt_label(gem_obj, label);
+
+err:
+ 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);
+
+ if (gem_obj->label) {
+ kfree(gem_obj->label);
+ gem_obj->label = NULL;
+ }
+
+ gem_obj->label = adopted_label;
+}
+EXPORT_SYMBOL(drm_gem_adopt_label);
+
/**
* drm_gem_object_release - release GEM buffer object resources
* @obj: GEM buffer object
@@ -958,6 +1023,11 @@ drm_gem_object_release(struct drm_gem_object *obj)
dma_resv_fini(&obj->_resv);
drm_gem_free_mmap_offset(obj);
+
+ if (obj->label) {
+ kfree(obj->label);
+ obj->label = NULL;
+ }
}
EXPORT_SYMBOL(drm_gem_object_release);
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 51a2055c8f18..cbc3f7b7fb9b 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -137,6 +137,14 @@ 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_dumb_set_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);
/* 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 fcd728d7cf72..f34e1571d70f 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -714,6 +714,7 @@ 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_DUMB_SET_LABEL, drm_dumb_set_label_ioctl, DRM_RENDER_ALLOW),
};
#define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index cf13470810a5..501a3924354a 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -715,6 +715,22 @@ struct drm_driver {
struct drm_device *dev,
uint32_t handle);
+ /**
+ * @label:
+ *
+ * This label's a buffer object.
+ *
+ * Called by the user via ioctl.
+ *
+ * Returns:
+ *
+ * Zero on success, negative errno on failure.
+ */
+ int (*label)(struct drm_device *dev,
+ struct drm_file *file_priv,
+ uint32_t handle,
+ char *label);
+
/**
* @gem_vm_ops: Driver private ops for this object
*
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 6aaba14f5972..f801c054e972 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -237,6 +237,13 @@ struct drm_gem_object {
*/
int name;
+ /**
+ * @label:
+ *
+ * Label for this object, should be a human readable string.
+ */
+ char *label;
+
/**
* @dma_buf:
*
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 8a5b2f8f8eb9..309c3c091385 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -626,6 +626,25 @@ struct drm_gem_open {
__u64 size;
};
+/** struct drm_dumb_set_label_object - ioctl argument for labelling BOs.
+ *
+ * This label's a BO with a userspace label
+ *
+ */
+struct drm_dumb_set_label_object {
+ /** Handle for the object being labelled. */
+ __u32 handle;
+
+ /** Label and label length.
+ * len includes the trailing NUL.
+ */
+ __u32 len;
+ __u64 name;
+
+ /** Flags */
+ int flags;
+};
+
#define DRM_CAP_DUMB_BUFFER 0x1
#define DRM_CAP_VBLANK_HIGH_CRTC 0x2
#define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
@@ -946,6 +965,7 @@ 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_DUMB_SET_LABEL DRM_IOWR(0xCE, struct drm_dumb_set_label_object)
/**
* 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects
2019-10-11 14:30 [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects Rohan Garg
@ 2019-10-11 17:09 ` Daniel Stone
2019-10-11 17:55 ` Thomas Zimmermann
2019-10-22 8:58 ` Rohan Garg
2019-10-11 17:31 ` Boris Brezillon
` (2 subsequent siblings)
3 siblings, 2 replies; 14+ messages in thread
From: Daniel Stone @ 2019-10-11 17:09 UTC (permalink / raw)
To: Rohan Garg; +Cc: kernel, dri-devel
Hi Rohan,
On Fri, 11 Oct 2019 at 15:30, Rohan Garg <rohan.garg@collabora.com> wrote:
> DRM_IOCTL_DUMB_SET_LABEL lets you label GEM objects, making it
> easier to debug issues in userspace applications.
I'm not sure if this was pointed out already, but dumb buffers != GEM
buffers. GEM buffers _can_ be dumb, but might not be.
Could you please rename to GEM_SET_LABEL?
Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects
2019-10-11 14:30 [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects Rohan Garg
2019-10-11 17:09 ` Daniel Stone
@ 2019-10-11 17:31 ` Boris Brezillon
2019-10-12 13:35 ` Dan Carpenter
2019-10-14 8:59 ` Daniel Vetter
3 siblings, 0 replies; 14+ messages in thread
From: Boris Brezillon @ 2019-10-11 17:31 UTC (permalink / raw)
To: Rohan Garg; +Cc: kernel, dri-devel
Hello Rohan,
On Fri, 11 Oct 2019 16:30:09 +0200
Rohan Garg <rohan.garg@collabora.com> wrote:
> DRM_IOCTL_DUMB_SET_LABEL lets you label GEM objects, making it
> easier to debug issues in userspace applications.
>
> 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
>
> Signed-off-by: Rohan Garg <rohan.garg@collabora.com>
> ---
> drivers/gpu/drm/drm_gem.c | 70 ++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/drm_internal.h | 8 ++++
> drivers/gpu/drm/drm_ioctl.c | 1 +
> include/drm/drm_drv.h | 16 ++++++++
> include/drm/drm_gem.h | 7 ++++
> include/uapi/drm/drm.h | 20 ++++++++++
> 6 files changed, 122 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 6854f5867d51..0fa4609b2817 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -941,6 +941,71 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private)
> idr_destroy(&file_private->object_idr);
> }
>
> +int drm_dumb_set_label_ioctl(struct drm_device *dev,
Why dumb? Not sure what a smart set_label_ioctl() function would
do differently :-).
Oh, and if this function can be used to label TTM BOs it should be moved
somewhere else (drm_ioctl.c ?).
> + void *data, struct drm_file *file_priv)
Indentation is broken here.
> +{
> + char *label;
> + struct drm_dumb_set_label_object *args = data;
> + int ret = 0;
> +
> + if (!args->len || !args->name)
Hm, I thought args->len == 0 was a valid case and meant "free the
existing label". Has it changed. The case that's not allowed is
args->len && !args->name.
> + return -EINVAL;
> +
> + if (!dev->driver->label)
> + return -EOPNOTSUPP;
> +
> + label = strndup_user(u64_to_user_ptr(args->name), args->len);
> +
Remove this blank line.
> + if (IS_ERR(label)) {
> + ret = PTR_ERR(label);
> + goto err;
> + }
> +
> + ret = dev->driver->label(dev, file_priv, args->handle, label);
> +
> +err:
I would s/err/out/ as this is not an error-only path.
> + kfree(label);
> + return ret;
> +}
> +
> +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 err;
> + }
> + drm_gem_adopt_label(gem_obj, label);
> +
> +err:
Ditto: s/err/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);
Add
WARN_ON(adopted_label);
> +
> + if (gem_obj->label) {
> + kfree(gem_obj->label);
> + gem_obj->label = NULL;
This assignment is useless since gem_obj->label is assigned below.
> + }
> +
> + gem_obj->label = adopted_label;
> +}
> +EXPORT_SYMBOL(drm_gem_adopt_label);
> +
> /**
> * drm_gem_object_release - release GEM buffer object resources
> * @obj: GEM buffer object
> @@ -958,6 +1023,11 @@ drm_gem_object_release(struct drm_gem_object *obj)
>
> dma_resv_fini(&obj->_resv);
> drm_gem_free_mmap_offset(obj);
> +
> + if (obj->label) {
> + kfree(obj->label);
> + obj->label = NULL;
> + }
You can call kfree(obj->label) directly (kfree() checks for NULL vals),
and no need to reset obj->label (obj should be free when the function
returns).
> }
> EXPORT_SYMBOL(drm_gem_object_release);
>
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 51a2055c8f18..cbc3f7b7fb9b 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -137,6 +137,14 @@ 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_dumb_set_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);
>
> /* 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 fcd728d7cf72..f34e1571d70f 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -714,6 +714,7 @@ 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_DUMB_SET_LABEL, drm_dumb_set_label_ioctl, DRM_RENDER_ALLOW),
> };
>
> #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index cf13470810a5..501a3924354a 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -715,6 +715,22 @@ struct drm_driver {
> struct drm_device *dev,
> uint32_t handle);
>
> + /**
> + * @label:
> + *
> + * This label's a buffer object.
> + *
> + * Called by the user via ioctl.
> + *
> + * Returns:
> + *
> + * Zero on success, negative errno on failure.
> + */
> + int (*label)(struct drm_device *dev,
Maybe label_bo or set_bo_label instead of just label, just to make it
clear that the label is applied to a buffer object.
> + struct drm_file *file_priv,
> + uint32_t handle,
> + char *label);
Indentation issue here as well.
> +
> /**
> * @gem_vm_ops: Driver private ops for this object
> *
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 6aaba14f5972..f801c054e972 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -237,6 +237,13 @@ struct drm_gem_object {
> */
> int name;
>
> + /**
> + * @label:
> + *
> + * Label for this object, should be a human readable string.
> + */
> + char *label;
> +
> /**
> * @dma_buf:
> *
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 8a5b2f8f8eb9..309c3c091385 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -626,6 +626,25 @@ struct drm_gem_open {
> __u64 size;
> };
>
> +/** struct drm_dumb_set_label_object - ioctl argument for labelling BOs.
> + *
> + * This label's a BO with a userspace label
> + *
> + */
> +struct drm_dumb_set_label_object {
> + /** Handle for the object being labelled. */
> + __u32 handle;
> +
> + /** Label and label length.
> + * len includes the trailing NUL.
> + */
Nitpick: the comment fits on a single line.
/** Label and label length (len includes the trailing NUL). */
> + __u32 len;
> + __u64 name;
> +
> + /** Flags */
> + int flags;
> +};
> +
> #define DRM_CAP_DUMB_BUFFER 0x1
> #define DRM_CAP_VBLANK_HIGH_CRTC 0x2
> #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
> @@ -946,6 +965,7 @@ 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_DUMB_SET_LABEL DRM_IOWR(0xCE, struct drm_dumb_set_label_object)
Maybe s/DRM_IOCTL_DUMB_SET_LABEL/DRM_IOCTL_SET_BO_LABEL/ and
s/drm_dumb_set_label_object/drm_set_bo_label/
>
> /**
> * Device specific ioctls should only be in their respective headers
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects
2019-10-11 17:09 ` Daniel Stone
@ 2019-10-11 17:55 ` Thomas Zimmermann
2019-10-14 8:58 ` Daniel Vetter
2019-10-22 8:58 ` Rohan Garg
2019-10-22 8:58 ` Rohan Garg
1 sibling, 2 replies; 14+ messages in thread
From: Thomas Zimmermann @ 2019-10-11 17:55 UTC (permalink / raw)
To: Daniel Stone, Rohan Garg; +Cc: kernel, dri-devel
Hi
Am 11.10.19 um 19:09 schrieb Daniel Stone:
> Hi Rohan,
>
> On Fri, 11 Oct 2019 at 15:30, Rohan Garg <rohan.garg@collabora.com> wrote:
>> DRM_IOCTL_DUMB_SET_LABEL lets you label GEM objects, making it
>> easier to debug issues in userspace applications.
>
> I'm not sure if this was pointed out already, but dumb buffers != GEM
> buffers. GEM buffers _can_ be dumb, but might not be.
>
> Could you please rename to GEM_SET_LABEL?
This change to build upon dumb buffers was suggested by me, as dumb
buffers is the closes thing there is to a buffer management interface.
Drivers with 'smart buffers' would have to add their own ioctl interfaces.
What I really miss here is a userspace application that uses this
functionality. It would be much easier to discuss if there was an actual
example.
Best regards
> Cheers,
> Daniel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects
2019-10-11 14:30 [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects Rohan Garg
2019-10-11 17:09 ` Daniel Stone
@ 2019-10-12 13:35 ` Dan Carpenter
2019-10-12 13:35 ` Dan Carpenter
2019-10-14 8:59 ` Daniel Vetter
3 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2019-10-12 13:35 UTC (permalink / raw)
To: kbuild, Rohan Garg; +Cc: kernel, kbuild-all, dri-devel
Hi Rohan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.4-rc2 next-20191011]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Rohan-Garg/drm-ioctl-Add-a-ioctl-to-label-GEM-objects/20191012-062955
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/gpu/drm/drm_gem.c:967 drm_dumb_set_label_ioctl() error: 'label' dereferencing possible ERR_PTR()
# https://github.com/0day-ci/linux/commit/0f0cd7ef9f3b1623ab982f12dc748998f31e10b4
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 0f0cd7ef9f3b1623ab982f12dc748998f31e10b4
vim +/label +967 drivers/gpu/drm/drm_gem.c
673a394b1e3b69 Eric Anholt 2008-07-30 943
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 944 int drm_dumb_set_label_ioctl(struct drm_device *dev,
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 945 void *data, struct drm_file *file_priv)
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 946 {
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 947 char *label;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 948 struct drm_dumb_set_label_object *args = data;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 949 int ret = 0;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 950
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 951 if (!args->len || !args->name)
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 952 return -EINVAL;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 953
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 954 if (!dev->driver->label)
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 955 return -EOPNOTSUPP;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 956
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 957 label = strndup_user(u64_to_user_ptr(args->name), args->len);
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 958
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 959 if (IS_ERR(label)) {
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 960 ret = PTR_ERR(label);
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 961 goto err;
^^^^^^^^
Just return PTR_ERR(label);
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 962 }
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 963
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 964 ret = dev->driver->label(dev, file_priv, args->handle, label);
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 965
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 966 err:
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 @967 kfree(label);
^^^^^^^^^^^^
This will Oops.
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 968 return ret;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 969 }
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 970
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects
@ 2019-10-12 13:35 ` Dan Carpenter
0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2019-10-12 13:35 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 3186 bytes --]
Hi Rohan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.4-rc2 next-20191011]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Rohan-Garg/drm-ioctl-Add-a-ioctl-to-label-GEM-objects/20191012-062955
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/gpu/drm/drm_gem.c:967 drm_dumb_set_label_ioctl() error: 'label' dereferencing possible ERR_PTR()
# https://github.com/0day-ci/linux/commit/0f0cd7ef9f3b1623ab982f12dc748998f31e10b4
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 0f0cd7ef9f3b1623ab982f12dc748998f31e10b4
vim +/label +967 drivers/gpu/drm/drm_gem.c
673a394b1e3b69 Eric Anholt 2008-07-30 943
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 944 int drm_dumb_set_label_ioctl(struct drm_device *dev,
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 945 void *data, struct drm_file *file_priv)
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 946 {
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 947 char *label;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 948 struct drm_dumb_set_label_object *args = data;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 949 int ret = 0;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 950
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 951 if (!args->len || !args->name)
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 952 return -EINVAL;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 953
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 954 if (!dev->driver->label)
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 955 return -EOPNOTSUPP;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 956
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 957 label = strndup_user(u64_to_user_ptr(args->name), args->len);
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 958
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 959 if (IS_ERR(label)) {
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 960 ret = PTR_ERR(label);
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 961 goto err;
^^^^^^^^
Just return PTR_ERR(label);
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 962 }
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 963
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 964 ret = dev->driver->label(dev, file_priv, args->handle, label);
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 965
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 966 err:
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 @967 kfree(label);
^^^^^^^^^^^^
This will Oops.
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 968 return ret;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 969 }
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 970
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects
@ 2019-10-12 13:35 ` Dan Carpenter
0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2019-10-12 13:35 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 3186 bytes --]
Hi Rohan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.4-rc2 next-20191011]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Rohan-Garg/drm-ioctl-Add-a-ioctl-to-label-GEM-objects/20191012-062955
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/gpu/drm/drm_gem.c:967 drm_dumb_set_label_ioctl() error: 'label' dereferencing possible ERR_PTR()
# https://github.com/0day-ci/linux/commit/0f0cd7ef9f3b1623ab982f12dc748998f31e10b4
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 0f0cd7ef9f3b1623ab982f12dc748998f31e10b4
vim +/label +967 drivers/gpu/drm/drm_gem.c
673a394b1e3b69 Eric Anholt 2008-07-30 943
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 944 int drm_dumb_set_label_ioctl(struct drm_device *dev,
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 945 void *data, struct drm_file *file_priv)
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 946 {
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 947 char *label;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 948 struct drm_dumb_set_label_object *args = data;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 949 int ret = 0;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 950
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 951 if (!args->len || !args->name)
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 952 return -EINVAL;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 953
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 954 if (!dev->driver->label)
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 955 return -EOPNOTSUPP;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 956
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 957 label = strndup_user(u64_to_user_ptr(args->name), args->len);
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 958
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 959 if (IS_ERR(label)) {
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 960 ret = PTR_ERR(label);
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 961 goto err;
^^^^^^^^
Just return PTR_ERR(label);
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 962 }
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 963
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 964 ret = dev->driver->label(dev, file_priv, args->handle, label);
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 965
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 966 err:
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 @967 kfree(label);
^^^^^^^^^^^^
This will Oops.
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 968 return ret;
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 969 }
0f0cd7ef9f3b16 Rohan Garg 2019-10-11 970
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects
2019-10-11 17:55 ` Thomas Zimmermann
@ 2019-10-14 8:58 ` Daniel Vetter
2019-10-22 8:58 ` Rohan Garg
1 sibling, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2019-10-14 8:58 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: Rohan Garg, kernel, dri-devel
On Fri, Oct 11, 2019 at 07:55:36PM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 11.10.19 um 19:09 schrieb Daniel Stone:
> > Hi Rohan,
> >
> > On Fri, 11 Oct 2019 at 15:30, Rohan Garg <rohan.garg@collabora.com> wrote:
> > > DRM_IOCTL_DUMB_SET_LABEL lets you label GEM objects, making it
> > > easier to debug issues in userspace applications.
> >
> > I'm not sure if this was pointed out already, but dumb buffers != GEM
> > buffers. GEM buffers _can_ be dumb, but might not be.
> >
> > Could you please rename to GEM_SET_LABEL?
>
> This change to build upon dumb buffers was suggested by me, as dumb buffers
> is the closes thing there is to a buffer management interface. Drivers with
> 'smart buffers' would have to add their own ioctl interfaces.
We already have some IOCTLs that apply to gem buffers, not just dumb
buffers, so GEM_SET_LABEL seems a lot more reasonable to me, too.
> What I really miss here is a userspace application that uses this
> functionality. It would be much easier to discuss if there was an actual
> example.
+1.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects
2019-10-11 14:30 [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects Rohan Garg
` (2 preceding siblings ...)
2019-10-12 13:35 ` Dan Carpenter
@ 2019-10-14 8:59 ` Daniel Vetter
2019-10-22 8:58 ` Rohan Garg
3 siblings, 1 reply; 14+ messages in thread
From: Daniel Vetter @ 2019-10-14 8:59 UTC (permalink / raw)
To: Rohan Garg; +Cc: kernel, dri-devel
On Fri, Oct 11, 2019 at 04:30:09PM +0200, Rohan Garg wrote:
> DRM_IOCTL_DUMB_SET_LABEL lets you label GEM objects, making it
> easier to debug issues in userspace applications.
>
> 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
I still think we should just make this GEM-only and avoid the indirection.
Except if your userspace actually does run on vmwgfx, and you absolutely
want/need it to run there. Everything else is GEM-only.
-Daniel
>
> Signed-off-by: Rohan Garg <rohan.garg@collabora.com>
> ---
> drivers/gpu/drm/drm_gem.c | 70 ++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/drm_internal.h | 8 ++++
> drivers/gpu/drm/drm_ioctl.c | 1 +
> include/drm/drm_drv.h | 16 ++++++++
> include/drm/drm_gem.h | 7 ++++
> include/uapi/drm/drm.h | 20 ++++++++++
> 6 files changed, 122 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 6854f5867d51..0fa4609b2817 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -941,6 +941,71 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private)
> idr_destroy(&file_private->object_idr);
> }
>
> +int drm_dumb_set_label_ioctl(struct drm_device *dev,
> + void *data, struct drm_file *file_priv)
> +{
> + char *label;
> + struct drm_dumb_set_label_object *args = data;
> + int ret = 0;
> +
> + if (!args->len || !args->name)
> + return -EINVAL;
> +
> + if (!dev->driver->label)
> + return -EOPNOTSUPP;
> +
> + label = strndup_user(u64_to_user_ptr(args->name), args->len);
> +
> + if (IS_ERR(label)) {
> + ret = PTR_ERR(label);
> + goto err;
> + }
> +
> + ret = dev->driver->label(dev, file_priv, args->handle, label);
> +
> +err:
> + kfree(label);
> + return ret;
> +}
> +
> +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 err;
> + }
> + drm_gem_adopt_label(gem_obj, label);
> +
> +err:
> + 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);
> +
> + if (gem_obj->label) {
> + kfree(gem_obj->label);
> + gem_obj->label = NULL;
> + }
> +
> + gem_obj->label = adopted_label;
> +}
> +EXPORT_SYMBOL(drm_gem_adopt_label);
> +
> /**
> * drm_gem_object_release - release GEM buffer object resources
> * @obj: GEM buffer object
> @@ -958,6 +1023,11 @@ drm_gem_object_release(struct drm_gem_object *obj)
>
> dma_resv_fini(&obj->_resv);
> drm_gem_free_mmap_offset(obj);
> +
> + if (obj->label) {
> + kfree(obj->label);
> + obj->label = NULL;
> + }
> }
> EXPORT_SYMBOL(drm_gem_object_release);
>
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 51a2055c8f18..cbc3f7b7fb9b 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -137,6 +137,14 @@ 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_dumb_set_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);
>
> /* 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 fcd728d7cf72..f34e1571d70f 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -714,6 +714,7 @@ 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_DUMB_SET_LABEL, drm_dumb_set_label_ioctl, DRM_RENDER_ALLOW),
> };
>
> #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index cf13470810a5..501a3924354a 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -715,6 +715,22 @@ struct drm_driver {
> struct drm_device *dev,
> uint32_t handle);
>
> + /**
> + * @label:
> + *
> + * This label's a buffer object.
> + *
> + * Called by the user via ioctl.
> + *
> + * Returns:
> + *
> + * Zero on success, negative errno on failure.
> + */
> + int (*label)(struct drm_device *dev,
> + struct drm_file *file_priv,
> + uint32_t handle,
> + char *label);
> +
> /**
> * @gem_vm_ops: Driver private ops for this object
> *
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 6aaba14f5972..f801c054e972 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -237,6 +237,13 @@ struct drm_gem_object {
> */
> int name;
>
> + /**
> + * @label:
> + *
> + * Label for this object, should be a human readable string.
> + */
> + char *label;
> +
> /**
> * @dma_buf:
> *
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 8a5b2f8f8eb9..309c3c091385 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -626,6 +626,25 @@ struct drm_gem_open {
> __u64 size;
> };
>
> +/** struct drm_dumb_set_label_object - ioctl argument for labelling BOs.
> + *
> + * This label's a BO with a userspace label
> + *
> + */
> +struct drm_dumb_set_label_object {
> + /** Handle for the object being labelled. */
> + __u32 handle;
> +
> + /** Label and label length.
> + * len includes the trailing NUL.
> + */
> + __u32 len;
> + __u64 name;
> +
> + /** Flags */
> + int flags;
> +};
> +
> #define DRM_CAP_DUMB_BUFFER 0x1
> #define DRM_CAP_VBLANK_HIGH_CRTC 0x2
> #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
> @@ -946,6 +965,7 @@ 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_DUMB_SET_LABEL DRM_IOWR(0xCE, struct drm_dumb_set_label_object)
>
> /**
> * 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects
2019-10-14 8:59 ` Daniel Vetter
@ 2019-10-22 8:58 ` Rohan Garg
2019-10-22 9:30 ` Daniel Vetter
0 siblings, 1 reply; 14+ messages in thread
From: Rohan Garg @ 2019-10-22 8:58 UTC (permalink / raw)
To: dri-devel
Hey Daniel
On lunes, 14 de octubre de 2019 10:59:38 (CEST) Daniel Vetter wrote:
> On Fri, Oct 11, 2019 at 04:30:09PM +0200, Rohan Garg wrote:
> > DRM_IOCTL_DUMB_SET_LABEL lets you label GEM objects, making it
> > easier to debug issues in userspace applications.
> >
> > 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
>
> I still think we should just make this GEM-only and avoid the indirection.
> Except if your userspace actually does run on vmwgfx, and you absolutely
> want/need it to run there. Everything else is GEM-only.
> -Daniel
>
The plan for TTM drivers is to call drm_gem_adopt_label internally. So in
practice, there really won't be too much TTM specific code.
This approach also future proof's us to be able to label any handles, not just
GEM handle.
For the reasons above, I'd like to stick with my approach.
Cheers
Rohan Garg
> > Signed-off-by: Rohan Garg <rohan.garg@collabora.com>
> > ---
> >
> > drivers/gpu/drm/drm_gem.c | 70 ++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/drm_internal.h | 8 ++++
> > drivers/gpu/drm/drm_ioctl.c | 1 +
> > include/drm/drm_drv.h | 16 ++++++++
> > include/drm/drm_gem.h | 7 ++++
> > include/uapi/drm/drm.h | 20 ++++++++++
> > 6 files changed, 122 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 6854f5867d51..0fa4609b2817 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -941,6 +941,71 @@ drm_gem_release(struct drm_device *dev, struct
> > drm_file *file_private)>
> > idr_destroy(&file_private->object_idr);
> >
> > }
> >
> > +int drm_dumb_set_label_ioctl(struct drm_device *dev,
> > + void *data, struct drm_file
*file_priv)
> > +{
> > + char *label;
> > + struct drm_dumb_set_label_object *args = data;
> > + int ret = 0;
> > +
> > + if (!args->len || !args->name)
> > + return -EINVAL;
> > +
> > + if (!dev->driver->label)
> > + return -EOPNOTSUPP;
> > +
> > + label = strndup_user(u64_to_user_ptr(args->name), args->len);
> > +
> > + if (IS_ERR(label)) {
> > + ret = PTR_ERR(label);
> > + goto err;
> > + }
> > +
> > + ret = dev->driver->label(dev, file_priv, args->handle, label);
> > +
> > +err:
> > + kfree(label);
> > + return ret;
> > +}
> > +
> > +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 err;
> > + }
> > + drm_gem_adopt_label(gem_obj, label);
> > +
> > +err:
> > + 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);
> > +
> > + if (gem_obj->label) {
> > + kfree(gem_obj->label);
> > + gem_obj->label = NULL;
> > + }
> > +
> > + gem_obj->label = adopted_label;
> > +}
> > +EXPORT_SYMBOL(drm_gem_adopt_label);
> > +
> >
> > /**
> >
> > * drm_gem_object_release - release GEM buffer object resources
> > * @obj: GEM buffer object
> >
> > @@ -958,6 +1023,11 @@ drm_gem_object_release(struct drm_gem_object *obj)
> >
> > dma_resv_fini(&obj->_resv);
> > drm_gem_free_mmap_offset(obj);
> >
> > +
> > + if (obj->label) {
> > + kfree(obj->label);
> > + obj->label = NULL;
> > + }
> >
> > }
> > EXPORT_SYMBOL(drm_gem_object_release);
> >
> > diff --git a/drivers/gpu/drm/drm_internal.h
> > b/drivers/gpu/drm/drm_internal.h index 51a2055c8f18..cbc3f7b7fb9b 100644
> > --- a/drivers/gpu/drm/drm_internal.h
> > +++ b/drivers/gpu/drm/drm_internal.h
> > @@ -137,6 +137,14 @@ 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_dumb_set_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);>
> > /* 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 fcd728d7cf72..f34e1571d70f 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -714,6 +714,7 @@ 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_DUMB_SET_LABEL, drm_dumb_set_label_ioctl,
> > DRM_RENDER_ALLOW),>
> > };
> >
> > #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
> >
> > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > index cf13470810a5..501a3924354a 100644
> > --- a/include/drm/drm_drv.h
> > +++ b/include/drm/drm_drv.h
> > @@ -715,6 +715,22 @@ struct drm_driver {
> >
> > struct drm_device *dev,
> > uint32_t handle);
> >
> > + /**
> > + * @label:
> > + *
> > + * This label's a buffer object.
> > + *
> > + * Called by the user via ioctl.
> > + *
> > + * Returns:
> > + *
> > + * Zero on success, negative errno on failure.
> > + */
> > + int (*label)(struct drm_device *dev,
> > + struct drm_file *file_priv,
> > + uint32_t handle,
> > + char *label);
> > +
> >
> > /**
> >
> > * @gem_vm_ops: Driver private ops for this object
> > *
> >
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > index 6aaba14f5972..f801c054e972 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -237,6 +237,13 @@ struct drm_gem_object {
> >
> > */
> >
> > int name;
> >
> > + /**
> > + * @label:
> > + *
> > + * Label for this object, should be a human readable string.
> > + */
> > + char *label;
> > +
> >
> > /**
> >
> > * @dma_buf:
> > *
> >
> > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > index 8a5b2f8f8eb9..309c3c091385 100644
> > --- a/include/uapi/drm/drm.h
> > +++ b/include/uapi/drm/drm.h
> > @@ -626,6 +626,25 @@ struct drm_gem_open {
> >
> > __u64 size;
> >
> > };
> >
> > +/** struct drm_dumb_set_label_object - ioctl argument for labelling BOs.
> > + *
> > + * This label's a BO with a userspace label
> > + *
> > + */
> > +struct drm_dumb_set_label_object {
> > + /** Handle for the object being labelled. */
> > + __u32 handle;
> > +
> > + /** Label and label length.
> > + * len includes the trailing NUL.
> > + */
> > + __u32 len;
> > + __u64 name;
> > +
> > + /** Flags */
> > + int flags;
> > +};
> > +
> >
> > #define DRM_CAP_DUMB_BUFFER 0x1
> > #define DRM_CAP_VBLANK_HIGH_CRTC 0x2
> > #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
> >
> > @@ -946,6 +965,7 @@ 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_DUMB_SET_LABEL DRM_IOWR(0xCE, struct
> > drm_dumb_set_label_object)>
> > /**
> >
> > * Device specific ioctls should only be in their respective headers
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects
2019-10-11 17:55 ` Thomas Zimmermann
2019-10-14 8:58 ` Daniel Vetter
@ 2019-10-22 8:58 ` Rohan Garg
1 sibling, 0 replies; 14+ messages in thread
From: Rohan Garg @ 2019-10-22 8:58 UTC (permalink / raw)
To: kernel; +Cc: dri-devel, kernel, Thomas Zimmermann
Hi Thomas
On viernes, 11 de octubre de 2019 19:55:36 (CEST) Thomas Zimmermann wrote:
> Hi
>
> Am 11.10.19 um 19:09 schrieb Daniel Stone:
> > Hi Rohan,
> >
> > On Fri, 11 Oct 2019 at 15:30, Rohan Garg <rohan.garg@collabora.com> wrote:
> >> DRM_IOCTL_DUMB_SET_LABEL lets you label GEM objects, making it
> >> easier to debug issues in userspace applications.
> >
> > I'm not sure if this was pointed out already, but dumb buffers != GEM
> > buffers. GEM buffers _can_ be dumb, but might not be.
> >
> > Could you please rename to GEM_SET_LABEL?
>
> This change to build upon dumb buffers was suggested by me, as dumb
> buffers is the closes thing there is to a buffer management interface.
> Drivers with 'smart buffers' would have to add their own ioctl interfaces.
>
> What I really miss here is a userspace application that uses this
> functionality. It would be much easier to discuss if there was an actual
> example.
>
I'm currently working on implementing something for Mesa. I'll send a v5 based
on the lessons learnt from that patchset.
> Best regards
>
> > Cheers,
> > Daniel
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects
2019-10-11 17:09 ` Daniel Stone
2019-10-11 17:55 ` Thomas Zimmermann
@ 2019-10-22 8:58 ` Rohan Garg
1 sibling, 0 replies; 14+ messages in thread
From: Rohan Garg @ 2019-10-22 8:58 UTC (permalink / raw)
To: dri-devel; +Cc: kernel
Hey
On viernes, 11 de octubre de 2019 19:09:52 (CEST) Daniel Stone wrote:
> Hi Rohan,
>
> On Fri, 11 Oct 2019 at 15:30, Rohan Garg <rohan.garg@collabora.com> wrote:
> > DRM_IOCTL_DUMB_SET_LABEL lets you label GEM objects, making it
> > easier to debug issues in userspace applications.
>
> I'm not sure if this was pointed out already, but dumb buffers != GEM
> buffers. GEM buffers _can_ be dumb, but might not be.
>
> Could you please rename to GEM_SET_LABEL?
>
Daniel and I had a opportunity to talk about this in person and we agreed that
HANDLE_SET_LABEL would be a more sensible name.
Cheers
Rohan Garg
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects
2019-10-22 8:58 ` Rohan Garg
@ 2019-10-22 9:30 ` Daniel Vetter
2019-10-22 13:14 ` Daniel Stone
0 siblings, 1 reply; 14+ messages in thread
From: Daniel Vetter @ 2019-10-22 9:30 UTC (permalink / raw)
To: Rohan Garg; +Cc: dri-devel
On Tue, Oct 22, 2019 at 10:58:02AM +0200, Rohan Garg wrote:
> Hey Daniel
> On lunes, 14 de octubre de 2019 10:59:38 (CEST) Daniel Vetter wrote:
> > On Fri, Oct 11, 2019 at 04:30:09PM +0200, Rohan Garg wrote:
> > > DRM_IOCTL_DUMB_SET_LABEL lets you label GEM objects, making it
> > > easier to debug issues in userspace applications.
> > >
> > > 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
> >
> > I still think we should just make this GEM-only and avoid the indirection.
> > Except if your userspace actually does run on vmwgfx, and you absolutely
> > want/need it to run there. Everything else is GEM-only.
> > -Daniel
> >
>
> The plan for TTM drivers is to call drm_gem_adopt_label internally. So in
> practice, there really won't be too much TTM specific code.
>
> This approach also future proof's us to be able to label any handles, not just
> GEM handle.
I don't expect we'll ever merge any non-gem drivers in the future anymore.
Hence this really only makes sense if vmwgfx wants it, that's the only
use-case for this generic-ism here. If vmwgfx doesn't have a use or jump
on board with this, imo better to just make this specific to gem and be
done.
-Daniel
> For the reasons above, I'd like to stick with my approach.
>
> Cheers
> Rohan Garg
>
> > > Signed-off-by: Rohan Garg <rohan.garg@collabora.com>
> > > ---
> > >
> > > drivers/gpu/drm/drm_gem.c | 70 ++++++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/drm_internal.h | 8 ++++
> > > drivers/gpu/drm/drm_ioctl.c | 1 +
> > > include/drm/drm_drv.h | 16 ++++++++
> > > include/drm/drm_gem.h | 7 ++++
> > > include/uapi/drm/drm.h | 20 ++++++++++
> > > 6 files changed, 122 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > index 6854f5867d51..0fa4609b2817 100644
> > > --- a/drivers/gpu/drm/drm_gem.c
> > > +++ b/drivers/gpu/drm/drm_gem.c
> > > @@ -941,6 +941,71 @@ drm_gem_release(struct drm_device *dev, struct
> > > drm_file *file_private)>
> > > idr_destroy(&file_private->object_idr);
> > >
> > > }
> > >
> > > +int drm_dumb_set_label_ioctl(struct drm_device *dev,
> > > + void *data, struct drm_file
> *file_priv)
> > > +{
> > > + char *label;
> > > + struct drm_dumb_set_label_object *args = data;
> > > + int ret = 0;
> > > +
> > > + if (!args->len || !args->name)
> > > + return -EINVAL;
> > > +
> > > + if (!dev->driver->label)
> > > + return -EOPNOTSUPP;
> > > +
> > > + label = strndup_user(u64_to_user_ptr(args->name), args->len);
> > > +
> > > + if (IS_ERR(label)) {
> > > + ret = PTR_ERR(label);
> > > + goto err;
> > > + }
> > > +
> > > + ret = dev->driver->label(dev, file_priv, args->handle, label);
> > > +
> > > +err:
> > > + kfree(label);
> > > + return ret;
> > > +}
> > > +
> > > +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 err;
> > > + }
> > > + drm_gem_adopt_label(gem_obj, label);
> > > +
> > > +err:
> > > + 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);
> > > +
> > > + if (gem_obj->label) {
> > > + kfree(gem_obj->label);
> > > + gem_obj->label = NULL;
> > > + }
> > > +
> > > + gem_obj->label = adopted_label;
> > > +}
> > > +EXPORT_SYMBOL(drm_gem_adopt_label);
> > > +
> > >
> > > /**
> > >
> > > * drm_gem_object_release - release GEM buffer object resources
> > > * @obj: GEM buffer object
> > >
> > > @@ -958,6 +1023,11 @@ drm_gem_object_release(struct drm_gem_object *obj)
> > >
> > > dma_resv_fini(&obj->_resv);
> > > drm_gem_free_mmap_offset(obj);
> > >
> > > +
> > > + if (obj->label) {
> > > + kfree(obj->label);
> > > + obj->label = NULL;
> > > + }
> > >
> > > }
> > > EXPORT_SYMBOL(drm_gem_object_release);
> > >
> > > diff --git a/drivers/gpu/drm/drm_internal.h
> > > b/drivers/gpu/drm/drm_internal.h index 51a2055c8f18..cbc3f7b7fb9b 100644
> > > --- a/drivers/gpu/drm/drm_internal.h
> > > +++ b/drivers/gpu/drm/drm_internal.h
> > > @@ -137,6 +137,14 @@ 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_dumb_set_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);>
> > > /* 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 fcd728d7cf72..f34e1571d70f 100644
> > > --- a/drivers/gpu/drm/drm_ioctl.c
> > > +++ b/drivers/gpu/drm/drm_ioctl.c
> > > @@ -714,6 +714,7 @@ 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_DUMB_SET_LABEL, drm_dumb_set_label_ioctl,
> > > DRM_RENDER_ALLOW),>
> > > };
> > >
> > > #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
> > >
> > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > > index cf13470810a5..501a3924354a 100644
> > > --- a/include/drm/drm_drv.h
> > > +++ b/include/drm/drm_drv.h
> > > @@ -715,6 +715,22 @@ struct drm_driver {
> > >
> > > struct drm_device *dev,
> > > uint32_t handle);
> > >
> > > + /**
> > > + * @label:
> > > + *
> > > + * This label's a buffer object.
> > > + *
> > > + * Called by the user via ioctl.
> > > + *
> > > + * Returns:
> > > + *
> > > + * Zero on success, negative errno on failure.
> > > + */
> > > + int (*label)(struct drm_device *dev,
> > > + struct drm_file *file_priv,
> > > + uint32_t handle,
> > > + char *label);
> > > +
> > >
> > > /**
> > >
> > > * @gem_vm_ops: Driver private ops for this object
> > > *
> > >
> > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > > index 6aaba14f5972..f801c054e972 100644
> > > --- a/include/drm/drm_gem.h
> > > +++ b/include/drm/drm_gem.h
> > > @@ -237,6 +237,13 @@ struct drm_gem_object {
> > >
> > > */
> > >
> > > int name;
> > >
> > > + /**
> > > + * @label:
> > > + *
> > > + * Label for this object, should be a human readable string.
> > > + */
> > > + char *label;
> > > +
> > >
> > > /**
> > >
> > > * @dma_buf:
> > > *
> > >
> > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > index 8a5b2f8f8eb9..309c3c091385 100644
> > > --- a/include/uapi/drm/drm.h
> > > +++ b/include/uapi/drm/drm.h
> > > @@ -626,6 +626,25 @@ struct drm_gem_open {
> > >
> > > __u64 size;
> > >
> > > };
> > >
> > > +/** struct drm_dumb_set_label_object - ioctl argument for labelling BOs.
> > > + *
> > > + * This label's a BO with a userspace label
> > > + *
> > > + */
> > > +struct drm_dumb_set_label_object {
> > > + /** Handle for the object being labelled. */
> > > + __u32 handle;
> > > +
> > > + /** Label and label length.
> > > + * len includes the trailing NUL.
> > > + */
> > > + __u32 len;
> > > + __u64 name;
> > > +
> > > + /** Flags */
> > > + int flags;
> > > +};
> > > +
> > >
> > > #define DRM_CAP_DUMB_BUFFER 0x1
> > > #define DRM_CAP_VBLANK_HIGH_CRTC 0x2
> > > #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
> > >
> > > @@ -946,6 +965,7 @@ 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_DUMB_SET_LABEL DRM_IOWR(0xCE, struct
> > > drm_dumb_set_label_object)>
> > > /**
> > >
> > > * Device specific ioctls should only be in their respective headers
>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects
2019-10-22 9:30 ` Daniel Vetter
@ 2019-10-22 13:14 ` Daniel Stone
0 siblings, 0 replies; 14+ messages in thread
From: Daniel Stone @ 2019-10-22 13:14 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Rohan Garg, dri-devel
Hey,
On Tue, 22 Oct 2019 at 11:30, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Oct 22, 2019 at 10:58:02AM +0200, Rohan Garg wrote:
> > This approach also future proof's us to be able to label any handles, not just
> > GEM handle.
>
> I don't expect we'll ever merge any non-gem drivers in the future anymore.
> Hence this really only makes sense if vmwgfx wants it, that's the only
> use-case for this generic-ism here. If vmwgfx doesn't have a use or jump
> on board with this, imo better to just make this specific to gem and be
> done.
VMware were the exact people who asked it for to be handle-agnostic
and not GEM-specific ...
Given that we already have handle-agnostic calls like FBs in
particular, I think it makes sense to have this one just follow that.
It's not much code and doesn't really imply any heavy change for the
rest of DRM.
Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-10-22 13:14 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-11 14:30 [PATCH v4] drm/ioctl: Add a ioctl to label GEM objects Rohan Garg
2019-10-11 17:09 ` Daniel Stone
2019-10-11 17:55 ` Thomas Zimmermann
2019-10-14 8:58 ` Daniel Vetter
2019-10-22 8:58 ` Rohan Garg
2019-10-22 8:58 ` Rohan Garg
2019-10-11 17:31 ` Boris Brezillon
2019-10-12 13:35 ` Dan Carpenter
2019-10-12 13:35 ` Dan Carpenter
2019-10-12 13:35 ` Dan Carpenter
2019-10-14 8:59 ` Daniel Vetter
2019-10-22 8:58 ` Rohan Garg
2019-10-22 9:30 ` Daniel Vetter
2019-10-22 13:14 ` Daniel Stone
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.