[Spice-devel] [PATCH v2 08/12] qxl-wddm-dod: Prepare for failure to allocate memory
Frediano Ziglio
fziglio at redhat.com
Mon Apr 3 10:55:15 UTC 2017
>
> From: "yuri.benditovich at daynix.com" <yuri.benditovich at daynix.com>
>
> Preparation for further scenarios when memory allocation failure
> can happen and we'll need to use fallback when possible.
> Memory allocation can fail in following cases:
> - when non-forced memory allocation used and the attempt to allocate
> the memory must be as fast as possible, without waits
> - when forced memory allocation used, but the driver already received
> stop command and waits for thread termination. Note that in case
> the VSync control is enabled stop command may happen even after the video
> subsystem executes switch to VGA mode. In such case QEMU will not return
> previously allocated objects (assuming the QXL driver already disabled
> and ignoring callbacks from Spice server in VGA mode).
>
Just to confirm, by VGA mode you mean when Windows attempt to unload the
driver and use VESA/VGA modes, not VgaDevice, right?
In this case I assume the memory will be freed when the driver is
unloaded by Windows.
I would expect the number of commands sent in this case is small.
Also why is not possible to just ignore the commands sent by Windows
in this case?
> In case of forced memory allocation the allocation routine waits
> unpredictable time until the request is satisfied. In current commit
> we do not acquire m_MemLock mutex for all this time, but release it
> when entering long wait to allow another caller to try allocating memory.
>
Looking at final code AllocMem can return NULL in all cases depending
on the state of the driver however not all paths check if this function
returns NULL. Specifically InitMonitorConfig and SetClip.
InitMonitorConfig is called just after initializing memory so is
unlikely to fail and SetClip is always called with clip == NULL however
I don't like to have unexploded bombs in the code.
> Signed-off-by: Yuri Benditovich <yuri.benditovich at daynix.com>
> ---
> qxldod/QxlDod.cpp | 86
> +++++++++++++++++++++++++++++++++++++++++++++++++------
> qxldod/QxlDod.h | 3 +-
> 2 files changed, 79 insertions(+), 10 deletions(-)
>
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> index aa70f39..f8440d4 100755
> --- a/qxldod/QxlDod.cpp
> +++ b/qxldod/QxlDod.cpp
> @@ -3064,6 +3064,7 @@ QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod)
> m_FreeOutputs = 0;
> m_Pending = 0;
> m_PresentThread = NULL;
> + m_bActive = FALSE;
> }
>
> QxlDevice::~QxlDevice(void)
> @@ -3490,14 +3491,19 @@ NTSTATUS QxlDevice::QxlInit(DXGK_DISPLAY_INFORMATION*
> pDispInfo)
> Status = AcquireDisplayInfo(*(pDispInfo));
> if (NT_SUCCESS(Status))
> {
> + m_bActive = TRUE;
> Status = StartPresentThread();
> }
> + if (!NT_SUCCESS(Status)) {
> + m_bActive = FALSE;
> + }
> return Status;
> }
>
> void QxlDevice::QxlClose()
> {
> PAGED_CODE();
> + m_bActive = FALSE;
> StopPresentThread();
> DestroyMemSlots();
> }
> @@ -3945,14 +3951,18 @@ void QxlDevice::WaitForReleaseRing(void)
> {
> PAGED_CODE();
> int wait;
> + BOOLEAN locked;
>
> DbgPrint(TRACE_LEVEL_VERBOSE, ("--->%s\n", __FUNCTION__));
>
> + locked = WaitForObject(&m_MemLock, NULL);
> for (;;) {
> LARGE_INTEGER timeout;
>
> if (SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
> + ReleaseMutex(&m_MemLock, locked);
> QXL_SLEEP(10);
> + locked = WaitForObject(&m_MemLock, NULL);
> if (!SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
> break;
> }
> @@ -3960,17 +3970,20 @@ void QxlDevice::WaitForReleaseRing(void)
> }
> SPICE_RING_CONS_WAIT(m_ReleaseRing, wait);
>
> - if (!wait) {
> + if (!wait || !m_bActive) {
> break;
> }
>
> + ReleaseMutex(&m_MemLock, locked);
> timeout.QuadPart = -30 * 1000 * 10; //30ms
> WaitForObject(&m_DisplayEvent, &timeout);
> + locked = WaitForObject(&m_MemLock, NULL);
>
> if (SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
> SyncIo(QXL_IO_NOTIFY_OOM, 0);
> }
> }
> + ReleaseMutex(&m_MemLock, locked);
> DbgPrint(TRACE_LEVEL_VERBOSE, ("%s: <---\n", __FUNCTION__));
> }
>
> @@ -4052,7 +4065,16 @@ void *QxlDevice::AllocMem(UINT32 mspace_type, size_t
> size, BOOL force)
> mspace_malloc_stats(m_MSInfo[mspace_type]._mspace);
> #endif
>
> - locked = WaitForObject(&m_MemLock, NULL);
> + if (force)
> + locked = WaitForObject(&m_MemLock, NULL);
> + else {
> + LARGE_INTEGER doNotWait;
> + doNotWait.QuadPart = 0;
> + locked = WaitForObject(&m_MemLock, &doNotWait);
> + if (!locked) {
> + return NULL;
> + }
> + }
>
> while (1) {
> /* Release lots of queued resources, before allocating, as we
> @@ -4070,17 +4092,20 @@ void *QxlDevice::AllocMem(UINT32 mspace_type, size_t
> size, BOOL force)
> continue;
> }
>
> - if (force) {
> + if (force && m_bActive) {
> /* Ask spice to free some stuff */
> + ReleaseMutex(&m_MemLock, locked);
> WaitForReleaseRing();
> + locked = WaitForObject(&m_MemLock, NULL);
> } else {
> /* Fail */
> break;
> }
> }
> +
> ReleaseMutex(&m_MemLock, locked);
>
> - ASSERT((!ptr && !force) || (ptr >= m_MSInfo[mspace_type].mspace_start &&
> + ASSERT((!ptr && (!force || !m_bActive)) || (ptr >=
> m_MSInfo[mspace_type].mspace_start &&
> ptr <
> m_MSInfo[mspace_type].mspace_end));
> DbgPrint(TRACE_LEVEL_VERBOSE, ("<---%s: ptr 0x%x\n", __FUNCTION__,
> ptr));
> return ptr;
> @@ -4113,6 +4138,9 @@ QXLDrawable *QxlDevice::GetDrawable()
> QXLOutput *output;
>
> output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput) +
> sizeof(QXLDrawable), TRUE);
> + if (!output) {
> + return NULL;
> + }
> output->num_res = 0;
> RESOURCE_TYPE(output, RESOURCE_TYPE_DRAWABLE);
> ((QXLDrawable *)output->data)->release_info.id = (UINT64)output;
> @@ -4128,6 +4156,9 @@ QXLCursorCmd *QxlDevice::CursorCmd()
>
> DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
> output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput) +
> sizeof(QXLCursorCmd), TRUE);
> + if (!output) {
> + return NULL;
> + }
> output->num_res = 0;
> RESOURCE_TYPE(output, RESOURCE_TYPE_CURSOR);
> cursor_cmd = (QXLCursorCmd *)output->data;
> @@ -4277,6 +4308,9 @@ QXLDrawable *QxlDevice::Drawable(UINT8 type, CONST RECT
> *area, CONST RECT *clip,
> DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
>
> drawable = GetDrawable();
> + if (!drawable) {
> + return NULL;
> + }
> drawable->surface_id = surface_id;
> drawable->type = type;
> drawable->effect = QXL_EFFECT_OPAQUE;
> @@ -4420,6 +4454,11 @@ QXLDrawable *QxlDevice::PrepareBltBits (
> alloc_size = BITMAP_ALLOC_BASE + BITS_BUF_MAX - BITS_BUF_MAX %
> line_size;
> alloc_size = MIN(BITMAP_ALLOC_BASE + height * line_size, alloc_size);
> image_res = (Resource*)AllocMem(MSPACE_TYPE_VRAM, alloc_size, TRUE);
> + if (!image_res) {
> + ReleaseOutput(drawable->release_info.id);
> + DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate bitmap for
> drawable\n"));
> + return NULL;
> + }
>
> image_res->refs = 1;
> image_res->free = FreeBitmapImageEx;
> @@ -4450,8 +4489,12 @@ QXLDrawable *QxlDevice::PrepareBltBits (
> alloc_size = height * line_size;
>
> for (; src != src_end; src -= pSrc->Pitch, alloc_size -= line_size) {
> - PutBytesAlign(&chunk, &dest, &dest_end, src,
> - line_size, alloc_size, line_size);
> + if (!PutBytesAlign(&chunk, &dest, &dest_end, src,
> + line_size, alloc_size, line_size)) {
> + ReleaseOutput(drawable->release_info.id);
> + DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate additional bitmap
> for drawable\n"));
> + return NULL;
> + }
> }
>
> internal->image.bitmap.palette = 0;
> @@ -4472,7 +4515,7 @@ QXLDrawable *QxlDevice::PrepareBltBits (
> return drawable;
> }
>
> -VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
> +BOOLEAN QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
> UINT8 **end_ptr, UINT8 *src, int size,
> size_t alloc_size, uint32_t alignment)
> {
> @@ -4490,6 +4533,9 @@ VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr,
> UINT8 **now_ptr,
> aligned_size -= aligned_size % alignment;
>
> void *ptr = AllocMem(MSPACE_TYPE_VRAM, size +
> sizeof(QXLDataChunk), TRUE);
> + if (!ptr) {
> + return FALSE;
> + }
> chunk->next_chunk = PA(ptr, m_SurfaceMemSlot);
> ((QXLDataChunk *)ptr)->prev_chunk = PA(chunk, m_SurfaceMemSlot);
> chunk = (QXLDataChunk *)ptr;
> @@ -4510,6 +4556,7 @@ VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr,
> UINT8 **now_ptr,
> *now_ptr = now;
> *end_ptr = end;
> DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
> + return TRUE;
> }
>
> VOID QxlDevice::BlackOutScreen(CURRENT_BDD_MODE* pCurrentBddMod)
> @@ -4573,6 +4620,11 @@ NTSTATUS QxlDevice::SetPointerShape(_In_ CONST
> DXGKARG_SETPOINTERSHAPE* pSetPoi
> int num_images = 1;
>
> cursor_cmd = CursorCmd();
> + if (!cursor_cmd) {
> + DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> command\n", __FUNCTION__));
> + return STATUS_INSUFFICIENT_RESOURCES;
> + }
> +
> cursor_cmd->type = QXL_CURSOR_SET;
>
> cursor_cmd->u.set.visible = TRUE;
> @@ -4580,6 +4632,12 @@ NTSTATUS QxlDevice::SetPointerShape(_In_ CONST
> DXGKARG_SETPOINTERSHAPE* pSetPoi
> cursor_cmd->u.set.position.y = 0;
>
> res = (Resource *)AllocMem(MSPACE_TYPE_VRAM, CURSOR_ALLOC_SIZE, TRUE);
> + if (!res) {
> + DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor data\n",
> __FUNCTION__));
> + ReleaseOutput(cursor_cmd->release_info.id);
> + return STATUS_INSUFFICIENT_RESOURCES;
> + }
> +
> res->refs = 1;
> res->free = FreeCursorEx;
> res->ptr = this;
> @@ -4616,8 +4674,13 @@ NTSTATUS QxlDevice::SetPointerShape(_In_ CONST
> DXGKARG_SETPOINTERSHAPE* pSetPoi
> end = (UINT8 *)res + CURSOR_ALLOC_SIZE;
> src_end = src + (pSetPointerShape->Pitch * pSetPointerShape->Height *
> num_images);
> for (; src != src_end; src += pSetPointerShape->Pitch) {
> - PutBytesAlign(&chunk, &now, &end, src, line_size,
> - PAGE_SIZE, 1);
> + if (!PutBytesAlign(&chunk, &now, &end, src, line_size,
> + PAGE_SIZE, 1))
> + {
> + DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> bitmap\n", __FUNCTION__));
> + ReleaseOutput(cursor_cmd->release_info.id);
> + return STATUS_INSUFFICIENT_RESOURCES;
> + }
> }
> CursorCmdAddRes(cursor_cmd, res);
> RELEASE_RES(res);
> @@ -4638,6 +4701,11 @@ NTSTATUS QxlDevice::SetPointerPosition(_In_ CONST
> DXGKARG_SETPOINTERPOSITION* pS
> pSetPointerPosition->X,
> pSetPointerPosition->Y));
> QXLCursorCmd *cursor_cmd = CursorCmd();
> + if (!cursor_cmd) {
> + DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> command\n", __FUNCTION__));
> + return STATUS_INSUFFICIENT_RESOURCES;
> + }
> +
> if (pSetPointerPosition->X < 0 || !pSetPointerPosition->Flags.Visible) {
> cursor_cmd->type = QXL_CURSOR_HIDE;
> } else {
> diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> index a1e9634..cbcd11e 100755
> --- a/qxldod/QxlDod.h
> +++ b/qxldod/QxlDod.h
> @@ -601,7 +601,7 @@ private:
> void PushCmd(void);
> void WaitForCursorRing(void);
> void PushCursor(void);
> - void PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
> + BOOLEAN PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
> UINT8 **end_ptr, UINT8 *src, int size,
> size_t alloc_size, uint32_t alignment);
> void AsyncIo(UCHAR Port, UCHAR Value);
> @@ -671,6 +671,7 @@ private:
>
> QXLPresentOnlyRing m_PresentRing[1];
> HANDLE m_PresentThread;
> + BOOLEAN m_bActive;
> };
>
> class QxlDod {
More information about the Spice-devel
mailing list