[libvirt] [PATCH] qemu: add spice opengl support
Add Spice graphics gl attribute. qemu 2.6 should have -spice gl=on argument to enable opengl rendering context (patches on the ML). This is necessary to actually enable virgl rendering. Add a qemuxml2argv test for virtio-gpu + spice with virgl. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- docs/formatdomain.html.in | 6 +++ docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 19 ++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 11 ++++++ tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.5.0-1.replies | 4 ++ .../qemuxml2argv-video-virtio-gpu-spice-gl.args | 24 ++++++++++++ .../qemuxml2argv-video-virtio-gpu-spice-gl.xml | 36 ++++++++++++++++++ tests/qemuxml2argvtest.c | 6 +++ .../qemuxml2xmlout-video-virtio-gpu-spice-gl.xml | 43 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 14 files changed, 160 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-video-virtio-gpu-spice-gl.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index e96798f..cb2c178 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4991,6 +4991,12 @@ qemu-kvm -net nic,model=? /dev/null 0.8.8</span>); and <code>usbredir</code> (<span class="since">since 0.9.12</span>). </p> + <p> + Spice may provide accelerated server-side rendering with + OpenGL. You can enable or disable OpenGL support explicitly with + the <code>gl</code> attribute. + (<span class="since">since 1.3.2</span>). + </p> <pre> <graphics type='spice' port='-1' tlsPort='-1' autoport='yes'> <channel name='main' mode='secure'/> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 67af93a..d4672d0 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2711,6 +2711,11 @@ </choice> </attribute> </optional> + <optional> + <attribute name="gl"> + <ref name="virYesNo"/> + </attribute> + </optional> <interleave> <ref name="listenElements"/> <zeroOrMore> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index acd58a1..0976e09 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10779,6 +10779,7 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, char *port = virXMLPropString(node, "port"); char *tlsPort; char *autoport; + char *gl; char *defaultMode; int defaultModeVal; @@ -10813,6 +10814,20 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, VIR_FREE(autoport); } + if ((gl = virXMLPropString(node, "gl")) != NULL) { + int glVal; + + if ((glVal = virTristateBoolTypeFromString(gl)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown gl value '%s'"), gl); + VIR_FREE(gl); + goto error; + } + + def->data.spice.gl = glVal; + VIR_FREE(gl); + } + def->data.spice.defaultMode = VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY; if ((defaultMode = virXMLPropString(node, "defaultMode")) != NULL) { @@ -21032,6 +21047,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf, break; case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: + if (def->data.spice.gl) + virBufferAsprintf(buf, " gl='%s'", + virTristateBoolTypeToString(def->data.spice.gl)); + if (def->data.spice.port) virBufferAsprintf(buf, " port='%d'", def->data.spice.port); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1de3be3..d3b3ed2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1586,6 +1586,7 @@ struct _virDomainGraphicsDef { int streaming; int copypaste; /* enum virTristateBool */ int filetransfer; /* enum virTristateBool */ + int gl; /* enum virTristateBool */ } spice; } data; /* nListens, listens, and *port are only useful if type is vnc, diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 3099e34..cab4d7d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -314,6 +314,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "vserport-change-event", /* 210 */ "virtio-balloon-pci.deflate-on-oom", + "spice-gl", ); @@ -2625,6 +2626,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { { "machine", "aes-key-wrap", QEMU_CAPS_AES_KEY_WRAP }, { "machine", "dea-key-wrap", QEMU_CAPS_DEA_KEY_WRAP }, { "chardev", "append", QEMU_CAPS_CHARDEV_FILE_APPEND }, + { "spice", "gl", QEMU_CAPS_SPICE_GL }, }; static int diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index e5353de..0c857d2 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -343,6 +343,7 @@ typedef enum { QEMU_CAPS_VSERPORT_CHANGE, /* VSERPORT_CHANGE event */ QEMU_CAPS_VIRTIO_BALLOON_AUTODEFLATE, /* virtio-balloon-{device,pci,ccw}. * deflate-on-oom */ + QEMU_CAPS_SPICE_GL, /* -spice gl */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b751f04..666dfc6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6047,6 +6047,17 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, } } + if (graphics->data.spice.gl) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_GL)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support spice OpenGL")); + goto error; + } + + virBufferAsprintf(&opt, ",gl=%s", + virTristateSwitchTypeToString(graphics->data.spice.gl)); + } + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION)) { /* If qemu supports seamless migration turn it * unconditionally on. If migration destination diff --git a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps index 931bc4f..679d65a 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps @@ -172,4 +172,5 @@ <flag name='ich9-disable-s4'/> <flag name='vserport-change-event'/> <flag name='virtio-balloon-pci.deflate-on-oom'/> + <flag name='spice-gl'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.5.0-1.replies b/tests/qemucapabilitiesdata/caps_2.5.0-1.replies index 7b692b5..07aadba 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0-1.replies +++ b/tests/qemucapabilitiesdata/caps_2.5.0-1.replies @@ -3345,6 +3345,10 @@ { "parameters": [ { + "name": "gl", + "type": "boolean" + }, + { "name": "seamless-migration", "type": "boolean" }, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args new file mode 100644 index 0000000..1361784 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=spice \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 1024 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/var/lib/libvirt/images/QEMUGuest1,format=qcow2,\ +if=none,id=drive-ide0-0-0,cache=none \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-spice port=0,gl=on \ +-device virtio-vga,id=video0,virgl=on,bus=pci.0,addr=0x2 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.xml b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.xml new file mode 100644 index 0000000..07afad2 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source file='/var/lib/libvirt/images/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='spice' gl='yes' autoport='no'/> + <video> + <model type='virtio' heads='1'> + <acceleration accel3d='yes'/> + </model> + </video> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 349e6ed..542141a 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1458,6 +1458,12 @@ mymain(void) QEMU_CAPS_DEVICE_VIRTIO_GPU, QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); + DO_TEST("video-virtio-gpu-spice-gl", QEMU_CAPS_DEVICE, + QEMU_CAPS_DEVICE_VIRTIO_GPU, + QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL, + QEMU_CAPS_SPICE, + QEMU_CAPS_SPICE_GL, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY); DO_TEST_PARSE_ERROR("video-invalid", NONE); DO_TEST("virtio-rng-default", QEMU_CAPS_DEVICE_VIRTIO_RNG, diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-video-virtio-gpu-spice-gl.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-video-virtio-gpu-spice-gl.xml new file mode 100644 index 0000000..51b8a1e --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-video-virtio-gpu-spice-gl.xml @@ -0,0 +1,43 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source file='/var/lib/libvirt/images/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='spice' gl='yes' autoport='no'/> + <video> + <model type='virtio' heads='1'> + <acceleration accel3d='yes'/> + </model> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 56dc821..d24ad2b 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -751,6 +751,7 @@ mymain(void) DO_TEST("video-virtio-gpu-device"); DO_TEST("video-virtio-gpu-virgl"); + DO_TEST("video-virtio-gpu-spice-gl"); DO_TEST("virtio-input"); DO_TEST("virtio-input-passthrough"); -- 2.5.0
On Thu, 2016-02-18 at 17:39 +0100, Marc-André Lureau wrote:
Add Spice graphics gl attribute. qemu 2.6 should have -spice gl=on argument to enable opengl rendering context (patches on the ML). This is necessary to actually enable virgl rendering.
I don't think we want to merge this before a QEMU version that supports this attribute has been released. Still, it's good to get ready ahead of time :)
@@ -4991,6 +4991,12 @@ qemu-kvm -net nic,model=? /dev/null 0.8.8</span>); and <code>usbredir</code> (<span class="since">since 0.9.12</span>). </p> + <p> + Spice may provide accelerated server-side rendering with + OpenGL. You can enable or disable OpenGL support explicitly with + the <code>gl</code> attribute. + (<span class="since">since 1.3.2</span>). + </p>
I think this should be a sub-element rather than an attribute, something like <graphics type='spice'> <gl enable='yes'/> </graphics> just like eg. the <filetransfer> sub-element. Also this looks like it's QEMU only, at least for the time being, which is something worth mentioning in the documentation.
<pre> <graphics type='spice' port='-1' tlsPort='-1' autoport='yes'> <channel name='main' mode='secure'/>
There's an example XML snippet here, you should probably update it.
@@ -10813,6 +10814,20 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, VIR_FREE(autoport); } + if ((gl = virXMLPropString(node, "gl")) != NULL) {
You can just use if ((gl = virXMLPropString(node, "gl"))) { here, no need to compare explicitly agains NULL.
@@ -6047,6 +6047,17 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, } } + if (graphics->data.spice.gl) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_GL)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support spice OpenGL")); + goto error; + } + + virBufferAsprintf(&opt, ",gl=%s", + virTristateSwitchTypeToString(graphics->data.spice.gl));
graphics->data.spice.gl is a virTristateBool, yet you're using virTristateSwitchTypeToString() on it. I know you have "yes" / "no" in the XML but need "on" / "off" for the QEMU attribute, and I know that virTristateBool and virTristateSwitch are basically the same thing, but this still looks weird. I'd rather use something like switch (graphics->data.spice.gl) { case VIR_TRISTATE_BOOL_TRUE: virBufferAsprintf(&opt, ",gl=on"); break; case VIR_TRISTATE_BOOL_FALSE: virBufferAsprintf(&opt, ",gl=off"); break; default: break; } but other people probably feel differently :)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 349e6ed..542141a 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1458,6 +1458,12 @@ mymain(void) QEMU_CAPS_DEVICE_VIRTIO_GPU, QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); + DO_TEST("video-virtio-gpu-spice-gl", QEMU_CAPS_DEVICE,
Drop QEMU_CAPS_DEVICE here, we no longer support QEMU versions that lack -device so that capability is always enabled[1]. Cheers. [1] Just realized I accidentaly introduced it again in this test program with one of my recent commits, better clean up :) -- Andrea Bolognani Software Engineer - Virtualization Team
Hi On Fri, Feb 19, 2016 at 11:52 AM, Andrea Bolognani <abologna@redhat.com> wrote:
On Thu, 2016-02-18 at 17:39 +0100, Marc-André Lureau wrote:
Add Spice graphics gl attribute. qemu 2.6 should have -spice gl=on argument to enable opengl rendering context (patches on the ML). This is necessary to actually enable virgl rendering.
I don't think we want to merge this before a QEMU version that supports this attribute has been released. Still, it's good to get ready ahead of time :)
That would delay the support for other projects too (virt-manager/boxes etc). I don't know what rules applies, but similarly we added virtio-gpu support in Nov, a month before the qemu 2.5 release. This gl=yes argument has been in the work for over a year (in development branches), and it is simple I don't think it will change.
@@ -4991,6 +4991,12 @@ qemu-kvm -net nic,model=? /dev/null 0.8.8</span>); and <code>usbredir</code> (<span class="since">since 0.9.12</span>). </p> + <p> + Spice may provide accelerated server-side rendering with + OpenGL. You can enable or disable OpenGL support explicitly with + the <code>gl</code> attribute. + (<span class="since">since 1.3.2</span>). + </p>
I think this should be a sub-element rather than an attribute, something like
Any guideline on choosing attribute or sub elements here?
<graphics type='spice'> <gl enable='yes'/> </graphics>
just like eg. the <filetransfer> sub-element.
ok
Also this looks like it's QEMU only, at least for the time being, which is something worth mentioning in the documentation.
ok
<pre> <graphics type='spice' port='-1' tlsPort='-1' autoport='yes'> <channel name='main' mode='secure'/>
There's an example XML snippet here, you should probably update it.
ok
@@ -10813,6 +10814,20 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, VIR_FREE(autoport); }
+ if ((gl = virXMLPropString(node, "gl")) != NULL) {
You can just use
if ((gl = virXMLPropString(node, "gl"))) {
here, no need to compare explicitly agains NULL.
ok (mixing different projects rules...)
@@ -6047,6 +6047,17 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, } }
+ if (graphics->data.spice.gl) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_GL)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support spice OpenGL")); + goto error; + } + + virBufferAsprintf(&opt, ",gl=%s", + virTristateSwitchTypeToString(graphics->data.spice.gl));
graphics->data.spice.gl is a virTristateBool, yet you're using virTristateSwitchTypeToString() on it.
I know you have "yes" / "no" in the XML but need "on" / "off" for the QEMU attribute, and I know that virTristateBool and virTristateSwitch are basically the same thing, but this still looks weird.
I'd rather use something like
switch (graphics->data.spice.gl) { case VIR_TRISTATE_BOOL_TRUE: virBufferAsprintf(&opt, ",gl=on"); break; case VIR_TRISTATE_BOOL_FALSE: virBufferAsprintf(&opt, ",gl=off"); break; default: break; }
but other people probably feel differently :)
I understand your concern, but having that switch seems worse to me. Maybe we should make it statically explicit that VIR_TRISTATE_BOOL_YES == VIR_TRISTATE_SWITCH_ON..
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 349e6ed..542141a 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1458,6 +1458,12 @@ mymain(void) QEMU_CAPS_DEVICE_VIRTIO_GPU, QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); + DO_TEST("video-virtio-gpu-spice-gl", QEMU_CAPS_DEVICE,
Drop QEMU_CAPS_DEVICE here, we no longer support QEMU versions that lack -device so that capability is always enabled[1].
ok
Cheers.
[1] Just realized I accidentaly introduced it again in this test program with one of my recent commits, better clean up :) -- Andrea Bolognani Software Engineer - Virtualization Team
-- Marc-André Lureau
On Fri, 2016-02-19 at 13:49 +0100, Marc-André Lureau wrote:
I don't think we want to merge this before a QEMU version that supports this attribute has been released. Still, it's good to get ready ahead of time :) That would delay the support for other projects too (virt-manager/boxes etc). I don't know what rules applies, but similarly we added virtio-gpu support in Nov, a month before the qemu 2.5 release. This gl=yes argument has been in the work for over a year (in development branches), and it is simple I don't think it will change.
Seems to me like any work intended to use features that only exist in branches should be done, well, in branches :)
@@ -4991,6 +4991,12 @@ qemu-kvm -net nic,model=? /dev/null 0.8.8</span>); and <code>usbredir</code> (<span class="since">since 0.9.12</span>). </p> + <p> + Spice may provide accelerated server-side rendering with + OpenGL. You can enable or disable OpenGL support explicitly with + the <code>gl</code> attribute. + (<span class="since">since 1.3.2</span>). + </p> I think this should be a sub-element rather than an attribute, something like Any guideline on choosing attribute or sub elements here?
There's no formal guideline I'm aware of, but looks like all the SPICE specific settings, the ones that are not shared with other <graphics> types, are represented as sub-elements so I believe we should follow that lead.
@@ -6047,6 +6047,17 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, } } + if (graphics->data.spice.gl) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_GL)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support spice OpenGL")); + goto error; + } + + virBufferAsprintf(&opt, ",gl=%s", + virTristateSwitchTypeToString(graphics->data.spice.gl)); graphics->data.spice.gl is a virTristateBool, yet you're using virTristateSwitchTypeToString() on it. I know you have "yes" / "no" in the XML but need "on" / "off" for the QEMU attribute, and I know that virTristateBool and virTristateSwitch are basically the same thing, but this still looks weird. I'd rather use something like switch (graphics->data.spice.gl) { case VIR_TRISTATE_BOOL_TRUE: virBufferAsprintf(&opt, ",gl=on"); break; case VIR_TRISTATE_BOOL_FALSE: virBufferAsprintf(&opt, ",gl=off"); break; default: break; } but other people probably feel differently :) I understand your concern, but having that switch seems worse to me. Maybe we should make it statically explicit that VIR_TRISTATE_BOOL_YES == VIR_TRISTATE_SWITCH_ON..
Well, it's certainly more verbose but also more explicit. I, for one, had to do a double take because I didn't notice right away that you were using virTristateSwitch* in that case, and was wondering how it could work at all. At the very least, I believe it deserves an explanatory comment. Let's wait for other people to weigh in on these three remaining issues... Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Forgot something :) On Thu, 2016-02-18 at 17:39 +0100, Marc-André Lureau wrote:
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args b/tests/qemuxml2argvdata/qemuxml2argv- video-virtio-gpu-spice-gl.args new file mode 100644 index 0000000..1361784 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=spice \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 1024 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/var/lib/libvirt/images/QEMUGuest1,format=qcow2,\ +if=none,id=drive-ide0-0-0,cache=none \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-spice port=0,gl=on \ +-device virtio-vga,id=video0,virgl=on,bus=pci.0,addr=0x2 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.xml b/tests/qemuxml2argvdata/qemuxml2argv- video-virtio-gpu-spice-gl.xml
Squash diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args b/tests/qemuxml2argvdata/qemuxml2argv- video-virtio-gpu-spice-gl.args index 1361784..16ac8ca 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args @@ -16,8 +16,8 @@ QEMU_AUDIO_DRV=spice \ -no-acpi \ -boot c \ -usb \ --drive file=/var/lib/libvirt/images/QEMUGuest1,format=qcow2,\ -if=none,id=drive-ide0-0-0,cache=none \ +-drive file=/var/lib/libvirt/images/QEMUGuest1,format=qcow2,if=none,\ +id=drive-ide0-0-0,cache=none \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -spice port=0,gl=on \ -device virtio-vga,id=video0,virgl=on,bus=pci.0,addr=0x2 \ in to stop 'make syntax-check' from complaining. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Thu, Feb 18, 2016 at 17:39:08 +0100, Marc-André Lureau wrote:
Add Spice graphics gl attribute. qemu 2.6 should have -spice gl=on argument to enable opengl rendering context (patches on the ML). This is necessary to actually enable virgl rendering.
Add a qemuxml2argv test for virtio-gpu + spice with virgl.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> ...
And another small issue:
diff --git a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps index 931bc4f..679d65a 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps @@ -172,4 +172,5 @@ <flag name='ich9-disable-s4'/> <flag name='vserport-change-event'/> <flag name='virtio-balloon-pci.deflate-on-oom'/> + <flag name='spice-gl'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.5.0-1.replies b/tests/qemucapabilitiesdata/caps_2.5.0-1.replies index 7b692b5..07aadba 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0-1.replies +++ b/tests/qemucapabilitiesdata/caps_2.5.0-1.replies @@ -3345,6 +3345,10 @@ { "parameters": [ { + "name": "gl", + "type": "boolean" + }, + { "name": "seamless-migration", "type": "boolean" },
spice-gl should not be artificially included in QEMU 2.5.0 capabilities, you should add files describing real QEMU 2.6.0. Jirka
participants (3)
-
Andrea Bolognani -
Jiri Denemark -
Marc-André Lureau