| Message ID | 1308920577-31569-13-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
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;
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; >
Hi, >> 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. I agree with yonit here, moving to UNDEFINED makes sense. Keeping track of guest driver state in qxl is asking for trouble. It will blow up when it comes to S4 support and we'll exit and restart the qemu process. cheers, Gerd
On Wed, Jun 29, 2011 at 10:41:43AM +0200, Gerd Hoffmann wrote: > Hi, > > >>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. > > I agree with yonit here, moving to UNDEFINED makes sense. Keeping > track of guest driver state in qxl is asking for trouble. It will > blow up when it comes to S4 support and we'll exit and restart the > qemu process. Actually S4 support == S3 support. There is absolutely no difference. Since qemu still does a reset. But I already checked dropping this patch and using UNDEFINED instead and it works fine. I just had to additionally allow io when in UNDEFINED. I think that makes sense, since UNDEFINED is just the same as "driver working but primary destroyed". > > cheers, > Gerd > >
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;