On Mon, Apr 3, 2017 at 9:12 PM, Frediano Ziglio <[email protected]> wrote:
> > On Mon, Apr 3, 2017 at 1:55 PM, Frediano Ziglio <[email protected]> > wrote: > >> > >> > From: "[email protected]" <[email protected]> >> > >> > 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? >> > > When VSync is enabled and the system activates watchdog policy, there are > 2 possible flows > related to driver stopping: > First one is regular PnP stop (for example, on disable), on stop the > driver waits until thread > finishes and the thread is able to complete all the operations, in the > worst case it will take > some time until thread allocates all the pending drawables and sends them > down. > Second one is forced stop due to long processing of presentation callback. > We do > our best to avoid this, but this is still possible, at least in theory. In > this case OS > may not not wait until the driver completes stop flow and can activate > fallback > mode and make switch to VGA mode. If the thread is processing outstanding > drawables, it needs to allocate memory (forced) and may pend until some > previously allocated > objects must be reported by QEMU/spice server to the guest. As the switch > to VGA > already happened, QEMU does not report them (assuming the guest does need > this, > which is completely correct in case of regular stop). At the moment of VGA > switch the > thread can be in the middle of allocation, so the driver shall know to > abort the allocation if > stop flow already started. > > Wouldn't be helpful to have a flag set on Stop so the thread could start > deleting drawables directly > instead of processing them? This should make the close faster. > > Yes, this can make the stop faster. I'll prepare such patch over 12/12 as the driver anyway shall review the chain of chunks and free the memory it allocated from OS, in any. > > BTW, In earlier notes you mentioned possibility to use VGA memory as > alternate area for > objects allocation. I think that in stop flow due to watchdog the VGA area > may be used by > fallback video engine as frame buffer and this can corrupt the memory > arena managed by mspace code. > > This should not be an issue. The first part of Bar0 is reserved for the > frame buffer and excluded > mspace. VGA should use only this part. But better to test it. > > >> > 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 <[email protected]> >> > --- >> > 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 { >> > > >
_______________________________________________ Spice-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/spice-devel
