diff mbox series

drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs

Message ID 20250326143903.24380-1-robdclark@gmail.com (mailing list archive)
State New
Headers show
Series drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs | expand

Commit Message

Rob Clark March 26, 2025, 2:39 p.m. UTC
From: Rob Clark <robdclark@chromium.org>

Add support for exporting a dma_fence fd for a specific point on a
timeline.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/drm_syncobj.c | 8 ++++++--
 include/uapi/drm/drm.h        | 2 ++
 2 files changed, 8 insertions(+), 2 deletions(-)

Comments

Christian König March 26, 2025, 2:41 p.m. UTC | #1
Am 26.03.25 um 15:39 schrieb Rob Clark:
> From: Rob Clark <robdclark@chromium.org>
>
> Add support for exporting a dma_fence fd for a specific point on a
> timeline.

Looks good on first glance. What's the userspace use case?

Regards,
Christian.

>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/drm_syncobj.c | 8 ++++++--
>  include/uapi/drm/drm.h        | 2 ++
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 4f2ab8a7b50f..eb7a2dd2e261 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -762,7 +762,7 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
>  }
>  
>  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;
> @@ -772,7 +772,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;
>  
> @@ -882,8 +882,12 @@ 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->point,
>  						    &args->fd);
>  
> +	if (args->point)
> +		return -EINVAL;
> +
>  	return drm_syncobj_handle_to_fd(file_private, args->handle,
>  					&args->fd);
>  }
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 7fba37b94401..c71a8f4439f2 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -912,6 +912,8 @@ struct drm_syncobj_handle {
>  
>  	__s32 fd;
>  	__u32 pad;
> +
> +	__u64 point;
>  };
>  
>  struct drm_syncobj_transfer {
Rob Clark March 26, 2025, 2:46 p.m. UTC | #2
On Wed, Mar 26, 2025 at 7:41 AM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 26.03.25 um 15:39 schrieb Rob Clark:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Add support for exporting a dma_fence fd for a specific point on a
> > timeline.
>
> Looks good on first glance. What's the userspace use case?

Timeline syncobj support for vtest/vpipe[1][2].. since core
virglrender and drm native ctx works in terms of fences (since in the
VM case, everything is a fence below the guest kernel uabi), we need
to be able to turn a point on a timeline back into a fence fd.  (Plus
it seemed like an odd omission from the existing uabi.)

BR,
-R

[1] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/33433
[2] https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/805

>
> Regards,
> Christian.
>
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >  drivers/gpu/drm/drm_syncobj.c | 8 ++++++--
> >  include/uapi/drm/drm.h        | 2 ++
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> > index 4f2ab8a7b50f..eb7a2dd2e261 100644
> > --- a/drivers/gpu/drm/drm_syncobj.c
> > +++ b/drivers/gpu/drm/drm_syncobj.c
> > @@ -762,7 +762,7 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
> >  }
> >
> >  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;
> > @@ -772,7 +772,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;
> >
> > @@ -882,8 +882,12 @@ 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->point,
> >                                                   &args->fd);
> >
> > +     if (args->point)
> > +             return -EINVAL;
> > +
> >       return drm_syncobj_handle_to_fd(file_private, args->handle,
> >                                       &args->fd);
> >  }
> > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > index 7fba37b94401..c71a8f4439f2 100644
> > --- a/include/uapi/drm/drm.h
> > +++ b/include/uapi/drm/drm.h
> > @@ -912,6 +912,8 @@ struct drm_syncobj_handle {
> >
> >       __s32 fd;
> >       __u32 pad;
> > +
> > +     __u64 point;
> >  };
> >
> >  struct drm_syncobj_transfer {
>
Rob Clark March 26, 2025, 8:46 p.m. UTC | #3
On Wed, Mar 26, 2025 at 7:46 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Wed, Mar 26, 2025 at 7:41 AM Christian König
> <christian.koenig@amd.com> wrote:
> >
> > Am 26.03.25 um 15:39 schrieb Rob Clark:
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > Add support for exporting a dma_fence fd for a specific point on a
> > > timeline.
> >
> > Looks good on first glance. What's the userspace use case?
>
> Timeline syncobj support for vtest/vpipe[1][2].. since core
> virglrender and drm native ctx works in terms of fences (since in the
> VM case, everything is a fence below the guest kernel uabi), we need
> to be able to turn a point on a timeline back into a fence fd.  (Plus
> it seemed like an odd omission from the existing uabi.)
>
> BR,
> -R
>
> [1] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/33433
> [2] https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/805
>
> >
> > Regards,
> > Christian.
> >
> > >
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > ---
> > >  drivers/gpu/drm/drm_syncobj.c | 8 ++++++--
> > >  include/uapi/drm/drm.h        | 2 ++
> > >  2 files changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> > > index 4f2ab8a7b50f..eb7a2dd2e261 100644
> > > --- a/drivers/gpu/drm/drm_syncobj.c
> > > +++ b/drivers/gpu/drm/drm_syncobj.c
> > > @@ -762,7 +762,7 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
> > >  }
> > >
> > >  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;
> > > @@ -772,7 +772,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;
> > >
> > > @@ -882,8 +882,12 @@ 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->point,
> > >                                                   &args->fd);

Hmm, maybe I should add DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE so
that userspace gets a clean error on older kernels, rather than having
the point param be silently ignored..

BR,
-R

> > >
> > > +     if (args->point)
> > > +             return -EINVAL;
> > > +
> > >       return drm_syncobj_handle_to_fd(file_private, args->handle,
> > >                                       &args->fd);
> > >  }
> > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > index 7fba37b94401..c71a8f4439f2 100644
> > > --- a/include/uapi/drm/drm.h
> > > +++ b/include/uapi/drm/drm.h
> > > @@ -912,6 +912,8 @@ struct drm_syncobj_handle {
> > >
> > >       __s32 fd;
> > >       __u32 pad;
> > > +
> > > +     __u64 point;
> > >  };
> > >
> > >  struct drm_syncobj_transfer {
> >
Christian König March 27, 2025, 8:23 a.m. UTC | #4
Am 26.03.25 um 21:46 schrieb Rob Clark:
> On Wed, Mar 26, 2025 at 7:46 AM Rob Clark <robdclark@gmail.com> wrote:
>> On Wed, Mar 26, 2025 at 7:41 AM Christian König
>> <christian.koenig@amd.com> wrote:
>>> Am 26.03.25 um 15:39 schrieb Rob Clark:
>>>> From: Rob Clark <robdclark@chromium.org>
>>>>
>>>> Add support for exporting a dma_fence fd for a specific point on a
>>>> timeline.
>>> Looks good on first glance. What's the userspace use case?
>> Timeline syncobj support for vtest/vpipe[1][2].. since core
>> virglrender and drm native ctx works in terms of fences (since in the
>> VM case, everything is a fence below the guest kernel uabi), we need
>> to be able to turn a point on a timeline back into a fence fd.  (Plus
>> it seemed like an odd omission from the existing uabi.)
>>
>> BR,
>> -R
>>
>> [1] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/33433
>> [2] https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/805
>>
>>> Regards,
>>> Christian.
>>>
>>>> Signed-off-by: Rob Clark <robdclark@chromium.org>
>>>> ---
>>>>  drivers/gpu/drm/drm_syncobj.c | 8 ++++++--
>>>>  include/uapi/drm/drm.h        | 2 ++
>>>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>>>> index 4f2ab8a7b50f..eb7a2dd2e261 100644
>>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>>> @@ -762,7 +762,7 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
>>>>  }
>>>>
>>>>  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;
>>>> @@ -772,7 +772,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;
>>>>
>>>> @@ -882,8 +882,12 @@ 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->point,
>>>>                                                   &args->fd);
> Hmm, maybe I should add DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE so
> that userspace gets a clean error on older kernels, rather than having
> the point param be silently ignored..

Sounds reasonable to me as well.

And please include the links to the userspace code in the commit message.

Apart from that looks totally reasonable to me.

Regards,
Christian.

>
> BR,
> -R
>
>>>> +     if (args->point)
>>>> +             return -EINVAL;
>>>> +
>>>>       return drm_syncobj_handle_to_fd(file_private, args->handle,
>>>>                                       &args->fd);
>>>>  }
>>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>>>> index 7fba37b94401..c71a8f4439f2 100644
>>>> --- a/include/uapi/drm/drm.h
>>>> +++ b/include/uapi/drm/drm.h
>>>> @@ -912,6 +912,8 @@ struct drm_syncobj_handle {
>>>>
>>>>       __s32 fd;
>>>>       __u32 pad;
>>>> +
>>>> +     __u64 point;
>>>>  };
>>>>
>>>>  struct drm_syncobj_transfer {
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 4f2ab8a7b50f..eb7a2dd2e261 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -762,7 +762,7 @@  static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
 }
 
 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;
@@ -772,7 +772,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;
 
@@ -882,8 +882,12 @@  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->point,
 						    &args->fd);
 
+	if (args->point)
+		return -EINVAL;
+
 	return drm_syncobj_handle_to_fd(file_private, args->handle,
 					&args->fd);
 }
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 7fba37b94401..c71a8f4439f2 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -912,6 +912,8 @@  struct drm_syncobj_handle {
 
 	__s32 fd;
 	__u32 pad;
+
+	__u64 point;
 };
 
 struct drm_syncobj_transfer {