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

[yhalperi <yhalperi@redhat.com>]

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)
> 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;