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

[Marc-André Lureau]

Hi

On Fri, Feb 19, 2016 at 11:52 AM, Andrea Bolognani <abologna at 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