On 7/20/23 02:43, Simon Ser wrote: > On Wednesday, July 19th, 2023 at 19:05, Erik Kurzinger <ekurzinger@xxxxxxxxxx> 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.) > Thanks for taking a look, Simon. If that's the case I could also update my Xwayland MR to use the new functions. >> Signed-off-by: Erik Kurzinger <ekurzinger@xxxxxxxxxx> >> --- >> 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
