>
> 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
> and it is possible that video subsystem already executes switch
> to VGA mode, so QEMU will not return previously allocated 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@xxxxxxxxxx>
> ---
> 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;
What does it mean that the device is not active?
> }
>
> 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;
So this means that if the thread cannot be created the device
will work in a an inactive state?
> + }
> 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);
> +}
> +
Is not really clear why we need additional locking here.
We are allocating resources, failing to allocate some and cleaning up.
The FreeMem calls already use a MemLock to make mspace serialized.
Why we need additional serialization?
> 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)
Why don't you use force here instead?
I don't see the reason for another variable.
Also note that this code is aligned with 5 spaces instead of 4.
> + locked = WaitForObject(&m_MemLock, NULL);
> + else
> + {
> + LARGE_INTEGER doNotWait;
> + doNotWait.QuadPart = 0;
> + locked = WaitForObject(&m_MemLock, &doNotWait);
> + if (!locked)
> + {
> + ptr = NULL;
> + goto skip_allocation;
I would prefer a return NULL here without the goto.
> + }
> + }
>
> 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;
> + }
We really should define a style!
Is it
if (cond) code;
or
if (cond) {
code;
}
or
if (cond)
code;
or
if (cond)
{
code;
}
only this patch contains at least 3 styles of if indentation!
> 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 {
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel