<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Tue, May 2, 2017 at 4:33 PM, Frediano Ziglio <<a href="mailto:fziglio@redhat.com" target="_blank">fziglio@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Upon change of display mode the driver waits until all the queued drawables pushed to the host or discarded. This eliminates server warnings "rendering incorrect" in "get_drawable" when the drawable command was created by guest driver just before change of display mode and posted to the server during or after the change. This patch and comments are heavily based on a patch sent by Yuri Benditovich (with serialization code completely changed and added generation to discard pending commands on old surface). Signed-off-by: Frediano Ziglio <<a href="mailto:fziglio@redhat.com">fziglio@redhat.com</a>> --- qxldod/QxlDod.cpp | 95 ++++++++++++++++++++++++++++++++++++++++--------------- qxldod/QxlDod.h | 20 ++++++++++-- 2 files changed, 88 insertions(+), 27 deletions(-) Changes since v1: - fix code analizer warnings. diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp index 1953688..5168941 100755 --- a/qxldod/QxlDod.cpp +++ b/qxldod/QxlDod.cpp @@ -3250,6 +3250,22 @@ NTSTATUS QxlDevice::QueryCurrentMode(PVIDEO_MODE RequestedMode) return Status; } +template<typename Closure> +class QxlGenericOperation: public QxlPresentOperation +{ +public: + QxlGenericOperation(const Closure &_closure) : closure(_closure) { PAGED_CODE(); } + void Run() override { PAGED_CODE(); closure(); } +private: + Closure closure; +}; + +template<typename Closure> +__forceinline QxlPresentOperation *BuildQxlOperation(Closure &&closure) +{ + return new (NonPagedPoolNx) QxlGenericOperation<Closure>(closure); +} + NTSTATUS QxlDevice::SetCurrentMode(ULONG Mode) { PAGED_CODE(); @@ -3258,11 +3274,27 @@ NTSTATUS QxlDevice::SetCurrentMode(ULONG Mode) { if (Mode == m_ModeNumbers[idx]) { - DestroyPrimarySurface(); - CreatePrimarySurface(&m_ModeInfo[idx]); + if (!m_PresentThread) + break; DbgPrint(TRACE_LEVEL_INFORMATION, ("%s device %d: setting current mode %d (%d x %d)\n", __FUNCTION__, m_Id, Mode, m_ModeInfo[idx].VisScreenWidth, m_ModeInfo[idx].VisScreenHeight)); + + // execute the operation in the worker thread to avoiding + // executing drawing commands while changing resolution + KEVENT finishEvent; + KeInitializeEvent(&finishEvent, SynchronizationEvent, FALSE); + ++m_DrawGeneration; + QxlPresentOperation *operation = BuildQxlOperation([=, this, &finishEvent]() { + PAGED_CODE(); + DestroyPrimarySurface(); + CreatePrimarySurface(&m_ModeInfo[idx]); + KeSetEvent(&finishEvent, IO_NO_INCREMENT, FALSE); + }); + if (!operation) + return STATUS_NO_MEMORY; + PostToWorkerThread(operation); + WaitForObject(&finishEvent, NULL); return STATUS_SUCCESS; } } @@ -3892,6 +3924,30 @@ QxlDevice::ExecutePresentDisplayOnly( SrcBltInfo.Height = DstBltInfo.Height; } + uint16_t currentGeneration = m_DrawGeneration; + QxlPresentOperation *operation = BuildQxlOperation([=, this]() { + PAGED_CODE(); + ULONG delayed = 0; + + for (UINT i = 0; pDrawables[i]; ++i) + { + ULONG n = PrepareDrawable(pDrawables[i]); + // only reason why drawables[i] is zeroed is stop flow + if (pDrawables[i]) { + delayed += n; + if (currentGeneration == m_DrawGeneration) + PushDrawable(pDrawables[i]); + } + } + delete[] pDrawables; + if (delayed) { + DbgPrint(TRACE_LEVEL_WARNING, ("%s: %d delayed chunks\n", __FUNCTION__, delayed)); + } + }); + if (!operation) { + delete[] pDrawables; + return STATUS_NO_MEMORY; + } // Copy all the scroll rects from source image to video frame buffer. for (UINT i = 0; i < ctx->NumMoves; i++) @@ -3936,7 +3992,7 @@ QxlDevice::ExecutePresentDisplayOnly( pDrawables[nIndex] = NULL; - PostToWorkerThread(pDrawables); + PostToWorkerThread(operation); return STATUS_SUCCESS; } @@ -5157,6 +5213,10 @@ void QxlDevice::StopPresentThread() if (m_PresentThread) { DbgPrint(TRACE_LEVEL_INFORMATION, ("---> %s\n", __FUNCTION__)); + // this cause pending drawing operation to be discarded instead + // of executed, there's no reason to execute them if we are + // destroying the device + ++m_DrawGeneration; PostToWorkerThread(NULL); NTSTATUS Status = ObReferenceObjectByHandle( m_PresentThread, 0, NULL, KernelMode, &pDispatcherObject, NULL); @@ -5240,14 +5300,12 @@ void QxlDevice::PresentThreadRoutine() PAGED_CODE(); int wait; int notify; - ULONG delayed = 0; - QXLDrawable** drawables; DbgPrint(TRACE_LEVEL_INFORMATION, ("--->%s\n", __FUNCTION__)); while (1) { - // Pop a drawables list from the ring + // Pop an operation from the ring // No need for a mutex, only one consumer thread SPICE_RING_CONS_WAIT(m_PresentRing, wait); while (wait) { @@ -5255,35 +5313,22 @@ void QxlDevice::PresentThreadRoutine() <div><div class="h5"> DoWaitForObject(&m_PresentEvent, NULL, NULL); SPICE_RING_CONS_WAIT(m_PresentRing, wait); } - drawables = *SPICE_RING_CONS_ITEM(m_PresentRing); + QxlPresentOperation *operation = *SPICE_RING_CONS_ITEM(m_PresentRing); SPICE_RING_POP(m_PresentRing, notify); if (notify) { KeSetEvent(&m_PresentThreadReadyEvent, 0, FALSE); } - if (drawables) { - for (UINT i = 0; drawables[i]; ++i) - { - ULONG n = PrepareDrawable(drawables[i]); - // only reason why drawables[i] is zeroed is stop flow - if (drawables[i]) { - delayed += n; - PushDrawable(drawables[i]); - } - } - delete[] drawables; - } else { + if (!operation) { DbgPrint(TRACE_LEVEL_WARNING, ("%s is being terminated\n", __FUNCTION__)); break; } - if (delayed) { - DbgPrint(TRACE_LEVEL_WARNING, ("%s: %d delayed chunks\n", __FUNCTION__, delayed)); - delayed = 0; - } + operation->Run(); + delete operation; } } -void QxlDevice::PostToWorkerThread(QXLDrawable** drawables) +void QxlDevice::PostToWorkerThread(QxlPresentOperation *operation) { PAGED_CODE(); // Push drawables into PresentRing and notify worker thread </div></div>@@ -5293,7 +5338,7 @@ void QxlDevice::PostToWorkerThread(QXLDrawable** drawables) <div class="HOEnZb"><div class="h5"> WaitForObject(&m_PresentThreadReadyEvent, NULL); SPICE_RING_PROD_WAIT(m_PresentRing, wait); } - *SPICE_RING_PROD_ITEM(m_PresentRing) = drawables; + *SPICE_RING_PROD_ITEM(m_PresentRing) = operation; SPICE_RING_PUSH(m_PresentRing, notify); if (notify) { KeSetEvent(&m_PresentEvent, 0, FALSE); diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h index e865032..2ce3ce5 100755 --- a/qxldod/QxlDod.h +++ b/qxldod/QxlDod.h @@ -506,8 +506,20 @@ typedef struct DpcCbContext { #define MAX(x, y) (((x) >= (y)) ? (x) : (y)) #define ALIGN(a, b) (((a) + ((b) - 1)) & ~((b) - 1)) +// operation to be run by the presentation thread +class QxlPresentOperation +{ +public: + QxlPresentOperation() {} + QxlPresentOperation(const QxlPresentOperation&) = delete; + void operator=(const QxlPresentOperation&) = delete; + // execute the operation + virtual void Run()=0; + virtual ~QxlPresentOperation() {} +}; + #include "start-packed.h" -SPICE_RING_DECLARE(QXLPresentOnlyRing, QXLDrawable**, 1024); +SPICE_RING_DECLARE(QXLPresentOnlyRing, QxlPresentOperation*, 1024); #include "end-packed.h" class QxlDevice : @@ -619,7 +631,7 @@ private: static void PresentThreadRoutineWrapper(HANDLE dev) { ((QxlDevice *)dev)->PresentThreadRoutine(); } - void PostToWorkerThread(QXLDrawable** drawables); + void PostToWorkerThread(QxlPresentOperation *operation); static LONG GetMaxSourceMappingHeight(RECT* DirtyRects, ULONG NumDirtyRects); @@ -674,6 +686,10 @@ private: QXLPHYSICAL* m_monitor_config_pa; QXLPresentOnlyRing m_PresentRing[1]; + // generation, updated when resolution change + // this is used to detect if a draw command is obsoleted + // and should not be executed + uint16_t m_DrawGeneration; HANDLE m_PresentThread; BOOLEAN m_bActive; }; -- 2.9.3 </div></div></blockquote><div> </div><div>ACK. Verified in WHCK setup.</div><div>There are 2 memory leaks (one is real as I'm able to reproduce the flow), I'm submitting the patch for them.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"> _______________________________________________ Spice-devel mailing list <a href="mailto:Spice-devel@lists.freedesktop.org">Spice-devel@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/spice-devel" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/spice-devel</a> </div></div></blockquote></div> </div></div>