<html><body><div style="font-family: times new roman, new york, times, serif; font-size: 12pt; color: #000000"><div> </div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div dir="ltr"><div class="gmail_extra"> <div class="gmail_quote">On Fri, May 5, 2017 at 1:36 PM, Frediano Ziglio <<a href="mailto:fziglio@redhat.com" target="_blank">fziglio@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">> > 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" target="_blank">fziglio@redhat.com</a>> > --- > qxldod/QxlDod.cpp | 99 > +++++++++++++++++++++++++++++++++++++++++-------------- > qxldod/QxlDod.h | 20 +++++++++-- > 2 files changed, 92 insertions(+), 27 deletions(-) > > Changes since v2: > - merged leak fix from Yuri Benditovich basing on "Move code for discarding > drawable to separate procedure" patch. > > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp > index 5ee54f5..07246fa 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); </div></div>About this line. Maybe should be PagedPool instead? </blockquote><div> </div><div>Of course, PagedPool is enough as well as when allocating array of drawables.</div><div>The change is not mandatory.</div><div>When driver verifier is active for the driver, it tries to keep pageable allocations paged out </div><div>to verify there are not accesses to them on dispatch irql, so in this case the cost of pageable</div><div>allocation is higher.</div></div></div></div></blockquote><div>But this should happen only during debugging, during normal usage the difference is </div><div>that paged pool is bigger than nonpaged pool so less prone to fail. </div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5"> > +} > + > 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,34 @@ 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]); > + else > + DiscardDrawable(pDrawables[i]); > + } > + } > + delete[] pDrawables; > + if (delayed) { > + DbgPrint(TRACE_LEVEL_WARNING, ("%s: %d delayed chunks\n", > __FUNCTION__, delayed)); > + } > + }); > + if (!operation) { > + MmUnlockPages(ctx->Mdl); > + IoFreeMdl(ctx->Mdl); > + 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 +3996,7 @@ QxlDevice::ExecutePresentDisplayOnly( > > pDrawables[nIndex] = NULL; > > - PostToWorkerThread(pDrawables); > + PostToWorkerThread(operation); > > return STATUS_SUCCESS; > } > @@ -5164,6 +5224,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); > @@ -5247,14 +5311,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) { > @@ -5262,35 +5324,22 @@ void QxlDevice::PresentThreadRoutine() > 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 > @@ -5300,7 +5349,7 @@ void QxlDevice::PostToWorkerThread(QXLDrawable** > drawables) > 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 b524577..695b83a 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 : > @@ -620,7 +632,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); > > @@ -675,6 +687,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; > }; </div></div></blockquote></div></div></div></blockquote><div> </div></div></body></html>