[Spice-devel,v3] qemu: Use heads parameter for QXL driver

Submitted by Frediano Ziglio on July 6, 2015, 8:18 a.m.

Details

Message ID 1436170739-8714-1-git-send-email-fziglio@redhat.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Frediano Ziglio July 6, 2015, 8:18 a.m.
Allows to specify maximum number of head to QXL driver.

The patch to support the "max_outputs" in Qemu is still not merged but
I got agreement on the name of the argument.

Actually can be a compatiblity problem as heads in the XML configuration
was set by default to '1'.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 src/qemu/qemu_capabilities.c | 2 ++
 src/qemu/qemu_capabilities.h | 1 +
 src/qemu/qemu_command.c      | 5 +++++
 3 files changed, 8 insertions(+)

Changes from v2:
- removed capability tests (Martin Kletzander)

Patch hide | download patch | download mbox

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 27686c3..68060cd 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -287,6 +287,7 @@  VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
               "aarch64-off",
 
               "vhost-user-multiqueue", /* 190 */
+              "qxl-vga.max_outputs",
     );
 
 
@@ -1649,6 +1650,7 @@  static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsQxl[] = {
 
 static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsQxlVga[] = {
     { "vgamem_mb", QEMU_CAPS_QXL_VGA_VGAMEM },
+    { "max_outputs", QEMU_CAPS_QXL_VGA_MAX_OUTPUTS },
 };
 
 struct virQEMUCapsObjectTypeProps {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 30aa504..02f9e81 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -230,6 +230,7 @@  typedef enum {
     QEMU_CAPS_DEVICE_PCI_SERIAL  = 188, /* -device pci-serial */
     QEMU_CAPS_CPU_AARCH64_OFF    = 189, /* -cpu ...,aarch64=off */
     QEMU_CAPS_VHOSTUSER_MULTIQUEUE = 190, /* vhost-user with -netdev queues= */
+    QEMU_CAPS_QXL_VGA_MAX_OUTPUTS = 191, /* qxl-vga.max_outputs */
 
     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 25a7bc6..59666e7 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5661,6 +5661,11 @@  qemuBuildDeviceVideoStr(virDomainDefPtr def,
             /* QEMU accepts mebibytes for vgamem_mb. */
             virBufferAsprintf(&buf, ",vgamem_mb=%u", video->vgamem / 1024);
         }
+
+        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VGA_MAX_OUTPUTS) &&
+            video->heads > 0) {
+            virBufferAsprintf(&buf, ",max_outputs=%u", video->heads);
+        }
     } else if (video->vram &&
         ((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA &&
           virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM)) ||

Comments

On Mon, Jul 06, 2015 at 09:18:59AM +0100, Frediano Ziglio wrote:
>Allows to specify maximum number of head to QXL driver.
>
>The patch to support the "max_outputs" in Qemu is still not merged but
>I got agreement on the name of the argument.
>

This shouldn't be part of the commit message, we can't push this in
until the code is in qemu anyways, so I'll remove it.

>Actually can be a compatiblity problem as heads in the XML configuration
>was set by default to '1'.
>
>Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
>---
> src/qemu/qemu_capabilities.c | 2 ++
> src/qemu/qemu_capabilities.h | 1 +
> src/qemu/qemu_command.c      | 5 +++++
> 3 files changed, 8 insertions(+)
>
>Changes from v2:
>- removed capability tests (Martin Kletzander)
>
>diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>index 27686c3..68060cd 100644
>--- a/src/qemu/qemu_capabilities.c
>+++ b/src/qemu/qemu_capabilities.c
>@@ -287,6 +287,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>               "aarch64-off",
>
>               "vhost-user-multiqueue", /* 190 */
>+              "qxl-vga.max_outputs",
>     );
>
>
>@@ -1649,6 +1650,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsQxl[] = {
>
> static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsQxlVga[] = {
>     { "vgamem_mb", QEMU_CAPS_QXL_VGA_VGAMEM },
>+    { "max_outputs", QEMU_CAPS_QXL_VGA_MAX_OUTPUTS },
> };
>
> struct virQEMUCapsObjectTypeProps {
>diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
>index 30aa504..02f9e81 100644
>--- a/src/qemu/qemu_capabilities.h
>+++ b/src/qemu/qemu_capabilities.h
>@@ -230,6 +230,7 @@ typedef enum {
>     QEMU_CAPS_DEVICE_PCI_SERIAL  = 188, /* -device pci-serial */
>     QEMU_CAPS_CPU_AARCH64_OFF    = 189, /* -cpu ...,aarch64=off */
>     QEMU_CAPS_VHOSTUSER_MULTIQUEUE = 190, /* vhost-user with -netdev queues= */
>+    QEMU_CAPS_QXL_VGA_MAX_OUTPUTS = 191, /* qxl-vga.max_outputs */
>
>     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 25a7bc6..59666e7 100644
>--- a/src/qemu/qemu_command.c
>+++ b/src/qemu/qemu_command.c
>@@ -5661,6 +5661,11 @@ qemuBuildDeviceVideoStr(virDomainDefPtr def,
>             /* QEMU accepts mebibytes for vgamem_mb. */
>             virBufferAsprintf(&buf, ",vgamem_mb=%u", video->vgamem / 1024);
>         }
>+
>+        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VGA_MAX_OUTPUTS) &&
>+            video->heads > 0) {
>+            virBufferAsprintf(&buf, ",max_outputs=%u", video->heads);
>+        }

Looks good to me.  I would add a comment here why we're not erroring
out, but rahter using the parameter only if supported, maybe also add
some VIR_INFO output, but no need to resend it just for that.

Let's just wait for QEMU to have this in.  If it's in and I missed the
info, feel free to remind me about pushing this patch to libvirt.

Martin

>     } else if (video->vram &&
>         ((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA &&
>           virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM)) ||
>--
>2.1.0
>
On Tue, Jul 07, 2015 at 11:44:11AM +0200, Martin Kletzander wrote:
>On Mon, Jul 06, 2015 at 09:18:59AM +0100, Frediano Ziglio wrote:
>>Allows to specify maximum number of head to QXL driver.
>>
>>The patch to support the "max_outputs" in Qemu is still not merged but
>>I got agreement on the name of the argument.
>>
>
>This shouldn't be part of the commit message, we can't push this in
>until the code is in qemu anyways, so I'll remove it.
>
>>Actually can be a compatiblity problem as heads in the XML configuration
>>was set by default to '1'.
>>
>>Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
>>---
>>src/qemu/qemu_capabilities.c | 2 ++
>>src/qemu/qemu_capabilities.h | 1 +
>>src/qemu/qemu_command.c      | 5 +++++
>>3 files changed, 8 insertions(+)
>>
>>Changes from v2:
>>- removed capability tests (Martin Kletzander)
>>
>>diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>>index 27686c3..68060cd 100644
>>--- a/src/qemu/qemu_capabilities.c
>>+++ b/src/qemu/qemu_capabilities.c
>>@@ -287,6 +287,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>>              "aarch64-off",
>>
>>              "vhost-user-multiqueue", /* 190 */
>>+              "qxl-vga.max_outputs",
>>    );
>>
>>
>>@@ -1649,6 +1650,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsQxl[] = {
>>
>>static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsQxlVga[] = {
>>    { "vgamem_mb", QEMU_CAPS_QXL_VGA_VGAMEM },
>>+    { "max_outputs", QEMU_CAPS_QXL_VGA_MAX_OUTPUTS },
>>};
>>
>>struct virQEMUCapsObjectTypeProps {
>>diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
>>index 30aa504..02f9e81 100644
>>--- a/src/qemu/qemu_capabilities.h
>>+++ b/src/qemu/qemu_capabilities.h
>>@@ -230,6 +230,7 @@ typedef enum {
>>    QEMU_CAPS_DEVICE_PCI_SERIAL  = 188, /* -device pci-serial */
>>    QEMU_CAPS_CPU_AARCH64_OFF    = 189, /* -cpu ...,aarch64=off */
>>    QEMU_CAPS_VHOSTUSER_MULTIQUEUE = 190, /* vhost-user with -netdev queues= */
>>+    QEMU_CAPS_QXL_VGA_MAX_OUTPUTS = 191, /* qxl-vga.max_outputs */
>>
>>    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 25a7bc6..59666e7 100644
>>--- a/src/qemu/qemu_command.c
>>+++ b/src/qemu/qemu_command.c
>>@@ -5661,6 +5661,11 @@ qemuBuildDeviceVideoStr(virDomainDefPtr def,
>>            /* QEMU accepts mebibytes for vgamem_mb. */
>>            virBufferAsprintf(&buf, ",vgamem_mb=%u", video->vgamem / 1024);
>>        }
>>+
>>+        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VGA_MAX_OUTPUTS) &&
>>+            video->heads > 0) {
>>+            virBufferAsprintf(&buf, ",max_outputs=%u", video->heads);
>>+        }
>
>Looks good to me.  I would add a comment here why we're not erroring
>out, but rahter using the parameter only if supported, maybe also add
>some VIR_INFO output, but no need to resend it just for that.
>
>Let's just wait for QEMU to have this in.  If it's in and I missed the
>info, feel free to remind me about pushing this patch to libvirt.
>

Looking back at this patch, I figured there are no tests to make sure
this doesn't break few commits afterwards.  Take a look at the
tests/qemuxml2argvtest and add at least one there, please.  If the
patch looks like it's adding multiple things, feel free to split it
(one for the capability, other for tests and the hunk in
qemu_command.c).

Sorry for not noticing this earlier.

>Martin
>
>>    } else if (video->vram &&
>>        ((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA &&
>>          virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM)) ||
>>--
>>2.1.0
>>