[Spice-devel] [PATCH qxl-win v3] display/surface: add surfaces from/to ram

Thu Jul 7 06:58:15 PDT 2011

On 07/07/2011 04:47 PM, Yonit Halperin wrote:
> Hi,
> Generally, when parameters of a function are divided into several lines,
> they should be aligned to the same column.
>
>
> On 07/07/2011 12:43 PM, Alon Levy wrote:
>> Adds fields to SurfaceInfo to cache data previously only available
>> via SurfaceArea::draw_area.
>>
>> Adds two functions to save and restore surfaces from ram:
>>
>> MoveAllSurfacesToVideoRam
>> allocates and copies surfaces from vram to ram, and calls
>> EngModifySurface
>> with an empty hook list to make those surfaces completely managed by
>> gdi and not
>> us.
>>
>> MoveAllSurfacesToRam
>> recreates surfaces on vram and calls EngModifySurface with
>> QXL_SURFACE_HOOKS, and
>> finally sends a QXL_SURFACE_CMD_CREATE with the valid data flag to
>> make the server
>> send a surface image message after the surface create message.
>>
>> Cc: Yonit Halperin<yhalperi at redhat.com>
>> ---
>> display/driver.c | 3 +-
>> display/qxldd.h | 8 ++
>> display/surface.c | 212
>> ++++++++++++++++++++++++++++++++++++++++++++++++----
>> display/surface.h | 3 +
>> 4 files changed, 208 insertions(+), 18 deletions(-)
>>
>> diff --git a/display/driver.c b/display/driver.c
>> index 8375eff..bbad696 100644
>> --- a/display/driver.c
>> +++ b/display/driver.c
>> @@ -1302,7 +1302,8 @@ VOID APIENTRY DrvDeleteDeviceBitmap(DHSURF dhsurf)
>>
>> ASSERT(pdev, surface_id< pdev->n_surfaces);
>>
>> - DeleteDeviceBitmap(surface->u.pdev, surface_id,
>> DEVICE_BITMAP_ALLOCATION_TYPE_VRAM);
>> + DeleteDeviceBitmap(surface->u.pdev, surface_id,
>> + surface->copy ? DEVICE_BITMAP_ALLOCATION_TYPE_RAM :
>> DEVICE_BITMAP_ALLOCATION_TYPE_VRAM);
>> }
>>
>> #ifdef CALL_TEST
>> diff --git a/display/qxldd.h b/display/qxldd.h
>> index af9cb3d..15be2fa 100644
>> --- a/display/qxldd.h
>> +++ b/display/qxldd.h
>> @@ -189,6 +189,14 @@ typedef struct DrawArea {
>> typedef struct SurfaceInfo SurfaceInfo;
>> struct SurfaceInfo {
>> DrawArea draw_area;
>> + /* TODO: all these fields are dual to DrawArea. But we destroy
>> draw_area when
>> + * switching to ram. Can we not destroy it but just disable it and
>> keep using it's
>> + * fields opaquely? */
> I guess you meant the SURFOBJ inside DrawArea and the size and
> bitmap_format fields. I think it is o.k to save these fields inside
> surface_info, and we should unlock the SURFOBJ, since the surface its
> referring to no longer exists. From msdn:
> The driver is responsible for unlocking the surface when it no longer
> needs it. Surfaces should be locked only for very short periods of time.
>
>> + QXLPHYSICAL phys_mem;
> we don't use this field
>> + HBITMAP hbitmap;
>> + SIZEL size;
>> + UINT8 *copy;
>> + ULONG bitmap_format;
>> union {
>> PDev *pdev;
>> SurfaceInfo *next_free;
>> diff --git a/display/surface.c b/display/surface.c
>> index 63e830c..8f7aae1 100644
>> --- a/display/surface.c
>> +++ b/display/surface.c
>> @@ -83,48 +83,70 @@ static VOID FreeDrawArea(DrawArea *drawarea)
>> }
>> }
>>
>> -HBITMAP CreateDeviceBitmap(PDev *pdev, SIZEL size, ULONG format,
>> QXLPHYSICAL *phys_mem,
>> - UINT8 **base_mem, UINT32 surface_id, UINT8 allocation_type)
>> +static void BitmapFormatToDepthAndSurfaceFormat(ULONG format, UINT32
>> *depth, UINT32 *surface_format)
>> {
>> - UINT32 surface_format, depth;
>> - HBITMAP surf;
>> - UINT32 stride;
>> -
>> switch (format) {
>> case BMF_16BPP:
>> - surface_format = SPICE_SURFACE_FMT_16_555;
>> - depth = 16;
>> + *surface_format = SPICE_SURFACE_FMT_16_555;
>> + *depth = 16;
>> break;
>> case BMF_24BPP:
>> case BMF_32BPP:
>> - surface_format = SPICE_SURFACE_FMT_32_xRGB;
>> - depth = 32;
>> + *surface_format = SPICE_SURFACE_FMT_32_xRGB;
>> + *depth = 32;
>> break;
>> - case BMF_8BPP:
>> default:
>> - return 0;
>> + *depth = 0;
>> + break;
>> };
>> +}
>> +
>>
>> - if (!(surf = EngCreateDeviceBitmap((DHSURF)GetSurfaceInfo(pdev,
>> surface_id), size, format))) {
>> +HBITMAP CreateDeviceBitmap(PDev *pdev, SIZEL size, ULONG format,
>> QXLPHYSICAL *phys_mem,
>> + UINT8 **base_mem, UINT32 surface_id, UINT8 allocation_type)
>> +{
>> + UINT32 surface_format, depth;
>> + HBITMAP hbitmap;
>> + UINT32 stride;
>> + SurfaceInfo *surface_info;
>> +
>> + BitmapFormatToDepthAndSurfaceFormat(format,&depth,&surface_format);
>> + if (!depth) {
>> + return 0;
>> + }
>> +
>> + DEBUG_PRINT((pdev, 9, "%s: %p: %d, (%dx%d), %d\n", __FUNCTION__,
>> pdev, surface_id,
>> + size.cx, size.cy, format));
>> + if (!(hbitmap = EngCreateDeviceBitmap((DHSURF)GetSurfaceInfo(pdev,
>> surface_id), size, format))) {
>> DEBUG_PRINT((pdev, 0, "%s: EngCreateDeviceBitmap failed, pdev 0x%lx,
>> surface_id=%d\n",
>> __FUNCTION__, pdev, surface_id));
>> goto out_error1;
>> }
>>
>> - if (!EngAssociateSurface((HSURF)surf, pdev->eng, QXL_SURFACE_HOOKS)) {
>> + if (!EngAssociateSurface((HSURF)hbitmap, pdev->eng,
>> QXL_SURFACE_HOOKS)) {
>> DEBUG_PRINT((pdev, 0, "%s: EngAssociateSurface failed\n", __FUNCTION__));
>> goto out_error2;
>> }
>>
>> - GetSurfaceInfo(pdev, surface_id)->u.pdev = pdev;
>> + surface_info = GetSurfaceInfo(pdev, surface_id);
>> + surface_info->phys_mem = 0;
>> + surface_info->u.pdev = pdev;
>> + surface_info->hbitmap = hbitmap;
>> + surface_info->copy = NULL;
>> + surface_info->size = size;
>> + surface_info->bitmap_format = format;
>>
>> QXLGetSurface(pdev, phys_mem, size.cx, size.cy, depth,
>> &stride, base_mem, allocation_type);
>> if (!*base_mem) {
>> + DEBUG_PRINT((pdev, 0, "%s: %p: QXLGetSurface failed (%d)\n",
>> __FUNCTION__, pdev,
>> + allocation_type));
>> goto out_error2;
>> }
>> + surface_info->phys_mem = *phys_mem;
>>
>> if (!CreateDrawArea(pdev, *base_mem, format, size.cx, size.cy, stride,
>> surface_id)) {
>> + DEBUG_PRINT((pdev, 0, "%s: CreateDrawArea failed\n", __FUNCTION__));
>> goto out_error3;
>> }
>>
>> @@ -140,12 +162,12 @@ HBITMAP CreateDeviceBitmap(PDev *pdev, SIZEL
>> size, ULONG format, QXLPHYSICAL *ph
>> PushSurfaceCmd(pdev, surface);
>> }
>>
>> - return surf;
>> + return hbitmap;
>>
>> out_error3:
>> QXLDelSurface(pdev, *base_mem, allocation_type);
>> out_error2:
>> - EngDeleteSurface((HSURF)surf);
>> + EngDeleteSurface((HSURF)hbitmap);
>> out_error1:
>> return 0;
>> }
>> @@ -176,3 +198,159 @@ VOID DeleteDeviceBitmap(PDev *pdev, UINT32
>> surface_id, UINT8 allocation_type)
>> }
>> }
>> }
>> +
>> +BOOL MoveSurfaceToVideoRam(PDev *pdev, UINT32 surface_id)
>> +{
> I think the part of QxlGetSurface and CreateDrawArea can be shared with
> CreateDeviceBitmap
>
>> + QXLSurfaceCmd *surface;
>> + UINT32 surface_format;
>> + UINT32 depth;
>> + int count_used = 0;
>> + int size;
>> + INT32 stride = 0;
>> + UINT8 *base_mem;
>> + QXLPHYSICAL phys_mem;
>> + SurfaceInfo *surface_info = GetSurfaceInfo(pdev, surface_id);
>> + UINT32 cx = surface_info->size.cx;
>> + UINT32 cy = surface_info->size.cy;
>> +
>> +
>> BitmapFormatToDepthAndSurfaceFormat(surface_info->bitmap_format,&depth,&surface_format);
>>
>> + ASSERT(pdev, depth != 0);
>> + QXLGetSurface(pdev,&phys_mem, cx, cy, depth,&stride,
>> +&base_mem, DEVICE_BITMAP_ALLOCATION_TYPE_VRAM);
>> + DEBUG_PRINT((pdev, 3,
>> + "%s: %d, pm %0lX, fmt %d, d %d, s (%d, %d) st %d\n",
>> + __FUNCTION__, surface_id, (uint64_t)surface_info->phys_mem,
>> surface_format,
>> + depth, cx, cy, stride));
>> + size = abs(stride) * cy;
>> + if (!base_mem) {
>> + DEBUG_PRINT((pdev, 0, "%s: %p: %d: QXLGetSurface failed (%d bytes
>> alloc)\n",
>> + __FUNCTION__, pdev, surface_id, size));
>> + return FALSE;
>> + }
>> + if (!CreateDrawArea(pdev, base_mem, surface_info->bitmap_format, cx,
>> cy, stride, surface_id)) {
>> + DEBUG_PRINT((pdev, 0, "%s: %p: %d: CreateDrawArea failed\n",
>> + __FUNCTION__, pdev, surface_id, size));
>> + goto error_create_draw_area;
>> + }
>> + if (!EngModifySurface((HSURF)surface_info->hbitmap, pdev->eng,
>> QXL_SURFACE_HOOKS,
>> + MS_NOTSYSTEMMEMORY, (DHSURF)surface_info, NULL, 0, NULL)) {
>> + DEBUG_PRINT((pdev, 0, "%s: %p: %d: EngModifySurface failed\n",
>> + __FUNCTION__, pdev, surface_id, size));
>> + goto error_eng_modify_surface;
>> + }
>> + DEBUG_PRINT((pdev, 3, "%s: stride = %d, phys_mem = %p, base_mem =
>> %p\n",
>> + __FUNCTION__, -stride, surface_info->phys_mem,
>> + DEBUG_PRINT((pdev, 3, "%s: copy %d bytes to %d\n", __FUNCTION__,
>> size, surface_id));
>> + RtlCopyMemory(surface_info->draw_area.base_mem, surface_info->copy,
>> size);
>> + EngFreeMem(surface_info->copy);
>> + surface_info->copy = NULL;
>> + // Everything ok - commit changes to surface_info and send create
>> command to device.
>> + surface_info->phys_mem = phys_mem;
>> + surface_info->draw_area.base_mem = base_mem;
>> + surface = SurfaceCmd(pdev, QXL_SURFACE_CMD_CREATE, surface_id);
>> + surface->u.surface_create.format = surface_format;
>> + surface->u.surface_create.width = cx;
>> + surface->u.surface_create.height = cy;
>> + surface->u.surface_create.stride = -stride;
>> + surface->u.surface_create.data = surface_info->phys_mem;
>> + PushSurfaceCmd(pdev, surface);
>> + return TRUE;
>> +
>> +error_eng_modify_surface:
>> + FreeDrawArea(&surface_info->draw_area);
>> +error_create_draw_area:
>> + // TODO: Why did it fail? nothing in the MSDN
>> + QXLDelSurface(pdev, base_mem, DEVICE_BITMAP_ALLOCATION_TYPE_VRAM);
>> + return FALSE;
>> +}
>> +
>> +/* when we return from S3 we need to resend all the surface creation
>> commands.
>> + * Actually moving the memory vram<->guest is not strictly neccessary
>> since vram
>> + * is not reset during the suspend, so contents are not lost */
>> +VOID MoveAllSurfacesToVideoRam(PDev *pdev)
>> +{
>> + UINT32 surface_id;
>> + SurfaceInfo *surface_info;
>> +
>> + /* brute force implementation - alternative is to keep an updated
>> used_surfaces list */
>> + DEBUG_PRINT((pdev, 3, "%s\n", __FUNCTION__));
>> +
>> + for (surface_id = 1 ; surface_id< pdev->n_surfaces ; ++surface_id) {
>> + surface_info = GetSurfaceInfo(pdev, surface_id);
>> + if (!surface_info->draw_area.base_mem) {
>> + continue;
>> + }
>> + if (surface_info->u.pdev != pdev) {
>> + DEBUG_PRINT((pdev, 0, "%s: %p: not out pdev (%p)\n", __FUNCTION__,
>> pdev,
>> + surface_info->u.pdev));
>> + continue;
>> + }
>> + if (surface_info->draw_area.surf_obj) {
> Isn't it a bug if it happens? A surface info doesn't hold surf_obj after
> the surface is deleted (DrvDeleteDeviceBitmap), but not yet fully
> destroyed since the worker still holds references to it. But when we
> move surfaces to RAM, we make sure the worker doesn't hold references to
> surfaces, so for suce surfaces (base_mem == NULL).
> Maybe for clarity also better to use macros like
> #define SURFACE_IS_FREE(surface_info) (surface_info->draw_area.base_mem
> == NULL)
>> + DEBUG_PRINT((pdev, 0, "%s: surface_id = %d, surf_obj not empty\n",
>> __FUNCTION__,
>> + surface_id));
>> + continue;
>> + }
>> + if (surface_info->copy == NULL) {
> Isn't it a bug if it happens?
>> + DEBUG_PRINT((pdev, 0, "%s: %p: %d: no copy buffer, ignored\n",
>> __FUNCTION__,
>> + pdev, surface_id));
>> + }
>> + MoveSurfaceToVideoRam(pdev, surface_id);
>> + }
>> +}
>> +
>> +BOOL MoveAllSurfacesToRam(PDev *pdev)
>> +{
>> + UINT32 surface_id;
>> + SurfaceInfo *surface_info;
>> + SURFOBJ *surf_obj;
>> + UINT8 *copy;
>> + UINT8 *line0;
>> + int size;
>> +
>> + for (surface_id = 1 ; surface_id< pdev->n_surfaces ; ++surface_id) {
>> + surface_info = GetSurfaceInfo(pdev, surface_id);
>> + if (!surface_info->draw_area.base_mem) {
> macro
>> + continue;
>> + }
>> + surf_obj = surface_info->draw_area.surf_obj;
>> + if (!surf_obj) {
> shouldn't happen
sorry, can happen, since we call MoveAllSurfacesToRam before we make 
sure the release ring is empty
>> + DEBUG_PRINT((pdev, 3, "%s: %d: no surfobj, not copying\n",
>> __FUNCTION__, surface_id));
>> + continue;
>> +#if 0
>> + DEBUG_PRINT((pdev, 0, "%s: %d: no surfobj, finishing release of
>> surface\n",
>> + __FUNCTION__, surface_id));
>> + QXLDelSurface(pdev, surface_info->draw_area.base_mem,
>> + DEVICE_BITMAP_ALLOCATION_TYPE_VRAM);
>> + FreeSurfaceInfo(pdev, surface_id);
>> + continue;
>> +#endif
>> + }
>> + size = surf_obj->sizlBitmap.cy * abs(surf_obj->lDelta);
>> + copy = EngAllocMem(0, size, ALLOC_TAG);
>> + DEBUG_PRINT((pdev, 3, "%s: %d: copying #%d to %p (%d)\n",
>> __FUNCTION__, surface_id, size,
>> + copy, surf_obj->lDelta));
>> + RtlCopyMemory(copy, surface_info->draw_area.base_mem, size);
>> + surface_info->copy = copy;
>> + line0 = surf_obj->lDelta> 0 ? copy : copy + abs(surf_obj->lDelta) *
>> + (surf_obj->sizlBitmap.cy - 1);
>> + if (!EngModifySurface((HSURF)surface_info->hbitmap,
>> + pdev->eng,
>> + 0, /* from the example: used to monitor memory HOOK_COPYBITS |
>> HOOK_BITBLT, */
>> + 0, /* It's system-memory */
>> + (DHSURF)surface_info,
>> + line0,
>> + surf_obj->lDelta,
>> + NULL)) {
>> + /* NOTE: in this case we have a halfbreed - some surfaces are GDI
>> managed, some are
>> + * managed by us. */
>> + EngFreeMem(surface_info->copy);
>> + surface_info->copy = NULL;
>> + return FALSE;
>> + }
>> + QXLDelSurface(pdev, surface_info->draw_area.base_mem,
>> DEVICE_BITMAP_ALLOCATION_TYPE_VRAM);
>> + surface_info->draw_area.base_mem = copy;
>> + FreeDrawArea(&surface_info->draw_area);
>> + }
>> + return TRUE;
>> +}
>> diff --git a/display/surface.h b/display/surface.h
>> index f723392..0ce4e6c 100644
>> --- a/display/surface.h
>> +++ b/display/surface.h
>> @@ -105,4 +105,7 @@ HBITMAP CreateDeviceBitmap(PDev *pdev, SIZEL size,
>> ULONG format, QXLPHYSICAL *ph
>> UINT8 **base_mem, UINT32 surface_id, UINT8 allocation_type);
>> VOID DeleteDeviceBitmap(PDev *pdev, UINT32 surface_id, UINT8
>> allocation_type);
>>
>> +VOID MoveAllSurfacesToVideoRam(PDev *pdev);
>> +BOOL MoveAllSurfacesToRam(PDev *pdev);
>> +
>> #endif
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel