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

[Andrea Bolognani]

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