[2/5] drm/virtio: add uapi for in and out explicit fences
diff mbox series

Message ID 20181025183739.9375-3-robert.foss@collabora.com
State New
Headers show
Series
  • virgl: fence fd support
Related show

Commit Message

Robert Foss Oct. 25, 2018, 6:37 p.m. UTC
Add a new field called fence_fd that will be used by userspace to send
in-fences to the kernel and receive out-fences created by the kernel.

This uapi enables virtio to take advantage of explicit synchronization of
dma-bufs.

There are two new flags:

* VIRTGPU_EXECBUF_FENCE_FD_IN to be used when passing an in-fence fd.
* VIRTGPU_EXECBUF_FENCE_FD_OUT to be used when requesting an out-fence fd

The execbuffer IOCTL is now read-write to allow the userspace to read the
out-fence.

On error -1 should be returned in the fence_fd field.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
Signed-off-by: Robert Foss <robert.foss@collabora.com>
---
Changes since v2:
 - Since exbuf-flags is a new flag, check that unsupported
   flags aren't set.

 drivers/gpu/drm/virtio/virtgpu_ioctl.c |  5 +++++
 include/uapi/drm/virtgpu_drm.h         | 13 ++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

Comments

Gerd Hoffmann Oct. 30, 2018, 6:11 a.m. UTC | #1
Hi,

> The execbuffer IOCTL is now read-write to allow the userspace to read the
> out-fence.

>  #define DRM_IOCTL_VIRTGPU_EXECBUFFER \
> -	DRM_IOW(DRM_COMMAND_BASE + DRM_VIRTGPU_EXECBUFFER,\
> +	DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_EXECBUFFER,\
>  		struct drm_virtgpu_execbuffer)

That changes the ioctl number and breaks the userspace api.

cheers,
  Gerd
Emil Velikov Oct. 30, 2018, 11:31 a.m. UTC | #2
HI Gerd,

On Tue, 30 Oct 2018 at 06:11, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > The execbuffer IOCTL is now read-write to allow the userspace to read the
> > out-fence.
>
> >  #define DRM_IOCTL_VIRTGPU_EXECBUFFER \
> > -     DRM_IOW(DRM_COMMAND_BASE + DRM_VIRTGPU_EXECBUFFER,\
> > +     DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_EXECBUFFER,\
> >               struct drm_virtgpu_execbuffer)
>
> That changes the ioctl number and breaks the userspace api.
>
Have you looked at the drm_ioctl() implementation? AFAICT it
explicitly caters for this kind of changes.

-Emil
Gerd Hoffmann Oct. 30, 2018, 1:52 p.m. UTC | #3
On Tue, Oct 30, 2018 at 11:31:04AM +0000, Emil Velikov wrote:
> HI Gerd,
> 
> On Tue, 30 Oct 2018 at 06:11, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >   Hi,
> >
> > > The execbuffer IOCTL is now read-write to allow the userspace to read the
> > > out-fence.
> >
> > >  #define DRM_IOCTL_VIRTGPU_EXECBUFFER \
> > > -     DRM_IOW(DRM_COMMAND_BASE + DRM_VIRTGPU_EXECBUFFER,\
> > > +     DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_EXECBUFFER,\
> > >               struct drm_virtgpu_execbuffer)
> >
> > That changes the ioctl number and breaks the userspace api.
> >
> Have you looked at the drm_ioctl() implementation? AFAICT it
> explicitly caters for this kind of changes.

Looking ...

The direction bits are not used to lookup the ioctl functions,
so it should work indeed.

Series doesn't apply to drm-misc-next and needs a rebase.

cheers,
  Gerd
Emil Velikov Oct. 30, 2018, 3:48 p.m. UTC | #4
On Tue, 30 Oct 2018 at 13:52, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Tue, Oct 30, 2018 at 11:31:04AM +0000, Emil Velikov wrote:
> > HI Gerd,
> >
> > On Tue, 30 Oct 2018 at 06:11, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > >   Hi,
> > >
> > > > The execbuffer IOCTL is now read-write to allow the userspace to read the
> > > > out-fence.
> > >
> > > >  #define DRM_IOCTL_VIRTGPU_EXECBUFFER \
> > > > -     DRM_IOW(DRM_COMMAND_BASE + DRM_VIRTGPU_EXECBUFFER,\
> > > > +     DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_EXECBUFFER,\
> > > >               struct drm_virtgpu_execbuffer)
> > >
> > > That changes the ioctl number and breaks the userspace api.
> > >
> > Have you looked at the drm_ioctl() implementation? AFAICT it
> > explicitly caters for this kind of changes.
>
> Looking ...
>
> The direction bits are not used to lookup the ioctl functions,
> so it should work indeed.
>
Nice, thanks for confirming.

> Series doesn't apply to drm-misc-next and needs a rebase.
>
Might be nicer to address that alongside any feedback. Otherwise it'll
be spamming people just for the sake of rebasing.

If i find some time I'll post some comments later on today.

HTH
Emil
Emil Velikov Oct. 31, 2018, 9:38 a.m. UTC | #5
Hi Rob,

On Thu, 25 Oct 2018 at 19:38, Robert Foss <robert.foss@collabora.com> wrote:
>
> Add a new field called fence_fd that will be used by userspace to send
> in-fences to the kernel and receive out-fences created by the kernel.
>
> This uapi enables virtio to take advantage of explicit synchronization of
> dma-bufs.
>
> There are two new flags:
>
> * VIRTGPU_EXECBUF_FENCE_FD_IN to be used when passing an in-fence fd.
> * VIRTGPU_EXECBUF_FENCE_FD_OUT to be used when requesting an out-fence fd
>
> The execbuffer IOCTL is now read-write to allow the userspace to read the
> out-fence.
>
> On error -1 should be returned in the fence_fd field.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> ---
> Changes since v2:
>  - Since exbuf-flags is a new flag, check that unsupported
>    flags aren't set.
>
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c |  5 +++++
>  include/uapi/drm/virtgpu_drm.h         | 13 ++++++++++---
>  2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index d01a9ed100d1..1af289b28fc4 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -116,9 +116,14 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
>         struct ww_acquire_ctx ticket;
>         void *buf;
>
> +       exbuf->fence_fd = -1;
> +
Move this after the sanity checking.

>         if (vgdev->has_virgl_3d == false)
>                 return -ENOSYS;
>
> +       if ((exbuf->flags & ~VIRTGPU_EXECBUF_FLAGS))
> +               return -EINVAL;
> +
I assume this did this trigger when using old userspace?

With those the patch is
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

Thanks
Emil
Robert Foss Nov. 1, 2018, 12:56 p.m. UTC | #6
On 2018-10-31 10:38, Emil Velikov wrote:
> Hi Rob,
> 
> On Thu, 25 Oct 2018 at 19:38, Robert Foss <robert.foss@collabora.com> wrote:
>>
>> Add a new field called fence_fd that will be used by userspace to send
>> in-fences to the kernel and receive out-fences created by the kernel.
>>
>> This uapi enables virtio to take advantage of explicit synchronization of
>> dma-bufs.
>>
>> There are two new flags:
>>
>> * VIRTGPU_EXECBUF_FENCE_FD_IN to be used when passing an in-fence fd.
>> * VIRTGPU_EXECBUF_FENCE_FD_OUT to be used when requesting an out-fence fd
>>
>> The execbuffer IOCTL is now read-write to allow the userspace to read the
>> out-fence.
>>
>> On error -1 should be returned in the fence_fd field.
>>
>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>> Signed-off-by: Robert Foss <robert.foss@collabora.com>
>> ---
>> Changes since v2:
>>   - Since exbuf-flags is a new flag, check that unsupported
>>     flags aren't set.
>>
>>   drivers/gpu/drm/virtio/virtgpu_ioctl.c |  5 +++++
>>   include/uapi/drm/virtgpu_drm.h         | 13 ++++++++++---
>>   2 files changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
>> index d01a9ed100d1..1af289b28fc4 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
>> @@ -116,9 +116,14 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
>>          struct ww_acquire_ctx ticket;
>>          void *buf;
>>
>> +       exbuf->fence_fd = -1;
>> +
> Move this after the sanity checking.

Agreed. Fixed in v4

> 
>>          if (vgdev->has_virgl_3d == false)
>>                  return -ENOSYS;
>>
>> +       if ((exbuf->flags & ~VIRTGPU_EXECBUF_FLAGS))
>> +               return -EINVAL;
>> +
> I assume this did this trigger when using old userspace?

No, not as far as I'm aware. This check is there to prevent userspace from
polluting the bitspace of flag, so that all free bits can be used for new flags.

As far as I understand this is pointed out by a drm driver development document
written by danvet, which I unfortunately can't seem to find the link for at the
moment.

> 
> With those the patch is
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
> 
> Thanks
> Emil
>
Emil Velikov Nov. 2, 2018, 1:34 p.m. UTC | #7
On Thu, 1 Nov 2018 at 12:56, Robert Foss <robert.foss@collabora.com> wrote:
> On 2018-10-31 10:38, Emil Velikov wrote:
> > Hi Rob,
> >
> > On Thu, 25 Oct 2018 at 19:38, Robert Foss <robert.foss@collabora.com> wrote:
> >>
> >> Add a new field called fence_fd that will be used by userspace to send
> >> in-fences to the kernel and receive out-fences created by the kernel.
> >>
> >> This uapi enables virtio to take advantage of explicit synchronization of
> >> dma-bufs.
> >>
> >> There are two new flags:
> >>
> >> * VIRTGPU_EXECBUF_FENCE_FD_IN to be used when passing an in-fence fd.
> >> * VIRTGPU_EXECBUF_FENCE_FD_OUT to be used when requesting an out-fence fd
> >>
> >> The execbuffer IOCTL is now read-write to allow the userspace to read the
> >> out-fence.
> >>
> >> On error -1 should be returned in the fence_fd field.
> >>
> >> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> >> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> >> ---
> >> Changes since v2:
> >>   - Since exbuf-flags is a new flag, check that unsupported
> >>     flags aren't set.
> >>
> >>   drivers/gpu/drm/virtio/virtgpu_ioctl.c |  5 +++++
> >>   include/uapi/drm/virtgpu_drm.h         | 13 ++++++++++---
> >>   2 files changed, 15 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> >> index d01a9ed100d1..1af289b28fc4 100644
> >> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> >> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> >> @@ -116,9 +116,14 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> >>          struct ww_acquire_ctx ticket;
> >>          void *buf;
> >>
> >> +       exbuf->fence_fd = -1;
> >> +
> > Move this after the sanity checking.
>
> Agreed. Fixed in v4
>
> >
> >>          if (vgdev->has_virgl_3d == false)
> >>                  return -ENOSYS;
> >>
> >> +       if ((exbuf->flags & ~VIRTGPU_EXECBUF_FLAGS))
> >> +               return -EINVAL;
> >> +
> > I assume this did this trigger when using old userspace?
>
> No, not as far as I'm aware. This check is there to prevent userspace from
> polluting the bitspace of flag, so that all free bits can be used for new flags.
>
> As far as I understand this is pointed out by a drm driver development document
> written by danvet, which I unfortunately can't seem to find the link for at the
> moment.
>
Yes that is correct. What I was asking is:

Does a kernel with this patch, work with mesa lacking the corresponding updates?
I'd imagine things work just fine.

-Emil
Robert Foss Nov. 2, 2018, 2:42 p.m. UTC | #8
Hey Emil,

On 2018-11-02 14:34, Emil Velikov wrote:
> On Thu, 1 Nov 2018 at 12:56, Robert Foss <robert.foss@collabora.com> wrote:
>> On 2018-10-31 10:38, Emil Velikov wrote:
>>> Hi Rob,
>>>
>>> On Thu, 25 Oct 2018 at 19:38, Robert Foss <robert.foss@collabora.com> wrote:
>>>>
>>>> Add a new field called fence_fd that will be used by userspace to send
>>>> in-fences to the kernel and receive out-fences created by the kernel.
>>>>
>>>> This uapi enables virtio to take advantage of explicit synchronization of
>>>> dma-bufs.
>>>>
>>>> There are two new flags:
>>>>
>>>> * VIRTGPU_EXECBUF_FENCE_FD_IN to be used when passing an in-fence fd.
>>>> * VIRTGPU_EXECBUF_FENCE_FD_OUT to be used when requesting an out-fence fd
>>>>
>>>> The execbuffer IOCTL is now read-write to allow the userspace to read the
>>>> out-fence.
>>>>
>>>> On error -1 should be returned in the fence_fd field.
>>>>
>>>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>>>> Signed-off-by: Robert Foss <robert.foss@collabora.com>
>>>> ---
>>>> Changes since v2:
>>>>    - Since exbuf-flags is a new flag, check that unsupported
>>>>      flags aren't set.
>>>>
>>>>    drivers/gpu/drm/virtio/virtgpu_ioctl.c |  5 +++++
>>>>    include/uapi/drm/virtgpu_drm.h         | 13 ++++++++++---
>>>>    2 files changed, 15 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
>>>> index d01a9ed100d1..1af289b28fc4 100644
>>>> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
>>>> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
>>>> @@ -116,9 +116,14 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
>>>>           struct ww_acquire_ctx ticket;
>>>>           void *buf;
>>>>
>>>> +       exbuf->fence_fd = -1;
>>>> +
>>> Move this after the sanity checking.
>>
>> Agreed. Fixed in v4
>>
>>>
>>>>           if (vgdev->has_virgl_3d == false)
>>>>                   return -ENOSYS;
>>>>
>>>> +       if ((exbuf->flags & ~VIRTGPU_EXECBUF_FLAGS))
>>>> +               return -EINVAL;
>>>> +
>>> I assume this did this trigger when using old userspace?
>>
>> No, not as far as I'm aware. This check is there to prevent userspace from
>> polluting the bitspace of flag, so that all free bits can be used for new flags.
>>
>> As far as I understand this is pointed out by a drm driver development document
>> written by danvet, which I unfortunately can't seem to find the link for at the
>> moment.
>>
> Yes that is correct. What I was asking is:
> 
> Does a kernel with this patch, work with mesa lacking the corresponding updates?
> I'd imagine things work just fine.

Yes it does!

> 
> -Emil
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index d01a9ed100d1..1af289b28fc4 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -116,9 +116,14 @@  static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
 	struct ww_acquire_ctx ticket;
 	void *buf;
 
+	exbuf->fence_fd = -1;
+
 	if (vgdev->has_virgl_3d == false)
 		return -ENOSYS;
 
+	if ((exbuf->flags & ~VIRTGPU_EXECBUF_FLAGS))
+		return -EINVAL;
+
 	INIT_LIST_HEAD(&validate_list);
 	if (exbuf->num_bo_handles) {
 
diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
index 9a781f0611df..91062f4ac7c5 100644
--- a/include/uapi/drm/virtgpu_drm.h
+++ b/include/uapi/drm/virtgpu_drm.h
@@ -47,6 +47,13 @@  extern "C" {
 #define DRM_VIRTGPU_WAIT     0x08
 #define DRM_VIRTGPU_GET_CAPS  0x09
 
+#define VIRTGPU_EXECBUF_FENCE_FD_IN	0x01
+#define VIRTGPU_EXECBUF_FENCE_FD_OUT	0x02
+#define VIRTGPU_EXECBUF_FLAGS  (\
+		VIRTGPU_EXECBUF_FENCE_FD_IN |\
+		VIRTGPU_EXECBUF_FENCE_FD_OUT |\
+		0)
+
 struct drm_virtgpu_map {
 	__u64 offset; /* use for mmap system call */
 	__u32 handle;
@@ -54,12 +61,12 @@  struct drm_virtgpu_map {
 };
 
 struct drm_virtgpu_execbuffer {
-	__u32		flags;		/* for future use */
+	__u32 flags;
 	__u32 size;
 	__u64 command; /* void* */
 	__u64 bo_handles;
 	__u32 num_bo_handles;
-	__u32 pad;
+	__s32 fence_fd;
 };
 
 #define VIRTGPU_PARAM_3D_FEATURES 1 /* do we have 3D features in the hw */
@@ -137,7 +144,7 @@  struct drm_virtgpu_get_caps {
 	DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_MAP, struct drm_virtgpu_map)
 
 #define DRM_IOCTL_VIRTGPU_EXECBUFFER \
-	DRM_IOW(DRM_COMMAND_BASE + DRM_VIRTGPU_EXECBUFFER,\
+	DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_EXECBUFFER,\
 		struct drm_virtgpu_execbuffer)
 
 #define DRM_IOCTL_VIRTGPU_GETPARAM \