| Message ID | 20181025183739.9375-3-robert.foss@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virgl: fence fd support | expand |
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
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
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
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
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
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 >
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
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 >
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 \