On Mon, May 8, 2023 at 6:59 AM Rob Clark <robdclark@xxxxxxxxx> wrote:
On Wed, May 3, 2023 at 10:07 AM Gurchetan Singh
<gurchetansingh@xxxxxxxxxxxx> wrote:
>
>
>
> On Mon, May 1, 2023 at 8:38 AM Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx> 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.)
Can we get one of the Mesa MRs reviewed first? There's currently no virtio-intel MR AFAICT, and the amdgpu one is marked as "Draft:".
Even for the amdgpu, Pierre suggests the feature "will be marked as experimental both in Mesa and virglrenderer" and we can revise as needed. The DRM requirements seem to warn against adding an UAPI too hastily...
You can get the deqp-vk 1.2 tests to pass with the current UAPI, if you just change your mesa <--> virglrenderer protocol a little. Perhaps that way is even better, since you plumb the in sync-obj into host-side command submission.
Without inter-context sharing of the fence, this MR really only adds guest kernel syntactic sugar.
Note I'm not against syntactic sugar, but I just want to point out that you can likely merge the native context work without any UAPI changes, in case it's not clear.
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.
There are several possible and viable paths to get the features in question (VK1.2 syncobjs, and inter-context fence sharing). There are paths entirely without the syncobj, paths that only use the syncobj for the inter-context fence sharing case and create host syncobjs for VK1.2, paths that also use guest syncobjs in every proxied command submission.
It's really hard to tell which one is better. Here's my suggestion:
1) Get the native contexts reviewed/merged in Mesa/virglrenderer using the current UAPI. Options for VK1.2 include: pushing down the syncobjs to the host, and simulating the syncobj (as already done). It's fine to mark these contexts as "experimental" like msm-experimental. That will allow you to experiment with the protocols, come up with tests, and hopefully determine an answer to the host versus guest syncobj question.
2) Once you've completed (1), try to add UAPI changes for features that are missing or things that are suboptimal with the knowledge gained from doing (2).
WDYT?
BR,
-R
