* Re: [PATCH] drm/virtio: exbuf->fence_fd unmodified on interrupted wait
2023-01-26 22:58 [PATCH] drm/virtio: exbuf->fence_fd unmodified on interrupted wait Ryan Neph
@ 2023-01-27 15:44 ` Rob Clark
2023-02-01 13:28 ` Dmitry Osipenko
2023-02-02 2:24 ` Dmitry Osipenko
2 siblings, 0 replies; 8+ messages in thread
From: Rob Clark @ 2023-01-27 15:44 UTC (permalink / raw)
To: Ryan Neph
Cc: David Airlie, Gerd Hoffmann, Daniel Vetter, Gurchetan Singh,
Chia-I Wu, dri-devel, virtualization, linux-kernel, Rob Clark,
Robert Foss, Yiwei Zhang, Gustavo Padovan, Emil Velikov
On Fri, Jan 27, 2023 at 12:31 AM Ryan Neph <ryanneph@chromium.org> wrote:
>
> An interrupted dma_fence_wait() becomes an -ERESTARTSYS returned
> to userspace ioctl(DRM_IOCTL_VIRTGPU_EXECBUFFER) calls, prompting to
> retry the ioctl(), but the passed exbuf->fence_fd has been reset to -1,
> making the retry attempt fail at sync_file_get_fence().
>
> The uapi for DRM_IOCTL_VIRTGPU_EXECBUFFER is changed to retain the
> passed value for exbuf->fence_fd when returning ERESTARTSYS or EINTR.
>
> Fixes: 2cd7b6f08bc4 ("drm/virtio: add in/out fence support for explicit synchronization")
> Signed-off-by: Ryan Neph <ryanneph@chromium.org>
Reviewed-by: Rob Clark <robdclark@gmail.com>
> ---
>
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 9 ++++++---
> include/uapi/drm/virtgpu_drm.h | 3 +++
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index 9f4a90493aea..ffce4e2a409a 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -132,6 +132,8 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> uint64_t fence_ctx;
> uint32_t ring_idx;
>
> + exbuf->fence_fd = -1;
> +
> fence_ctx = vgdev->fence_drv.context;
> ring_idx = 0;
>
> @@ -152,8 +154,6 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> ring_idx = exbuf->ring_idx;
> }
>
> - exbuf->fence_fd = -1;
> -
> virtio_gpu_create_context(dev, file);
> if (exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_IN) {
> struct dma_fence *in_fence;
> @@ -173,7 +173,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
>
> dma_fence_put(in_fence);
> if (ret)
> - return ret;
> + goto out_err;
> }
>
> if (exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) {
> @@ -259,6 +259,9 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
>
> if (out_fence_fd >= 0)
> put_unused_fd(out_fence_fd);
> +out_err:
> + if (ret == -EINTR || ret == -ERESTARTSYS)
> + exbuf->fence_fd = in_fence_fd;
>
> return ret;
> }
> diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
> index 0512fde5e697..ac8d1eed12ab 100644
> --- a/include/uapi/drm/virtgpu_drm.h
> +++ b/include/uapi/drm/virtgpu_drm.h
> @@ -64,6 +64,9 @@ struct drm_virtgpu_map {
> __u32 pad;
> };
>
> +/* For ioctl() returning ERESTARTSYS or EINTR, fence_fd is unmodified.
> + * For all other errors it is set to -1.
> + */
> struct drm_virtgpu_execbuffer {
> __u32 flags;
> __u32 size;
> --
> 2.39.1.456.gfc5497dd1b-goog
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm/virtio: exbuf->fence_fd unmodified on interrupted wait
2023-01-26 22:58 [PATCH] drm/virtio: exbuf->fence_fd unmodified on interrupted wait Ryan Neph
2023-01-27 15:44 ` Rob Clark
@ 2023-02-01 13:28 ` Dmitry Osipenko
2023-02-01 15:48 ` Rob Clark
2023-02-02 2:24 ` Dmitry Osipenko
2 siblings, 1 reply; 8+ messages in thread
From: Dmitry Osipenko @ 2023-02-01 13:28 UTC (permalink / raw)
To: Ryan Neph, David Airlie, Gerd Hoffmann, Daniel Vetter,
Gurchetan Singh, Chia-I Wu, dri-devel, virtualization,
linux-kernel
Cc: Rob Clark, Yiwei Zhang, Gustavo Padovan, Emil Velikov
On 1/27/23 01:58, Ryan Neph wrote:
> An interrupted dma_fence_wait() becomes an -ERESTARTSYS returned
> to userspace ioctl(DRM_IOCTL_VIRTGPU_EXECBUFFER) calls, prompting to
> retry the ioctl(), but the passed exbuf->fence_fd has been reset to -1,
> making the retry attempt fail at sync_file_get_fence().
>
> The uapi for DRM_IOCTL_VIRTGPU_EXECBUFFER is changed to retain the
> passed value for exbuf->fence_fd when returning ERESTARTSYS or EINTR.
>
> Fixes: 2cd7b6f08bc4 ("drm/virtio: add in/out fence support for explicit synchronization")
> Signed-off-by: Ryan Neph <ryanneph@chromium.org>
> ---
>
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 9 ++++++---
> include/uapi/drm/virtgpu_drm.h | 3 +++
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index 9f4a90493aea..ffce4e2a409a 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -132,6 +132,8 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> uint64_t fence_ctx;
> uint32_t ring_idx;
>
> + exbuf->fence_fd = -1;
> +
> fence_ctx = vgdev->fence_drv.context;
> ring_idx = 0;
>
> @@ -152,8 +154,6 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> ring_idx = exbuf->ring_idx;
> }
>
> - exbuf->fence_fd = -1;
Is there any userspace relying on this -1 behaviour? Wouldn't be better
to remove this offending assignment?
--
Best regards,
Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm/virtio: exbuf->fence_fd unmodified on interrupted wait
2023-02-01 13:28 ` Dmitry Osipenko
@ 2023-02-01 15:48 ` Rob Clark
2023-02-02 2:17 ` Dmitry Osipenko
0 siblings, 1 reply; 8+ messages in thread
From: Rob Clark @ 2023-02-01 15:48 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Ryan Neph, David Airlie, Gerd Hoffmann, Daniel Vetter,
Gurchetan Singh, Chia-I Wu, dri-devel, virtualization,
linux-kernel, Yiwei Zhang, Gustavo Padovan, Emil Velikov
On Wed, Feb 1, 2023 at 5:28 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 1/27/23 01:58, Ryan Neph wrote:
> > An interrupted dma_fence_wait() becomes an -ERESTARTSYS returned
> > to userspace ioctl(DRM_IOCTL_VIRTGPU_EXECBUFFER) calls, prompting to
> > retry the ioctl(), but the passed exbuf->fence_fd has been reset to -1,
> > making the retry attempt fail at sync_file_get_fence().
> >
> > The uapi for DRM_IOCTL_VIRTGPU_EXECBUFFER is changed to retain the
> > passed value for exbuf->fence_fd when returning ERESTARTSYS or EINTR.
> >
> > Fixes: 2cd7b6f08bc4 ("drm/virtio: add in/out fence support for explicit synchronization")
> > Signed-off-by: Ryan Neph <ryanneph@chromium.org>
> > ---
> >
> > drivers/gpu/drm/virtio/virtgpu_ioctl.c | 9 ++++++---
> > include/uapi/drm/virtgpu_drm.h | 3 +++
> > 2 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > index 9f4a90493aea..ffce4e2a409a 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > @@ -132,6 +132,8 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> > uint64_t fence_ctx;
> > uint32_t ring_idx;
> >
> > + exbuf->fence_fd = -1;
> > +
> > fence_ctx = vgdev->fence_drv.context;
> > ring_idx = 0;
> >
> > @@ -152,8 +154,6 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> > ring_idx = exbuf->ring_idx;
> > }
> >
> > - exbuf->fence_fd = -1;
>
> Is there any userspace relying on this -1 behaviour? Wouldn't be better
> to remove this offending assignment?
Looking at current mesa, removing the assignment should be ok (and
more consistent with other drivers). But I can't say if this was
always true, or that there aren't other non-mesa users, so I can see
the argument for the more conservative uabi change that this patch
went with.
BR,
-R
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm/virtio: exbuf->fence_fd unmodified on interrupted wait
2023-02-01 15:48 ` Rob Clark
@ 2023-02-02 2:17 ` Dmitry Osipenko
2023-02-02 2:24 ` Dmitry Osipenko
0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Osipenko @ 2023-02-02 2:17 UTC (permalink / raw)
To: Rob Clark
Cc: Ryan Neph, David Airlie, Gerd Hoffmann, Daniel Vetter,
Gurchetan Singh, Chia-I Wu, dri-devel, virtualization,
linux-kernel, Yiwei Zhang, Gustavo Padovan, Emil Velikov
On 2/1/23 18:48, Rob Clark wrote:
> On Wed, Feb 1, 2023 at 5:28 AM Dmitry Osipenko
> <dmitry.osipenko@collabora.com> wrote:
>>
>> On 1/27/23 01:58, Ryan Neph wrote:
>>> An interrupted dma_fence_wait() becomes an -ERESTARTSYS returned
>>> to userspace ioctl(DRM_IOCTL_VIRTGPU_EXECBUFFER) calls, prompting to
>>> retry the ioctl(), but the passed exbuf->fence_fd has been reset to -1,
>>> making the retry attempt fail at sync_file_get_fence().
>>>
>>> The uapi for DRM_IOCTL_VIRTGPU_EXECBUFFER is changed to retain the
>>> passed value for exbuf->fence_fd when returning ERESTARTSYS or EINTR.
>>>
>>> Fixes: 2cd7b6f08bc4 ("drm/virtio: add in/out fence support for explicit synchronization")
>>> Signed-off-by: Ryan Neph <ryanneph@chromium.org>
>>> ---
>>>
>>> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 9 ++++++---
>>> include/uapi/drm/virtgpu_drm.h | 3 +++
>>> 2 files changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
>>> index 9f4a90493aea..ffce4e2a409a 100644
>>> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
>>> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
>>> @@ -132,6 +132,8 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
>>> uint64_t fence_ctx;
>>> uint32_t ring_idx;
>>>
>>> + exbuf->fence_fd = -1;
>>> +
>>> fence_ctx = vgdev->fence_drv.context;
>>> ring_idx = 0;
>>>
>>> @@ -152,8 +154,6 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
>>> ring_idx = exbuf->ring_idx;
>>> }
>>>
>>> - exbuf->fence_fd = -1;
>>
>> Is there any userspace relying on this -1 behaviour? Wouldn't be better
>> to remove this offending assignment?
>
> Looking at current mesa, removing the assignment should be ok (and
> more consistent with other drivers). But I can't say if this was
> always true, or that there aren't other non-mesa users, so I can see
> the argument for the more conservative uabi change that this patch
> went with.
Realistically, Mesa is the only user of this IOCTL. In general, in a
such case of doubt, I'll do the UABI change and then wait for complains.
If there is a complaint, then the change is reverted. Also will be good
to know about existence of other users :)
Given that -1 already wasn't consistently set for all error code paths,
it's tempting to see it removed.
The code change of this patch is trivial, hence should fine to keep the
-1 if you prefer that, but the patch won't apply cleanly to the stable
kernels because of the "exbuf->fence_fd = -1" movement. If stable
maintainers won't put effort into rebasing the patch, then better to do
the removal and live with a cleaner driver code, IMO.
--
Best regards,
Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm/virtio: exbuf->fence_fd unmodified on interrupted wait
2023-02-02 2:17 ` Dmitry Osipenko
@ 2023-02-02 2:24 ` Dmitry Osipenko
2023-02-03 19:04 ` Ryan Neph
0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Osipenko @ 2023-02-02 2:24 UTC (permalink / raw)
To: Rob Clark
Cc: Ryan Neph, David Airlie, Gerd Hoffmann, Daniel Vetter,
Gurchetan Singh, Chia-I Wu, dri-devel, virtualization,
linux-kernel, Yiwei Zhang, Gustavo Padovan, Emil Velikov
On 2/2/23 05:17, Dmitry Osipenko wrote:
> On 2/1/23 18:48, Rob Clark wrote:
>> On Wed, Feb 1, 2023 at 5:28 AM Dmitry Osipenko
>> <dmitry.osipenko@collabora.com> wrote:
>>>
>>> On 1/27/23 01:58, Ryan Neph wrote:
>>>> An interrupted dma_fence_wait() becomes an -ERESTARTSYS returned
>>>> to userspace ioctl(DRM_IOCTL_VIRTGPU_EXECBUFFER) calls, prompting to
>>>> retry the ioctl(), but the passed exbuf->fence_fd has been reset to -1,
>>>> making the retry attempt fail at sync_file_get_fence().
>>>>
>>>> The uapi for DRM_IOCTL_VIRTGPU_EXECBUFFER is changed to retain the
>>>> passed value for exbuf->fence_fd when returning ERESTARTSYS or EINTR.
>>>>
>>>> Fixes: 2cd7b6f08bc4 ("drm/virtio: add in/out fence support for explicit synchronization")
>>>> Signed-off-by: Ryan Neph <ryanneph@chromium.org>
>>>> ---
>>>>
>>>> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 9 ++++++---
>>>> include/uapi/drm/virtgpu_drm.h | 3 +++
>>>> 2 files changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
>>>> index 9f4a90493aea..ffce4e2a409a 100644
>>>> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
>>>> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
>>>> @@ -132,6 +132,8 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
>>>> uint64_t fence_ctx;
>>>> uint32_t ring_idx;
>>>>
>>>> + exbuf->fence_fd = -1;
>>>> +
>>>> fence_ctx = vgdev->fence_drv.context;
>>>> ring_idx = 0;
>>>>
>>>> @@ -152,8 +154,6 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
>>>> ring_idx = exbuf->ring_idx;
>>>> }
>>>>
>>>> - exbuf->fence_fd = -1;
>>>
>>> Is there any userspace relying on this -1 behaviour? Wouldn't be better
>>> to remove this offending assignment?
>>
>> Looking at current mesa, removing the assignment should be ok (and
>> more consistent with other drivers). But I can't say if this was
>> always true, or that there aren't other non-mesa users, so I can see
>> the argument for the more conservative uabi change that this patch
>> went with.
>
> Realistically, Mesa is the only user of this IOCTL. In general, in a
> such case of doubt, I'll do the UABI change and then wait for complains.
> If there is a complaint, then the change is reverted. Also will be good
> to know about existence of other users :)
>
> Given that -1 already wasn't consistently set for all error code paths,
> it's tempting to see it removed.
>
> The code change of this patch is trivial, hence should fine to keep the
> -1 if you prefer that, but the patch won't apply cleanly to the stable
> kernels because of the "exbuf->fence_fd = -1" movement. If stable
> maintainers won't put effort into rebasing the patch, then better to do
> the removal and live with a cleaner driver code, IMO.
Although, there will be a merge conflict either way. I'll give the r-b,
still removing -1 feels more attractive to me.
--
Best regards,
Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm/virtio: exbuf->fence_fd unmodified on interrupted wait
2023-02-02 2:24 ` Dmitry Osipenko
@ 2023-02-03 19:04 ` Ryan Neph
0 siblings, 0 replies; 8+ messages in thread
From: Ryan Neph @ 2023-02-03 19:04 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Rob Clark, David Airlie, Gerd Hoffmann, Daniel Vetter,
Gurchetan Singh, Chia-I Wu, dri-devel, virtualization,
linux-kernel, Yiwei Zhang, Gustavo Padovan, Emil Velikov
On Thu, Feb 02, 2023 at 05:24:34AM +0300, Dmitry Osipenko wrote:
> On 2/2/23 05:17, Dmitry Osipenko wrote:
> > On 2/1/23 18:48, Rob Clark wrote:
> >> On Wed, Feb 1, 2023 at 5:28 AM Dmitry Osipenko
> >> <dmitry.osipenko@collabora.com> wrote:
> >>>
> >>> On 1/27/23 01:58, Ryan Neph wrote:
> >>>> An interrupted dma_fence_wait() becomes an -ERESTARTSYS returned
> >>>> to userspace ioctl(DRM_IOCTL_VIRTGPU_EXECBUFFER) calls, prompting to
> >>>> retry the ioctl(), but the passed exbuf->fence_fd has been reset to -1,
> >>>> making the retry attempt fail at sync_file_get_fence().
> >>>>
> >>>> The uapi for DRM_IOCTL_VIRTGPU_EXECBUFFER is changed to retain the
> >>>> passed value for exbuf->fence_fd when returning ERESTARTSYS or EINTR.
> >>>>
> >>>> Fixes: 2cd7b6f08bc4 ("drm/virtio: add in/out fence support for explicit synchronization")
> >>>> Signed-off-by: Ryan Neph <ryanneph@chromium.org>
> >>>> ---
> >>>>
> >>>> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 9 ++++++---
> >>>> include/uapi/drm/virtgpu_drm.h | 3 +++
> >>>> 2 files changed, 9 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> >>>> index 9f4a90493aea..ffce4e2a409a 100644
> >>>> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> >>>> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> >>>> @@ -132,6 +132,8 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> >>>> uint64_t fence_ctx;
> >>>> uint32_t ring_idx;
> >>>>
> >>>> + exbuf->fence_fd = -1;
> >>>> +
> >>>> fence_ctx = vgdev->fence_drv.context;
> >>>> ring_idx = 0;
> >>>>
> >>>> @@ -152,8 +154,6 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> >>>> ring_idx = exbuf->ring_idx;
> >>>> }
> >>>>
> >>>> - exbuf->fence_fd = -1;
> >>>
> >>> Is there any userspace relying on this -1 behaviour? Wouldn't be better
> >>> to remove this offending assignment?
> >>
> >> Looking at current mesa, removing the assignment should be ok (and
> >> more consistent with other drivers). But I can't say if this was
> >> always true, or that there aren't other non-mesa users, so I can see
> >> the argument for the more conservative uabi change that this patch
> >> went with.
> >
> > Realistically, Mesa is the only user of this IOCTL. In general, in a
> > such case of doubt, I'll do the UABI change and then wait for complains.
> > If there is a complaint, then the change is reverted. Also will be good
> > to know about existence of other users :)
> >
> > Given that -1 already wasn't consistently set for all error code paths,
> > it's tempting to see it removed.
> >
> > The code change of this patch is trivial, hence should fine to keep the
> > -1 if you prefer that, but the patch won't apply cleanly to the stable
> > kernels because of the "exbuf->fence_fd = -1" movement. If stable
> > maintainers won't put effort into rebasing the patch, then better to do
> > the removal and live with a cleaner driver code, IMO.
>
> Although, there will be a merge conflict either way. I'll give the r-b,
> still removing -1 feels more attractive to me.
I'm not opposed to removing the "exbuf->fence_fd = -1" on error. I made the
v1 patch with extra care to fix the known issue while minimizing the uabi
change, but I'd prefer to see it changed too; thanks for the comments.
I'll follow up with a v2 that removes the modification of "exbuf->fence_fd"
unless the IOCTL succeeds.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/virtio: exbuf->fence_fd unmodified on interrupted wait
2023-01-26 22:58 [PATCH] drm/virtio: exbuf->fence_fd unmodified on interrupted wait Ryan Neph
2023-01-27 15:44 ` Rob Clark
2023-02-01 13:28 ` Dmitry Osipenko
@ 2023-02-02 2:24 ` Dmitry Osipenko
2 siblings, 0 replies; 8+ messages in thread
From: Dmitry Osipenko @ 2023-02-02 2:24 UTC (permalink / raw)
To: Ryan Neph, David Airlie, Gerd Hoffmann, Daniel Vetter,
Gurchetan Singh, Chia-I Wu, dri-devel, virtualization,
linux-kernel
Cc: Rob Clark, Yiwei Zhang, Gustavo Padovan, Emil Velikov
On 1/27/23 01:58, Ryan Neph wrote:
> An interrupted dma_fence_wait() becomes an -ERESTARTSYS returned
> to userspace ioctl(DRM_IOCTL_VIRTGPU_EXECBUFFER) calls, prompting to
> retry the ioctl(), but the passed exbuf->fence_fd has been reset to -1,
> making the retry attempt fail at sync_file_get_fence().
>
> The uapi for DRM_IOCTL_VIRTGPU_EXECBUFFER is changed to retain the
> passed value for exbuf->fence_fd when returning ERESTARTSYS or EINTR.
>
> Fixes: 2cd7b6f08bc4 ("drm/virtio: add in/out fence support for explicit synchronization")
> Signed-off-by: Ryan Neph <ryanneph@chromium.org>
> ---
>
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 9 ++++++---
> include/uapi/drm/virtgpu_drm.h | 3 +++
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index 9f4a90493aea..ffce4e2a409a 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -132,6 +132,8 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> uint64_t fence_ctx;
> uint32_t ring_idx;
>
> + exbuf->fence_fd = -1;
> +
> fence_ctx = vgdev->fence_drv.context;
> ring_idx = 0;
>
> @@ -152,8 +154,6 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> ring_idx = exbuf->ring_idx;
> }
>
> - exbuf->fence_fd = -1;
> -
> virtio_gpu_create_context(dev, file);
> if (exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_IN) {
> struct dma_fence *in_fence;
> @@ -173,7 +173,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
>
> dma_fence_put(in_fence);
> if (ret)
> - return ret;
> + goto out_err;
> }
>
> if (exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) {
> @@ -259,6 +259,9 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
>
> if (out_fence_fd >= 0)
> put_unused_fd(out_fence_fd);
> +out_err:
> + if (ret == -EINTR || ret == -ERESTARTSYS)
> + exbuf->fence_fd = in_fence_fd;
>
> return ret;
> }
> diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
> index 0512fde5e697..ac8d1eed12ab 100644
> --- a/include/uapi/drm/virtgpu_drm.h
> +++ b/include/uapi/drm/virtgpu_drm.h
> @@ -64,6 +64,9 @@ struct drm_virtgpu_map {
> __u32 pad;
> };
>
> +/* For ioctl() returning ERESTARTSYS or EINTR, fence_fd is unmodified.
> + * For all other errors it is set to -1.
> + */
> struct drm_virtgpu_execbuffer {
> __u32 flags;
> __u32 size;
Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
--
Best regards,
Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread