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@xxxxxxxxxxxxxxx>
>>>>> ---
>>>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
