[Spice-devel,qxl-win,v4,3/3] display/driver: reimplement DrvAssertMode for Suspend+Hibernate (S3+S4) support

Submitted by Alon Levy on July 27, 2011, 11:17 p.m.


Message ID 1311783443-7040-4-git-send-email-alevy@redhat.com
State New, archived
Headers show

Not browsing as part of any series.

Commit Message

Alon Levy July 27, 2011, 11:17 p.m.
our old code did a very minimal flow good for resolution change:
 DrvAssertMode False (disable)
  destroy primary surface
  delete vram memslot
 DrvAssertMode True (enable)
  create primary surface (destroyed on disable)
  create vram memslot

Aside: Importantly the flow for resolution change involves two pdevs, so actually the
enable call is not called, only:
 DrvAssertMode(PDEV#1, FALSE)

EnableSurface creates a primary surface, so the call to AssertMode must destroy it.

This fails on suspend for many reasons, one of them being that we don't disable
operations to any driver managed off screen surfaces, and the other is that
after the qxl reset done via acpi S3 request by windows, we don't reinitialize
the primary memslot (this is fixed by a previous commit).

The correct (per example drivers from WinDDK, and per msdn) thing to do in
DrvAssertMode is to not do any further interaction with the device. The
simplest way to achieve that is to fail any operation. The GDI is designed such
that it can work completely without any driver, so for any operation there is a
fallback in case the driver returns a failure error code. A simplification is
to use EngModifySurface to move a surface to GDI control and so not to get any
further callback on it (except for the deletion callback when it is deleted).
This is also done in the 3dlabs example driver.

There is zero synchronization between the miniport, which knows about the power
state, and the displayport, which knows about the pci device structure, command
rings, resources.

As a result the easiest and also consistent with the above requirements
implementation for suspend to ram and to disk is to reset any server side state
during DrvAssertMode(FALSE), copying all volatile (surface contents only atm)
memory to guest ram (from there windows will copy it to disk for us if we are

So the new flow for DriveAssertMode is then:

 1. set pdev->enable to False (all further operations will be punted)
 2. tell server to prepare for sleep, via new QXL_IO_UPDATE_MEM(QXL_UPDATE_MEM_RENDER_ALL):
  server updates all surfaces
  server destroys all surfaces. Since we never sent it a destroy command
   this doesn't trigger any guest side surface destruction, only a release of
   the creation command resources.
 3. release anything in the release ring.
 4. tell server to write it's current releasable items list (qxl->last_release)
 to the release_ring (we just made sure the release_ring is empty, and since we
 are not sending anything new to the worker and we already had it render
 anything outstanding it will not fill it) release the last resources (at this
 point there is nothing allocated on devram and vram - we verify that in debug
 5. Destroy primary surface
 6. Delete vram memslot

 1. Create primary surface
 2. create vram memslot
 3. copy surfaces from ram to vram, sending create commands which cause both
 create and surface image commands to be sent to the client.

In suspend there is a single PDEV involved which does exactly those two calls,
disable before sleep and enable after.

For resolution change this work is excessive but correct. Since
miniport:SetPowerState is called after DrvAssertMode during suspend (actually
there is no defined order in the documentation), there is no way to distinguish
between the two anyway.

From: Alon Levy <alevy@redhat.com>
From: Yonit Halperin <yhalperi@redhat.com>
 display/driver.c  |   84 +++++++++++++++++++++++++++++++++++++++++++++++++---
 display/surface.c |   14 ++++++--
 2 files changed, 89 insertions(+), 9 deletions(-)

Patch hide | download patch | download mbox

diff --git a/display/driver.c b/display/driver.c
index 54913b8..00dd7ec 100644
--- a/display/driver.c
+++ b/display/driver.c
@@ -937,7 +937,11 @@  VOID DrvDisableSurface(DHPDEV in_pdev)
     DEBUG_PRINT((pdev, 1, "%s: 0x%lx\n", __FUNCTION__, pdev));
-    DisableQXLPrimarySurface(pdev, 1 /* hide mouse */);
+    // Don't destroy the primary - it's destroyed by destroy_all_surfaces
+    // at AssertModeDisable. Also, msdn specifically mentions DrvDisableSurface
+    // should not touch the hardware, that should be done just via DrvAssertMode
+    // (http://msdn.microsoft.com/en-us/library/ff556200%28VS.85%29.aspx)
+    pdev->surf_enable = FALSE;
     if (pdev->surf) {
@@ -951,15 +955,56 @@  VOID DrvDisableSurface(DHPDEV in_pdev)
         pdev->mem_slots = NULL;
-    DEBUG_PRINT((NULL, 1, "%s: 0x%lx exit\n", __FUNCTION__, pdev));
+    DEBUG_PRINT((pdev, 1, "%s: 0x%lx exit\n", __FUNCTION__, pdev));
-BOOL DrvAssertMode(DHPDEV in_pdev, BOOL enable)
+static BOOL AssertModeDisable(PDev *pdev)
-    PDev* pdev = (PDev*)in_pdev;
+    DEBUG_PRINT((pdev, 3, "%s entry\n", __FUNCTION__));
+    /* flush command ring and update all surfaces */
+    async_io(pdev, ASYNCABLE_FLUSH_SURFACES, 0);
+    async_io(pdev, ASYNCABLE_DESTROY_ALL_SURFACES, 0);
+    /* move all surfaces from device to system memory */
+    if (!MoveAllSurfacesToRam(pdev)) {
+        return FALSE;
+    }
+    /* Free release ring contents */
+    ReleaseCacheDeviceMemoryResources(pdev);
+    EmptyReleaseRing(pdev);
+    /* Get the last free list onto the release ring */
+    sync_io(pdev, pdev->flush_release_port, 0);
+    DEBUG_PRINT((pdev, 4, "%s after FLUSH_RELEASE\n", __FUNCTION__));
+    /* And release that. mspace allocators should be clean after. */
+    EmptyReleaseRing(pdev);
+    RemoveVRamSlot(pdev);
+    DebugCountAliveSurfaces(pdev);
+    DEBUG_PRINT((pdev, 4, "%s: [%d,%d] [%d,%d] [%d,%d] %lx\n", __FUNCTION__,
+        pdev->cmd_ring->prod, pdev->cmd_ring->cons,
+        pdev->cursor_ring->prod, pdev->cursor_ring->cons,
+        pdev->release_ring->prod, pdev->release_ring->cons,
+        pdev->Res->free_outputs));
+    DEBUG_PRINT((pdev, 4, "%s exit\n", __FUNCTION__));
+    return TRUE;
-    DEBUG_PRINT((pdev, 1, "%s: 0x%lx %d\n", __FUNCTION__, pdev, enable));
+static void AssertModeEnable(PDev *pdev)
+    InitResources(pdev);
+    InitDeviceMemoryResources(pdev);
+    DEBUG_PRINT((pdev, 3, "%s: [%d,%d] [%d,%d] [%d,%d] %lx\n", __FUNCTION__,
+        pdev->cmd_ring->prod, pdev->cmd_ring->cons,
+        pdev->cursor_ring->prod, pdev->cursor_ring->cons,
+        pdev->release_ring->prod, pdev->release_ring->cons,
+        pdev->Res->free_outputs));
+    EnableQXLPrimarySurface(pdev);
+    CreateVRamSlot(pdev);
+    DebugCountAliveSurfaces(pdev);
+    MoveAllSurfacesToVideoRam(pdev);
+    DebugCountAliveSurfaces(pdev);
+BOOL DrvAssertModeOld(PDev *pdev, BOOL enable)
     if (enable) {
@@ -972,6 +1017,35 @@  BOOL DrvAssertMode(DHPDEV in_pdev, BOOL enable)
     return TRUE;
+BOOL DrvAssertMode(DHPDEV in_pdev, BOOL enable)
+    PDev* pdev = (PDev*)in_pdev;
+    BOOL ret = TRUE;
+    DEBUG_PRINT((pdev, 1, "%s: 0x%lx %d\n", __FUNCTION__, pdev, enable));
+    if (pdev->pci_revision < QXL_REVISION_STABLE_V10) {
+        return DrvAssertModeOld(pdev, enable);
+    }
+    /* new implementation that works correctly only with newer devices
+     * that implement QXL_IO_FLUSH_RELEASE */
+    if (pdev->enabled == enable) {
+        DEBUG_PRINT((pdev, 1, "%s: called twice with same argument (%d)\n", __FUNCTION__,
+            enable));
+        return TRUE;
+    }
+    pdev->enabled = enable;
+    if (enable) {
+        AssertModeEnable(pdev);
+    } else {
+        ret = AssertModeDisable(pdev);
+        if (!ret) {
+            pdev->enabled = !enable;
+        }
+    }
+    DEBUG_PRINT((pdev, 1, "%s: 0x%lx exit %d\n", __FUNCTION__, pdev, enable));
+    return ret;
 ULONG DrvGetModes(HANDLE driver, ULONG dev_modes_size, DEVMODEW *dev_modes)
diff --git a/display/surface.c b/display/surface.c
index 519d613..b40721d 100644
--- a/display/surface.c
+++ b/display/surface.c
@@ -136,11 +136,15 @@  static UINT8 *CreateSurfaceHelper(PDev *pdev, UINT32 surface_id,
 static void SendSurfaceCreateCommand(PDev *pdev, UINT32 surface_id, SIZEL size,
-                                     UINT32 surface_format, INT32 stride, QXLPHYSICAL phys_mem)
+                                     UINT32 surface_format, INT32 stride, QXLPHYSICAL phys_mem,
+                                     int keep_data)
     QXLSurfaceCmd *surface;
     surface = SurfaceCmd(pdev, QXL_SURFACE_CMD_CREATE, surface_id);
+    if (keep_data) {
+        surface->flags |= QXL_SURF_FLAG_KEEP_DATA;
+    }
     surface->u.surface_create.format = surface_format;
     surface->u.surface_create.width = size.cx;
     surface->u.surface_create.height = size.cy;
@@ -185,7 +189,7 @@  HBITMAP CreateDeviceBitmap(PDev *pdev, SIZEL size, ULONG format, QXLPHYSICAL *ph
     if (allocation_type != DEVICE_BITMAP_ALLOCATION_TYPE_SURF0) {
-        SendSurfaceCreateCommand(pdev, surface_id, size, surface_format, -stride, *phys_mem);
+        SendSurfaceCreateCommand(pdev, surface_id, size, surface_format, -stride, *phys_mem, 0);
     return hbitmap;
@@ -269,7 +273,7 @@  BOOL MoveSurfaceToVideoRam(PDev *pdev, UINT32 surface_id)
     surface_info->copy = NULL;
     SendSurfaceCreateCommand(pdev, surface_id, surface_info->size, surface_format,
-                             -stride, phys_mem);
+                             -stride, phys_mem, 1);
     return TRUE;
@@ -356,7 +360,9 @@  BOOL MoveAllSurfacesToRam(PDev *pdev)
                          __FUNCTION__, surface_id));
             phys_mem = SurfaceToPhysical(pdev, surface_info->draw_area.base_mem);
             SendSurfaceCreateCommand(pdev, surface_id, surf_obj->sizlBitmap,
-                                     surface_info->bitmap_format, -surf_obj->lDelta, phys_mem);
+                                     surface_info->bitmap_format, -surf_obj->lDelta, phys_mem,
+                                     /* the surface is still there, tell server not to erase */
+                                     1);
             return FALSE;
         QXLDelSurface(pdev, surface_info->draw_area.base_mem, DEVICE_BITMAP_ALLOCATION_TYPE_VRAM);