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

[Daniel P. Berrangé]

On Tue, May 08, 2018 at 12:12:42PM +0100, Maciej Wolny wrote:
> 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
> 
> 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).

Ok, thanks for checking - sounds like your simple code is fine as is.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|