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

[Yonit Halperin]

On 07/07/2011 12:43 PM, Alon Levy wrote:
> 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)
>   DrvEnableSurface(PDEV#2)
>
> 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
> hibernating).
>
> So the new flow for DriveAssertMode is then:
>
> AssertModeDisable:
>   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
>   mode with DUMP_MSPACE_VRAM and DUMP_MSPACE_DEVRAM).
>   5. Destroy primary surface
>   6. Delete vram memslot
>
> AssertModeEnable:
>   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.
>
> Cc: Yonit Halperin<yhalperi at redhat.com>
> ---
>   display/driver.c  |   69 ++++++++++++++++++++++++++++++++++++++++++++++------
>   display/surface.c |    1 +
>   2 files changed, 62 insertions(+), 8 deletions(-)
>
> diff --git a/display/driver.c b/display/driver.c
> index a231bb4..fff462d 100644
> --- a/display/driver.c
> +++ b/display/driver.c
> @@ -939,7 +939,9 @@ 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
I can't understand from msdn if DrvAssertMode(disable) is always called 
before DrvDisableSurface
> +    pdev->surf_enable = FALSE;
>       UnmapFB(pdev);
>
>       if (pdev->surf) {
> @@ -953,25 +955,76 @@ 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));
> +}
> +
> +static BOOL AssertModeDisable(PDev *pdev)
> +{
> +    DEBUG_PRINT((pdev, 3, "%s entry\n", __FUNCTION__));
> +    /* flush command ring and update all surfaces */
> +    WRITE_PORT_UCHAR(pdev->notify_cmd_port, 0);
> +    WRITE_PORT_UCHAR(pdev->notify_cursor_port, 0);
don't think the above two IOs are needed. flush_surface will do the job.
> +    WRITE_PORT_UCHAR(pdev->flush_surfaces_port, 0);
> +    WRITE_PORT_UCHAR(pdev->destroy_all_surfaces_port, 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 */
> +    WRITE_PORT_UCHAR(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;
> +}
> +
> +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 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->enabled == enable) {
> +        DEBUG_PRINT((pdev, 1, "%s: called twice with same argument (%d)\n", __FUNCTION__,
> +            enable));
> +        return TRUE;
> +    }
> +    pdev->enabled = enable;
>       if (enable) {
> -        InitResources(pdev);
> -        EnableQXLPrimarySurface(pdev);
> -        CreateVRamSlot(pdev);
> +        AssertModeEnable(pdev);
>       } else {
> -        DisableQXLPrimarySurface(pdev, 0);
> -        RemoveVRamSlot(pdev);
> +        ret = AssertModeDisable(pdev);
I'm not sure about changing  pdev->enabled if AssertModeDisable fails 
(which means part of the surfaces are still on the vram)
>       }
>       DEBUG_PRINT((pdev, 1, "%s: 0x%lx exit %d\n", __FUNCTION__, pdev, enable));
> -    return TRUE;
> +    return ret;
>   }
>
>   ULONG DrvGetModes(HANDLE driver, ULONG dev_modes_size, DEVMODEW *dev_modes)
> diff --git a/display/surface.c b/display/surface.c
> index 8f7aae1..b8628fe 100644
> --- a/display/surface.c
> +++ b/display/surface.c
> @@ -249,6 +249,7 @@ BOOL MoveSurfaceToVideoRam(PDev *pdev, UINT32 surface_id)
>       surface_info->phys_mem = phys_mem;
>       surface_info->draw_area.base_mem = base_mem;
>       surface = SurfaceCmd(pdev, QXL_SURFACE_CMD_CREATE, surface_id);
> +    surface->flags |= QXL_SURF_FLAG_KEEP_DATA;
>       surface->u.surface_create.format = surface_format;
>       surface->u.surface_create.width = cx;
>       surface->u.surface_create.height = cy;