Re: [PATCH v11 6/9] gfxstream + rutabaga: add initial support for gfxstream

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 19/09/2023 20.36, Bernhard Beschow wrote:
>> Am 15. September 2023 02:38:02 UTC schrieb Gurchetan Singh 
>> <gurchetansingh@chromium.org>:
>>> On Thu, Sep 14, 2023 at 12:23 AM Bernhard Beschow <shentey@gmail.com> wrote:
>>>
>>>>
>>>>
>>>> Am 14. September 2023 04:38:51 UTC schrieb Gurchetan Singh 
>>>> <gurchetansingh@chromium.org>:
>>>>> On Wed, Sep 13, 2023 at 4:58 AM Bernhard Beschow <shentey@gmail.com> 
>>>>> wrote:

[...]

>>>>>> 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.
>
> Sorry for the late reply ... but I'm also not really an expert here. But I 
> think the colon string (1) looks like a less common way for handling settings 
> in QEMU. The option (2) looks preferable to me.

For CLI option arguments, we definitely want (2) and not (1), and we
definitely want to use an existing parser.

For complex configuration, I'd recommend a QAPI-based approach.

To provide more specific advice, I'd have to understand the structure of
your intended configuration first.