Re: [PATCH 2/5] drm/virtio: add uapi for in and out explicit fences

[Robert Foss <robert.foss@collabora.com>]

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
>