On Thu, Jul 07, 2011 at 04:47:16PM +0300, Yonit Halperin wrote:
> Hi,
> Generally, when parameters of a function are divided into several
> lines, they should be aligned to the same column.
>
>
Found a single place to fix and fixing.
> 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<yhalp...@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.
Hi, Sorry for taking so long to get back to this, apologies for having
to reread the code.
wrt to the msdn snippet - then we are not really following the letter, since
we hold the surface locked for the duration we accelerate the operations on it,
no?
>
> >+ QXLPHYSICAL phys_mem;
> we don't use this field
Removed.
> >+ 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
>
Trying.
> >+ 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
> >+ 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
