[Spice-devel] [PATCH 08/12] qxl-wddm-dod: Prepare for failure to allocate memory
Jonathon Jongsma
jjongsma at redhat.com
Mon Mar 27 20:03:59 UTC 2017
I'm reading through some of these patches and the commit logs are not
very clear to me. Part of this may be that I'm unfamiliar with many
details of windows drivers. But I think part of it is that the comments
may be a bit unclear.
On Sun, 2017-03-12 at 10:45 +0200, Yuri Benditovich wrote:
> 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 is in stop flow
What does it mean for a driver to be "in stop flow"? Is this a common
name for a state that a driver can be in? If it's a common term that
would be understood by any competent windows driver developer, then
fine. But otherwise, perhaps it needs to be clarified.
> and it is possible that video subsystem already executes switch
> to VGA mode, so QEMU will not return previously allocated objects
Perhaps describe why it will not release these objects?
> In case of allocation failure we may need to free previously
> allocated
> object (when more than one allocation required for operation), this
> must be protected by m_MemLock mutex.
> In case of forced memory allocation the allocation routine waits
> unpredictable time until the request is satisfied. Now we do not
> acquire
> m_MemLock mutex for all this time, but release it when enter long
> wait
> to allow another caller to enter and try to allocate memory.
>
> Signed-off-by: Yuri Benditovich <yuri.benditovich at daynix.com>
> ---
> qxldod/QxlDod.cpp | 107
> ++++++++++++++++++++++++++++++++++++++++++++++++------
> qxldod/QxlDod.h | 4 +-
> 2 files changed, 99 insertions(+), 12 deletions(-)
>
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> index bb9b227..9a3803a 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)
> @@ -3494,14 +3495,20 @@ 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();
> }
> @@ -3982,14 +3989,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;
> }
> @@ -3997,17 +4008,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__));
> }
>
> @@ -4074,11 +4088,18 @@ UINT64 QxlDevice::ReleaseOutput(UINT64
> output_id)
> return next;
> }
>
> +void QxlDevice::ReleaseOutputUnderLock(UINT64 output_id)
> +{
> + WaitForObject(&m_MemLock, NULL);
> + ReleaseOutput(output_id);
> + ReleaseMutex(&m_MemLock, TRUE);
> +}
> +
> void *QxlDevice::AllocMem(UINT32 mspace_type, size_t size, BOOL
> force)
> {
> PAGED_CODE();
> PVOID ptr;
> - BOOLEAN locked = FALSE;
> + BOOLEAN locked = FALSE, bInfiniteWait = !!force;
>
> ASSERT(m_MSInfo[mspace_type]._mspace);
> DbgPrint(TRACE_LEVEL_VERBOSE, ("--->%s: %p(%d) size %u\n",
> __FUNCTION__,
> @@ -4089,7 +4110,19 @@ 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 (bInfiniteWait)
> + locked = WaitForObject(&m_MemLock, NULL);
> + else
> + {
> + LARGE_INTEGER doNotWait;
> + doNotWait.QuadPart = 0;
> + locked = WaitForObject(&m_MemLock, &doNotWait);
> + if (!locked)
> + {
> + ptr = NULL;
> + goto skip_allocation;
> + }
> + }
>
> while (1) {
> /* Release lots of queued resources, before allocating, as
> we
> @@ -4107,17 +4140,21 @@ 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;
> }
> }
> +
> +skip_allocation:
> 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;
> @@ -4150,6 +4187,10 @@ 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;
> @@ -4165,6 +4206,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;
> @@ -4314,6 +4358,10 @@ 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;
> @@ -4326,7 +4374,7 @@ QXLDrawable *QxlDevice::Drawable(UINT8 type,
> CONST RECT *area, CONST RECT *clip,
>
> if (!SetClip(clip, drawable)) {
> DbgPrint(TRACE_LEVEL_VERBOSE, ("%s: set clip failed\n",
> __FUNCTION__));
> - ReleaseOutput(drawable->release_info.id);
> + ReleaseOutputUnderLock(drawable->release_info.id);
> drawable = NULL;
> }
> DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
> @@ -4457,6 +4505,12 @@ QXLDrawable *QxlDevice::BltBits (
> 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)
> + {
> + ReleaseOutputUnderLock(drawable->release_info.id);
> + DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate bitmap for
> drawable\n"));
> + return NULL;
> + }
>
> image_res->refs = 1;
> image_res->free = FreeBitmapImageEx;
> @@ -4487,8 +4541,13 @@ QXLDrawable *QxlDevice::BltBits (
> 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))
> + {
> + ReleaseOutputUnderLock(drawable->release_info.id);
> + DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate additional
> bitmap for drawable\n"));
> + return NULL;
> + }
> }
>
> internal->image.bitmap.palette = 0;
> @@ -4509,7 +4568,7 @@ QXLDrawable *QxlDevice::BltBits (
> 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)
> {
> @@ -4527,6 +4586,10 @@ 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;
> @@ -4547,6 +4610,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)
> @@ -4610,6 +4674,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;
> @@ -4617,6 +4686,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__));
> + ReleaseOutputUnderLock(cursor_cmd->release_info.id);
> + return STATUS_INSUFFICIENT_RESOURCES;
> + }
> +
> res->refs = 1;
> res->free = FreeCursorEx;
> res->ptr = this;
> @@ -4653,8 +4728,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__));
> + ReleaseOutputUnderLock(cursor_cmd->release_info.id);
> + return STATUS_INSUFFICIENT_RESOURCES;
> + }
> }
> CursorCmdAddRes(cursor_cmd, res);
> RELEASE_RES(res);
> @@ -4675,6 +4755,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 7bb5de5..3ee16ae 100755
> --- a/qxldod/QxlDod.h
> +++ b/qxldod/QxlDod.h
> @@ -585,6 +585,7 @@ private:
> void FlushReleaseRing();
> void FreeMem(UINT32 mspace_type, void *ptr);
> UINT64 ReleaseOutput(UINT64 output_id);
> + void ReleaseOutputUnderLock(UINT64 output_id);
> void WaitForReleaseRing(void);
> void EmptyReleaseRing(void);
> BOOL SetClip(const RECT *clip, QXLDrawable *drawable);
> @@ -601,7 +602,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 +672,7 @@ private:
>
> QXLPresentOnlyRing m_PresentRing[1];
> HANDLE m_PresentThread;
> + BOOLEAN m_bActive;
> };
>
> class QxlDod {
More information about the Spice-devel
mailing list