[PATCH v3] drm/virtio: conditionally allocate virtio_gpu_fence

[Dmitry Osipenko]

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