Re: [PATCH v3 0/9] virtio-gpu: Support Venus Vulkan driver

[Gurchetan Singh]

On Tue, Jan 31, 2023 at 3:15 PM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> Hello,
>
> On 1/30/23 20:00, Alex Bennée wrote:
> >
> > Antonio Caggiano <antonio.caggiano@collabora.com> writes:
> >
> >> This series of patches enables support for the Venus VirtIO-GPU Vulkan
> >> driver by adding some features required by the driver:
> >>
> >> - CONTEXT_INIT
> >> - HOSTMEM
> >> - RESOURCE_UUID
> >> - BLOB_RESOURCES
> >>
> >> In addition to these features, Venus capset support was required
> >> together with the implementation for Virgl blob resource commands.
> >
> > I managed to apply to current master but I needed a bunch of patches to
> > get it to compile with my old virgl:
>
> Thank you for reviewing and testing the patches! Antonio isn't working
> on Venus anymore, I'm going to continue this effort. Last year we
> stabilized some of the virglrenderer Venus APIs, this year Venus may
> transition to supporting per-context fences only and require to init a
> renderserver, which will result in a more changes to Qemu. I'm going to
> wait a bit for Venus to settle down and then make a v4.
>
> In the end we will either need to add more #ifdefs if we will want to
> keep supporting older virglrenderer versions in Qemu, or bump the min
> required virglrenderer version.

Hi Dmitry,

Thanks for working on this, it's great to see QEMU graphics moving
forward.  I noticed a few things from your patchset:

1)  Older versions of virglrenderer -- supported or not?

As you alluded to, there have been significant changes to
virglrenderer since the last QEMU graphics update.  For example, the
asynchronous callback introduces an entirely different and
incompatible way to signal fence completion.

Notionally, QEMU must support older versions of virglrenderer, though
in practice I'm not sure how much that is true.  If we want to keep up
the notion that older versions must be supported, you'll need:

a) virtio-gpu-virgl.c
b) virtio-gpu-virgl2.c (or an equivalent)

Similarly for the vhost-user paths (if you want to support that).  If
older versions of virglrenderer don't need to be supported, then that
would simplify the amount of additional paths/#ifdefs.

2) Additional context type: gfxstream [i]?

One of the major motivations for adding context types in the
virtio-gpu spec was supporting gfxstream.  gfxstream is used in the
Android Studio emulator (a variant of QEMU) [ii], among other places.
That would move the Android emulator closer to the goal of using
upstream QEMU for everything.

If (1) is resolved, I don't think it's actually too bad to add
gfxstream support.  We just need an additional layer of dispatch
between virglrenderer and gfxstream (thus, virtio-gpu-virgl2.c would
be renamed virtio-gpu-context-types.c or something similar).  The QEMU
command line will have to be modified to pass in the enabled context
type (--context={virgl, venus, gfxstream}).  crosvm has been using the
same trick.

If (1) is resolved in v4, I would estimate adding gfxstream support
would at max take 1-2 months for a single engineer.  I'm not saying
gfxstream need necessarily be a part of a v5 patch-stack, but given
this patch-stack has been around for 1 year plus, it certainly could
be.  We can certainly design things in such a way that adding
gfxstream is easy subsequently.

The hardest part is actually package management (Debian) for
gfxstream, but those can be resolved.

I'm not sure exactly how QEMU accelerated graphics are utilized in
user-facing actual products currently, so not sure what the standard
is.

What do QEMU maintainers and users think about these issues,
particularly about the potential gfxstream addition in QEMU as a
context type?  We are most interested in Android guests.

[i] https://android.googlesource.com/device/generic/vulkan-cereal/
[ii] https://developer.android.com/studio/run/emulator

>
> --
> Best regards,
> Dmitry
>
>