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

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(a)codethink.co.uk&gt; >>>> --- >>>> 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).