First, sorry for responding after such a long time. I've been busy
with
work and I'm doing QEMU in my free time.
In iteration 1 I've raised the topic on capset_names [1] and I
haven't
seen it answered properly. Perhaps I need to rephrase a bit so here
we
go:
capset_names seems to be colon-separated list of bit options managed
by
rutabaga. This introduces yet another way of options handling. There
have
been talks about harmonizing options handling in QEMU since
apparently
it
is considered too complex [2,3].
Why not pass the "capset" as a bitfield like capset_mask and have
QEMU
create "capset" from QOM properties?
IIUC these flags could come from virtio_gpu.h which is already
present in
the QEMU tree. This would not inly shortcut the dependency on
rutabaga
here
but would also be more idiomatic QEMU (since it makes the options
more
introspectable by internal machinery).
Of course the bitfield approach would require modifications in QEMU
whenever rutabaga gains new features. However, I figure that in the
long
term rutabaga will be quite feature complete such that the benefits
of
idiomatic QEMU handling will outweigh the decoupling of the projects.
What do you think?
I think what you're suggesting is something like -device
virtio-gpu-rutabaga,capset_mask=0x10100 [40, which would be
gfxstream_vulkan + cross_domain]?
I was thinking more along the lines of
`virtio-gpu-rutabaga,gfxstream_vulkan=on,cross_domain=on` where
gfxstream_vulkan and cross_domain are boolean QOM properties. This
would
make for a human-readable format which follows QEMU style.
We actually did consider something like that when adding the
--context-types flag [with crosvm], but there was a desire for a
human-readable format rather than numbers [even if they are in the
virtio-gpu spec].
Additionally, there are quite a few context types that people are
playing
around with [gfxstream-gles, gfxstream-composer] that are launchable
and
aren't in the spec just yet.
Right, QEMU had to be modified for this kind of experimentation. I
considered this in my last paragraph and figured that in the long run
QEMU
*may* prefer more idiomatic option handling since it tries hard to not
break its command line interface. I'm just pointing this out -- the
decision is ultimately up to the community.
Why not have dedicated QEMU development branches for experimentation?
Wouldn't upstreaming new features into QEMU be a good motivation to
get the
missing pieces into the spec, once they are mature?
Also, a key feature we want to explicitly **not** turn on all
available
context-types and let the user decide.
How would you prevent that with the current colon-separated approach?
Splitting capset_mask in multiple parameters is just a different
syntactical representation of the same thing.
That'll allow guest Mesa in
particular to do its magic in its loader. So one may run Zink + ANV
with
ioctl forwarding, or Iris + ioctl forwarding and compare performance
with
the same guest image.
And another thing is one needs some knowledge of the host system to
choose
the right context type. You wouldn't do Zink + ANV ioctl forwarding
on
MacOS. So I think the task of choosing the right context type will
fall
to
projects that depend on QEMU (such as Android Emulator) which have
some
knowledge of the host environment.
We actually have a graphics detector somewhere that calls VK/OpenGL
before
launching the VM and sets the right options. Plan is to port into
gfxstream, maybe we could use that.
You could bail out in QEMU if rutabaga_calculate_capset_mask() detects
conflicting combinations, no?
So given the desire for human readable formats, being portable across
VMMs
(crosvm, qemu, rust-vmm??) and experimentation, the string -> capset
mask
conversion was put in the rutabaga API. So I wouldn't change it for
those
reasons.
What do you mean by being portable across VMMs?
Having the API inside rutabaga is (mildly) useful when multiple VMMs
have
the need to translate from a human-readable format to flags digestible
by
rutabaga.
https://android.googlesource.com/device/google/cuttlefish/+/refs/heads/main/host/libs/vm_manager/qemu_manager.cpp#452
https://android.googlesource.com/device/google/cuttlefish/+/refs/heads/main/host/libs/vm_manager/crosvm_manager.cpp#353
https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/vm_tools/concierge/vm_builder.cc#505
For these crosvm/qemu launchers, I imagine capset names will be plumbed
all
the way through eventually (launch_cvd
--gpu_context=gfxstream-vulkan:cross-domain if you've played around with
Cuttlefish, or vmc start --gpu_contexts=gfxstream-vulkan if you played
around with Termina VMs).
I think rust-vmm could also use the same API ("--capset_names") too.
Sure, QEMU had to be taught new flags before being able to use new
rutabaga features. I agree that this comes with a certain
inconvenience.
But it may also be inconvenient for QEMU to deal with additional ad-hoc
options parsing when there are efforts for harmonization.
Did my comments shed new light into the discussion?
Yes, they do. I agree with you that both crosvm/qemu have too many
flags,
and having a stable command line interface is important. We are aiming
for
stability with the `--capset_names={colon string}` command line, and at
least for crosvm looking to deprecate older options [since we've never
had
an official release of crosvm yet].
I do think:
1) "capset_names=gfxstream-vulkan:cross-domain"
2) "cross-domain=on,gfxstream-vulkan=on"
are similar enough. I would choose (1) for since I think not
duplicating
the [name] -> flag logic and having a similar interface across VMMs +
VMM
launchers is ever-so slightly useful.
I think we've now reached a good understanding of the issue. It's now up
to the QEMU community to make a choice. So I'm cc'ing Markus and Thomas as
the experts of the topic.
As a virtio-gpu user, I'm slightly inclined to (2) since it would be
easier to implement the same option for virtio-gpu-virgl when a need
arises.
The rutabaga/virgl implementations will likely be done via DEFINE_PROP_BIT,
no? For virgl, it'll set the virgl flags, and for rutabaga, it'll set the
capset mask. So it would be different.
That said, the change isn't too bad to make. Here's the key part:
+++ b/hw/display/virtio-gpu-rutabaga.c
@@ -1084,6 +1084,14 @@ static Property virtio_gpu_rutabaga_properties[] = {
DEFINE_PROP_STRING("wayland_socket_path", VirtIOGPURutabaga,
wayland_socket_path),
DEFINE_PROP_STRING("wsi", VirtIOGPURutabaga, wsi),
+ DEFINE_PROP_BIT("gfxstream-vulkan-experimental", VirtIOGPURutabaga,
+ capset_mask, RUTABAGA_CAPSET_GFXSTREAM_VULKAN, false),
+ DEFINE_PROP_BIT("cross-domain-experimental", VirtIOGPURutabaga,
+ capset_mask, RUTABAGA_CAPSET_CROSS_DOMAIN, false),
+ DEFINE_PROP_BIT("gfxstream-gles-experimental", VirtIOGPURutabaga,
+ capset_mask, RUTABAGA_CAPSET_GFXSTREAM_GLES, false),
+ DEFINE_PROP_BIT("gfxstream-composer-experimental", VirtIOGPURutabaga,
+ capset_mask, RUTABAGA_CAPSET_GFXSTREAM_COMPOSER,
false),
DEFINE_PROP_END_OF_LIST(),
};