Message ID | 1309348641-20061-13-git-send-email-alevy@redhat.com |
---|---|
State | New |
Headers | show |
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
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
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 >
>>> + 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
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 --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);