[libvirt] [PATCH] qemu: add sdl opengl support

[Maciej Wolny]

On 02/05/18 11:54, Daniel P. Berrangé wrote:
> On Wed, May 02, 2018 at 11:48:24AM +0100, Maciej Wolny wrote:
>> On 02/05/18 08:13, Daniel P. Berrangé wrote:
>>> On Tue, May 01, 2018 at 08:22:45PM +0100, Maciej Wolny wrote:
>>>> Add SDL graphics gl attribute, modify the domain XML schema, add a
>>>> test, modify the documentation to include the new option.
>>>>
>>>> Signed-off-by: Maciej Wolny <maciej.wolny at codethink.co.uk>
>>>> ---
>>>>   docs/schemas/domaincommon.rng                      |  8 +++++
>>>>   src/conf/domain_conf.c                             | 41 ++++++++++++++++++++++
>>>>   src/conf/domain_conf.h                             |  1 +
>>>>   src/qemu/qemu_capabilities.c                       |  2 ++
>>>>   src/qemu/qemu_capabilities.h                       |  1 +
>>>>   src/qemu/qemu_command.c                            | 19 ++++++++++
>>>>   .../qemuxml2argvdata/video-virtio-gpu-sdl-gl.args  | 28 +++++++++++++++
>>>>   tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml | 38 ++++++++++++++++++++
>>>>   tests/qemuxml2argvtest.c                           |  5 +++
>>>>   9 files changed, 143 insertions(+)
>>>>   create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args
>>>>   create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml
>>>>
>>>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>>>> index 3569b9212..a2ef93c09 100644
>>>> --- a/docs/schemas/domaincommon.rng
>>>> +++ b/docs/schemas/domaincommon.rng
>>>> @@ -3031,6 +3031,14 @@
>>>>                 <ref name="virYesNo"/>
>>>>               </attribute>
>>>>             </optional>
>>>> +          <optional>
>>>> +            <element name="gl">
>>>> +              <attribute name="enable">
>>>> +                <ref name="virYesNo"/>
>>>> +              </attribute>
>>>> +              <empty/>
>>>> +            </element>
>>>> +          </optional>
>>>>           </group>
>>>>           <group>
>>>>             <attribute name="type">
>>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>>> index b0257068d..7d65ca9df 100644
>>>> --- a/src/conf/domain_conf.c
>>>> +++ b/src/conf/domain_conf.c
>>>> @@ -13448,6 +13448,7 @@ static int
>>>>   virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDefPtr def,
>>>>                                   xmlNodePtr node)
>>>>   {
>>>> +    xmlNodePtr cur;
>>>>       char *fullscreen = virXMLPropString(node, "fullscreen");
>>>>       int ret = -1;
>>>> @@ -13468,6 +13469,34 @@ virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDefPtr def,
>>>>       def->data.sdl.xauth = virXMLPropString(node, "xauth");
>>>>       def->data.sdl.display = virXMLPropString(node, "display");
>>>> +    cur = node->children;
>>>> +    while (cur != NULL) {
>>>> +        if (cur->type == XML_ELEMENT_NODE) {
>>>> +            if (virXMLNodeNameEqual(cur, "gl")) {
>>>> +                char *enable = virXMLPropString(cur, "enable");
>>>> +                int enableVal;
>>>> +
>>>> +                if (!enable) {
>>>> +                    virReportError(VIR_ERR_XML_ERROR, "%s",
>>>> +                                   _("sdl gl element missing enable"));
>>>> +                    goto cleanup;
>>>> +                }
>>>> +
>>>> +                enableVal = virTristateBoolTypeFromString(enable);
>>>> +                if (enableVal < 0) {
>>>> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>>> +                                   _("unknown enable value '%s'"), enable);
>>>> +                    VIR_FREE(enable);
>>>> +                    goto cleanup;
>>>> +                }
>>>> +                VIR_FREE(enable);
>>>> +
>>>> +                def->data.sdl.gl = enableVal;
>>>> +            }
>>>> +        }
>>>> +        cur = cur->next;
>>>> +    }
>>>> +
>>>>       ret = 0;
>>>>    cleanup:
>>>>       VIR_FREE(fullscreen);
>>>> @@ -25652,6 +25681,18 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
>>>>           if (def->data.sdl.fullscreen)
>>>>               virBufferAddLit(buf, " fullscreen='yes'");
>>>> +        if (!children && def->data.sdl.gl) {
>>>> +            virBufferAddLit(buf, ">\n");
>>>> +            virBufferAdjustIndent(buf, 2);
>>>> +            children = true;
>>>> +        }
>>>> +
>>>> +        if (def->data.sdl.gl) {
>>>> +            virBufferAsprintf(buf, "<gl enable='%s'",
>>>> +                              virTristateBoolTypeToString(def->data.sdl.gl));
>>>> +            virBufferAddLit(buf, "/>\n");
>>>> +        }
>>>
>>> SPICE  GL support allows a "rendernode" property to be set - I'm wondering
>>> if that is relevant to SDL or not ?
>>>
>>>
>>> Regards,
>>> Daniel
>>>
>>
>> I purposefully didn't look into this because we didn't need that option for
>> our use cases - would you still merge this patch without implementing this
>> option?
> 
> My thought was that if rendernode is also applicable for SPICE, then
> instead of duplicating the struct fields and duplicating parsing, we
> should create a helper method for dealing with the <gl> element that
> can be shared with SPICE & SDL.   If rendernode is not allowed with
> SPICE in QEMU, then your simpler approach is sufficient
> 
> 
> Regards,
> Daniel
> 

Grepping through the source code of QEMU, it seems that the rendernode option
is only used for SPICE (for egl_rendernode_init in ui/spice-core.c).