| Message ID | 20220601161303.64797-1-contact@emersion.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] dma-buf: Add a capabilities directory | expand |
Am 01.06.22 um 18:13 schrieb Simon Ser: > To discover support for new DMA-BUF IOCTLs, user-space has no > choice but to try to perform the IOCTL on an existing DMA-BUF. > However, user-space may want to figure out whether or not the > IOCTL is available before it has a DMA-BUF at hand, e.g. at > initialization time in a Wayland compositor. > > Add a /sys/kernel/dmabuf/caps directory which allows the DMA-BUF > subsystem to advertise supported features. Add a > sync_file_import_export entry which indicates that importing and > exporting sync_files from/to DMA-BUFs is supported. > > v2: Add missing files lost in a rebase > > v3: > - Create separate file in Documentation/ABI/testing/, add it to > MAINTAINERS > - Fix kernel version (Daniel) > - Remove unnecessary brackets (Jason) > - Fix SPDX comment style > > v4: improve sysfs code (Greg) > > Signed-off-by: Simon Ser <contact@emersion.fr> > Reviewed-by: Jason Ekstrand <jason.ekstrand@collabora.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> > Cc: Christian König <christian.koenig@amd.com> > Cc: Greg KH <greg@kroah.com> I can't judge about the sysfs API usage stuff, but everything else is Reviewed-by: Christian König <christian.koenig@amd.com>. Regards, Christian. > --- > .../ABI/testing/sysfs-kernel-dmabuf-caps | 13 ++++++ > MAINTAINERS | 1 + > drivers/dma-buf/Makefile | 2 +- > drivers/dma-buf/dma-buf-sysfs-caps.c | 31 +++++++++++++ > drivers/dma-buf/dma-buf-sysfs-caps.h | 15 +++++++ > drivers/dma-buf/dma-buf-sysfs-stats.c | 16 ++----- > drivers/dma-buf/dma-buf-sysfs-stats.h | 6 ++- > drivers/dma-buf/dma-buf.c | 45 +++++++++++++++++-- > include/uapi/linux/dma-buf.h | 6 +++ > 9 files changed, 115 insertions(+), 20 deletions(-) > create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf-caps > create mode 100644 drivers/dma-buf/dma-buf-sysfs-caps.c > create mode 100644 drivers/dma-buf/dma-buf-sysfs-caps.h > > diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps b/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps > new file mode 100644 > index 000000000000..f83af422fd18 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps > @@ -0,0 +1,13 @@ > +What: /sys/kernel/dmabuf/caps > +Date: May 2022 > +KernelVersion: v5.20 > +Contact: Simon Ser <contact@emersion.fr> > +Description: This directory advertises DMA-BUF capabilities supported by the > + kernel. > + > +What: /sys/kernel/dmabuf/caps/sync_file_import_export > +Date: May 2022 > +KernelVersion: v5.20 > +Contact: Simon Ser <contact@emersion.fr> > +Description: This file is read-only and advertises support for importing and > + exporting sync_files from/to DMA-BUFs. > diff --git a/MAINTAINERS b/MAINTAINERS > index 11da16bfa123..8966686f7231 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5871,6 +5871,7 @@ L: dri-devel@lists.freedesktop.org > L: linaro-mm-sig@lists.linaro.org (moderated for non-subscribers) > S: Maintained > T: git git://anongit.freedesktop.org/drm/drm-misc > +F: Documentation/ABI/testing/sysfs-kernel-dmabuf-caps > F: Documentation/driver-api/dma-buf.rst > F: drivers/dma-buf/ > F: include/linux/*fence.h > diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile > index 4c9eb53ba3f8..afc874272710 100644 > --- a/drivers/dma-buf/Makefile > +++ b/drivers/dma-buf/Makefile > @@ -1,6 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0-only > obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \ > - dma-resv.o > + dma-resv.o dma-buf-sysfs-caps.o > obj-$(CONFIG_DMABUF_HEAPS) += dma-heap.o > obj-$(CONFIG_DMABUF_HEAPS) += heaps/ > obj-$(CONFIG_SYNC_FILE) += sync_file.o > diff --git a/drivers/dma-buf/dma-buf-sysfs-caps.c b/drivers/dma-buf/dma-buf-sysfs-caps.c > new file mode 100644 > index 000000000000..6eb27b81469f > --- /dev/null > +++ b/drivers/dma-buf/dma-buf-sysfs-caps.c > @@ -0,0 +1,31 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * DMA-BUF sysfs capabilities. > + * > + * Copyright (C) 2022 Simon Ser > + */ > + > +#include <linux/kobject.h> > +#include <linux/sysfs.h> > + > +#include "dma-buf-sysfs-caps.h" > + > +static ssize_t sync_file_import_export_show(struct kobject *kobj, > + struct kobj_attribute *attr, > + char *buf) > +{ > + return sysfs_emit(buf, "1\n"); > +} > + > +static struct kobj_attribute dma_buf_sync_file_import_export_attr = > + __ATTR_RO(sync_file_import_export); > + > +static struct attribute *dma_buf_caps_attrs[] = { > + &dma_buf_sync_file_import_export_attr.attr, > + NULL, > +}; > + > +const struct attribute_group dma_buf_caps_group = { > + .name = "caps", > + .attrs = dma_buf_caps_attrs, > +}; > diff --git a/drivers/dma-buf/dma-buf-sysfs-caps.h b/drivers/dma-buf/dma-buf-sysfs-caps.h > new file mode 100644 > index 000000000000..e40a93513f06 > --- /dev/null > +++ b/drivers/dma-buf/dma-buf-sysfs-caps.h > @@ -0,0 +1,15 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * DMA-BUF sysfs capabilities. > + * > + * Copyright (C) 2022 Simon Ser > + */ > + > +#ifndef _DMA_BUF_SYSFS_CAPS_H > +#define _DMA_BUF_SYSFS_CAPS_H > + > +struct attribute_group; > + > +extern const struct attribute_group dma_buf_caps_group; > + > +#endif // _DMA_BUF_SYSFS_CAPS_H > diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c > index 2bba0babcb62..e2e62f83ce18 100644 > --- a/drivers/dma-buf/dma-buf-sysfs-stats.c > +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c > @@ -141,23 +141,14 @@ static const struct kset_uevent_ops dmabuf_sysfs_no_uevent_ops = { > .filter = dmabuf_sysfs_uevent_filter, > }; > > -static struct kset *dma_buf_stats_kset; > static struct kset *dma_buf_per_buffer_stats_kset; > -int dma_buf_init_sysfs_statistics(void) > +int dma_buf_init_sysfs_statistics(struct kset *kset) > { > - dma_buf_stats_kset = kset_create_and_add("dmabuf", > - &dmabuf_sysfs_no_uevent_ops, > - kernel_kobj); > - if (!dma_buf_stats_kset) > - return -ENOMEM; > - > dma_buf_per_buffer_stats_kset = kset_create_and_add("buffers", > &dmabuf_sysfs_no_uevent_ops, > - &dma_buf_stats_kset->kobj); > - if (!dma_buf_per_buffer_stats_kset) { > - kset_unregister(dma_buf_stats_kset); > + &kset->kobj); > + if (!dma_buf_per_buffer_stats_kset) > return -ENOMEM; > - } > > return 0; > } > @@ -165,7 +156,6 @@ int dma_buf_init_sysfs_statistics(void) > void dma_buf_uninit_sysfs_statistics(void) > { > kset_unregister(dma_buf_per_buffer_stats_kset); > - kset_unregister(dma_buf_stats_kset); > } > > int dma_buf_stats_setup(struct dma_buf *dmabuf) > diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.h b/drivers/dma-buf/dma-buf-sysfs-stats.h > index a49c6e2650cc..798c54fb8ee3 100644 > --- a/drivers/dma-buf/dma-buf-sysfs-stats.h > +++ b/drivers/dma-buf/dma-buf-sysfs-stats.h > @@ -8,9 +8,11 @@ > #ifndef _DMA_BUF_SYSFS_STATS_H > #define _DMA_BUF_SYSFS_STATS_H > > +struct kset; > + > #ifdef CONFIG_DMABUF_SYSFS_STATS > > -int dma_buf_init_sysfs_statistics(void); > +int dma_buf_init_sysfs_statistics(struct kset *kset); > void dma_buf_uninit_sysfs_statistics(void); > > int dma_buf_stats_setup(struct dma_buf *dmabuf); > @@ -18,7 +20,7 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf); > void dma_buf_stats_teardown(struct dma_buf *dmabuf); > #else > > -static inline int dma_buf_init_sysfs_statistics(void) > +static inline int dma_buf_init_sysfs_statistics(struct kset *kset) > { > return 0; > } > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 5e1b0534b3ce..61c5be57558e 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -30,6 +30,7 @@ > #include <uapi/linux/dma-buf.h> > #include <uapi/linux/magic.h> > > +#include "dma-buf-sysfs-caps.h" > #include "dma-buf-sysfs-stats.h" > > static inline int is_dma_buf_file(struct file *); > @@ -1546,22 +1547,57 @@ static inline void dma_buf_uninit_debugfs(void) > } > #endif > > +/* Capabilities and statistics files do not need to send uevents. */ > +static int dmabuf_sysfs_uevent_filter(struct kobject *kobj) > +{ > + return 0; > +} > + > +static const struct kset_uevent_ops dmabuf_sysfs_no_uevent_ops = { > + .filter = dmabuf_sysfs_uevent_filter, > +}; > + > +static const struct attribute_group *dma_buf_sysfs_groups[] = { > + &dma_buf_caps_group, > + NULL, > +}; > + > +static struct kset *dma_buf_kset; > + > static int __init dma_buf_init(void) > { > int ret; > > - ret = dma_buf_init_sysfs_statistics(); > + dma_buf_kset = kset_create_and_add("dmabuf", > + &dmabuf_sysfs_no_uevent_ops, > + kernel_kobj); > + if (!dma_buf_kset) > + return -ENOMEM; > + > + ret = sysfs_create_groups(&dma_buf_kset->kobj, dma_buf_sysfs_groups); > if (ret) > - return ret; > + goto err_kset; > + > + ret = dma_buf_init_sysfs_statistics(dma_buf_kset); > + if (ret) > + goto err_kset; > > dma_buf_mnt = kern_mount(&dma_buf_fs_type); > - if (IS_ERR(dma_buf_mnt)) > - return PTR_ERR(dma_buf_mnt); > + if (IS_ERR(dma_buf_mnt)) { > + ret = PTR_ERR(dma_buf_mnt); > + goto err_sysfs_stats; > + } > > mutex_init(&db_list.lock); > INIT_LIST_HEAD(&db_list.head); > dma_buf_init_debugfs(); > return 0; > + > +err_sysfs_stats: > + dma_buf_uninit_sysfs_statistics(); > +err_kset: > + kset_unregister(dma_buf_kset); > + return ret; > } > subsys_initcall(dma_buf_init); > > @@ -1570,5 +1606,6 @@ static void __exit dma_buf_deinit(void) > dma_buf_uninit_debugfs(); > kern_unmount(dma_buf_mnt); > dma_buf_uninit_sysfs_statistics(); > + kset_unregister(dma_buf_kset); > } > __exitcall(dma_buf_deinit); > diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h > index 70e213a0d7d9..ab3afd5da75a 100644 > --- a/include/uapi/linux/dma-buf.h > +++ b/include/uapi/linux/dma-buf.h > @@ -114,6 +114,9 @@ struct dma_buf_sync { > * ordering via these fences, it is the respnosibility of userspace to use > * locks or other mechanisms to ensure that no other context adds fences or > * submits work between steps 1 and 3 above. > + * > + * Userspace can check the availability of this API via > + * /sys/kernel/dmabuf/caps/sync_file_import_export. > */ > struct dma_buf_export_sync_file { > /** > @@ -146,6 +149,9 @@ struct dma_buf_export_sync_file { > * synchronized APIs such as Vulkan to inter-op with dma-buf consumers > * which expect implicit synchronization such as OpenGL or most media > * drivers/video. > + * > + * Userspace can check the availability of this API via > + * /sys/kernel/dmabuf/caps/sync_file_import_export. > */ > struct dma_buf_import_sync_file { > /**
v4 looks good to me as well. --Jason On Wed, 2022-06-01 at 16:13 +0000, Simon Ser wrote: > To discover support for new DMA-BUF IOCTLs, user-space has no > choice but to try to perform the IOCTL on an existing DMA-BUF. > However, user-space may want to figure out whether or not the > IOCTL is available before it has a DMA-BUF at hand, e.g. at > initialization time in a Wayland compositor. > > Add a /sys/kernel/dmabuf/caps directory which allows the DMA-BUF > subsystem to advertise supported features. Add a > sync_file_import_export entry which indicates that importing and > exporting sync_files from/to DMA-BUFs is supported. > > v2: Add missing files lost in a rebase > > v3: > - Create separate file in Documentation/ABI/testing/, add it to > MAINTAINERS > - Fix kernel version (Daniel) > - Remove unnecessary brackets (Jason) > - Fix SPDX comment style > > v4: improve sysfs code (Greg) > > Signed-off-by: Simon Ser <contact@emersion.fr> > Reviewed-by: Jason Ekstrand <jason.ekstrand@collabora.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> > Cc: Christian König <christian.koenig@amd.com> > Cc: Greg KH <greg@kroah.com> > --- > .../ABI/testing/sysfs-kernel-dmabuf-caps | 13 ++++++ > MAINTAINERS | 1 + > drivers/dma-buf/Makefile | 2 +- > drivers/dma-buf/dma-buf-sysfs-caps.c | 31 +++++++++++++ > drivers/dma-buf/dma-buf-sysfs-caps.h | 15 +++++++ > drivers/dma-buf/dma-buf-sysfs-stats.c | 16 ++----- > drivers/dma-buf/dma-buf-sysfs-stats.h | 6 ++- > drivers/dma-buf/dma-buf.c | 45 > +++++++++++++++++-- > include/uapi/linux/dma-buf.h | 6 +++ > 9 files changed, 115 insertions(+), 20 deletions(-) > create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf- > caps > create mode 100644 drivers/dma-buf/dma-buf-sysfs-caps.c > create mode 100644 drivers/dma-buf/dma-buf-sysfs-caps.h > > diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps > b/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps > new file mode 100644 > index 000000000000..f83af422fd18 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps > @@ -0,0 +1,13 @@ > +What: /sys/kernel/dmabuf/caps > +Date: May 2022 > +KernelVersion: v5.20 > +Contact: Simon Ser <contact@emersion.fr> > +Description: This directory advertises DMA-BUF capabilities > supported by the > + kernel. > + > +What: /sys/kernel/dmabuf/caps/sync_file_import_export > +Date: May 2022 > +KernelVersion: v5.20 > +Contact: Simon Ser <contact@emersion.fr> > +Description: This file is read-only and advertises support for > importing and > + exporting sync_files from/to DMA-BUFs. > diff --git a/MAINTAINERS b/MAINTAINERS > index 11da16bfa123..8966686f7231 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5871,6 +5871,7 @@ L: dri-devel@lists.freedesktop.org > L: linaro-mm-sig@lists.linaro.org (moderated for non- > subscribers) > S: Maintained > T: git git://anongit.freedesktop.org/drm/drm-misc > +F: Documentation/ABI/testing/sysfs-kernel-dmabuf-caps > F: Documentation/driver-api/dma-buf.rst > F: drivers/dma-buf/ > F: include/linux/*fence.h > diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile > index 4c9eb53ba3f8..afc874272710 100644 > --- a/drivers/dma-buf/Makefile > +++ b/drivers/dma-buf/Makefile > @@ -1,6 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0-only > obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \ > - dma-resv.o > + dma-resv.o dma-buf-sysfs-caps.o > obj-$(CONFIG_DMABUF_HEAPS) += dma-heap.o > obj-$(CONFIG_DMABUF_HEAPS) += heaps/ > obj-$(CONFIG_SYNC_FILE) += sync_file.o > diff --git a/drivers/dma-buf/dma-buf-sysfs-caps.c b/drivers/dma- > buf/dma-buf-sysfs-caps.c > new file mode 100644 > index 000000000000..6eb27b81469f > --- /dev/null > +++ b/drivers/dma-buf/dma-buf-sysfs-caps.c > @@ -0,0 +1,31 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * DMA-BUF sysfs capabilities. > + * > + * Copyright (C) 2022 Simon Ser > + */ > + > +#include <linux/kobject.h> > +#include <linux/sysfs.h> > + > +#include "dma-buf-sysfs-caps.h" > + > +static ssize_t sync_file_import_export_show(struct kobject *kobj, > + struct kobj_attribute > *attr, > + char *buf) > +{ > + return sysfs_emit(buf, "1\n"); > +} > + > +static struct kobj_attribute dma_buf_sync_file_import_export_attr = > + __ATTR_RO(sync_file_import_export); > + > +static struct attribute *dma_buf_caps_attrs[] = { > + &dma_buf_sync_file_import_export_attr.attr, > + NULL, > +}; > + > +const struct attribute_group dma_buf_caps_group = { > + .name = "caps", > + .attrs = dma_buf_caps_attrs, > +}; > diff --git a/drivers/dma-buf/dma-buf-sysfs-caps.h b/drivers/dma- > buf/dma-buf-sysfs-caps.h > new file mode 100644 > index 000000000000..e40a93513f06 > --- /dev/null > +++ b/drivers/dma-buf/dma-buf-sysfs-caps.h > @@ -0,0 +1,15 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * DMA-BUF sysfs capabilities. > + * > + * Copyright (C) 2022 Simon Ser > + */ > + > +#ifndef _DMA_BUF_SYSFS_CAPS_H > +#define _DMA_BUF_SYSFS_CAPS_H > + > +struct attribute_group; > + > +extern const struct attribute_group dma_buf_caps_group; > + > +#endif // _DMA_BUF_SYSFS_CAPS_H > diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma- > buf/dma-buf-sysfs-stats.c > index 2bba0babcb62..e2e62f83ce18 100644 > --- a/drivers/dma-buf/dma-buf-sysfs-stats.c > +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c > @@ -141,23 +141,14 @@ static const struct kset_uevent_ops > dmabuf_sysfs_no_uevent_ops = { > .filter = dmabuf_sysfs_uevent_filter, > }; > > -static struct kset *dma_buf_stats_kset; > static struct kset *dma_buf_per_buffer_stats_kset; > -int dma_buf_init_sysfs_statistics(void) > +int dma_buf_init_sysfs_statistics(struct kset *kset) > { > - dma_buf_stats_kset = kset_create_and_add("dmabuf", > - > &dmabuf_sysfs_no_uevent_ops, > - kernel_kobj); > - if (!dma_buf_stats_kset) > - return -ENOMEM; > - > dma_buf_per_buffer_stats_kset = > kset_create_and_add("buffers", > > &dmabuf_sysfs_no_uevent_ops, > - > &dma_buf_stats_kset->kobj); > - if (!dma_buf_per_buffer_stats_kset) { > - kset_unregister(dma_buf_stats_kset); > + &kset- > >kobj); > + if (!dma_buf_per_buffer_stats_kset) > return -ENOMEM; > - } > > return 0; > } > @@ -165,7 +156,6 @@ int dma_buf_init_sysfs_statistics(void) > void dma_buf_uninit_sysfs_statistics(void) > { > kset_unregister(dma_buf_per_buffer_stats_kset); > - kset_unregister(dma_buf_stats_kset); > } > > int dma_buf_stats_setup(struct dma_buf *dmabuf) > diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.h b/drivers/dma- > buf/dma-buf-sysfs-stats.h > index a49c6e2650cc..798c54fb8ee3 100644 > --- a/drivers/dma-buf/dma-buf-sysfs-stats.h > +++ b/drivers/dma-buf/dma-buf-sysfs-stats.h > @@ -8,9 +8,11 @@ > #ifndef _DMA_BUF_SYSFS_STATS_H > #define _DMA_BUF_SYSFS_STATS_H > > +struct kset; > + > #ifdef CONFIG_DMABUF_SYSFS_STATS > > -int dma_buf_init_sysfs_statistics(void); > +int dma_buf_init_sysfs_statistics(struct kset *kset); > void dma_buf_uninit_sysfs_statistics(void); > > int dma_buf_stats_setup(struct dma_buf *dmabuf); > @@ -18,7 +20,7 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf); > void dma_buf_stats_teardown(struct dma_buf *dmabuf); > #else > > -static inline int dma_buf_init_sysfs_statistics(void) > +static inline int dma_buf_init_sysfs_statistics(struct kset *kset) > { > return 0; > } > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 5e1b0534b3ce..61c5be57558e 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -30,6 +30,7 @@ > #include <uapi/linux/dma-buf.h> > #include <uapi/linux/magic.h> > > +#include "dma-buf-sysfs-caps.h" > #include "dma-buf-sysfs-stats.h" > > static inline int is_dma_buf_file(struct file *); > @@ -1546,22 +1547,57 @@ static inline void > dma_buf_uninit_debugfs(void) > } > #endif > > +/* Capabilities and statistics files do not need to send uevents. */ > +static int dmabuf_sysfs_uevent_filter(struct kobject *kobj) > +{ > + return 0; > +} > + > +static const struct kset_uevent_ops dmabuf_sysfs_no_uevent_ops = { > + .filter = dmabuf_sysfs_uevent_filter, > +}; > + > +static const struct attribute_group *dma_buf_sysfs_groups[] = { > + &dma_buf_caps_group, > + NULL, > +}; > + > +static struct kset *dma_buf_kset; > + > static int __init dma_buf_init(void) > { > int ret; > > - ret = dma_buf_init_sysfs_statistics(); > + dma_buf_kset = kset_create_and_add("dmabuf", > + > &dmabuf_sysfs_no_uevent_ops, > + kernel_kobj); > + if (!dma_buf_kset) > + return -ENOMEM; > + > + ret = sysfs_create_groups(&dma_buf_kset->kobj, > dma_buf_sysfs_groups); > if (ret) > - return ret; > + goto err_kset; > + > + ret = dma_buf_init_sysfs_statistics(dma_buf_kset); > + if (ret) > + goto err_kset; > > dma_buf_mnt = kern_mount(&dma_buf_fs_type); > - if (IS_ERR(dma_buf_mnt)) > - return PTR_ERR(dma_buf_mnt); > + if (IS_ERR(dma_buf_mnt)) { > + ret = PTR_ERR(dma_buf_mnt); > + goto err_sysfs_stats; > + } > > mutex_init(&db_list.lock); > INIT_LIST_HEAD(&db_list.head); > dma_buf_init_debugfs(); > return 0; > + > +err_sysfs_stats: > + dma_buf_uninit_sysfs_statistics(); > +err_kset: > + kset_unregister(dma_buf_kset); > + return ret; > } > subsys_initcall(dma_buf_init); > > @@ -1570,5 +1606,6 @@ static void __exit dma_buf_deinit(void) > dma_buf_uninit_debugfs(); > kern_unmount(dma_buf_mnt); > dma_buf_uninit_sysfs_statistics(); > + kset_unregister(dma_buf_kset); > } > __exitcall(dma_buf_deinit); > diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma- > buf.h > index 70e213a0d7d9..ab3afd5da75a 100644 > --- a/include/uapi/linux/dma-buf.h > +++ b/include/uapi/linux/dma-buf.h > @@ -114,6 +114,9 @@ struct dma_buf_sync { > * ordering via these fences, it is the respnosibility of userspace > to use > * locks or other mechanisms to ensure that no other context adds > fences or > * submits work between steps 1 and 3 above. > + * > + * Userspace can check the availability of this API via > + * /sys/kernel/dmabuf/caps/sync_file_import_export. > */ > struct dma_buf_export_sync_file { > /** > @@ -146,6 +149,9 @@ struct dma_buf_export_sync_file { > * synchronized APIs such as Vulkan to inter-op with dma-buf > consumers > * which expect implicit synchronization such as OpenGL or most > media > * drivers/video. > + * > + * Userspace can check the availability of this API via > + * /sys/kernel/dmabuf/caps/sync_file_import_export. > */ > struct dma_buf_import_sync_file { > /**
On Wed, Jun 01, 2022 at 04:13:14PM +0000, Simon Ser wrote: > To discover support for new DMA-BUF IOCTLs, user-space has no > choice but to try to perform the IOCTL on an existing DMA-BUF. Which is correct and how all kernel features work (sorry I missed the main goal of this patch earlier and focused only on the sysfs stuff). > However, user-space may want to figure out whether or not the > IOCTL is available before it has a DMA-BUF at hand, e.g. at > initialization time in a Wayland compositor. Why not just do the ioctl in a test way? That's how we determine kernel features, we do not poke around in sysfs to determine what is, or is not, present at runtime. > Add a /sys/kernel/dmabuf/caps directory which allows the DMA-BUF > subsystem to advertise supported features. Add a > sync_file_import_export entry which indicates that importing and > exporting sync_files from/to DMA-BUFs is supported. No, sorry, this is not a sustainable thing to do for all kernel features over time. Please just do the ioctl and go from there. sysfs is not for advertising what is and is not enabled/present in a kernel with regards to functionality or capabilities of the system. If sysfs were to export this type of thing, it would have to do it for everything, not just some random tiny thing of one kernel driver. So no, sorry, this is not ok to be merged. greg k-h
On Thursday, June 2nd, 2022 at 07:40, Greg KH <greg@kroah.com> wrote: > On Wed, Jun 01, 2022 at 04:13:14PM +0000, Simon Ser wrote: > > > To discover support for new DMA-BUF IOCTLs, user-space has no > > choice but to try to perform the IOCTL on an existing DMA-BUF. > > Which is correct and how all kernel features work (sorry I missed the > main goal of this patch earlier and focused only on the sysfs stuff). > > > However, user-space may want to figure out whether or not the > > IOCTL is available before it has a DMA-BUF at hand, e.g. at > > initialization time in a Wayland compositor. > > Why not just do the ioctl in a test way? That's how we determine kernel > features, we do not poke around in sysfs to determine what is, or is > not, present at runtime. > > > Add a /sys/kernel/dmabuf/caps directory which allows the DMA-BUF > > subsystem to advertise supported features. Add a > > sync_file_import_export entry which indicates that importing and > > exporting sync_files from/to DMA-BUFs is supported. > > No, sorry, this is not a sustainable thing to do for all kernel features > over time. Please just do the ioctl and go from there. sysfs is not > for advertising what is and is not enabled/present in a kernel with > regards to functionality or capabilities of the system. > > If sysfs were to export this type of thing, it would have to do it for > everything, not just some random tiny thing of one kernel driver. I'd argue that DMA-BUF is a special case here. To check whether the import/export IOCTLs are available, user-space needs a DMA-BUF to try to perform the IOCTL. To get a DMA-BUF, user-space needs to enumerate GPUs, pick one at random, load GBM or Vulkan, use that heavy-weight API to allocate a "fake" buffer on the GPU, export that buffer into a DMA-BUF, try the IOCTL, then teardown all of this. There is no other way. This sounds like a roundabout way to answer the simple question "is the IOCTL available?". Do you have another suggestion to address this problem?
On Thu, Jun 02, 2022 at 06:17:31AM +0000, Simon Ser wrote: > On Thursday, June 2nd, 2022 at 07:40, Greg KH <greg@kroah.com> wrote: > > > On Wed, Jun 01, 2022 at 04:13:14PM +0000, Simon Ser wrote: > > > > > To discover support for new DMA-BUF IOCTLs, user-space has no > > > choice but to try to perform the IOCTL on an existing DMA-BUF. > > > > Which is correct and how all kernel features work (sorry I missed the > > main goal of this patch earlier and focused only on the sysfs stuff). > > > > > However, user-space may want to figure out whether or not the > > > IOCTL is available before it has a DMA-BUF at hand, e.g. at > > > initialization time in a Wayland compositor. > > > > Why not just do the ioctl in a test way? That's how we determine kernel > > features, we do not poke around in sysfs to determine what is, or is > > not, present at runtime. > > > > > Add a /sys/kernel/dmabuf/caps directory which allows the DMA-BUF > > > subsystem to advertise supported features. Add a > > > sync_file_import_export entry which indicates that importing and > > > exporting sync_files from/to DMA-BUFs is supported. > > > > No, sorry, this is not a sustainable thing to do for all kernel features > > over time. Please just do the ioctl and go from there. sysfs is not > > for advertising what is and is not enabled/present in a kernel with > > regards to functionality or capabilities of the system. > > > > If sysfs were to export this type of thing, it would have to do it for > > everything, not just some random tiny thing of one kernel driver. > > I'd argue that DMA-BUF is a special case here. So this is special and unique just like everything else? :) > To check whether the import/export IOCTLs are available, user-space > needs a DMA-BUF to try to perform the IOCTL. To get a DMA-BUF, > user-space needs to enumerate GPUs, pick one at random, load GBM or > Vulkan, use that heavy-weight API to allocate a "fake" buffer on the > GPU, export that buffer into a DMA-BUF, try the IOCTL, then teardown > all of this. There is no other way. > > This sounds like a roundabout way to answer the simple question "is the > IOCTL available?". Do you have another suggestion to address this > problem? What does userspace do differently if the ioctl is present or not? And why is this somehow more special than of the tens of thousands of other ioctl calls where you have to do exactly the same thing you list above to determine if it is present or not? And how have you specifically tied this sysfs to the ioctl so that if it changes or is ported elsewhere, that sysfs attribute will also know to be added? You already have shipping kernels today without this attribute, you can't go back in time and add the attribute to those kernels just to reflect the ioctl being present or not, so you have to handle this case in userspace today, making this not needed at all. Do you want to have two test cases in your userspace code, one that does "is the sysfs file there? No, ok, let's see if we are on an older kernel without it, yet the ioctl is present..." When really you can just do "let's see if the ioctl is present" logic as you already do that today. DMA bufs are not special, they are merely one of tens of thousands of ioctls in the kernel. Think of the overall picture here please, that's the only way to create a maintainable system over long periods of time, like the kernel needs to be. thanks, greg k-h
On Thursday, June 2nd, 2022 at 08:25, Greg KH <greg@kroah.com> wrote: > On Thu, Jun 02, 2022 at 06:17:31AM +0000, Simon Ser wrote: > > > On Thursday, June 2nd, 2022 at 07:40, Greg KH greg@kroah.com wrote: > > > > > On Wed, Jun 01, 2022 at 04:13:14PM +0000, Simon Ser wrote: > > > > > > > To discover support for new DMA-BUF IOCTLs, user-space has no > > > > choice but to try to perform the IOCTL on an existing DMA-BUF. > > > > > > Which is correct and how all kernel features work (sorry I missed the > > > main goal of this patch earlier and focused only on the sysfs stuff). > > > > > > > However, user-space may want to figure out whether or not the > > > > IOCTL is available before it has a DMA-BUF at hand, e.g. at > > > > initialization time in a Wayland compositor. > > > > > > Why not just do the ioctl in a test way? That's how we determine kernel > > > features, we do not poke around in sysfs to determine what is, or is > > > not, present at runtime. > > > > > > > Add a /sys/kernel/dmabuf/caps directory which allows the DMA-BUF > > > > subsystem to advertise supported features. Add a > > > > sync_file_import_export entry which indicates that importing and > > > > exporting sync_files from/to DMA-BUFs is supported. > > > > > > No, sorry, this is not a sustainable thing to do for all kernel features > > > over time. Please just do the ioctl and go from there. sysfs is not > > > for advertising what is and is not enabled/present in a kernel with > > > regards to functionality or capabilities of the system. > > > > > > If sysfs were to export this type of thing, it would have to do it for > > > everything, not just some random tiny thing of one kernel driver. > > > > I'd argue that DMA-BUF is a special case here. > > So this is special and unique just like everything else? :) > > > To check whether the import/export IOCTLs are available, user-space > > needs a DMA-BUF to try to perform the IOCTL. To get a DMA-BUF, > > user-space needs to enumerate GPUs, pick one at random, load GBM or > > Vulkan, use that heavy-weight API to allocate a "fake" buffer on the > > GPU, export that buffer into a DMA-BUF, try the IOCTL, then teardown > > all of this. There is no other way. > > > > This sounds like a roundabout way to answer the simple question "is the > > IOCTL available?". Do you have another suggestion to address this > > problem? > > What does userspace do differently if the ioctl is present or not? Globally enable a synchronization API for Wayland clients, for instance in the case of a Wayland compositor. > And why is this somehow more special than of the tens of thousands of > other ioctl calls where you have to do exactly the same thing you list > above to determine if it is present or not? For other IOCTLs it's not as complicated to obtain a FD to do the test with. > And how have you specifically tied this sysfs to the ioctl so that if it > changes or is ported elsewhere, that sysfs attribute will also know to > be added? What do you mean by "ported elsewhere"? > You already have shipping kernels today without this attribute, you > can't go back in time and add the attribute to those kernels just to > reflect the ioctl being present or not, so you have to handle this case > in userspace today, making this not needed at all. Do you want to have > two test cases in your userspace code, one that does "is the sysfs file > there? No, ok, let's see if we are on an older kernel without it, yet > the ioctl is present..." When really you can just do "let's see if the > ioctl is present" logic as you already do that today. The IOCTL has not been shipped yet.
On Thu, 2 Jun 2022 at 08:34, Simon Ser <contact@emersion.fr> wrote: > > On Thursday, June 2nd, 2022 at 08:25, Greg KH <greg@kroah.com> wrote: > > > On Thu, Jun 02, 2022 at 06:17:31AM +0000, Simon Ser wrote: > > > > > On Thursday, June 2nd, 2022 at 07:40, Greg KH greg@kroah.com wrote: > > > > > > > On Wed, Jun 01, 2022 at 04:13:14PM +0000, Simon Ser wrote: > > > > > > > > > To discover support for new DMA-BUF IOCTLs, user-space has no > > > > > choice but to try to perform the IOCTL on an existing DMA-BUF. > > > > > > > > Which is correct and how all kernel features work (sorry I missed the > > > > main goal of this patch earlier and focused only on the sysfs stuff). > > > > > > > > > However, user-space may want to figure out whether or not the > > > > > IOCTL is available before it has a DMA-BUF at hand, e.g. at > > > > > initialization time in a Wayland compositor. > > > > > > > > Why not just do the ioctl in a test way? That's how we determine kernel > > > > features, we do not poke around in sysfs to determine what is, or is > > > > not, present at runtime. > > > > > > > > > Add a /sys/kernel/dmabuf/caps directory which allows the DMA-BUF > > > > > subsystem to advertise supported features. Add a > > > > > sync_file_import_export entry which indicates that importing and > > > > > exporting sync_files from/to DMA-BUFs is supported. > > > > > > > > No, sorry, this is not a sustainable thing to do for all kernel features > > > > over time. Please just do the ioctl and go from there. sysfs is not > > > > for advertising what is and is not enabled/present in a kernel with > > > > regards to functionality or capabilities of the system. > > > > > > > > If sysfs were to export this type of thing, it would have to do it for > > > > everything, not just some random tiny thing of one kernel driver. > > > > > > I'd argue that DMA-BUF is a special case here. > > > > So this is special and unique just like everything else? :) > > > > > To check whether the import/export IOCTLs are available, user-space > > > needs a DMA-BUF to try to perform the IOCTL. To get a DMA-BUF, > > > user-space needs to enumerate GPUs, pick one at random, load GBM or > > > Vulkan, use that heavy-weight API to allocate a "fake" buffer on the > > > GPU, export that buffer into a DMA-BUF, try the IOCTL, then teardown > > > all of this. There is no other way. > > > > > > This sounds like a roundabout way to answer the simple question "is the > > > IOCTL available?". Do you have another suggestion to address this > > > problem? > > > > What does userspace do differently if the ioctl is present or not? > > Globally enable a synchronization API for Wayland clients, for instance > in the case of a Wayland compositor. > > > And why is this somehow more special than of the tens of thousands of > > other ioctl calls where you have to do exactly the same thing you list > > above to determine if it is present or not? > > For other IOCTLs it's not as complicated to obtain a FD to do the test > with. Two expand on this: - compositor opens the drm render /dev node - compositor initializes the opengl or vulkan userspace driver on top of that - compositor asks that userspace driver to allocate some buffer, which can be pretty expensive - compositor asks the userspace driver to export that buffer into a dma-buf - compositor can finally do the test ioctl, realizes support isn't there and tosses the entire thing read() on a sysfs file is so much more reasonable it's not even funny. Plan B we discussed is to add a getparam to signify this to the drm ioctl interface, but that has the design problem that a feature in the dma-buf subsystem is announced in a totally different subsystem (ok same maintainers), and if that ever gets out of sync your userspace breaks. So really no good. So if sysfs also isn't the right approach, and the getparam ioctl on drm is defo worse, what is the right approach? Ideally without setting up the entire userspace render driver and doing some expensive-ish (depending upon driver at least) buffer allocations just to check for a feature. Note that some compositors want to gate their "should I use vk for rendering and expose some additional features to client" decision on this, so setting up the wrong renderer just to test whether that would work is not a very great approach. Also the last time we added a feature to dma-buf was in 3.17, so I guess everyone just hardcodes nowadays that all dma-buf features are present. Which isn't great, and that's why we're trying to fix here. > > And how have you specifically tied this sysfs to the ioctl so that if it > > changes or is ported elsewhere, that sysfs attribute will also know to > > be added? > > What do you mean by "ported elsewhere"? > > > You already have shipping kernels today without this attribute, you > > can't go back in time and add the attribute to those kernels just to > > reflect the ioctl being present or not, so you have to handle this case > > in userspace today, making this not needed at all. Do you want to have > > two test cases in your userspace code, one that does "is the sysfs file > > there? No, ok, let's see if we are on an older kernel without it, yet > > the ioctl is present..." When really you can just do "let's see if the > > ioctl is present" logic as you already do that today. > > The IOCTL has not been shipped yet. To expand, Jason Ekstrand wrote the ioctl itself and Simon here the better discovery code. It should probably have been sent out as a joint patch series or something, but neither has been merged yet so there is no problem. -Daniel
On 2022-06-02 07:47, Daniel Vetter wrote: > On Thu, 2 Jun 2022 at 08:34, Simon Ser <contact@emersion.fr> wrote: >> >> On Thursday, June 2nd, 2022 at 08:25, Greg KH <greg@kroah.com> wrote: >> >>> On Thu, Jun 02, 2022 at 06:17:31AM +0000, Simon Ser wrote: >>> >>>> On Thursday, June 2nd, 2022 at 07:40, Greg KH greg@kroah.com wrote: >>>> >>>>> On Wed, Jun 01, 2022 at 04:13:14PM +0000, Simon Ser wrote: >>>>> >>>>>> To discover support for new DMA-BUF IOCTLs, user-space has no >>>>>> choice but to try to perform the IOCTL on an existing DMA-BUF. >>>>> >>>>> Which is correct and how all kernel features work (sorry I missed the >>>>> main goal of this patch earlier and focused only on the sysfs stuff). >>>>> >>>>>> However, user-space may want to figure out whether or not the >>>>>> IOCTL is available before it has a DMA-BUF at hand, e.g. at >>>>>> initialization time in a Wayland compositor. >>>>> >>>>> Why not just do the ioctl in a test way? That's how we determine kernel >>>>> features, we do not poke around in sysfs to determine what is, or is >>>>> not, present at runtime. >>>>> >>>>>> Add a /sys/kernel/dmabuf/caps directory which allows the DMA-BUF >>>>>> subsystem to advertise supported features. Add a >>>>>> sync_file_import_export entry which indicates that importing and >>>>>> exporting sync_files from/to DMA-BUFs is supported. >>>>> >>>>> No, sorry, this is not a sustainable thing to do for all kernel features >>>>> over time. Please just do the ioctl and go from there. sysfs is not >>>>> for advertising what is and is not enabled/present in a kernel with >>>>> regards to functionality or capabilities of the system. >>>>> >>>>> If sysfs were to export this type of thing, it would have to do it for >>>>> everything, not just some random tiny thing of one kernel driver. >>>> >>>> I'd argue that DMA-BUF is a special case here. >>> >>> So this is special and unique just like everything else? :) >>> >>>> To check whether the import/export IOCTLs are available, user-space >>>> needs a DMA-BUF to try to perform the IOCTL. To get a DMA-BUF, >>>> user-space needs to enumerate GPUs, pick one at random, load GBM or >>>> Vulkan, use that heavy-weight API to allocate a "fake" buffer on the >>>> GPU, export that buffer into a DMA-BUF, try the IOCTL, then teardown >>>> all of this. There is no other way. >>>> >>>> This sounds like a roundabout way to answer the simple question "is the >>>> IOCTL available?". Do you have another suggestion to address this >>>> problem? >>> >>> What does userspace do differently if the ioctl is present or not? >> >> Globally enable a synchronization API for Wayland clients, for instance >> in the case of a Wayland compositor. >> >>> And why is this somehow more special than of the tens of thousands of >>> other ioctl calls where you have to do exactly the same thing you list >>> above to determine if it is present or not? >> >> For other IOCTLs it's not as complicated to obtain a FD to do the test >> with. > > Two expand on this: > > - compositor opens the drm render /dev node > - compositor initializes the opengl or vulkan userspace driver on top of that > - compositor asks that userspace driver to allocate some buffer, which > can be pretty expensive > - compositor asks the userspace driver to export that buffer into a dma-buf > - compositor can finally do the test ioctl, realizes support isn't > there and tosses the entire thing > > read() on a sysfs file is so much more reasonable it's not even funny. Just a drive-by observation, so apologies if I'm overlooking something obvious, but it sounds like the ideal compromise would be to expose a sysfs file which behaves as a dummy exported dma-buf. That way userspace could just open() it and try ioctl() directly - assuming that supported operations can fail distinctly from unsupported ones, or succeed as a no-op - which seems even simpler still. Robin.
On Mon, Jun 06, 2022 at 04:10:09PM +0100, Robin Murphy wrote: > On 2022-06-02 07:47, Daniel Vetter wrote: > > On Thu, 2 Jun 2022 at 08:34, Simon Ser <contact@emersion.fr> wrote: > > > > > > On Thursday, June 2nd, 2022 at 08:25, Greg KH <greg@kroah.com> wrote: > > > > > > > On Thu, Jun 02, 2022 at 06:17:31AM +0000, Simon Ser wrote: > > > > > > > > > On Thursday, June 2nd, 2022 at 07:40, Greg KH greg@kroah.com wrote: > > > > > > > > > > > On Wed, Jun 01, 2022 at 04:13:14PM +0000, Simon Ser wrote: > > > > > > > > > > > > > To discover support for new DMA-BUF IOCTLs, user-space has no > > > > > > > choice but to try to perform the IOCTL on an existing DMA-BUF. > > > > > > > > > > > > Which is correct and how all kernel features work (sorry I missed the > > > > > > main goal of this patch earlier and focused only on the sysfs stuff). > > > > > > > > > > > > > However, user-space may want to figure out whether or not the > > > > > > > IOCTL is available before it has a DMA-BUF at hand, e.g. at > > > > > > > initialization time in a Wayland compositor. > > > > > > > > > > > > Why not just do the ioctl in a test way? That's how we determine kernel > > > > > > features, we do not poke around in sysfs to determine what is, or is > > > > > > not, present at runtime. > > > > > > > > > > > > > Add a /sys/kernel/dmabuf/caps directory which allows the DMA-BUF > > > > > > > subsystem to advertise supported features. Add a > > > > > > > sync_file_import_export entry which indicates that importing and > > > > > > > exporting sync_files from/to DMA-BUFs is supported. > > > > > > > > > > > > No, sorry, this is not a sustainable thing to do for all kernel features > > > > > > over time. Please just do the ioctl and go from there. sysfs is not > > > > > > for advertising what is and is not enabled/present in a kernel with > > > > > > regards to functionality or capabilities of the system. > > > > > > > > > > > > If sysfs were to export this type of thing, it would have to do it for > > > > > > everything, not just some random tiny thing of one kernel driver. > > > > > > > > > > I'd argue that DMA-BUF is a special case here. > > > > > > > > So this is special and unique just like everything else? :) > > > > > > > > > To check whether the import/export IOCTLs are available, user-space > > > > > needs a DMA-BUF to try to perform the IOCTL. To get a DMA-BUF, > > > > > user-space needs to enumerate GPUs, pick one at random, load GBM or > > > > > Vulkan, use that heavy-weight API to allocate a "fake" buffer on the > > > > > GPU, export that buffer into a DMA-BUF, try the IOCTL, then teardown > > > > > all of this. There is no other way. > > > > > > > > > > This sounds like a roundabout way to answer the simple question "is the > > > > > IOCTL available?". Do you have another suggestion to address this > > > > > problem? > > > > > > > > What does userspace do differently if the ioctl is present or not? > > > > > > Globally enable a synchronization API for Wayland clients, for instance > > > in the case of a Wayland compositor. > > > > > > > And why is this somehow more special than of the tens of thousands of > > > > other ioctl calls where you have to do exactly the same thing you list > > > > above to determine if it is present or not? > > > > > > For other IOCTLs it's not as complicated to obtain a FD to do the test > > > with. > > > > Two expand on this: > > > > - compositor opens the drm render /dev node > > - compositor initializes the opengl or vulkan userspace driver on top of that > > - compositor asks that userspace driver to allocate some buffer, which > > can be pretty expensive > > - compositor asks the userspace driver to export that buffer into a dma-buf > > - compositor can finally do the test ioctl, realizes support isn't > > there and tosses the entire thing > > > > read() on a sysfs file is so much more reasonable it's not even funny. > > Just a drive-by observation, so apologies if I'm overlooking something > obvious, but it sounds like the ideal compromise would be to expose a sysfs > file which behaves as a dummy exported dma-buf. That way userspace could > just open() it and try ioctl() directly - assuming that supported operations > can fail distinctly from unsupported ones, or succeed as a no-op - which > seems even simpler still. ioctl() will not work on a sysfs file, sorry.
On 2022-06-06 16:22, Greg KH wrote: > On Mon, Jun 06, 2022 at 04:10:09PM +0100, Robin Murphy wrote: >> On 2022-06-02 07:47, Daniel Vetter wrote: >>> On Thu, 2 Jun 2022 at 08:34, Simon Ser <contact@emersion.fr> wrote: >>>> >>>> On Thursday, June 2nd, 2022 at 08:25, Greg KH <greg@kroah.com> wrote: >>>> >>>>> On Thu, Jun 02, 2022 at 06:17:31AM +0000, Simon Ser wrote: >>>>> >>>>>> On Thursday, June 2nd, 2022 at 07:40, Greg KH greg@kroah.com wrote: >>>>>> >>>>>>> On Wed, Jun 01, 2022 at 04:13:14PM +0000, Simon Ser wrote: >>>>>>> >>>>>>>> To discover support for new DMA-BUF IOCTLs, user-space has no >>>>>>>> choice but to try to perform the IOCTL on an existing DMA-BUF. >>>>>>> >>>>>>> Which is correct and how all kernel features work (sorry I missed the >>>>>>> main goal of this patch earlier and focused only on the sysfs stuff). >>>>>>> >>>>>>>> However, user-space may want to figure out whether or not the >>>>>>>> IOCTL is available before it has a DMA-BUF at hand, e.g. at >>>>>>>> initialization time in a Wayland compositor. >>>>>>> >>>>>>> Why not just do the ioctl in a test way? That's how we determine kernel >>>>>>> features, we do not poke around in sysfs to determine what is, or is >>>>>>> not, present at runtime. >>>>>>> >>>>>>>> Add a /sys/kernel/dmabuf/caps directory which allows the DMA-BUF >>>>>>>> subsystem to advertise supported features. Add a >>>>>>>> sync_file_import_export entry which indicates that importing and >>>>>>>> exporting sync_files from/to DMA-BUFs is supported. >>>>>>> >>>>>>> No, sorry, this is not a sustainable thing to do for all kernel features >>>>>>> over time. Please just do the ioctl and go from there. sysfs is not >>>>>>> for advertising what is and is not enabled/present in a kernel with >>>>>>> regards to functionality or capabilities of the system. >>>>>>> >>>>>>> If sysfs were to export this type of thing, it would have to do it for >>>>>>> everything, not just some random tiny thing of one kernel driver. >>>>>> >>>>>> I'd argue that DMA-BUF is a special case here. >>>>> >>>>> So this is special and unique just like everything else? :) >>>>> >>>>>> To check whether the import/export IOCTLs are available, user-space >>>>>> needs a DMA-BUF to try to perform the IOCTL. To get a DMA-BUF, >>>>>> user-space needs to enumerate GPUs, pick one at random, load GBM or >>>>>> Vulkan, use that heavy-weight API to allocate a "fake" buffer on the >>>>>> GPU, export that buffer into a DMA-BUF, try the IOCTL, then teardown >>>>>> all of this. There is no other way. >>>>>> >>>>>> This sounds like a roundabout way to answer the simple question "is the >>>>>> IOCTL available?". Do you have another suggestion to address this >>>>>> problem? >>>>> >>>>> What does userspace do differently if the ioctl is present or not? >>>> >>>> Globally enable a synchronization API for Wayland clients, for instance >>>> in the case of a Wayland compositor. >>>> >>>>> And why is this somehow more special than of the tens of thousands of >>>>> other ioctl calls where you have to do exactly the same thing you list >>>>> above to determine if it is present or not? >>>> >>>> For other IOCTLs it's not as complicated to obtain a FD to do the test >>>> with. >>> >>> Two expand on this: >>> >>> - compositor opens the drm render /dev node >>> - compositor initializes the opengl or vulkan userspace driver on top of that >>> - compositor asks that userspace driver to allocate some buffer, which >>> can be pretty expensive >>> - compositor asks the userspace driver to export that buffer into a dma-buf >>> - compositor can finally do the test ioctl, realizes support isn't >>> there and tosses the entire thing >>> >>> read() on a sysfs file is so much more reasonable it's not even funny. >> >> Just a drive-by observation, so apologies if I'm overlooking something >> obvious, but it sounds like the ideal compromise would be to expose a sysfs >> file which behaves as a dummy exported dma-buf. That way userspace could >> just open() it and try ioctl() directly - assuming that supported operations >> can fail distinctly from unsupported ones, or succeed as a no-op - which >> seems even simpler still. > > ioctl() will not work on a sysfs file, sorry. Ah, fair enough - TBH I should have just said "a file", since I presume some sort of /dev/dma-buf might also be an option, if a bit more work to implement. I'll scuttle back to my low-level DMA corner now :) Cheers, Robin.
On Thu, Jun 02, 2022 at 08:47:56AM +0200, Daniel Vetter wrote: > > Two expand on this: > > - compositor opens the drm render /dev node > - compositor initializes the opengl or vulkan userspace driver on top of that > - compositor asks that userspace driver to allocate some buffer, which > can be pretty expensive > - compositor asks the userspace driver to export that buffer into a dma-buf > - compositor can finally do the test ioctl, realizes support isn't > there and tosses the entire thing > > read() on a sysfs file is so much more reasonable it's not even funny. > What about dma-buf heaps? That should be a shorter route to getting a dma-buf fd if that's all that's needed. Cheers, -Brian
On Thu, Jun 02, 2022 at 08:47:56AM +0200, Daniel Vetter wrote: > On Thu, 2 Jun 2022 at 08:34, Simon Ser <contact@emersion.fr> wrote: > > > > On Thursday, June 2nd, 2022 at 08:25, Greg KH <greg@kroah.com> wrote: > > > > > On Thu, Jun 02, 2022 at 06:17:31AM +0000, Simon Ser wrote: > > > > > > > On Thursday, June 2nd, 2022 at 07:40, Greg KH greg@kroah.com wrote: > > > > > > > > > On Wed, Jun 01, 2022 at 04:13:14PM +0000, Simon Ser wrote: > > > > > > > > > > > To discover support for new DMA-BUF IOCTLs, user-space has no > > > > > > choice but to try to perform the IOCTL on an existing DMA-BUF. > > > > > > > > > > Which is correct and how all kernel features work (sorry I missed the > > > > > main goal of this patch earlier and focused only on the sysfs stuff). > > > > > > > > > > > However, user-space may want to figure out whether or not the > > > > > > IOCTL is available before it has a DMA-BUF at hand, e.g. at > > > > > > initialization time in a Wayland compositor. > > > > > > > > > > Why not just do the ioctl in a test way? That's how we determine kernel > > > > > features, we do not poke around in sysfs to determine what is, or is > > > > > not, present at runtime. > > > > > > > > > > > Add a /sys/kernel/dmabuf/caps directory which allows the DMA-BUF > > > > > > subsystem to advertise supported features. Add a > > > > > > sync_file_import_export entry which indicates that importing and > > > > > > exporting sync_files from/to DMA-BUFs is supported. > > > > > > > > > > No, sorry, this is not a sustainable thing to do for all kernel features > > > > > over time. Please just do the ioctl and go from there. sysfs is not > > > > > for advertising what is and is not enabled/present in a kernel with > > > > > regards to functionality or capabilities of the system. > > > > > > > > > > If sysfs were to export this type of thing, it would have to do it for > > > > > everything, not just some random tiny thing of one kernel driver. > > > > > > > > I'd argue that DMA-BUF is a special case here. > > > > > > So this is special and unique just like everything else? :) > > > > > > > To check whether the import/export IOCTLs are available, user-space > > > > needs a DMA-BUF to try to perform the IOCTL. To get a DMA-BUF, > > > > user-space needs to enumerate GPUs, pick one at random, load GBM or > > > > Vulkan, use that heavy-weight API to allocate a "fake" buffer on the > > > > GPU, export that buffer into a DMA-BUF, try the IOCTL, then teardown > > > > all of this. There is no other way. > > > > > > > > This sounds like a roundabout way to answer the simple question "is the > > > > IOCTL available?". Do you have another suggestion to address this > > > > problem? > > > > > > What does userspace do differently if the ioctl is present or not? > > > > Globally enable a synchronization API for Wayland clients, for instance > > in the case of a Wayland compositor. > > > > > And why is this somehow more special than of the tens of thousands of > > > other ioctl calls where you have to do exactly the same thing you list > > > above to determine if it is present or not? > > > > For other IOCTLs it's not as complicated to obtain a FD to do the test > > with. > > Two expand on this: > > - compositor opens the drm render /dev node > - compositor initializes the opengl or vulkan userspace driver on top of that > - compositor asks that userspace driver to allocate some buffer, which > can be pretty expensive > - compositor asks the userspace driver to export that buffer into a dma-buf > - compositor can finally do the test ioctl, realizes support isn't > there and tosses the entire thing > > read() on a sysfs file is so much more reasonable it's not even funny. I agree it seems trivial and "simple", but that is NOT how to determine what is, and is not, a valid ioctl command for a device node. The only sane way to do this is like we have been doing for the past 30+ years, make the ioctl and look at the return value. Now if we want to come up with a new generic "here's the capabilities/ioctls/whatever" that the kernel currently supports at this point in time api, wonderful, but PLEASE do not overload sysfs to do something like this as that is not what it is for at this moment in time. Don't just do this for one specific ioctl as there really is nothing special about it at all ("it's special and unique just like all other ioctls...") > Plan B we discussed is to add a getparam to signify this to the drm > ioctl interface, but that has the design problem that a feature in the > dma-buf subsystem is announced in a totally different subsystem (ok > same maintainers), and if that ever gets out of sync your userspace > breaks. So really no good. getparam makes sense in a way, if it doesn't change over time (i.e. if you call it now, will it be the same if you call it again if some kernel module is added/removed in the meantime?) Also be aware of suspend/resume where you can swap out the kernel underneath running userspace and that kernel might have different capabilities now. So you can't just not check error values of ioctl commands (not that you are saying you want to here, just that it's more complex than might originally seem.) > So if sysfs also isn't the right approach, and the getparam ioctl on > drm is defo worse, what is the right approach? Ideally without setting > up the entire userspace render driver and doing some expensive-ish > (depending upon driver at least) buffer allocations just to check for > a feature. > > Note that some compositors want to gate their "should I use vk for > rendering and expose some additional features to client" decision on > this, so setting up the wrong renderer just to test whether that would > work is not a very great approach. > > Also the last time we added a feature to dma-buf was in 3.17, so I > guess everyone just hardcodes nowadays that all dma-buf features are > present. Which isn't great, and that's why we're trying to fix here. Why can't you call the test ioctl with an invalid value to see if it is present or not (-ENOTTY vs. -EINVAL) right at the beginning before you set up anything? thanks, greg k-h
On Tue, 2022-06-07 at 12:55 +0200, Greg KH wrote: > On Thu, Jun 02, 2022 at 08:47:56AM +0200, Daniel Vetter wrote: > > On Thu, 2 Jun 2022 at 08:34, Simon Ser <contact@emersion.fr> wrote: > > > > > > On Thursday, June 2nd, 2022 at 08:25, Greg KH <greg@kroah.com> > > > wrote: > > > > > > > On Thu, Jun 02, 2022 at 06:17:31AM +0000, Simon Ser wrote: > > > > > > > > > On Thursday, June 2nd, 2022 at 07:40, Greg KH > > > > > greg@kroah.com wrote: > > > > > > > > > > > On Wed, Jun 01, 2022 at 04:13:14PM +0000, Simon Ser wrote: > > > > > > > > > > > > > To discover support for new DMA-BUF IOCTLs, user-space > > > > > > > has no > > > > > > > choice but to try to perform the IOCTL on an existing > > > > > > > DMA-BUF. > > > > > > > > > > > > Which is correct and how all kernel features work (sorry I > > > > > > missed the > > > > > > main goal of this patch earlier and focused only on the > > > > > > sysfs stuff). > > > > > > > > > > > > > However, user-space may want to figure out whether or not > > > > > > > the > > > > > > > IOCTL is available before it has a DMA-BUF at hand, e.g. > > > > > > > at > > > > > > > initialization time in a Wayland compositor. > > > > > > > > > > > > Why not just do the ioctl in a test way? That's how we > > > > > > determine kernel > > > > > > features, we do not poke around in sysfs to determine what > > > > > > is, or is > > > > > > not, present at runtime. > > > > > > > > > > > > > Add a /sys/kernel/dmabuf/caps directory which allows the > > > > > > > DMA-BUF > > > > > > > subsystem to advertise supported features. Add a > > > > > > > sync_file_import_export entry which indicates that > > > > > > > importing and > > > > > > > exporting sync_files from/to DMA-BUFs is supported. > > > > > > > > > > > > No, sorry, this is not a sustainable thing to do for all > > > > > > kernel features > > > > > > over time. Please just do the ioctl and go from there. > > > > > > sysfs is not > > > > > > for advertising what is and is not enabled/present in a > > > > > > kernel with > > > > > > regards to functionality or capabilities of the system. > > > > > > > > > > > > If sysfs were to export this type of thing, it would have > > > > > > to do it for > > > > > > everything, not just some random tiny thing of one kernel > > > > > > driver. > > > > > > > > > > I'd argue that DMA-BUF is a special case here. > > > > > > > > So this is special and unique just like everything else? :) > > > > > > > > > To check whether the import/export IOCTLs are available, > > > > > user-space > > > > > needs a DMA-BUF to try to perform the IOCTL. To get a DMA- > > > > > BUF, > > > > > user-space needs to enumerate GPUs, pick one at random, load > > > > > GBM or > > > > > Vulkan, use that heavy-weight API to allocate a "fake" buffer > > > > > on the > > > > > GPU, export that buffer into a DMA-BUF, try the IOCTL, then > > > > > teardown > > > > > all of this. There is no other way. > > > > > > > > > > This sounds like a roundabout way to answer the simple > > > > > question "is the > > > > > IOCTL available?". Do you have another suggestion to address > > > > > this > > > > > problem? > > > > > > > > What does userspace do differently if the ioctl is present or > > > > not? > > > > > > Globally enable a synchronization API for Wayland clients, for > > > instance > > > in the case of a Wayland compositor. > > > > > > > And why is this somehow more special than of the tens of > > > > thousands of > > > > other ioctl calls where you have to do exactly the same thing > > > > you list > > > > above to determine if it is present or not? > > > > > > For other IOCTLs it's not as complicated to obtain a FD to do the > > > test > > > with. > > > > Two expand on this: > > > > - compositor opens the drm render /dev node > > - compositor initializes the opengl or vulkan userspace driver on > > top of that > > - compositor asks that userspace driver to allocate some buffer, > > which > > can be pretty expensive > > - compositor asks the userspace driver to export that buffer into a > > dma-buf > > - compositor can finally do the test ioctl, realizes support isn't > > there and tosses the entire thing > > > > read() on a sysfs file is so much more reasonable it's not even > > funny. > > I agree it seems trivial and "simple", but that is NOT how to > determine > what is, and is not, a valid ioctl command for a device node. > > The only sane way to do this is like we have been doing for the past > 30+ > years, make the ioctl and look at the return value. > > Now if we want to come up with a new generic "here's the > capabilities/ioctls/whatever" that the kernel currently supports at > this > point in time api, wonderful, but PLEASE do not overload sysfs to do > something like this as that is not what it is for at this moment in > time. > > Don't just do this for one specific ioctl as there really is nothing > special about it at all ("it's special and unique just like all other > ioctls...") > > > Plan B we discussed is to add a getparam to signify this to the drm > > ioctl interface, but that has the design problem that a feature in > > the > > dma-buf subsystem is announced in a totally different subsystem (ok > > same maintainers), and if that ever gets out of sync your userspace > > breaks. So really no good. > > getparam makes sense in a way, if it doesn't change over time (i.e. > if > you call it now, will it be the same if you call it again if some > kernel > module is added/removed in the meantime?) Also be aware of > suspend/resume where you can swap out the kernel underneath running > userspace and that kernel might have different capabilities now. So > you > can't just not check error values of ioctl commands (not that you are > saying you want to here, just that it's more complex than might > originally seem.) > > > So if sysfs also isn't the right approach, and the getparam ioctl > > on > > drm is defo worse, what is the right approach? Ideally without > > setting > > up the entire userspace render driver and doing some expensive-ish > > (depending upon driver at least) buffer allocations just to check > > for > > a feature. > > > > Note that some compositors want to gate their "should I use vk for > > rendering and expose some additional features to client" decision > > on > > this, so setting up the wrong renderer just to test whether that > > would > > work is not a very great approach. > > > > Also the last time we added a feature to dma-buf was in 3.17, so I > > guess everyone just hardcodes nowadays that all dma-buf features > > are > > present. Which isn't great, and that's why we're trying to fix > > here. > > Why can't you call the test ioctl with an invalid value to see if it > is > present or not (-ENOTTY vs. -EINVAL) right at the beginning before > you > set up anything? Because we need a file descriptor to call that ioctl on. Currently, that file descriptor is the dma-buf itself. We could move it to be a DRM IOCTL but then it ends up unnecessarily tied to DRM even if someone's trying to use it with V4L or some other dma-buf producer/consumer. IDK if that's a real problem in practice but there didn't seem to be a good reason to tie it to things outside dma-buf. --Jason
On Tue, Jun 07, 2022 at 09:52:56AM -0500, Jason Ekstrand wrote: > On Tue, 2022-06-07 at 12:55 +0200, Greg KH wrote: > > On Thu, Jun 02, 2022 at 08:47:56AM +0200, Daniel Vetter wrote: > > > On Thu, 2 Jun 2022 at 08:34, Simon Ser <contact@emersion.fr> wrote: > > > > > > > > On Thursday, June 2nd, 2022 at 08:25, Greg KH <greg@kroah.com> > > > > wrote: > > > > > > > > > On Thu, Jun 02, 2022 at 06:17:31AM +0000, Simon Ser wrote: > > > > > > > > > > > On Thursday, June 2nd, 2022 at 07:40, Greg KH > > > > > > greg@kroah.com wrote: > > > > > > > > > > > > > On Wed, Jun 01, 2022 at 04:13:14PM +0000, Simon Ser wrote: > > > > > > > > > > > > > > > To discover support for new DMA-BUF IOCTLs, user-space > > > > > > > > has no > > > > > > > > choice but to try to perform the IOCTL on an existing > > > > > > > > DMA-BUF. > > > > > > > > > > > > > > Which is correct and how all kernel features work (sorry I > > > > > > > missed the > > > > > > > main goal of this patch earlier and focused only on the > > > > > > > sysfs stuff). > > > > > > > > > > > > > > > However, user-space may want to figure out whether or not > > > > > > > > the > > > > > > > > IOCTL is available before it has a DMA-BUF at hand, e.g. > > > > > > > > at > > > > > > > > initialization time in a Wayland compositor. > > > > > > > > > > > > > > Why not just do the ioctl in a test way? That's how we > > > > > > > determine kernel > > > > > > > features, we do not poke around in sysfs to determine what > > > > > > > is, or is > > > > > > > not, present at runtime. > > > > > > > > > > > > > > > Add a /sys/kernel/dmabuf/caps directory which allows the > > > > > > > > DMA-BUF > > > > > > > > subsystem to advertise supported features. Add a > > > > > > > > sync_file_import_export entry which indicates that > > > > > > > > importing and > > > > > > > > exporting sync_files from/to DMA-BUFs is supported. > > > > > > > > > > > > > > No, sorry, this is not a sustainable thing to do for all > > > > > > > kernel features > > > > > > > over time. Please just do the ioctl and go from there. > > > > > > > sysfs is not > > > > > > > for advertising what is and is not enabled/present in a > > > > > > > kernel with > > > > > > > regards to functionality or capabilities of the system. > > > > > > > > > > > > > > If sysfs were to export this type of thing, it would have > > > > > > > to do it for > > > > > > > everything, not just some random tiny thing of one kernel > > > > > > > driver. > > > > > > > > > > > > I'd argue that DMA-BUF is a special case here. > > > > > > > > > > So this is special and unique just like everything else? :) > > > > > > > > > > > To check whether the import/export IOCTLs are available, > > > > > > user-space > > > > > > needs a DMA-BUF to try to perform the IOCTL. To get a DMA- > > > > > > BUF, > > > > > > user-space needs to enumerate GPUs, pick one at random, load > > > > > > GBM or > > > > > > Vulkan, use that heavy-weight API to allocate a "fake" buffer > > > > > > on the > > > > > > GPU, export that buffer into a DMA-BUF, try the IOCTL, then > > > > > > teardown > > > > > > all of this. There is no other way. > > > > > > > > > > > > This sounds like a roundabout way to answer the simple > > > > > > question "is the > > > > > > IOCTL available?". Do you have another suggestion to address > > > > > > this > > > > > > problem? > > > > > > > > > > What does userspace do differently if the ioctl is present or > > > > > not? > > > > > > > > Globally enable a synchronization API for Wayland clients, for > > > > instance > > > > in the case of a Wayland compositor. > > > > > > > > > And why is this somehow more special than of the tens of > > > > > thousands of > > > > > other ioctl calls where you have to do exactly the same thing > > > > > you list > > > > > above to determine if it is present or not? > > > > > > > > For other IOCTLs it's not as complicated to obtain a FD to do the > > > > test > > > > with. > > > > > > Two expand on this: > > > > > > - compositor opens the drm render /dev node > > > - compositor initializes the opengl or vulkan userspace driver on > > > top of that > > > - compositor asks that userspace driver to allocate some buffer, > > > which > > > can be pretty expensive > > > - compositor asks the userspace driver to export that buffer into a > > > dma-buf > > > - compositor can finally do the test ioctl, realizes support isn't > > > there and tosses the entire thing > > > > > > read() on a sysfs file is so much more reasonable it's not even > > > funny. > > > > I agree it seems trivial and "simple", but that is NOT how to > > determine > > what is, and is not, a valid ioctl command for a device node. > > > > The only sane way to do this is like we have been doing for the past > > 30+ > > years, make the ioctl and look at the return value. > > > > Now if we want to come up with a new generic "here's the > > capabilities/ioctls/whatever" that the kernel currently supports at > > this > > point in time api, wonderful, but PLEASE do not overload sysfs to do > > something like this as that is not what it is for at this moment in > > time. > > > > Don't just do this for one specific ioctl as there really is nothing > > special about it at all ("it's special and unique just like all other > > ioctls...") > > > > > Plan B we discussed is to add a getparam to signify this to the drm > > > ioctl interface, but that has the design problem that a feature in > > > the > > > dma-buf subsystem is announced in a totally different subsystem (ok > > > same maintainers), and if that ever gets out of sync your userspace > > > breaks. So really no good. > > > > getparam makes sense in a way, if it doesn't change over time (i.e. > > if > > you call it now, will it be the same if you call it again if some > > kernel > > module is added/removed in the meantime?) Also be aware of > > suspend/resume where you can swap out the kernel underneath running > > userspace and that kernel might have different capabilities now. So > > you > > can't just not check error values of ioctl commands (not that you are > > saying you want to here, just that it's more complex than might > > originally seem.) > > > > > So if sysfs also isn't the right approach, and the getparam ioctl > > > on > > > drm is defo worse, what is the right approach? Ideally without > > > setting > > > up the entire userspace render driver and doing some expensive-ish > > > (depending upon driver at least) buffer allocations just to check > > > for > > > a feature. > > > > > > Note that some compositors want to gate their "should I use vk for > > > rendering and expose some additional features to client" decision > > > on > > > this, so setting up the wrong renderer just to test whether that > > > would > > > work is not a very great approach. > > > > > > Also the last time we added a feature to dma-buf was in 3.17, so I > > > guess everyone just hardcodes nowadays that all dma-buf features > > > are > > > present. Which isn't great, and that's why we're trying to fix > > > here. > > > > Why can't you call the test ioctl with an invalid value to see if it > > is > > present or not (-ENOTTY vs. -EINVAL) right at the beginning before > > you > > set up anything? > > Because we need a file descriptor to call that ioctl on. Currently, > that file descriptor is the dma-buf itself. We could move it to be a > DRM IOCTL but then it ends up unnecessarily tied to DRM even if > someone's trying to use it with V4L or some other dma-buf > producer/consumer. IDK if that's a real problem in practice but there > didn't seem to be a good reason to tie it to things outside dma-buf. So you all have an api that is so hard to use, you can't use it until you know what the capabilities of that api is before you call it. So you want an "out-of-band" way to know if this api is present or not. Sounds like your api is the problem here, not the capability issue... Again, I understand your problem, but sysfs is NOT the place for this. Linux has a well-defined way to tell userspace if an ioctl is present or not. Just because it is "hard" to get to does not mean you get to create a-brand-new-way-to-do-things for this, sorry. Kernel API design and evolution is hard, that's the job description of a kernel developer. Boiler-plate code that you write once to do this "does the kernel support this ioctl for a dma-buf" will be able to be used by everyone for forever. Best of all, it will already be done for when you have this issue for the next ioctl you create :) greg k-h
On Tue, 7 Jun 2022 at 18:01, Greg KH <greg@kroah.com> wrote: > > On Tue, Jun 07, 2022 at 09:52:56AM -0500, Jason Ekstrand wrote: > > On Tue, 2022-06-07 at 12:55 +0200, Greg KH wrote: > > > On Thu, Jun 02, 2022 at 08:47:56AM +0200, Daniel Vetter wrote: > > > > On Thu, 2 Jun 2022 at 08:34, Simon Ser <contact@emersion.fr> wrote: > > > > > > > > > > On Thursday, June 2nd, 2022 at 08:25, Greg KH <greg@kroah.com> > > > > > wrote: > > > > > > > > > > > On Thu, Jun 02, 2022 at 06:17:31AM +0000, Simon Ser wrote: > > > > > > > > > > > > > On Thursday, June 2nd, 2022 at 07:40, Greg KH > > > > > > > greg@kroah.com wrote: > > > > > > > > > > > > > > > On Wed, Jun 01, 2022 at 04:13:14PM +0000, Simon Ser wrote: > > > > > > > > > > > > > > > > > To discover support for new DMA-BUF IOCTLs, user-space > > > > > > > > > has no > > > > > > > > > choice but to try to perform the IOCTL on an existing > > > > > > > > > DMA-BUF. > > > > > > > > > > > > > > > > Which is correct and how all kernel features work (sorry I > > > > > > > > missed the > > > > > > > > main goal of this patch earlier and focused only on the > > > > > > > > sysfs stuff). > > > > > > > > > > > > > > > > > However, user-space may want to figure out whether or not > > > > > > > > > the > > > > > > > > > IOCTL is available before it has a DMA-BUF at hand, e.g. > > > > > > > > > at > > > > > > > > > initialization time in a Wayland compositor. > > > > > > > > > > > > > > > > Why not just do the ioctl in a test way? That's how we > > > > > > > > determine kernel > > > > > > > > features, we do not poke around in sysfs to determine what > > > > > > > > is, or is > > > > > > > > not, present at runtime. > > > > > > > > > > > > > > > > > Add a /sys/kernel/dmabuf/caps directory which allows the > > > > > > > > > DMA-BUF > > > > > > > > > subsystem to advertise supported features. Add a > > > > > > > > > sync_file_import_export entry which indicates that > > > > > > > > > importing and > > > > > > > > > exporting sync_files from/to DMA-BUFs is supported. > > > > > > > > > > > > > > > > No, sorry, this is not a sustainable thing to do for all > > > > > > > > kernel features > > > > > > > > over time. Please just do the ioctl and go from there. > > > > > > > > sysfs is not > > > > > > > > for advertising what is and is not enabled/present in a > > > > > > > > kernel with > > > > > > > > regards to functionality or capabilities of the system. > > > > > > > > > > > > > > > > If sysfs were to export this type of thing, it would have > > > > > > > > to do it for > > > > > > > > everything, not just some random tiny thing of one kernel > > > > > > > > driver. > > > > > > > > > > > > > > I'd argue that DMA-BUF is a special case here. > > > > > > > > > > > > So this is special and unique just like everything else? :) > > > > > > > > > > > > > To check whether the import/export IOCTLs are available, > > > > > > > user-space > > > > > > > needs a DMA-BUF to try to perform the IOCTL. To get a DMA- > > > > > > > BUF, > > > > > > > user-space needs to enumerate GPUs, pick one at random, load > > > > > > > GBM or > > > > > > > Vulkan, use that heavy-weight API to allocate a "fake" buffer > > > > > > > on the > > > > > > > GPU, export that buffer into a DMA-BUF, try the IOCTL, then > > > > > > > teardown > > > > > > > all of this. There is no other way. > > > > > > > > > > > > > > This sounds like a roundabout way to answer the simple > > > > > > > question "is the > > > > > > > IOCTL available?". Do you have another suggestion to address > > > > > > > this > > > > > > > problem? > > > > > > > > > > > > What does userspace do differently if the ioctl is present or > > > > > > not? > > > > > > > > > > Globally enable a synchronization API for Wayland clients, for > > > > > instance > > > > > in the case of a Wayland compositor. > > > > > > > > > > > And why is this somehow more special than of the tens of > > > > > > thousands of > > > > > > other ioctl calls where you have to do exactly the same thing > > > > > > you list > > > > > > above to determine if it is present or not? > > > > > > > > > > For other IOCTLs it's not as complicated to obtain a FD to do the > > > > > test > > > > > with. > > > > > > > > Two expand on this: > > > > > > > > - compositor opens the drm render /dev node > > > > - compositor initializes the opengl or vulkan userspace driver on > > > > top of that > > > > - compositor asks that userspace driver to allocate some buffer, > > > > which > > > > can be pretty expensive > > > > - compositor asks the userspace driver to export that buffer into a > > > > dma-buf > > > > - compositor can finally do the test ioctl, realizes support isn't > > > > there and tosses the entire thing > > > > > > > > read() on a sysfs file is so much more reasonable it's not even > > > > funny. > > > > > > I agree it seems trivial and "simple", but that is NOT how to > > > determine > > > what is, and is not, a valid ioctl command for a device node. > > > > > > The only sane way to do this is like we have been doing for the past > > > 30+ > > > years, make the ioctl and look at the return value. > > > > > > Now if we want to come up with a new generic "here's the > > > capabilities/ioctls/whatever" that the kernel currently supports at > > > this > > > point in time api, wonderful, but PLEASE do not overload sysfs to do > > > something like this as that is not what it is for at this moment in > > > time. > > > > > > Don't just do this for one specific ioctl as there really is nothing > > > special about it at all ("it's special and unique just like all other > > > ioctls...") > > > > > > > Plan B we discussed is to add a getparam to signify this to the drm > > > > ioctl interface, but that has the design problem that a feature in > > > > the > > > > dma-buf subsystem is announced in a totally different subsystem (ok > > > > same maintainers), and if that ever gets out of sync your userspace > > > > breaks. So really no good. > > > > > > getparam makes sense in a way, if it doesn't change over time (i.e. > > > if > > > you call it now, will it be the same if you call it again if some > > > kernel > > > module is added/removed in the meantime?) Also be aware of > > > suspend/resume where you can swap out the kernel underneath running > > > userspace and that kernel might have different capabilities now. So > > > you > > > can't just not check error values of ioctl commands (not that you are > > > saying you want to here, just that it's more complex than might > > > originally seem.) > > > > > > > So if sysfs also isn't the right approach, and the getparam ioctl > > > > on > > > > drm is defo worse, what is the right approach? Ideally without > > > > setting > > > > up the entire userspace render driver and doing some expensive-ish > > > > (depending upon driver at least) buffer allocations just to check > > > > for > > > > a feature. > > > > > > > > Note that some compositors want to gate their "should I use vk for > > > > rendering and expose some additional features to client" decision > > > > on > > > > this, so setting up the wrong renderer just to test whether that > > > > would > > > > work is not a very great approach. > > > > > > > > Also the last time we added a feature to dma-buf was in 3.17, so I > > > > guess everyone just hardcodes nowadays that all dma-buf features > > > > are > > > > present. Which isn't great, and that's why we're trying to fix > > > > here. > > > > > > Why can't you call the test ioctl with an invalid value to see if it > > > is > > > present or not (-ENOTTY vs. -EINVAL) right at the beginning before > > > you > > > set up anything? > > > > Because we need a file descriptor to call that ioctl on. Currently, > > that file descriptor is the dma-buf itself. We could move it to be a > > DRM IOCTL but then it ends up unnecessarily tied to DRM even if > > someone's trying to use it with V4L or some other dma-buf > > producer/consumer. IDK if that's a real problem in practice but there > > didn't seem to be a good reason to tie it to things outside dma-buf. > > So you all have an api that is so hard to use, you can't use it until > you know what the capabilities of that api is before you call it. So > you want an "out-of-band" way to know if this api is present or not. > > Sounds like your api is the problem here, not the capability issue... > > Again, I understand your problem, but sysfs is NOT the place for this. > Linux has a well-defined way to tell userspace if an ioctl is present or > not. Just because it is "hard" to get to does not mean you get to > create a-brand-new-way-to-do-things for this, sorry. > > Kernel API design and evolution is hard, that's the job description of a > kernel developer. Boiler-plate code that you write once to do this > "does the kernel support this ioctl for a dma-buf" will be able to be > used by everyone for forever. Best of all, it will already be done for > when you have this issue for the next ioctl you create :) This code exists. It is: - load your userspace gl/vk driver - the userspace gl/vk driver knows how to create a dma-buf It's the entire "load userspace gl/vk driver" step we'd like to avoid, at least in some cases. And the way to do that would be: - some random thing in sysfs - some random thing in procfs - some randome getparam ioctl on each subsystem supporting dma-buf - some random syscall/ioctl/char-misc to create an fd you can call dma-buf ioctl on, but _only_ to figure out whether that ioctl is in theory support on a real dma-buf, but the fd you get through this is entirely fake and not a real dma-buf They're all kinda crap, especially the fake fd thing because - where do you create that fake fd? You're just back to adding a random crap interface in one of the above places (sysfs/procfs/subsystem ioctl) - you need to make sure that this fake dma-buf fd is only good for "does this ioctl exist", but not "can I actually use this as a dma-buf with a driver/subsystem", since it's not the real thing. This are all crap options, and we know. Consensus among userspace developers (aside from the vk/gl driver, which can do the "right" check) is that they'll check the kernel version number. Which is also crap, but it has the upside that it doesn't need an ack from kernel developers. I'm still hoping we can get something better than that, which is at least somewhat tied to the actual code running in the kernel? -Daniel
diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps b/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps new file mode 100644 index 000000000000..f83af422fd18 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps @@ -0,0 +1,13 @@ +What: /sys/kernel/dmabuf/caps +Date: May 2022 +KernelVersion: v5.20 +Contact: Simon Ser <contact@emersion.fr> +Description: This directory advertises DMA-BUF capabilities supported by the + kernel. + +What: /sys/kernel/dmabuf/caps/sync_file_import_export +Date: May 2022 +KernelVersion: v5.20 +Contact: Simon Ser <contact@emersion.fr> +Description: This file is read-only and advertises support for importing and + exporting sync_files from/to DMA-BUFs. diff --git a/MAINTAINERS b/MAINTAINERS index 11da16bfa123..8966686f7231 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5871,6 +5871,7 @@ L: dri-devel@lists.freedesktop.org L: linaro-mm-sig@lists.linaro.org (moderated for non-subscribers) S: Maintained T: git git://anongit.freedesktop.org/drm/drm-misc +F: Documentation/ABI/testing/sysfs-kernel-dmabuf-caps F: Documentation/driver-api/dma-buf.rst F: drivers/dma-buf/ F: include/linux/*fence.h diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index 4c9eb53ba3f8..afc874272710 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0-only obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \ - dma-resv.o + dma-resv.o dma-buf-sysfs-caps.o obj-$(CONFIG_DMABUF_HEAPS) += dma-heap.o obj-$(CONFIG_DMABUF_HEAPS) += heaps/ obj-$(CONFIG_SYNC_FILE) += sync_file.o diff --git a/drivers/dma-buf/dma-buf-sysfs-caps.c b/drivers/dma-buf/dma-buf-sysfs-caps.c new file mode 100644 index 000000000000..6eb27b81469f --- /dev/null +++ b/drivers/dma-buf/dma-buf-sysfs-caps.c @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * DMA-BUF sysfs capabilities. + * + * Copyright (C) 2022 Simon Ser + */ + +#include <linux/kobject.h> +#include <linux/sysfs.h> + +#include "dma-buf-sysfs-caps.h" + +static ssize_t sync_file_import_export_show(struct kobject *kobj, + struct kobj_attribute *attr, + char *buf) +{ + return sysfs_emit(buf, "1\n"); +} + +static struct kobj_attribute dma_buf_sync_file_import_export_attr = + __ATTR_RO(sync_file_import_export); + +static struct attribute *dma_buf_caps_attrs[] = { + &dma_buf_sync_file_import_export_attr.attr, + NULL, +}; + +const struct attribute_group dma_buf_caps_group = { + .name = "caps", + .attrs = dma_buf_caps_attrs, +}; diff --git a/drivers/dma-buf/dma-buf-sysfs-caps.h b/drivers/dma-buf/dma-buf-sysfs-caps.h new file mode 100644 index 000000000000..e40a93513f06 --- /dev/null +++ b/drivers/dma-buf/dma-buf-sysfs-caps.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * DMA-BUF sysfs capabilities. + * + * Copyright (C) 2022 Simon Ser + */ + +#ifndef _DMA_BUF_SYSFS_CAPS_H +#define _DMA_BUF_SYSFS_CAPS_H + +struct attribute_group; + +extern const struct attribute_group dma_buf_caps_group; + +#endif // _DMA_BUF_SYSFS_CAPS_H diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c index 2bba0babcb62..e2e62f83ce18 100644 --- a/drivers/dma-buf/dma-buf-sysfs-stats.c +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c @@ -141,23 +141,14 @@ static const struct kset_uevent_ops dmabuf_sysfs_no_uevent_ops = { .filter = dmabuf_sysfs_uevent_filter, }; -static struct kset *dma_buf_stats_kset; static struct kset *dma_buf_per_buffer_stats_kset; -int dma_buf_init_sysfs_statistics(void) +int dma_buf_init_sysfs_statistics(struct kset *kset) { - dma_buf_stats_kset = kset_create_and_add("dmabuf", - &dmabuf_sysfs_no_uevent_ops, - kernel_kobj); - if (!dma_buf_stats_kset) - return -ENOMEM; - dma_buf_per_buffer_stats_kset = kset_create_and_add("buffers", &dmabuf_sysfs_no_uevent_ops, - &dma_buf_stats_kset->kobj); - if (!dma_buf_per_buffer_stats_kset) { - kset_unregister(dma_buf_stats_kset); + &kset->kobj); + if (!dma_buf_per_buffer_stats_kset) return -ENOMEM; - } return 0; } @@ -165,7 +156,6 @@ int dma_buf_init_sysfs_statistics(void) void dma_buf_uninit_sysfs_statistics(void) { kset_unregister(dma_buf_per_buffer_stats_kset); - kset_unregister(dma_buf_stats_kset); } int dma_buf_stats_setup(struct dma_buf *dmabuf) diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.h b/drivers/dma-buf/dma-buf-sysfs-stats.h index a49c6e2650cc..798c54fb8ee3 100644 --- a/drivers/dma-buf/dma-buf-sysfs-stats.h +++ b/drivers/dma-buf/dma-buf-sysfs-stats.h @@ -8,9 +8,11 @@ #ifndef _DMA_BUF_SYSFS_STATS_H #define _DMA_BUF_SYSFS_STATS_H +struct kset; + #ifdef CONFIG_DMABUF_SYSFS_STATS -int dma_buf_init_sysfs_statistics(void); +int dma_buf_init_sysfs_statistics(struct kset *kset); void dma_buf_uninit_sysfs_statistics(void); int dma_buf_stats_setup(struct dma_buf *dmabuf); @@ -18,7 +20,7 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf); void dma_buf_stats_teardown(struct dma_buf *dmabuf); #else -static inline int dma_buf_init_sysfs_statistics(void) +static inline int dma_buf_init_sysfs_statistics(struct kset *kset) { return 0; } diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 5e1b0534b3ce..61c5be57558e 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -30,6 +30,7 @@ #include <uapi/linux/dma-buf.h> #include <uapi/linux/magic.h> +#include "dma-buf-sysfs-caps.h" #include "dma-buf-sysfs-stats.h" static inline int is_dma_buf_file(struct file *); @@ -1546,22 +1547,57 @@ static inline void dma_buf_uninit_debugfs(void) } #endif +/* Capabilities and statistics files do not need to send uevents. */ +static int dmabuf_sysfs_uevent_filter(struct kobject *kobj) +{ + return 0; +} + +static const struct kset_uevent_ops dmabuf_sysfs_no_uevent_ops = { + .filter = dmabuf_sysfs_uevent_filter, +}; + +static const struct attribute_group *dma_buf_sysfs_groups[] = { + &dma_buf_caps_group, + NULL, +}; + +static struct kset *dma_buf_kset; + static int __init dma_buf_init(void) { int ret; - ret = dma_buf_init_sysfs_statistics(); + dma_buf_kset = kset_create_and_add("dmabuf", + &dmabuf_sysfs_no_uevent_ops, + kernel_kobj); + if (!dma_buf_kset) + return -ENOMEM; + + ret = sysfs_create_groups(&dma_buf_kset->kobj, dma_buf_sysfs_groups); if (ret) - return ret; + goto err_kset; + + ret = dma_buf_init_sysfs_statistics(dma_buf_kset); + if (ret) + goto err_kset; dma_buf_mnt = kern_mount(&dma_buf_fs_type); - if (IS_ERR(dma_buf_mnt)) - return PTR_ERR(dma_buf_mnt); + if (IS_ERR(dma_buf_mnt)) { + ret = PTR_ERR(dma_buf_mnt); + goto err_sysfs_stats; + } mutex_init(&db_list.lock); INIT_LIST_HEAD(&db_list.head); dma_buf_init_debugfs(); return 0; + +err_sysfs_stats: + dma_buf_uninit_sysfs_statistics(); +err_kset: + kset_unregister(dma_buf_kset); + return ret; } subsys_initcall(dma_buf_init); @@ -1570,5 +1606,6 @@ static void __exit dma_buf_deinit(void) dma_buf_uninit_debugfs(); kern_unmount(dma_buf_mnt); dma_buf_uninit_sysfs_statistics(); + kset_unregister(dma_buf_kset); } __exitcall(dma_buf_deinit); diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h index 70e213a0d7d9..ab3afd5da75a 100644 --- a/include/uapi/linux/dma-buf.h +++ b/include/uapi/linux/dma-buf.h @@ -114,6 +114,9 @@ struct dma_buf_sync { * ordering via these fences, it is the respnosibility of userspace to use * locks or other mechanisms to ensure that no other context adds fences or * submits work between steps 1 and 3 above. + * + * Userspace can check the availability of this API via + * /sys/kernel/dmabuf/caps/sync_file_import_export. */ struct dma_buf_export_sync_file { /** @@ -146,6 +149,9 @@ struct dma_buf_export_sync_file { * synchronized APIs such as Vulkan to inter-op with dma-buf consumers * which expect implicit synchronization such as OpenGL or most media * drivers/video. + * + * Userspace can check the availability of this API via + * /sys/kernel/dmabuf/caps/sync_file_import_export. */ struct dma_buf_import_sync_file { /**