[libvirt] [PATCH v2 4/5] qemu: Add QEMU_CAPS_SDL_GL to qemu capabilities

[John Ferlan]

On 05/11/2018 07:47 AM, Martin Kletzander wrote:
> On Fri, May 11, 2018 at 06:34:54AM -0400, John Ferlan wrote:
>> On 05/11/2018 04:26 AM, Martin Kletzander wrote:
>>> On Thu, May 10, 2018 at 04:17:52PM -0400, John Ferlan wrote:
>>>>
>>>>
>>>> On 05/10/2018 06:53 AM, Maciej Wolny wrote:
>>>>> Support OpenGL acceleration capability when using SDL graphics.
>>>>>
>>>>> Signed-off-by: Maciej Wolny <maciej.wolny at codethink.co.uk>
>>>>> ---
>>>>>  src/qemu/qemu_capabilities.c                         | 2 ++
>>>>>  src/qemu/qemu_capabilities.h                         | 1 +
>>>>>  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.replies | 9 +++++++++
>>>>>  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml     | 3 ++-
>>>>>  4 files changed, 14 insertions(+), 1 deletion(-)
>>>>>
>>>>
>>>> As I rather lengthily noted in the v1 - I'm assuming you handed
>>>> edited the .replies file.  What that perhaps works and gets you
>>>> the answer, the fact that none of other files were adjusted leads
>>>> me to the hand editing belief.
>>>>
>>>>> diff --git a/src/qemu/qemu_capabilities.c
>>>>> b/src/qemu/qemu_capabilities.c
>>>>> index 920a59617..23f917b66 100644
>>>>> --- a/src/qemu/qemu_capabilities.c
>>>>> +++ b/src/qemu/qemu_capabilities.c
>>>>> @@ -475,6 +475,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>>>>>                "disk-write-cache",
>>>>>                "nbd-tls",
>>>>>                "tpm-crb",
>>>>> +              "sdl-gl",
>>>>>      );
>>>>>
>>>>>
>>>>> @@ -2456,6 +2457,7 @@ static struct virQEMUCapsCommandLineProps
>>>>> virQEMUCapsCommandLine[] = {
>>>>>      { "vnc", "vnc", QEMU_CAPS_VNC_MULTI_SERVERS },
>>>>>      { "chardev", "reconnect", QEMU_CAPS_CHARDEV_RECONNECT },
>>>>>      { "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_BLACKLIST },
>>>>> +    { "sdl", "gl", QEMU_CAPS_SDL_GL },
>>>>>  };
>>>>
>>>> Rather than this, I'll apply the following diff:
>>>>
>>>> diff --git a/src/qemu/qemu_capabilities.c
>>>> b/src/qemu/qemu_capabilities.c
>>>> index 23f917b66e..28079fa7ab 100644
>>>> --- a/src/qemu/qemu_capabilities.c
>>>> +++ b/src/qemu/qemu_capabilities.c
>>>> @@ -2457,7 +2457,6 @@ static struct virQEMUCapsCommandLineProps
>>>> virQEMUCapsCommandLine[] = {
>>>>     { "vnc", "vnc", QEMU_CAPS_VNC_MULTI_SERVERS },
>>>>     { "chardev", "reconnect", QEMU_CAPS_CHARDEV_RECONNECT },
>>>>     { "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_BLACKLIST },
>>>> -    { "sdl", "gl", QEMU_CAPS_SDL_GL },
>>>> };
>>>>
>>>> static int
>>>> @@ -3828,6 +3827,10 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr
>>>> qemuCaps,
>>>>     if (qemuCaps->version >= 2004000)
>>>>         virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_SMM_OPT);
>>>>
>>>> +    /* sdl -gl option is supported from v2.4.0 (qemu commit id
>>>> 0b71a5d5) */
>>>> +    if (qemuCaps->version >= 2004000)
>>>> +        virQEMUCapsSet(qemuCaps, QEMU_CAPS_SDL_GL);
>>>> +
>>>>     /* Since 2.4.50 ARM virt machine supports gic-version option */
>>>>     if (qemuCaps->version >= 2004050)
>>>>         virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACH_VIRT_GIC_VERSION);
>>>>
>>>>
>>>
>>> NO! Why? We only result to setting capabilities by version if there is
>>> no other
>>> way to do that.  If query-commandline-options (or whatever that name is)
>>> works
>>> for this capability, why would you even consider this change?
>>>
>>> NACK to the diff you added.
>>
>> Are you sure? Did you read my responses from v1:
>>
>> https://www.redhat.com/archives/libvir-list/2018-May/msg00250.html
>> https://www.redhat.com/archives/libvir-list/2018-May/msg00671.html
>>
>> I actually spent some time trying to figure out which magic incantation
>> of the virQEMUCaps* would work. I even tried various forms in
>> virQEMUCapsQMPSchemaQueries, but could not get the flag to be added from
>> qemu 2.4 and beyond.  Again, my "assumption" is that it was hand edited
>> into the 2.4 replies that was added here mainly because none of the
>> other available sdl * options were addressed - just the one that was
>> wanted.
>>
>> Looking at the QEMU source code - AFAICT the gl option is only "known"
>> or handled within parse_options of vl.c.  I found nothing in the .json
>> files where I'd usually find some mechanism that would allow
>> introspection for the "-sdl gl" option. There is something in
>> qapi/ui.json that has 'gl' that looks like it could be used in some new
>> command syntax, but that seems to imply QEMU 2.12 only.
>>
>> Please enlighten me/us with code that will work before just pushing the
>> NACK button.
>>
> 
> I'm so sorry for that.  I totally missed the part that the reply was
> hand-edited.  In that case, unfortunately, the only way is to use the
> version
> _again_.  So my apologies once again, especially if that sounded rude
> (which it
> does to me now when I'm reading after myself).  I am adding a capability
> currently and I'm dealing with part of code that I reworked few times
> due to a
> similar misunderstanding from a previous developer.
> 

We're good - don't worry - but thanks for the note. I certainly get the
don't change virQEMUCapsInitQMPMonitor sentiment - it was my last resort
(and at the bottom of my initial reply). It's frustrating when spice gl
has the capability but sdl gl doesn't. Almost makes one want to say -
well don't check and if someone tries with pre qemu 2.4 and it fails,
send the bug report to qemu to add the proper capability.

John

> So feel free to squash that in if Maciej is OK with it as well (which he
> should
> be, otherwise it won't work for anyone anyway).  ACK.
> 
>> Tks,
>>
>> John