> On Fri, May 5, 2017 at 1:36 PM, Frediano Ziglio < [email protected] > wrote:
> > > > > > > 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 < [email protected] > > > > > --- > > > > 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); > > > About this line. Maybe should be PagedPool instead? > > Of course, PagedPool is enough as well as when allocating array of drawables. > The change is not mandatory. > When driver verifier is active for the driver, it tries to keep pageable > allocations paged out > to verify there are not accesses to them on dispatch irql, so in this case > the cost of pageable > allocation is higher. But this should happen only during debugging, during normal usage the difference is that paged pool is bigger than nonpaged pool so less prone to fail. > > > +} > > > > + > > > > 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; > > > > }; >
_______________________________________________ Spice-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/spice-devel
