| Message ID | 20250411150357.3308921-3-adrian.larumbe@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Panthor BO tagging and GEMS debug display | expand |
On 11/04/2025 16:03, Adrián Larumbe wrote: > Allow UM to label a BO for which it possesses a DRM handle. > > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com> > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> Reviewed-by: Steven Price <steven.price@arm.com> Although very minor NITs below which you can consider. > --- > drivers/gpu/drm/panthor/panthor_drv.c | 42 ++++++++++++++++++++++++++- > drivers/gpu/drm/panthor/panthor_gem.h | 2 ++ > include/uapi/drm/panthor_drm.h | 23 +++++++++++++++ > 3 files changed, 66 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > index 06fe46e32073..983b24f1236c 100644 > --- a/drivers/gpu/drm/panthor/panthor_drv.c > +++ b/drivers/gpu/drm/panthor/panthor_drv.c > @@ -1331,6 +1331,44 @@ static int panthor_ioctl_vm_get_state(struct drm_device *ddev, void *data, > return 0; > } > > +static int panthor_ioctl_bo_set_label(struct drm_device *ddev, void *data, > + struct drm_file *file) > +{ > + struct drm_panthor_bo_set_label *args = data; > + struct drm_gem_object *obj; > + const char *label; > + int ret = 0; > + > + obj = drm_gem_object_lookup(file, args->handle); > + if (!obj) > + return -ENOENT; > + > + if (args->size && args->label) { > + if (args->size > PANTHOR_BO_LABEL_MAXLEN) { > + ret = -E2BIG; > + goto err_label; > + } > + > + label = strndup_user(u64_to_user_ptr(args->label), args->size); > + if (IS_ERR(label)) { > + ret = PTR_ERR(label); > + goto err_label; > + } > + } else if (args->size && !args->label) { > + ret = -EINVAL; > + goto err_label; > + } else { > + label = NULL; > + } > + > + panthor_gem_bo_set_label(obj, label); > + > +err_label: > + drm_gem_object_put(obj); > + > + return ret; > +} > + > static int > panthor_open(struct drm_device *ddev, struct drm_file *file) > { > @@ -1400,6 +1438,7 @@ static const struct drm_ioctl_desc panthor_drm_driver_ioctls[] = { > PANTHOR_IOCTL(TILER_HEAP_CREATE, tiler_heap_create, DRM_RENDER_ALLOW), > PANTHOR_IOCTL(TILER_HEAP_DESTROY, tiler_heap_destroy, DRM_RENDER_ALLOW), > PANTHOR_IOCTL(GROUP_SUBMIT, group_submit, DRM_RENDER_ALLOW), > + PANTHOR_IOCTL(BO_SET_LABEL, bo_set_label, DRM_RENDER_ALLOW), > }; > > static int panthor_mmap(struct file *filp, struct vm_area_struct *vma) > @@ -1509,6 +1548,7 @@ static void panthor_debugfs_init(struct drm_minor *minor) > * - 1.2 - adds DEV_QUERY_GROUP_PRIORITIES_INFO query > * - adds PANTHOR_GROUP_PRIORITY_REALTIME priority > * - 1.3 - adds DRM_PANTHOR_GROUP_STATE_INNOCENT flag > + * - 1.4 - adds DRM_IOCTL_PANTHOR_BO_SET_LABEL ioctl > */ > static const struct drm_driver panthor_drm_driver = { > .driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ | > @@ -1522,7 +1562,7 @@ static const struct drm_driver panthor_drm_driver = { > .name = "panthor", > .desc = "Panthor DRM driver", > .major = 1, > - .minor = 3, > + .minor = 4, > > .gem_create_object = panthor_gem_create_object, > .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h > index af0d77338860..beba066b4974 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.h > +++ b/drivers/gpu/drm/panthor/panthor_gem.h > @@ -13,6 +13,8 @@ > > struct panthor_vm; > > +#define PANTHOR_BO_LABEL_MAXLEN PAGE_SIZE > + > /** > * struct panthor_gem_object - Driver specific GEM object. > */ > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h > index 97e2c4510e69..12b1994499a9 100644 > --- a/include/uapi/drm/panthor_drm.h > +++ b/include/uapi/drm/panthor_drm.h > @@ -127,6 +127,9 @@ enum drm_panthor_ioctl_id { > > /** @DRM_PANTHOR_TILER_HEAP_DESTROY: Destroy a tiler heap. */ > DRM_PANTHOR_TILER_HEAP_DESTROY, > + > + /** @DRM_PANTHOR_BO_SET_LABEL: Label a BO. */ > + DRM_PANTHOR_BO_SET_LABEL, > }; > > /** > @@ -977,6 +980,24 @@ struct drm_panthor_tiler_heap_destroy { > __u32 pad; > }; > > +/** > + * struct drm_panthor_bo_set_label - Arguments passed to DRM_IOCTL_PANTHOR_BO_SET_LABEL > + */ > +struct drm_panthor_bo_set_label { > + /** @handle: Handle of the buffer object to label. */ > + __u32 handle; > + > + /** > + * @size: Length of the label, including the NULL terminator. > + * > + * Cannot be greater than the OS page size. > + */ > + __u32 size; > + > + /** @label: User pointer to a NULL-terminated string */ > + __u64 label; > +}; First very minor NIT: * NULL is a pointer, i.e. (void*)0 * NUL is the ASCII code point '\0'. So it's a NUL-terminated string. Second NIT: We don't actually need 'size' - since the string is NUL-terminated we can just strndup_user(__user_pointer__, PAGE_SIZE). As things stand we validate that strlen(label) < size <= PAGE_SIZE - which is a little odd (user space might as well just pass PAGE_SIZE rather than calculate the actual length). Thanks, Steve > + > /** > * DRM_IOCTL_PANTHOR() - Build a Panthor IOCTL number > * @__access: Access type. Must be R, W or RW. > @@ -1019,6 +1040,8 @@ enum { > DRM_IOCTL_PANTHOR(WR, TILER_HEAP_CREATE, tiler_heap_create), > DRM_IOCTL_PANTHOR_TILER_HEAP_DESTROY = > DRM_IOCTL_PANTHOR(WR, TILER_HEAP_DESTROY, tiler_heap_destroy), > + DRM_IOCTL_PANTHOR_BO_SET_LABEL = > + DRM_IOCTL_PANTHOR(WR, BO_SET_LABEL, bo_set_label), > }; > > #if defined(__cplusplus)
On 14.04.2025 11:01, Steven Price wrote: > On 11/04/2025 16:03, Adrián Larumbe wrote: > > Allow UM to label a BO for which it possesses a DRM handle. > > > > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com> > > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> > > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > > Reviewed-by: Steven Price <steven.price@arm.com> > > Although very minor NITs below which you can consider. > > > --- > > drivers/gpu/drm/panthor/panthor_drv.c | 42 ++++++++++++++++++++++++++- > > drivers/gpu/drm/panthor/panthor_gem.h | 2 ++ > > include/uapi/drm/panthor_drm.h | 23 +++++++++++++++ > > 3 files changed, 66 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > > index 06fe46e32073..983b24f1236c 100644 > > --- a/drivers/gpu/drm/panthor/panthor_drv.c > > +++ b/drivers/gpu/drm/panthor/panthor_drv.c > > @@ -1331,6 +1331,44 @@ static int panthor_ioctl_vm_get_state(struct drm_device *ddev, void *data, > > return 0; > > } > > > > +static int panthor_ioctl_bo_set_label(struct drm_device *ddev, void *data, > > + struct drm_file *file) > > +{ > > + struct drm_panthor_bo_set_label *args = data; > > + struct drm_gem_object *obj; > > + const char *label; > > + int ret = 0; > > + > > + obj = drm_gem_object_lookup(file, args->handle); > > + if (!obj) > > + return -ENOENT; > > + > > + if (args->size && args->label) { > > + if (args->size > PANTHOR_BO_LABEL_MAXLEN) { > > + ret = -E2BIG; > > + goto err_label; > > + } > > + > > + label = strndup_user(u64_to_user_ptr(args->label), args->size); > > + if (IS_ERR(label)) { > > + ret = PTR_ERR(label); > > + goto err_label; > > + } > > + } else if (args->size && !args->label) { > > + ret = -EINVAL; > > + goto err_label; > > + } else { > > + label = NULL; > > + } > > + > > + panthor_gem_bo_set_label(obj, label); > > + > > +err_label: > > + drm_gem_object_put(obj); > > + > > + return ret; > > +} > > + > > static int > > panthor_open(struct drm_device *ddev, struct drm_file *file) > > { > > @@ -1400,6 +1438,7 @@ static const struct drm_ioctl_desc panthor_drm_driver_ioctls[] = { > > PANTHOR_IOCTL(TILER_HEAP_CREATE, tiler_heap_create, DRM_RENDER_ALLOW), > > PANTHOR_IOCTL(TILER_HEAP_DESTROY, tiler_heap_destroy, DRM_RENDER_ALLOW), > > PANTHOR_IOCTL(GROUP_SUBMIT, group_submit, DRM_RENDER_ALLOW), > > + PANTHOR_IOCTL(BO_SET_LABEL, bo_set_label, DRM_RENDER_ALLOW), > > }; > > > > static int panthor_mmap(struct file *filp, struct vm_area_struct *vma) > > @@ -1509,6 +1548,7 @@ static void panthor_debugfs_init(struct drm_minor *minor) > > * - 1.2 - adds DEV_QUERY_GROUP_PRIORITIES_INFO query > > * - adds PANTHOR_GROUP_PRIORITY_REALTIME priority > > * - 1.3 - adds DRM_PANTHOR_GROUP_STATE_INNOCENT flag > > + * - 1.4 - adds DRM_IOCTL_PANTHOR_BO_SET_LABEL ioctl > > */ > > static const struct drm_driver panthor_drm_driver = { > > .driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ | > > @@ -1522,7 +1562,7 @@ static const struct drm_driver panthor_drm_driver = { > > .name = "panthor", > > .desc = "Panthor DRM driver", > > .major = 1, > > - .minor = 3, > > + .minor = 4, > > > > .gem_create_object = panthor_gem_create_object, > > .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h > > index af0d77338860..beba066b4974 100644 > > --- a/drivers/gpu/drm/panthor/panthor_gem.h > > +++ b/drivers/gpu/drm/panthor/panthor_gem.h > > @@ -13,6 +13,8 @@ > > > > struct panthor_vm; > > > > +#define PANTHOR_BO_LABEL_MAXLEN PAGE_SIZE > > + > > /** > > * struct panthor_gem_object - Driver specific GEM object. > > */ > > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h > > index 97e2c4510e69..12b1994499a9 100644 > > --- a/include/uapi/drm/panthor_drm.h > > +++ b/include/uapi/drm/panthor_drm.h > > @@ -127,6 +127,9 @@ enum drm_panthor_ioctl_id { > > > > /** @DRM_PANTHOR_TILER_HEAP_DESTROY: Destroy a tiler heap. */ > > DRM_PANTHOR_TILER_HEAP_DESTROY, > > + > > + /** @DRM_PANTHOR_BO_SET_LABEL: Label a BO. */ > > + DRM_PANTHOR_BO_SET_LABEL, > > }; > > > > /** > > @@ -977,6 +980,24 @@ struct drm_panthor_tiler_heap_destroy { > > __u32 pad; > > }; > > > > +/** > > + * struct drm_panthor_bo_set_label - Arguments passed to DRM_IOCTL_PANTHOR_BO_SET_LABEL > > + */ > > +struct drm_panthor_bo_set_label { > > + /** @handle: Handle of the buffer object to label. */ > > + __u32 handle; > > + > > + /** > > + * @size: Length of the label, including the NULL terminator. > > + * > > + * Cannot be greater than the OS page size. > > + */ > > + __u32 size; > > + > > + /** @label: User pointer to a NULL-terminated string */ > > + __u64 label; > > +}; > > First very minor NIT: > * NULL is a pointer, i.e. (void*)0 > * NUL is the ASCII code point '\0'. > So it's a NUL-terminated string. Fixed > Second NIT: We don't actually need 'size' - since the string is > NUL-terminated we can just strndup_user(__user_pointer__, PAGE_SIZE). > As things stand we validate that strlen(label) < size <= PAGE_SIZE - > which is a little odd (user space might as well just pass PAGE_SIZE > rather than calculate the actual length). The snag I see in this approach is that the only way to make sure strlen(label) + 1 <= PAGE_SIZE would be doing something like label = strndup_user(u64_to_user_ptr(args->label), args->size); if (strlen(label) + 1 <= PAGE_SIZE) { kfree(label) return -E2BIG; } In the meantime, we've duplicated the string and traversed a whole page of bytes, all to be discarded at once. In this case, I think it's alright to expect some cooperation from UM in supplying the actual size, although I'm really not an expert in designing elegant uAPIs, so if you think this looks very odd I'd be glad to replace it with. Actually, as I was writing this, I realised that strndup_user() calls strnlen_user(), which is publicly available for other drivers, so I might check the length first, and if it falls within bounds, do the actual user stringdup. I shall also mention the size bound on the uAPI for the 'label' pointer. > Thanks, > Steve > > + > /** > * DRM_IOCTL_PANTHOR() - Build a Panthor IOCTL number > * @__access: Access type. Must be R, W or RW. > @@ -1019,6 +1040,8 @@ enum { > DRM_IOCTL_PANTHOR(WR, TILER_HEAP_CREATE, tiler_heap_create), > DRM_IOCTL_PANTHOR_TILER_HEAP_DESTROY = > DRM_IOCTL_PANTHOR(WR, TILER_HEAP_DESTROY, tiler_heap_destroy), > + DRM_IOCTL_PANTHOR_BO_SET_LABEL = > + DRM_IOCTL_PANTHOR(WR, BO_SET_LABEL, bo_set_label), > }; > > #if defined(__cplusplus) Adrian Larumbe
Hi,
On Fri, 11 Apr 2025 at 16:05, Adrián Larumbe
<adrian.larumbe@collabora.com> wrote:
> +#define PANTHOR_BO_LABEL_MAXLEN PAGE_SIZE
PAGE_SIZE can change between kernel builds with a config setting.
If the thinking here is '4KiB is big enough' (which I agree with),
then just define it to 4096.
Cheers,
Daniel
diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c index 06fe46e32073..983b24f1236c 100644 --- a/drivers/gpu/drm/panthor/panthor_drv.c +++ b/drivers/gpu/drm/panthor/panthor_drv.c @@ -1331,6 +1331,44 @@ static int panthor_ioctl_vm_get_state(struct drm_device *ddev, void *data, return 0; } +static int panthor_ioctl_bo_set_label(struct drm_device *ddev, void *data, + struct drm_file *file) +{ + struct drm_panthor_bo_set_label *args = data; + struct drm_gem_object *obj; + const char *label; + int ret = 0; + + obj = drm_gem_object_lookup(file, args->handle); + if (!obj) + return -ENOENT; + + if (args->size && args->label) { + if (args->size > PANTHOR_BO_LABEL_MAXLEN) { + ret = -E2BIG; + goto err_label; + } + + label = strndup_user(u64_to_user_ptr(args->label), args->size); + if (IS_ERR(label)) { + ret = PTR_ERR(label); + goto err_label; + } + } else if (args->size && !args->label) { + ret = -EINVAL; + goto err_label; + } else { + label = NULL; + } + + panthor_gem_bo_set_label(obj, label); + +err_label: + drm_gem_object_put(obj); + + return ret; +} + static int panthor_open(struct drm_device *ddev, struct drm_file *file) { @@ -1400,6 +1438,7 @@ static const struct drm_ioctl_desc panthor_drm_driver_ioctls[] = { PANTHOR_IOCTL(TILER_HEAP_CREATE, tiler_heap_create, DRM_RENDER_ALLOW), PANTHOR_IOCTL(TILER_HEAP_DESTROY, tiler_heap_destroy, DRM_RENDER_ALLOW), PANTHOR_IOCTL(GROUP_SUBMIT, group_submit, DRM_RENDER_ALLOW), + PANTHOR_IOCTL(BO_SET_LABEL, bo_set_label, DRM_RENDER_ALLOW), }; static int panthor_mmap(struct file *filp, struct vm_area_struct *vma) @@ -1509,6 +1548,7 @@ static void panthor_debugfs_init(struct drm_minor *minor) * - 1.2 - adds DEV_QUERY_GROUP_PRIORITIES_INFO query * - adds PANTHOR_GROUP_PRIORITY_REALTIME priority * - 1.3 - adds DRM_PANTHOR_GROUP_STATE_INNOCENT flag + * - 1.4 - adds DRM_IOCTL_PANTHOR_BO_SET_LABEL ioctl */ static const struct drm_driver panthor_drm_driver = { .driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ | @@ -1522,7 +1562,7 @@ static const struct drm_driver panthor_drm_driver = { .name = "panthor", .desc = "Panthor DRM driver", .major = 1, - .minor = 3, + .minor = 4, .gem_create_object = panthor_gem_create_object, .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h index af0d77338860..beba066b4974 100644 --- a/drivers/gpu/drm/panthor/panthor_gem.h +++ b/drivers/gpu/drm/panthor/panthor_gem.h @@ -13,6 +13,8 @@ struct panthor_vm; +#define PANTHOR_BO_LABEL_MAXLEN PAGE_SIZE + /** * struct panthor_gem_object - Driver specific GEM object. */ diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h index 97e2c4510e69..12b1994499a9 100644 --- a/include/uapi/drm/panthor_drm.h +++ b/include/uapi/drm/panthor_drm.h @@ -127,6 +127,9 @@ enum drm_panthor_ioctl_id { /** @DRM_PANTHOR_TILER_HEAP_DESTROY: Destroy a tiler heap. */ DRM_PANTHOR_TILER_HEAP_DESTROY, + + /** @DRM_PANTHOR_BO_SET_LABEL: Label a BO. */ + DRM_PANTHOR_BO_SET_LABEL, }; /** @@ -977,6 +980,24 @@ struct drm_panthor_tiler_heap_destroy { __u32 pad; }; +/** + * struct drm_panthor_bo_set_label - Arguments passed to DRM_IOCTL_PANTHOR_BO_SET_LABEL + */ +struct drm_panthor_bo_set_label { + /** @handle: Handle of the buffer object to label. */ + __u32 handle; + + /** + * @size: Length of the label, including the NULL terminator. + * + * Cannot be greater than the OS page size. + */ + __u32 size; + + /** @label: User pointer to a NULL-terminated string */ + __u64 label; +}; + /** * DRM_IOCTL_PANTHOR() - Build a Panthor IOCTL number * @__access: Access type. Must be R, W or RW. @@ -1019,6 +1040,8 @@ enum { DRM_IOCTL_PANTHOR(WR, TILER_HEAP_CREATE, tiler_heap_create), DRM_IOCTL_PANTHOR_TILER_HEAP_DESTROY = DRM_IOCTL_PANTHOR(WR, TILER_HEAP_DESTROY, tiler_heap_destroy), + DRM_IOCTL_PANTHOR_BO_SET_LABEL = + DRM_IOCTL_PANTHOR(WR, BO_SET_LABEL, bo_set_label), }; #if defined(__cplusplus)