On Fri, Jul 7, 2023 at 10:35 AM Dmitry Osipenko
<dmitry.osipenko@xxxxxxxxxxxxx> 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
>
