Re: [Qemu-devel] [PATCH v2] qxl: add primary_created state, change mode lifetimes
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 > >
Re: [Qemu-devel] [PATCH v2] qxl: add primary_created state, change mode lifetimes
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
Re: [Qemu-devel] [PATCH v2] qxl: add primary_created state, change mode lifetimes
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)
> > cas
Re: [Qemu-devel] [PATCH v2] qxl: add primary_created state, change mode lifetimes
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) {
+
