Virtualization - May 2023 - [PATCH v6 0/3] Add sync object UAPI support to VirtIO-GPU driver

On Mon, May 1, 2023 at 8:38?AM Dmitry Osipenko <
dmitry.osipenko at collabora.com> wrote:
> On 4/16/23 14:52, Dmitry Osipenko wrote:
> > We have multiple Vulkan context types that are awaiting for the
addition
> > of the sync object DRM UAPI support to the VirtIO-GPU kernel driver:
> >
> >  1. Venus context
> >  2. Native contexts (virtio-freedreno, virtio-intel, virtio-amdgpu)
> >
> > Mesa core supports DRM sync object UAPI, providing Vulkan drivers with
a
> > generic fencing implementation that we want to utilize.
> >
> > This patch adds initial sync objects support. It creates fundament for
a
> > further fencing improvements. Later on we will want to extend the
> VirtIO-GPU
> > fencing API with passing fence IDs to host for waiting, it will be a
new
> > additional VirtIO-GPU IOCTL and more. Today we have several VirtIO-GPU
> context
> > drivers in works that require VirtIO-GPU to support sync objects UAPI.
> >
> > The patch is heavily inspired by the sync object UAPI implementation
of
> the
> > MSM driver.
>
> Gerd, do you have any objections to merging this series?
>
> We have AMDGPU [1] and Intel [2] native context WIP drivers depending on
> the sync object support. It is the only part missing from kernel today
> that is wanted by the native context drivers. Otherwise, there are few
> other things in Qemu and virglrenderer left to sort out.
>
> [1] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21658
> [2]
> https://gitlab.freedesktop.org/digetx/mesa/-/commits/native-context-iris

I'm not saying this change isn't good, just it's probably possible
to
implement the native contexts (even up to even VK1.2) without it.  But this
patch series may be the most ergonomic way to do it, given how Mesa is
designed.  But you probably want one of Mesa MRs reviewed first before
merging (I added a comment on the amdgpu change) and that is a requirement
[a].

[a] "The userspace side must be fully reviewed and tested to the standards
of that user space project. For e.g. mesa this means piglit testcases and
review on the mailing list. This is again to ensure that the new interface
actually gets the job done." -- from the requirements

>
>
> --
> Best regards,
> Dmitry
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20230503/9c56b4aa/attachment.html>

On Wed, May 3, 2023 at 10:07?AM Gurchetan Singh
<gurchetansingh at chromium.org> wrote:
>
>
>
> On Mon, May 1, 2023 at 8:38?AM Dmitry Osipenko <dmitry.osipenko at
collabora.com> wrote:
>>
>> On 4/16/23 14:52, Dmitry Osipenko wrote:
>> > We have multiple Vulkan context types that are awaiting for the
addition
>> > of the sync object DRM UAPI support to the VirtIO-GPU kernel
driver:
>> >
>> >  1. Venus context
>> >  2. Native contexts (virtio-freedreno, virtio-intel,
virtio-amdgpu)
>> >
>> > Mesa core supports DRM sync object UAPI, providing Vulkan drivers
with a
>> > generic fencing implementation that we want to utilize.
>> >
>> > This patch adds initial sync objects support. It creates fundament
for a
>> > further fencing improvements. Later on we will want to extend the
VirtIO-GPU
>> > fencing API with passing fence IDs to host for waiting, it will be
a new
>> > additional VirtIO-GPU IOCTL and more. Today we have several
VirtIO-GPU context
>> > drivers in works that require VirtIO-GPU to support sync objects
UAPI.
>> >
>> > The patch is heavily inspired by the sync object UAPI
implementation of the
>> > MSM driver.
>>
>> Gerd, do you have any objections to merging this series?
>>
>> We have AMDGPU [1] and Intel [2] native context WIP drivers depending
on
>> the sync object support. It is the only part missing from kernel today
>> that is wanted by the native context drivers. Otherwise, there are few
>> other things in Qemu and virglrenderer left to sort out.
>>
>> [1] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21658
>> [2]
https://gitlab.freedesktop.org/digetx/mesa/-/commits/native-context-iris
>
>
> I'm not saying this change isn't good, just it's probably
possible to implement the native contexts (even up to even VK1.2) without it. 
But this patch series may be the most ergonomic way to do it, given how Mesa is
designed.  But you probably want one of Mesa MRs reviewed first before merging
(I added a comment on the amdgpu change) and that is a requirement [a].
>
> [a] "The userspace side must be fully reviewed and tested to the
standards of that user space project. For e.g. mesa this means piglit testcases
and review on the mailing list. This is again to ensure that the new interface
actually gets the job done." -- from the requirements
>
tbh, the syncobj support is all drm core, the only driver specifics is
the ioctl parsing.  IMHO existing tests and the two existing consumers
are sufficient.  (Also, considering that additional non-drm
dependencies involved.)

If this was for the core drm syncobj implementation, and not just
driver ioctl parsing and wiring up the core helpers, I would agree
with you.

BR,
-R