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

On Thu, May 11, 2023 at 7:33?PM Dmitry Osipenko <
dmitry.osipenko at collabora.com> wrote:
> On 5/12/23 03:17, Gurchetan Singh wrote:
> ...
> > 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?
>
> Having syncobj support available by DRM driver is a mandatory
> requirement for native contexts because userspace (Mesa) relies on sync
> objects support presence. In particular, Intel Mesa driver checks
> whether DRM driver supports sync objects to decide which features are
> available, ANV depends on the syncobj support.
> I'm not familiar with a history of Venus and its limitations. Perhaps
> the reason it's using host-side syncobjs is to have 1:1 Vulkan API
> mapping between guest and host. Not sure if Venus could use guest
> syncobjs instead or there are problems with that.
>
Why not submit a Venus MR?  It's already in-tree, and you can see how your
API works in scenarios with a host side timeline semaphore (aka syncobj).
I think they are also interested in fencing/sync improvements.

>
> When syncobj was initially added to kernel, it was done from the needs
> of supporting Vulkan wait API. For Venus the actual Vulkan driver is on
> host side, while for native contexts it's on guest side. Native
contexts
> don't need syncobj on host side, it will be unnecessary overhead for
> every nctx to have it on host. Hence, if there is no good reason for
> host-side syncobjs, then why do that?

Depends on your threading model.  You can have the following scenarios:

1) N guest contexts : 1 host thread
2) N guest contexts : N host threads for each context
3) 1:1 thread

I think the native context is single-threaded (1), IIRC?  If the goal is to
push command submission to the host (for inter-context sharing), I think
you'll at-least want (2).  For a 1:1 model (a la gfxstream), one host
thread can put another thread's out_sync_objs as it's in_sync_objs (in
the
same virtgpu context).  I think that's kind of the goal of timeline
semaphores, with the example given by Khronos as with a compute thread + a
graphics thread.

I'm not saying one threading model is better than any other, perhaps the
native context using the host driver in the guest is so good, it doesn't
matter.  I'm just saying these are the types of discussions we can have if
we tried to get one the Mesa MRs merged first ;-)

> Native contexts pass deqp synchronization tests, they use sync objects
> universally for both GL and VK. Games work, piglit/deqp passing. What
> else you're wanting to test? Turnip?
>
Turnip would also fulfill the requirements, since most of the native
context stuff is already wired for freedreno.

>
> The AMDGPU code has been looked and it looks good. It's a draft for now
> because of the missing sync objects UAPI and other virglrender/Qemu
> changes required to get KMS working.

Get it out of draft mode then :-).  How long would that take?

Also, there's crosvm which builds on standard Linux, so I wouldn't
consider
QEMU patches as a requirement.  Just Mesa/virglrenderer part.

> Maybe it will be acceptable to
> merge the Mesa part once kernel will get sync objects supported, will
> need to revisit it.
>
You can think of my commentary as the following suggestions:

- You can probably get native contexts and deqp-vk 1.2 working with the
current UAPI
- It would be terrific to see inter-context fence sharing working (with the
wait pushed down to the host), that's something the current UAPI can't
do
- Work iteratively (i.e, it's fine to merge Mesa/virglrenderer MRs as
"experimental") and in steps, no need to figure everything out at once

Now these are just suggestions, and while I think they are good, you can
safely ignore them.

But there's also the DRM requirements, which state "userspace side must
be
fully reviewed and tested to the standards of that user-space project.".
So I think to meet the minimum requirements, I think we should at-least
have one of the following (not all, just one) reviewed:

1) venus using the new uapi
2) gfxstream vk using the new uapi
3) amdgpu nctx out of "draft" mode and using the new uapi.
4) virtio-intel using new uapi
5) turnip using your new uapi

Depending on which one you chose, maybe we can get it done within 1-2 weeks?

I'm not opening MR for virtio-intel because it has open questions
that
> need to be resolved first.
>
> --
> Best regards,
> Dmitry
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20230512/aa249b5b/attachment-0001.html>

On Fri, May 12, 2023 at 2:23?PM Gurchetan Singh
<gurchetansingh at chromium.org> wrote:
>
>
>
> On Thu, May 11, 2023 at 7:33?PM Dmitry Osipenko <dmitry.osipenko at
collabora.com> wrote:
>>
>> On 5/12/23 03:17, Gurchetan Singh wrote:
>> ...
>> > 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?
>>
>> Having syncobj support available by DRM driver is a mandatory
>> requirement for native contexts because userspace (Mesa) relies on sync
>> objects support presence. In particular, Intel Mesa driver checks
>> whether DRM driver supports sync objects to decide which features are
>> available, ANV depends on the syncobj support.
>>
>>
>> I'm not familiar with a history of Venus and its limitations.
Perhaps
>> the reason it's using host-side syncobjs is to have 1:1 Vulkan API
>> mapping between guest and host. Not sure if Venus could use guest
>> syncobjs instead or there are problems with that.
>
>
> Why not submit a Venus MR?  It's already in-tree, and you can see how
your API works in scenarios with a host side timeline semaphore (aka syncobj). 
I think they are also interested in fencing/sync improvements.
>
>>
>>
>> When syncobj was initially added to kernel, it was done from the needs
>> of supporting Vulkan wait API. For Venus the actual Vulkan driver is on
>> host side, while for native contexts it's on guest side. Native
contexts
>> don't need syncobj on host side, it will be unnecessary overhead
for
>> every nctx to have it on host. Hence, if there is no good reason for
>> host-side syncobjs, then why do that?
>
>
> Depends on your threading model.  You can have the following scenarios:
>
> 1) N guest contexts : 1 host thread
> 2) N guest contexts : N host threads for each context
> 3) 1:1 thread
>
> I think the native context is single-threaded (1), IIRC?  If the goal is to
push command submission to the host (for inter-context sharing), I think
you'll at-least want (2).  For a 1:1 model (a la gfxstream), one host thread
can put another thread's out_sync_objs as it's in_sync_objs (in the same
virtgpu context).  I think that's kind of the goal of timeline semaphores,
with the example given by Khronos as with a compute thread + a graphics thread.
>
> I'm not saying one threading model is better than any other, perhaps
the native context using the host driver in the guest is so good, it doesn't
matter.  I'm just saying these are the types of discussions we can have if
we tried to get one the Mesa MRs merged first ;-)
>
>>
>> Native contexts pass deqp synchronization tests, they use sync objects
>> universally for both GL and VK. Games work, piglit/deqp passing. What
>> else you're wanting to test? Turnip?
>
>
> Turnip would also fulfill the requirements, since most of the native
context stuff is already wired for freedreno.
>
>>
>>
>> The AMDGPU code has been looked and it looks good. It's a draft for
now
>> because of the missing sync objects UAPI and other virglrender/Qemu
>> changes required to get KMS working.
>
>
> Get it out of draft mode then :-).  How long would that take?
>
> Also, there's crosvm which builds on standard Linux, so I wouldn't
consider QEMU patches as a requirement.  Just Mesa/virglrenderer part.
>
>>
>> Maybe it will be acceptable to
>> merge the Mesa part once kernel will get sync objects supported, will
>> need to revisit it.
>
>
> You can think of my commentary as the following suggestions:
>
> - You can probably get native contexts and deqp-vk 1.2 working with the
current UAPI
> - It would be terrific to see inter-context fence sharing working (with the
wait pushed down to the host), that's something the current UAPI can't
do
> - Work iteratively (i.e, it's fine to merge Mesa/virglrenderer MRs as
"experimental") and in steps, no need to figure everything out at once
>
> Now these are just suggestions, and while I think they are good, you can
safely ignore them.
>
> But there's also the DRM requirements, which state "userspace side
must be fully reviewed and tested to the standards of that user-space
project.".  So I think to meet the minimum requirements, I think we should
at-least have one of the following (not all, just one) reviewed:
>
> 1) venus using the new uapi
> 2) gfxstream vk using the new uapi
> 3) amdgpu nctx out of "draft" mode and using the new uapi.
> 4) virtio-intel using new uapi
> 5) turnip using your new uapi
forgot to mention this earlier, but
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/23533

Dmitry, you can also add, if you haven't already:

Tested-by: Rob Clark <robdclark at gmail.com>
> Depending on which one you chose, maybe we can get it done within 1-2
weeks?
>
>> I'm not opening MR for virtio-intel because it has open questions
that
>> need to be resolved first.
>>
>> --
>> Best regards,
>> Dmitry
>>