Patchwork [PATCHv3] qxl: move to UNDEFINED on DESTROY_ALL_SURFACES{, _ASYNC}

login
register
mail settings
Submitter Alon Levy
Date June 29, 2011, 11:57 a.m.
Message ID <1309348641-20061-14-git-send-email-alevy@redhat.com>
Download mbox | patch
Permalink /patch/102589/
State New
Headers show

Comments

Alon Levy - June 29, 2011, 11:57 a.m.
---
 hw/qxl.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Gerd Hoffmann - June 29, 2011, 1:09 p.m.
Hi,

> +    case QXL_IO_DESTROY_ALL_SURFACES_ASYNC:
> +        d->mode = QXL_MODE_UNDEFINED;

Should go to the async thread.

cheers,
   Gerd
Alon Levy - June 29, 2011, 2:29 p.m.
On Wed, Jun 29, 2011 at 03:09:36PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >+    case QXL_IO_DESTROY_ALL_SURFACES_ASYNC:
> >+        d->mode = QXL_MODE_UNDEFINED;
> 
> Should go to the async thread.

doesn't it make more sense to do all state changes from the vcpu thread? async thread can run much
later, if you have a QXL_IO_DESTROY_ALL_SURFACES_ASYNC followed by a QXL_IO_CREATE_PRIMARY_ASYNC where
the driver did not wait for the completion of the ASYNC first, I would still like to support that,
but it won't work if I move this to the async thread.

> 
> cheers,
>   Gerd
Gerd Hoffmann - June 29, 2011, 3 p.m.
On 06/29/11 16:29, Alon Levy wrote:
> On Wed, Jun 29, 2011 at 03:09:36PM +0200, Gerd Hoffmann wrote:
>> Hi,
>>
>>> +    case QXL_IO_DESTROY_ALL_SURFACES_ASYNC: +        d->mode =
>>> QXL_MODE_UNDEFINED;
>>
>> Should go to the async thread.
>
> doesn't it make more sense to do all state changes from the vcpu
> thread? async thread can run much later, if you have a
> QXL_IO_DESTROY_ALL_SURFACES_ASYNC followed by a
> QXL_IO_CREATE_PRIMARY_ASYNC where the driver did not wait for the
> completion of the ASYNC first, I would still like to support that,
> but it won't work if I move this to the async thread.

I think we should disallow doing any I/O ops while one is in progress 
(except maybe QXL_IO_LOG).  Most I/O commands are I/O commands because 
they either needed for device setup or must be synchronous anyway.

QXL_IO_CREATE_PRIMARY_ASYNC wasn't exactly clever designed I think. 
Would have been better to enter native mode with another I/O command, 
then send the create request for the primary through the rings like all 
other surface commands.  But given it is a rare event it isn't that a 
big issue either.

cheers,
   Gerd
Alon Levy - June 29, 2011, 6:28 p.m.
On Wed, Jun 29, 2011 at 05:00:20PM +0200, Gerd Hoffmann wrote:
> On 06/29/11 16:29, Alon Levy wrote:
> >On Wed, Jun 29, 2011 at 03:09:36PM +0200, Gerd Hoffmann wrote:
> >>Hi,
> >>
> >>>+    case QXL_IO_DESTROY_ALL_SURFACES_ASYNC: +        d->mode =
> >>>QXL_MODE_UNDEFINED;
> >>
> >>Should go to the async thread.
> >
> >doesn't it make more sense to do all state changes from the vcpu
> >thread? async thread can run much later, if you have a
> >QXL_IO_DESTROY_ALL_SURFACES_ASYNC followed by a
> >QXL_IO_CREATE_PRIMARY_ASYNC where the driver did not wait for the
> >completion of the ASYNC first, I would still like to support that,
> >but it won't work if I move this to the async thread.
> 
> I think we should disallow doing any I/O ops while one is in
> progress (except maybe QXL_IO_LOG).  Most I/O commands are I/O
> commands because they either needed for device setup or must be
> synchronous anyway.
> 
> QXL_IO_CREATE_PRIMARY_ASYNC wasn't exactly clever designed I think.
> Would have been better to enter native mode with another I/O
> command, then send the create request for the primary through the
> rings like all other surface commands.  But given it is a rare event
> it isn't that a big issue either.
> 

So - leave or change? I prefer to leave..
Maybe add a "pending async" flag to catch such occasions?

> cheers,
>   Gerd
> 
>
Gerd Hoffmann - June 30, 2011, 7:16 a.m.
Hi,

> Maybe add a "pending async" flag to catch such occasions?

Yes, good idea.

cheers,
   Gerd

Patch

diff --git a/hw/qxl.c b/hw/qxl.c
index f158d45..b794b2c 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1245,6 +1245,7 @@  static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
         qxl_spice_destroy_surface_wait(d, val);
         break;
     case QXL_IO_DESTROY_ALL_SURFACES:
+        d->mode = QXL_MODE_UNDEFINED;
         qxl_spice_destroy_surfaces(d);
         break;
     case QXL_IO_FLUSH_SURFACES:
@@ -1302,9 +1303,10 @@  static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
         async->update_area = d->ram->update_area;
         async->update_surface = d->ram->update_surface;
         goto async_common;
+    case QXL_IO_DESTROY_ALL_SURFACES_ASYNC:
+        d->mode = QXL_MODE_UNDEFINED;
     case QXL_IO_NOTIFY_OOM_ASYNC:
     case QXL_IO_DESTROY_SURFACE_ASYNC:
-    case QXL_IO_DESTROY_ALL_SURFACES_ASYNC:
         async = qemu_mallocz(sizeof(*async));
     async_common:
         async->port = io_port;