On 7/7/23 20:59, Gurchetan Singh wrote: /// >>> 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 Thanks for the links! I tested v2 with sommelier, though wasn't aware about CL4605854 and alternatives to sommelier >>> 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? More detail will be fine >> 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 Indeed, good catch -- Best regards, Dmitry
