6:12 a.m.
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>
>>> ---
>>> 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).