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