[PATCH v3] drm/virtio: conditionally allocate virtio_gpu_fence

[Gurchetan Singh]

On Fri, Jul 7, 2023 at 10:35 AM Dmitry Osipenko
<dmitry.osipenko at collabora.com> wrote:
>
> On 7/7/23 20:04, Dmitry Osipenko wrote:
> > On 7/7/23 18:43, Gurchetan Singh wrote:
> >> @@ -161,21 +157,27 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
> >>                                struct drm_file *file,
> >>                                u64 fence_ctx, u32 ring_idx)
> >>  {
> >> +    int err;
> >> +    struct virtio_gpu_fence *out_fence;
> >>      struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
> >>      struct virtio_gpu_device *vgdev = dev->dev_private;
> >> -    struct virtio_gpu_fence *out_fence;
> >> -    int err;
> >> +    bool drm_fence_event = (exbuf->flags & VIRTGPU_EXECBUF_RING_IDX) &&
> >> +                           (vfpriv->ring_idx_mask & BIT_ULL(ring_idx));
> >
> > Previously, when VIRTGPU_EXECBUF_RING_IDX flag wasn't specified, the
> > fence event was created for a default ring_idx=0. Now you changed this
> > behaviour and event will never be created without
> > VIRTGPU_EXECBUF_RING_IDX flag being set.

ring_idx = 0 is fine, but without VIRTGPU_EXECBUF_RING_IDX that means
the global timeline.

It's an additional check for where the userspace specifies they want
to use per-context fencing and polling, but actually uses the global
timeline.  Userspace never does this since it wouldn't work, so it's a
bit of a pathological edge case check than any UAPI change.

> >
> > Could you please point me at the corresponding userspace code that polls
> > DRM FD fence event?

https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/vm_tools/sommelier/virtualization/virtgpu_channel.cc#216

Used with the following flow:

https://crosvm.dev/book/devices/wayland.html

If you wish to test, please do apply this change:

https://chromium-review.googlesource.com/c/chromiumos/platform2/+/4605854

> >
> > It's unclear whether there is a possible userspace regression here or
> > not. If there is no regression, then in general such behavioural changes
> > should be done in a separate commit having detailed description
> > explaining why behaviour changes.

Sommelier isn't formally packaged yet in the Linux distro style and it
always specifies RING_IDX when polling, so no regressions here.  Maybe
a separate commit is overkill (since the 2nd commit would delete the
newly added checks), what about just more detail in the commit
message?

>
> I see that venus does the polling and ring_idx_mask is a
> context-specific param, hence it's irrelevant to a generic ctx 0. Still
> it's now necessary to specify the EXECBUF_RING_IDX flag even if ctx has
> one ring, which is UAPI change.

It doesn't seem like venus enables POLL_RINGS_MASK to poll since that
param is zero?

https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/virtio/vulkan/vn_renderer_virtgpu.c#L617



>
> --
> Best regards,
> Dmitry
>