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

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 29/09/2023 17.06, Bernhard Beschow wrote:
>> Am 21. September 2023 23:44:42 UTC schrieb Gurchetan Singh 
>> <gurchetansingh@chromium.org>:
>>> On Tue, Sep 19, 2023 at 3:07 PM Akihiko Odaki <akihiko.odaki@gmail.com>
>>> wrote:
>>>
>>>> On 2023/09/20 3:36, Bernhard Beschow wrote:
>>>>>
>>>>>
>>>>> Am 15. September 2023 02:38:02 UTC schrieb Gurchetan Singh 
>>>>> <gurchetansingh@chromium.org>:

[...]

>>>>>> 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(),
>>> };
>>
>> Nice!
>>
>> I think the current approach for experimental and deprecated properties is 
>> to not pre- or postfix them but issue a warning at runtime when used, see 
>> e.g. here: 
>> https://lore.kernel.org/qemu-devel/20230710121543.197250-18-thuth@redhat.com/
>>  That way, the command line interface won't change once the properties 
>> become stable. So if you omit the "-experimental" postfixes Android Studio 
>> wouldn't need to adapt.
>
> That's for deprecated options only. For experimental new properties, please 
> use the "x-" prefix instead of the "-experimental" suffix.

Thomas is right.

In the QAPI schema, we have the means to avoid renaming on transition
from unstable to stable.  However, device properties remain outside
QAPI.

In case you're curious, docs/devel/qapi-code-gen.rst:

    Special features
    ~~~~~~~~~~~~~~~~

    Feature "deprecated" marks a command, event, enum value, or struct
    member as deprecated.  It is not supported elsewhere so far.
    Interfaces so marked may be withdrawn in future releases in accordance
    with QEMU's deprecation policy.

    Feature "unstable" marks a command, event, enum value, or struct
    member as unstable.  It is not supported elsewhere so far.  Interfaces
    so marked may be withdrawn or changed incompatibly in future releases.

You can even configure policy for handling use of deprecated or unstable
stuff:

    -compat 
[deprecated-input=accept|reject|crash][,deprecated-output=accept|hide]
                    Policy for handling deprecated management interfaces
    -compat [unstable-input=accept|reject|crash][,unstable-output=accept|hide]
                    Policy for handling unstable management interfaces

For more, see qapi/compat.json and commit 6dd75472d5^..dbb675c19a.