[Spice-devel] "qxl: add optinal 64bit vram bar" patch significantly slows down Spice

Submitted by Gerd Hoffmann on April 16, 2012, 12:43 p.m.

Details

Message ID 4F8C13E7.7010903@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Gerd Hoffmann April 16, 2012, 12:43 p.m.
Hi,

> Looks like both the BAR and it's backing region were initialized to 4k. 
> The number 4096 does show up in the patch, probably it sneaked into the
> BAR somehow.

Yea, one page is the minimum size, that shouldn't have been the default
though.  Alon has a fix for it (attached), still in the queue due to me
having been sick for two weeks.  Hans, does the patch improve things?

cheers,
  Gerd
From d7129236321adb89b094f3ae4b858642bb9dcbde Mon Sep 17 00:00:00 2001
From: Alon Levy <alevy@redhat.com>
Date: Thu, 29 Mar 2012 22:24:38 +0200
Subject: [PATCH] qxl: set default values of vram*_size_mb to -1

The addition of those values caused a regression where not specifying
any value for the vram bar size would result in a 4096 _byte_ surface
area. This is ok for the windows driver but causes the X driver to be
unusable. Also, it's a regression. This patch returns the default
behavior of having a 64 megabyte vram BAR.

Signed-off-by: Alon Levy <alevy@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qxl.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/hw/qxl.c b/hw/qxl.c
index 47a162e..db2318e 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1959,8 +1959,8 @@  static Property qxl_properties[] = {
         DEFINE_PROP_UINT32("guestdebug", PCIQXLDevice, guestdebug, 0),
         DEFINE_PROP_UINT32("cmdlog", PCIQXLDevice, cmdlog, 0),
         DEFINE_PROP_UINT32("ram_size_mb",  PCIQXLDevice, ram_size_mb, -1),
-        DEFINE_PROP_UINT32("vram_size_mb", PCIQXLDevice, vram32_size_mb, 0),
-        DEFINE_PROP_UINT32("vram64_size_mb", PCIQXLDevice, vram_size_mb, 0),
+        DEFINE_PROP_UINT32("vram_size_mb", PCIQXLDevice, vram32_size_mb, -1),
+        DEFINE_PROP_UINT32("vram64_size_mb", PCIQXLDevice, vram_size_mb, -1),
         DEFINE_PROP_END_OF_LIST(),
 };
 

Comments

On 04/16/2012 03:43 PM, Gerd Hoffmann wrote:
>   Hi,
>
>> > Looks like both the BAR and it's backing region were initialized to 4k. 
>> > The number 4096 does show up in the patch, probably it sneaked into the
>> > BAR somehow.
> Yea, one page is the minimum size, that shouldn't have been the default
> though.  Alon has a fix for it (attached), still in the queue due to me
> having been sick for two weeks.  Hans, does the patch improve things?
>
> cheers,
>   Gerd
>
> 0001-qxl-set-default-values-of-vram-_size_mb-to-1.patch
>
>
> From d7129236321adb89b094f3ae4b858642bb9dcbde Mon Sep 17 00:00:00 2001
> From: Alon Levy <alevy@redhat.com>
> Date: Thu, 29 Mar 2012 22:24:38 +0200
> Subject: [PATCH] qxl: set default values of vram*_size_mb to -1
>
> The addition of those values caused a regression where not specifying
> any value for the vram bar size would result in a 4096 _byte_ surface
> area. This is ok for the windows driver but causes the X driver to be
> unusable. Also, it's a regression. This patch returns the default
> behavior of having a 64 megabyte vram BAR.
>
> Signed-off-by: Alon Levy <alevy@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/qxl.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/qxl.c b/hw/qxl.c
> index 47a162e..db2318e 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -1959,8 +1959,8 @@ static Property qxl_properties[] = {
>          DEFINE_PROP_UINT32("guestdebug", PCIQXLDevice, guestdebug, 0),
>          DEFINE_PROP_UINT32("cmdlog", PCIQXLDevice, cmdlog, 0),
>          DEFINE_PROP_UINT32("ram_size_mb",  PCIQXLDevice, ram_size_mb, -1),
> -        DEFINE_PROP_UINT32("vram_size_mb", PCIQXLDevice, vram32_size_mb, 0),
> -        DEFINE_PROP_UINT32("vram64_size_mb", PCIQXLDevice, vram_size_mb, 0),
> +        DEFINE_PROP_UINT32("vram_size_mb", PCIQXLDevice, vram32_size_mb, -1),
> +        DEFINE_PROP_UINT32("vram64_size_mb", PCIQXLDevice, vram_size_mb, -1),
>          DEFINE_PROP_END_OF_LIST(),
>  };
>  
>

Minor nit - wouldn't it be better to put the actual defaults there,
instead of falling back to another code path?
Hi,

On 04/16/2012 02:43 PM, Gerd Hoffmann wrote:
>    Hi,
>
>> Looks like both the BAR and it's backing region were initialized to 4k.
>> The number 4096 does show up in the patch, probably it sneaked into the
>> BAR somehow.
>
> Yea, one page is the minimum size, that shouldn't have been the default
> though.  Alon has a fix for it (attached), still in the queue due to me
> having been sick for two weeks.  Hans, does the patch improve things?

Heh, I just re-invented Alon's patch :| It is exactly the same, no
surprise there once you've pinpointed the issue, the fix is trivial :)

I've just competed testing of my own re-invention of that patch, and with
it applied the slow down is gone. So that likely is the cause.

Regards,

Hans