diff mbox

[PATCHv3] qxl: add QXL_IO_FLUSH_{SURFACES, RELEASE} for guest S3&S4 support

Message ID 1309348641-20061-13-git-send-email-alevy@redhat.com
State New
Headers show

Commit Message

Alon Levy June 29, 2011, 11:57 a.m. UTC 
Add two new IOs.
 QXL_IO_FLUSH_SURFACES - equivalent to update area for all surfaces, used
  to reduce vmexits from NumSurfaces to 1 on guest S3, S4 and resolution change (windows
  driver implementation is such that this is done on each of those occasions).
 QXL_IO_FLUSH_RELEASE - used to ensure anything on last_release is put on the release ring
  for the client to free.

Cc: Yonit Halperin <yhalperi@redhat.com>
---
 hw/qxl.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

Comments

Gerd Hoffmann June 29, 2011, 1:06 p.m. UTC | #1 
Hi,

> +    case QXL_IO_FLUSH_SURFACES:
> +        dprint(d, 1, "QXL_IO_FLUSH_SURFACES (%d) entry (%s, s#=%d, res#=%d)\n",
> +            val, qxl_mode_to_string(d->mode), d->guest_surfaces.count,
> +            d->num_free_res);
> +        qemu_spice_stop(&d->ssd);
> +        qemu_spice_start(&d->ssd);
> +        dprint(d, 1, "QXL_IO_FLUSH_SURFACES exit (%s, s#=%d, res#=%d,%p)\n",
> +            qxl_mode_to_string(d->mode), d->guest_surfaces.count,
> +            d->num_free_res, d->last_release);
> +        break;

This should be async as we'll go sleep and wait for the spice server 
thread finish in qemu_spice_stop().

> +    case QXL_IO_FLUSH_RELEASE: {
> +        QXLReleaseRing *ring =&d->ram->release_ring;
> +        if (ring->prod - ring->cons + 1 == ring->num_items) {
> +            // TODO - "return" a value to the guest and let it loop?
                   ^^^^
Hmm.

cheers,
   Gerd
Alon Levy June 29, 2011, 2:27 p.m. UTC | #2 
On Wed, Jun 29, 2011 at 03:06:33PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >+    case QXL_IO_FLUSH_SURFACES:
> >+        dprint(d, 1, "QXL_IO_FLUSH_SURFACES (%d) entry (%s, s#=%d, res#=%d)\n",
> >+            val, qxl_mode_to_string(d->mode), d->guest_surfaces.count,
> >+            d->num_free_res);
> >+        qemu_spice_stop(&d->ssd);
> >+        qemu_spice_start(&d->ssd);
> >+        dprint(d, 1, "QXL_IO_FLUSH_SURFACES exit (%s, s#=%d, res#=%d,%p)\n",
> >+            qxl_mode_to_string(d->mode), d->guest_surfaces.count,
> >+            d->num_free_res, d->last_release);
> >+        break;
> 
> This should be async as we'll go sleep and wait for the spice server
> thread finish in qemu_spice_stop().

Yeah, I meant to do that, forgot, thanks for catching it.

> 
> >+    case QXL_IO_FLUSH_RELEASE: {
> >+        QXLReleaseRing *ring =&d->ram->release_ring;
> >+        if (ring->prod - ring->cons + 1 == ring->num_items) {
> >+            // TODO - "return" a value to the guest and let it loop?
>                   ^^^^
> Hmm.
So the story goes: I wrote this, but didn't actually see this happen in practice,
particularily since the driver empties the release ring. The simplest would be to
replace it with some fprintf(stderr)

> 
> cheers,
>   Gerd
Alon Levy June 29, 2011, 2:45 p.m. UTC | #3 
On Wed, Jun 29, 2011 at 03:06:33PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >+    case QXL_IO_FLUSH_SURFACES:
> >+        dprint(d, 1, "QXL_IO_FLUSH_SURFACES (%d) entry (%s, s#=%d, res#=%d)\n",
> >+            val, qxl_mode_to_string(d->mode), d->guest_surfaces.count,
> >+            d->num_free_res);
> >+        qemu_spice_stop(&d->ssd);
> >+        qemu_spice_start(&d->ssd);
> >+        dprint(d, 1, "QXL_IO_FLUSH_SURFACES exit (%s, s#=%d, res#=%d,%p)\n",
> >+            qxl_mode_to_string(d->mode), d->guest_surfaces.count,
> >+            d->num_free_res, d->last_release);
> >+        break;
> 
> This should be async as we'll go sleep and wait for the spice server
> thread finish in qemu_spice_stop().

ok, so don't cherry-pick the ioport_to_string patch yet since I'm doing
s/FLUSH_SURFACES/FLUSH_SURFACES_ASYNC/

> 
> >+    case QXL_IO_FLUSH_RELEASE: {
> >+        QXLReleaseRing *ring =&d->ram->release_ring;
> >+        if (ring->prod - ring->cons + 1 == ring->num_items) {
> >+            // TODO - "return" a value to the guest and let it loop?
>                   ^^^^
> Hmm.
> 
> cheers,
>   Gerd
>
Gerd Hoffmann June 29, 2011, 2:50 p.m. UTC | #4 
>>> +    case QXL_IO_FLUSH_RELEASE: {
>>> +        QXLReleaseRing *ring =&d->ram->release_ring;
>>> +        if (ring->prod - ring->cons + 1 == ring->num_items) {
>>> +            // TODO - "return" a value to the guest and let it loop?
>>                    ^^^^
>> Hmm.
> So the story goes: I wrote this, but didn't actually see this happen in practice,
> particularily since the driver empties the release ring. The simplest would be to
> replace it with some fprintf(stderr)

How do you think this could happen?  If there are no unprocessed 
requests in the pipeline (shouldn't be, all surfaces are flushed to 
device memory and destroyedv at that point) and the driver cares empty 
the release ring before calling this it should not happen, right?

cheers,
   Gerd
Alon Levy June 29, 2011, 6:22 p.m. UTC | #5 
On Wed, Jun 29, 2011 at 04:50:10PM +0200, Gerd Hoffmann wrote:
> >>>+    case QXL_IO_FLUSH_RELEASE: {
> >>>+        QXLReleaseRing *ring =&d->ram->release_ring;
> >>>+        if (ring->prod - ring->cons + 1 == ring->num_items) {
> >>>+            // TODO - "return" a value to the guest and let it loop?
> >>                   ^^^^
> >>Hmm.
> >So the story goes: I wrote this, but didn't actually see this happen in practice,
> >particularily since the driver empties the release ring. The simplest would be to
> >replace it with some fprintf(stderr)
> 
> How do you think this could happen?  If there are no unprocessed
> requests in the pipeline (shouldn't be, all surfaces are flushed to
> device memory and destroyedv at that point) and the driver cares
> empty the release ring before calling this it should not happen,
> right?

Yes. The point was to check anyway, it should never happen with our driver,
but a check can catch an error I guess.

> 
> cheers,
>   Gerd
>
diff mbox

Patch

diff --git a/hw/qxl.c b/hw/qxl.c
index 29425a5..f158d45 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1247,6 +1247,30 @@  static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
     case QXL_IO_DESTROY_ALL_SURFACES:
         qxl_spice_destroy_surfaces(d);
         break;
+    case QXL_IO_FLUSH_SURFACES:
+        dprint(d, 1, "QXL_IO_FLUSH_SURFACES (%d) entry (%s, s#=%d, res#=%d)\n",
+            val, qxl_mode_to_string(d->mode), d->guest_surfaces.count,
+            d->num_free_res);
+        qemu_spice_stop(&d->ssd);
+        qemu_spice_start(&d->ssd);
+        dprint(d, 1, "QXL_IO_FLUSH_SURFACES exit (%s, s#=%d, res#=%d,%p)\n",
+            qxl_mode_to_string(d->mode), d->guest_surfaces.count,
+            d->num_free_res, d->last_release);
+        break;
+    case QXL_IO_FLUSH_RELEASE: {
+        QXLReleaseRing *ring = &d->ram->release_ring;
+        if (ring->prod - ring->cons + 1 == ring->num_items) {
+            // TODO - "return" a value to the guest and let it loop?
+            fprintf(stderr,
+                "ERROR: no flush, full release ring [p%d,%dc]\n",
+                ring->prod, ring->cons);
+        }
+        qxl_push_free_res(d, 1 /* flush */);
+        dprint(d, 1, "QXL_IO_FLUSH_RELEASE exit (%s, s#=%d, res#=%d,%p)\n",
+            qxl_mode_to_string(d->mode), d->guest_surfaces.count,
+            d->num_free_res, d->last_release);
+        break;
+    }
     case QXL_IO_MEMSLOT_ADD_ASYNC:
         PANIC_ON(val >= NUM_MEMSLOTS);
         PANIC_ON(d->guest_slots[val].active);