Re: [Qemu-devel] [PATCH v2] qxl: add primary_created state, change mode lifetimes

[Alon Levy <alevy@redhat.com>]

On Mon, Jun 27, 2011 at 10:42:46AM +0300, yhalperi wrote:
> On 06/24/2011 04:02 PM, Alon Levy wrote:
> >Currently we define the following relation between modes and primary surface
> >existance:
> >
> >  QXL_MODE_COMPAT
> >  QXL_MODE_NATIVE  primary exists
> >  QXL_MODE_UNDEFINED
> >  QXL_MODE_VGA     primary doesn't exist
> >
> >This, coupled with disallowing various IO's when not in QXL_MODE_NATIVE or COMPAT
> >means that the S3 implementation gets more complex - This is wrong:
> >  DESTROY_SURFACES
> >  SURFACE_RELEASE
> >  .. wakeup
> >  PRIMARY_CREATE<-- error, we are already in NATIVE_MODE, not allowed!
> >
> 
> Shouldn't the mode change to QXL_MODE_UNDEFINED on DESTROY_SURFACES?
> (I can see it currently doesn't, but it seems like a mistake)

No, that is the point - this patch makes the lifetime of QXL_MODE_NATIVE be
the lifetime of the driver in the guest, kinda. we never unload the driver
in the guest, the only time it stops being operational is when we enter vga
mode and that's explicitly done when we get any vga port write/read.

In other words, with this patch the only thing that changes qxl->mode is:
 PRIMARY_CREATE - qxl->mode=QXL_MODE_NATIVE
 vga port write - qxl->mode=QXL_MODE_VGA

so yes, UNDEFINED becomes useless. Should be dropped then, no?

> >This patch adds an explicit primary_created status, changed by:
> >CREATE_PRIMARY, DESTROY_PRIMARY, DESTROY_SURFACES
> >
> >This patch also changes the lifetime of QXL_MODE_NATIVE.
> >  CREATE_PRIMARY - enter QXL_MODE_NATIVE
> >  vga io - exit QXL_MODE_NATIVE
> >
> >anything else doesn't effect it, specifically DESTROY_PRIMARY.
> >---
> >  hw/qxl.c |   38 +++++++++++++++++++++++++++-----------
> >  hw/qxl.h |    1 +
> >  2 files changed, 28 insertions(+), 11 deletions(-)
> >
> >diff --git a/hw/qxl.c b/hw/qxl.c
> >index fab0208..51a5270 100644
> >--- a/hw/qxl.c
> >+++ b/hw/qxl.c
> >@@ -666,6 +666,7 @@ static void qxl_enter_vga_mode(PCIQXLDevice *d)
> >          return;
> >      }
> >      dprint(d, 1, "%s\n", __FUNCTION__);
> >+    d->primary_created = 1;
> >      qemu_spice_create_host_primary(&d->ssd);
> >      d->mode = QXL_MODE_VGA;
> >      memset(&d->ssd.dirty, 0, sizeof(d->ssd.dirty));
> >@@ -677,10 +678,9 @@ static void qxl_exit_vga_mode(PCIQXLDevice *d)
> >          return;
> >      }
> >      dprint(d, 1, "%s\n", __FUNCTION__);
> >-    if (d->mode != QXL_MODE_UNDEFINED) {
> >-        d->mode = QXL_MODE_UNDEFINED;
> >-        qemu_spice_destroy_primary_surface(&d->ssd, 0);
> >-    }
> >+    d->mode = QXL_MODE_UNDEFINED;
> >+    d->primary_created = 0;
> >+    qemu_spice_destroy_primary_surface(&d->ssd, 0);
> >  }
> >
> >  static void qxl_set_irq(PCIQXLDevice *d)
> >@@ -780,7 +780,9 @@ static void qxl_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >          dprint(qxl, 1, "%s\n", __FUNCTION__);
> >          if (qxl->mode != QXL_MODE_UNDEFINED) {
> >              qxl->mode = QXL_MODE_UNDEFINED;
> >-            qemu_spice_destroy_primary_surface(&qxl->ssd, 0);
> >+            if (qxl->primary_created) {
> >+                qemu_spice_destroy_primary_surface(&qxl->ssd, 0);
> >+            }
> >          }
> >          qxl_soft_reset(qxl);
> >      }
> >@@ -912,8 +914,10 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
> >  {
> >      QXLSurfaceCreate *sc =&qxl->guest_primary.surface;
> >
> >-    assert(qxl->mode != QXL_MODE_NATIVE);
> >-    qxl_exit_vga_mode(qxl);
> >+    if (qxl->mode != QXL_MODE_NATIVE) {
> >+        qxl_exit_vga_mode(qxl);
> >+    }
> >+    assert(!qxl->primary_created);
> >
> >      dprint(qxl, 1, "%s: %dx%d\n", __FUNCTION__,
> >             le32_to_cpu(sc->width), le32_to_cpu(sc->height));
> >@@ -933,6 +937,7 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
> >          surface->flags |= QXL_SURF_FLAG_KEEP_DATA;
> >      }
> >
> >+    qxl->primary_created = 1;
> >      qxl->mode = QXL_MODE_NATIVE;
> >      qxl->cmdflags = 0;
> >
> >@@ -1156,15 +1161,19 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >      case QXL_IO_DESTROY_PRIMARY:
> >          PANIC_ON(val != 0);
> >          dprint(d, 1, "QXL_IO_DESTROY_PRIMARY (%s)\n", qxl_mode_to_string(d->mode));
> >-        if (d->mode != QXL_MODE_UNDEFINED) {
> >-            d->mode = QXL_MODE_UNDEFINED;
> >+        if (d->primary_created) {
> >+            d->primary_created = 0;
> >              qemu_spice_destroy_primary_surface(&d->ssd, 0);
> >          }
> >          break;
> >      case QXL_IO_DESTROY_SURFACE_WAIT:
> >+        if (val == 0) {
> >+            d->primary_created = 0;
> >+        }
> >          qemu_spice_destroy_surface_wait(&d->ssd, val);
> >          break;
> >      case QXL_IO_DESTROY_ALL_SURFACES:
> >+        d->primary_created = 0;
> >          qemu_spice_destroy_surfaces(&d->ssd);
> >          break;
> >      case QXL_IO_FLUSH_SURFACES:
> >@@ -1209,8 +1218,8 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >      case QXL_IO_DESTROY_PRIMARY_ASYNC:
> >          PANIC_ON(val != 0);
> >          dprint(d, 1, "QXL_IO_DESTROY_PRIMARY_ASYNC\n");
> >-        if (d->mode != QXL_MODE_UNDEFINED) {
> >-            d->mode = QXL_MODE_UNDEFINED;
> >+        if (d->primary_created) {
> >+            d->primary_created = 0;
> >              async = qemu_mallocz(sizeof(*async));
> >              goto async_common;
> >          } else {
> >@@ -1218,13 +1227,20 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >              break;
> >          }
> >      case QXL_IO_UPDATE_AREA_ASYNC:
> >+        dprint(d, 1, "QXL_IO_UPDATE_AREA_ASYNC %d\n", val);
> >          async = qemu_mallocz(sizeof(*async));
> >          async->update_area = d->ram->update_area;
> >          async->update_surface = d->ram->update_surface;
> >          goto async_common;
> >      case QXL_IO_NOTIFY_OOM_ASYNC:
> >+        goto async_common;
> >      case QXL_IO_DESTROY_SURFACE_ASYNC:
> >+        if (val == 0) {
> >+            d->primary_created = 0;
> >+        }
> >+        goto async_common;
> >      case QXL_IO_DESTROY_ALL_SURFACES_ASYNC:
> >+        d->primary_created = 0;
> >          async = qemu_mallocz(sizeof(*async));
> >      async_common:
> >          async->port = io_port;
> >diff --git a/hw/qxl.h b/hw/qxl.h
> >index 584f02b..893f8d8 100644
> >--- a/hw/qxl.h
> >+++ b/hw/qxl.h
> >@@ -23,6 +23,7 @@ typedef struct PCIQXLDevice {
> >      uint32_t           guestdebug;
> >      uint32_t           cmdlog;
> >      enum qxl_mode      mode;
> >+    uint32_t           primary_created;
> >      uint32_t           cmdflags;
> >      int                generation;
> >      uint32_t           revision;
>