On Wednesday, July 19th, 2023 at 19:05, Erik Kurzinger <ekurzin...@nvidia.com> wrote:
> These new ioctls perform a task similar to > DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD/FD_TO_HANDLE with the > IMPORT/EXPORT_SYNC_FILE flag set, except that they allow specifying the > timeline point to import or export the fence to or from on a timeline > syncobj. > > This eliminates the need to use a temporary binary syncobj along with > DRM_IOCTL_SYNCOBJ_TRANSFER to achieve such a thing, which is the > technique userspace has had to employ up to this point. While that does > work, it is rather awkward from the programmer's perspective. Since DRM > syncobjs have been proposed as the basis for display server explicit > synchronization protocols, e.g. [1] and [2], providing a more > streamlined interface now seems worthwhile. This looks like a good idea to me! The patch looks good as well, apart from one tricky issue, see below... > [1] > https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/90 > [2] https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/967 > > Accompanying userspace patches... > IGT: > https://gitlab.freedesktop.org/ekurzinger/igt-gpu-tools/-/commit/241e7f379aeaa9b22a32277e77ad4011c8717a57 > libdrm: > https://gitlab.freedesktop.org/ekurzinger/drm/-/commit/b3961a592fc6f8b05f7e3a12413fb58eca2dbfa2 (Unfortunately this isn't enough when it comes to user-space patches: the kernel rules require a "real" user of the new IOCTL, not just a libdr IOCTL wrapper. I will post a patch to make use of this from wlroots if that helps.) > Signed-off-by: Erik Kurzinger <ekurzin...@nvidia.com> > --- > drivers/gpu/drm/drm_internal.h | 4 +++ > drivers/gpu/drm/drm_ioctl.c | 4 +++ > drivers/gpu/drm/drm_syncobj.c | 60 ++++++++++++++++++++++++++++++---- > include/uapi/drm/drm.h | 9 +++++ > 4 files changed, 71 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h > index d7e023bbb0d5..64a28ed26a16 100644 > --- a/drivers/gpu/drm/drm_internal.h > +++ b/drivers/gpu/drm/drm_internal.h > @@ -253,6 +253,10 @@ int drm_syncobj_timeline_signal_ioctl(struct drm_device > *dev, void *data, > struct drm_file *file_private); > int drm_syncobj_query_ioctl(struct drm_device *dev, void *data, > struct drm_file *file_private); > +int drm_syncobj_import_sync_file_ioctl(struct drm_device *dev, void *data, > + struct drm_file *file_private); > +int drm_syncobj_export_sync_file_ioctl(struct drm_device *dev, void *data, > + struct drm_file *file_private); > > /* drm_framebuffer.c */ > void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent, > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 7c9d66ee917d..0344e8e447bc 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -710,6 +710,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = { > DRM_RENDER_ALLOW), > DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl, > DRM_RENDER_ALLOW), > + DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_IMPORT_SYNC_FILE, > drm_syncobj_import_sync_file_ioctl, > + DRM_RENDER_ALLOW), > + DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_EXPORT_SYNC_FILE, > drm_syncobj_export_sync_file_ioctl, > + DRM_RENDER_ALLOW), > DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, > 0), > DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, > drm_crtc_queue_sequence_ioctl, 0), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, > DRM_MASTER), > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index 0c2be8360525..bf0c1eae353a 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -181,6 +181,13 @@ > * Note that if you want to transfer a struct &dma_fence_chain from a given > * point on a timeline syncobj from/into a binary syncobj, you can use the > * point 0 to mean take/replace the fence in the syncobj. > + * > + * &DRM_IOCTL_SYNCOBJ_IMPORT_SYNC_FILE and > &DRM_IOCTL_SYNCOBJ_EXPORT_SYNC_FILE > + * let the client import or export the struct &dma_fence_chain of a syncobj > + * at a particular timeline point from or to a sync file. > + * These behave similarly to &DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE > + * and &DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE described above, > except > + * that they accommodate timeline syncobjs in addition to binary syncobjs. > */ > > #include <linux/anon_inodes.h> > @@ -682,10 +689,11 @@ static int drm_syncobj_fd_to_handle(struct drm_file > *file_private, > } > > static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private, > - int fd, int handle) > + int fd, u64 point, int handle) Nit: can we specify the point after the handle, for consistency with drm_syncobj_export_sync_file()? It's pretty easy to mix up these two arguments. > { > struct dma_fence *fence = sync_file_get_fence(fd); > struct drm_syncobj *syncobj; > + int ret = 0; > > if (!fence) > return -EINVAL; > @@ -696,14 +704,23 @@ static int drm_syncobj_import_sync_file_fence(struct > drm_file *file_private, > return -ENOENT; > } > > - drm_syncobj_replace_fence(syncobj, fence); > + if (point == 0) { > + drm_syncobj_replace_fence(syncobj, fence); > + } else { > + struct dma_fence_chain *chain = dma_fence_chain_alloc(); > + if (chain) { > + drm_syncobj_add_point(syncobj, chain, fence, point); > + } else { > + ret = -ENOMEM; > + } > + } > dma_fence_put(fence); > drm_syncobj_put(syncobj); > - return 0; > + return ret; > } > > static int drm_syncobj_export_sync_file(struct drm_file *file_private, > - int handle, int *p_fd) > + int handle, u64 point, int *p_fd) > { > int ret; > struct dma_fence *fence; > @@ -713,7 +730,7 @@ static int drm_syncobj_export_sync_file(struct drm_file > *file_private, > if (fd < 0) > return fd; > > - ret = drm_syncobj_find_fence(file_private, handle, 0, 0, &fence); > + ret = drm_syncobj_find_fence(file_private, handle, point, 0, &fence); > if (ret) > goto err_put_fd; > > @@ -823,7 +840,7 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, > void *data, > > if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE) > return drm_syncobj_export_sync_file(file_private, args->handle, > - &args->fd); > + 0 /* binary */, &args->fd); > > return drm_syncobj_handle_to_fd(file_private, args->handle, > &args->fd); > @@ -848,6 +865,7 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, > void *data, > if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE) > return drm_syncobj_import_sync_file_fence(file_private, > args->fd, > + 0 /* binary */, > args->handle); > > return drm_syncobj_fd_to_handle(file_private, args->fd, > @@ -1515,3 +1533,33 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, > void *data, > > return ret; > } > + > +int drm_syncobj_import_sync_file_ioctl(struct drm_device *dev, void *data, > + struct drm_file *file_private) > +{ > + struct drm_syncobj_sync_file *args = data; > + > + if (!drm_core_check_feature(dev, args->point == 0 ? > + DRIVER_SYNCOBJ : > + DRIVER_SYNCOBJ_TIMELINE)) > + return -EOPNOTSUPP; > + > + return drm_syncobj_import_sync_file_fence(file_private, > + args->fd, > + args->point, > + args->handle); > +} > + > +int drm_syncobj_export_sync_file_ioctl(struct drm_device *dev, void *data, > + struct drm_file *file_private) > +{ > + struct drm_syncobj_sync_file *args = data; > + > + if (!drm_core_check_feature(dev, args->point == 0 ? > + DRIVER_SYNCOBJ : > + DRIVER_SYNCOBJ_TIMELINE)) > + return -EOPNOTSUPP; > + > + return drm_syncobj_export_sync_file(file_private, args->handle, > + args->point, &args->fd); > +} > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index a87bbbbca2d4..e1f045011c22 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -884,6 +884,12 @@ struct drm_syncobj_transfer { > __u32 pad; > }; > > +struct drm_syncobj_sync_file { > + __u32 handle; > + __u32 fd; > + __u64 point; > +}; > + > #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0) > #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT (1 << 1) > #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE (1 << 2) /* wait for time > point to become available */ > @@ -1139,6 +1145,9 @@ extern "C" { > #define DRM_IOCTL_SYNCOBJ_TRANSFER DRM_IOWR(0xCC, struct > drm_syncobj_transfer) > #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL DRM_IOWR(0xCD, struct > drm_syncobj_timeline_array) > > +#define DRM_IOCTL_SYNCOBJ_IMPORT_SYNC_FILE DRM_IOWR(0xCE, struct > drm_syncobj_sync_file) > +#define DRM_IOCTL_SYNCOBJ_EXPORT_SYNC_FILE DRM_IOWR(0xCF, struct > drm_syncobj_sync_file) So, there is a footgun here, one that I hit myself with before: 0xCE is already used by DRM_IOCTL_MODE_GETFB2. And there is no check whatsoever about conflicting IOCTL numbers. The only reason I noticed is that I got some pretty weird behavior when trying to detect for the IOCTL availability in IGT, and someone from AMD noticed some GETFB2 IGT test breakage when trying my patches. > /** > * DRM_IOCTL_MODE_GETFB2 - Get framebuffer metadata. > * > -- > 2.41.0
