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

[Daniel P. Berrangé]

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
-- 
|: 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 :|