<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 5, 2017 at 2:07 PM, Frediano Ziglio <span dir="ltr"><<a href="mailto:fziglio@redhat.com" target="_blank">fziglio@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:times new roman,new york,times,serif;font-size:12pt;color:#000000"><div><div class="h5"><div><br></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"><br><div class="gmail_quote">On Fri, May 5, 2017 at 1:36 PM, Frediano Ziglio <span dir="ltr"><<a href="mailto:fziglio@redhat.com" target="_blank">fziglio@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="m_-2144622182965706565gmail-HOEnZb"><div class="m_-2144622182965706565gmail-h5">><br>
> Upon change of display mode the driver waits until all the<br>
> queued drawables pushed to the host or discarded. This eliminates<br>
> server warnings "rendering incorrect" in "get_drawable" when the<br>
> drawable command was created by guest driver just before change<br>
> of display mode and posted to the server during or after the change.<br>
><br>
> This patch and comments are heavily based on a patch sent by<br>
> Yuri Benditovich (with serialization code completely changed and<br>
> added generation to discard pending commands on old surface).<br>
><br>
> Signed-off-by: Frediano Ziglio <<a href="mailto:fziglio@redhat.com" target="_blank">fziglio@redhat.com</a>><br>
> ---<br>
> qxldod/QxlDod.cpp | 99<br>
> ++++++++++++++++++++++++++++++<wbr>+++++++++++--------------<br>
> qxldod/QxlDod.h | 20 +++++++++--<br>
> 2 files changed, 92 insertions(+), 27 deletions(-)<br>
><br>
> Changes since v2:<br>
> - merged leak fix from Yuri Benditovich basing on "Move code for discarding<br>
> drawable to separate procedure" patch.<br>
><br>
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp<br>
> index 5ee54f5..07246fa 100755<br>
> --- a/qxldod/QxlDod.cpp<br>
> +++ b/qxldod/QxlDod.cpp<br>
> @@ -3250,6 +3250,22 @@ NTSTATUS QxlDevice::QueryCurrentMode(<wbr>PVIDEO_MODE<br>
> RequestedMode)<br>
> return Status;<br>
> }<br>
><br>
> +template<typename Closure><br>
> +class QxlGenericOperation: public QxlPresentOperation<br>
> +{<br>
> +public:<br>
> + QxlGenericOperation(const Closure &_closure) : closure(_closure) {<br>
> PAGED_CODE(); }<br>
> + void Run() override { PAGED_CODE(); closure(); }<br>
> +private:<br>
> + Closure closure;<br>
> +};<br>
> +<br>
> +template<typename Closure><br>
> +__forceinline QxlPresentOperation *BuildQxlOperation(Closure &&closure)<br>
> +{<br>
> + return new (NonPagedPoolNx) QxlGenericOperation<Closure>(<wbr>closure);<br><br></div></div>About this line. Maybe should be PagedPool instead?<br></blockquote><div><br></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<br></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></div><div>But this should happen only during debugging, during normal usage the difference is<br></div><div>that paged pool is bigger than nonpaged pool so less prone to fail.<br></div></div></div></blockquote><div><br></div><div>Driver verifier is active during debugging and certification tests.</div><div>No objections to change to PagedPool, do not expect any problem. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:times new roman,new york,times,serif;font-size:12pt;color:#000000"><div></div><div><div class="h5"><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="m_-2144622182965706565gmail-HOEnZb"><div class="m_-2144622182965706565gmail-h5"><br>
> +}<br>
> +<br>
> NTSTATUS QxlDevice::SetCurrentMode(<wbr>ULONG Mode)<br>
> {<br>
> PAGED_CODE();<br>
> @@ -3258,11 +3274,27 @@ NTSTATUS QxlDevice::SetCurrentMode(<wbr>ULONG Mode)<br>
> {<br>
> if (Mode == m_ModeNumbers[idx])<br>
> {<br>
> - DestroyPrimarySurface();<br>
> - CreatePrimarySurface(&m_<wbr>ModeInfo[idx]);<br>
> + if (!m_PresentThread)<br>
> + break;<br>
> DbgPrint(TRACE_LEVEL_<wbr>INFORMATION, ("%s device %d: setting<br>
> current mode %d (%d x %d)\n",<br>
> __FUNCTION__, m_Id, Mode, m_ModeInfo[idx].<wbr>VisScreenWidth,<br>
> m_ModeInfo[idx].<wbr>VisScreenHeight));<br>
> +<br>
> + // execute the operation in the worker thread to avoiding<br>
> + // executing drawing commands while changing resolution<br>
> + KEVENT finishEvent;<br>
> + KeInitializeEvent(&<wbr>finishEvent, SynchronizationEvent, FALSE);<br>
> + ++m_DrawGeneration;<br>
> + QxlPresentOperation *operation = BuildQxlOperation([=, this,<br>
> &finishEvent]() {<br>
> + PAGED_CODE();<br>
> + DestroyPrimarySurface();<br>
> + CreatePrimarySurface(&m_<wbr>ModeInfo[idx]);<br>
> + KeSetEvent(&finishEvent, IO_NO_INCREMENT, FALSE);<br>
> + });<br>
> + if (!operation)<br>
> + return STATUS_NO_MEMORY;<br>
> + PostToWorkerThread(operation);<br>
> + WaitForObject(&finishEvent, NULL);<br>
> return STATUS_SUCCESS;<br>
> }<br>
> }<br>
> @@ -3892,6 +3924,34 @@ QxlDevice::<wbr>ExecutePresentDisplayOnly(<br>
> SrcBltInfo.Height = DstBltInfo.Height;<br>
> }<br>
><br>
> + uint16_t currentGeneration = m_DrawGeneration;<br>
> + QxlPresentOperation *operation = BuildQxlOperation([=, this]() {<br>
> + PAGED_CODE();<br>
> + ULONG delayed = 0;<br>
> +<br>
> + for (UINT i = 0; pDrawables[i]; ++i)<br>
> + {<br>
> + ULONG n = PrepareDrawable(pDrawables[i])<wbr>;<br>
> + // only reason why drawables[i] is zeroed is stop flow<br>
> + if (pDrawables[i]) {<br>
> + delayed += n;<br>
> + if (currentGeneration == m_DrawGeneration)<br>
> + PushDrawable(pDrawables[i]);<br>
> + else<br>
> + DiscardDrawable(pDrawables[i])<wbr>;<br>
> + }<br>
> + }<br>
> + delete[] pDrawables;<br>
> + if (delayed) {<br>
> + DbgPrint(TRACE_LEVEL_WARNING, ("%s: %d delayed chunks\n",<br>
> __FUNCTION__, delayed));<br>
> + }<br>
> + });<br>
> + if (!operation) {<br>
> + MmUnlockPages(ctx->Mdl);<br>
> + IoFreeMdl(ctx->Mdl);<br>
> + delete[] pDrawables;<br>
> + return STATUS_NO_MEMORY;<br>
> + }<br>
><br>
> // Copy all the scroll rects from source image to video frame buffer.<br>
> for (UINT i = 0; i < ctx->NumMoves; i++)<br>
> @@ -3936,7 +3996,7 @@ QxlDevice::<wbr>ExecutePresentDisplayOnly(<br>
><br>
> pDrawables[nIndex] = NULL;<br>
><br>
> - PostToWorkerThread(pDrawables)<wbr>;<br>
> + PostToWorkerThread(operation);<br>
><br>
> return STATUS_SUCCESS;<br>
> }<br>
> @@ -5164,6 +5224,10 @@ void QxlDevice::StopPresentThread()<br>
> if (m_PresentThread)<br>
> {<br>
> DbgPrint(TRACE_LEVEL_<wbr>INFORMATION, ("---> %s\n", __FUNCTION__));<br>
> + // this cause pending drawing operation to be discarded instead<br>
> + // of executed, there's no reason to execute them if we are<br>
> + // destroying the device<br>
> + ++m_DrawGeneration;<br>
> PostToWorkerThread(NULL);<br>
> NTSTATUS Status = ObReferenceObjectByHandle(<br>
> m_PresentThread, 0, NULL, KernelMode, &pDispatcherObject, NULL);<br>
> @@ -5247,14 +5311,12 @@ void QxlDevice::<wbr>PresentThreadRoutine()<br>
> PAGED_CODE();<br>
> int wait;<br>
> int notify;<br>
> - ULONG delayed = 0;<br>
> - QXLDrawable** drawables;<br>
><br>
> DbgPrint(TRACE_LEVEL_<wbr>INFORMATION, ("--->%s\n", __FUNCTION__));<br>
><br>
> while (1)<br>
> {<br>
> - // Pop a drawables list from the ring<br>
> + // Pop an operation from the ring<br>
> // No need for a mutex, only one consumer thread<br>
> SPICE_RING_CONS_WAIT(m_<wbr>PresentRing, wait);<br>
> while (wait) {<br>
> @@ -5262,35 +5324,22 @@ void QxlDevice::<wbr>PresentThreadRoutine()<br>
> DoWaitForObject(&m_<wbr>PresentEvent, NULL, NULL);<br>
> SPICE_RING_CONS_WAIT(m_<wbr>PresentRing, wait);<br>
> }<br>
> - drawables = *SPICE_RING_CONS_ITEM(m_<wbr>PresentRing);<br>
> + QxlPresentOperation *operation =<br>
> *SPICE_RING_CONS_ITEM(m_<wbr>PresentRing);<br>
> SPICE_RING_POP(m_PresentRing, notify);<br>
> if (notify) {<br>
> KeSetEvent(&m_<wbr>PresentThreadReadyEvent, 0, FALSE);<br>
> }<br>
><br>
> - if (drawables) {<br>
> - for (UINT i = 0; drawables[i]; ++i)<br>
> - {<br>
> - ULONG n = PrepareDrawable(drawables[i]);<br>
> - // only reason why drawables[i] is zeroed is stop flow<br>
> - if (drawables[i]) {<br>
> - delayed += n;<br>
> - PushDrawable(drawables[i]);<br>
> - }<br>
> - }<br>
> - delete[] drawables;<br>
> - } else {<br>
> + if (!operation) {<br>
> DbgPrint(TRACE_LEVEL_WARNING, ("%s is being terminated\n",<br>
> __FUNCTION__));<br>
> break;<br>
> }<br>
> - if (delayed) {<br>
> - DbgPrint(TRACE_LEVEL_WARNING, ("%s: %d delayed chunks\n",<br>
> __FUNCTION__, delayed));<br>
> - delayed = 0;<br>
> - }<br>
> + operation->Run();<br>
> + delete operation;<br>
> }<br>
> }<br>
><br>
> -void QxlDevice::PostToWorkerThread(<wbr>QXLDrawable** drawables)<br>
> +void QxlDevice::PostToWorkerThread(<wbr>QxlPresentOperation *operation)<br>
> {<br>
> PAGED_CODE();<br>
> // Push drawables into PresentRing and notify worker thread<br>
> @@ -5300,7 +5349,7 @@ void QxlDevice::PostToWorkerThread(<wbr>QXLDrawable**<br>
> drawables)<br>
> WaitForObject(&m_<wbr>PresentThreadReadyEvent, NULL);<br>
> SPICE_RING_PROD_WAIT(m_<wbr>PresentRing, wait);<br>
> }<br>
> - *SPICE_RING_PROD_ITEM(m_<wbr>PresentRing) = drawables;<br>
> + *SPICE_RING_PROD_ITEM(m_<wbr>PresentRing) = operation;<br>
> SPICE_RING_PUSH(m_PresentRing, notify);<br>
> if (notify) {<br>
> KeSetEvent(&m_PresentEvent, 0, FALSE);<br>
> diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h<br>
> index b524577..695b83a 100755<br>
> --- a/qxldod/QxlDod.h<br>
> +++ b/qxldod/QxlDod.h<br>
> @@ -506,8 +506,20 @@ typedef struct DpcCbContext {<br>
> #define MAX(x, y) (((x) >= (y)) ? (x) : (y))<br>
> #define ALIGN(a, b) (((a) + ((b) - 1)) & ~((b) - 1))<br>
><br>
> +// operation to be run by the presentation thread<br>
> +class QxlPresentOperation<br>
> +{<br>
> +public:<br>
> + QxlPresentOperation() {}<br>
> + QxlPresentOperation(const QxlPresentOperation&) = delete;<br>
> + void operator=(const QxlPresentOperation&) = delete;<br>
> + // execute the operation<br>
> + virtual void Run()=0;<br>
> + virtual ~QxlPresentOperation() {}<br>
> +};<br>
> +<br>
> #include "start-packed.h"<br>
> -SPICE_RING_DECLARE(<wbr>QXLPresentOnlyRing, QXLDrawable**, 1024);<br>
> +SPICE_RING_DECLARE(<wbr>QXLPresentOnlyRing, QxlPresentOperation*, 1024);<br>
> #include "end-packed.h"<br>
><br>
> class QxlDevice :<br>
> @@ -620,7 +632,7 @@ private:<br>
> static void PresentThreadRoutineWrapper(<wbr>HANDLE dev) {<br>
> ((QxlDevice *)dev)->PresentThreadRoutine()<wbr>;<br>
> }<br>
> - void PostToWorkerThread(<wbr>QXLDrawable** drawables);<br>
> + void PostToWorkerThread(<wbr>QxlPresentOperation *operation);<br>
><br>
> static LONG GetMaxSourceMappingHeight(<wbr>RECT* DirtyRects, ULONG<br>
> NumDirtyRects);<br>
><br>
> @@ -675,6 +687,10 @@ private:<br>
> QXLPHYSICAL* m_monitor_config_pa;<br>
><br>
> QXLPresentOnlyRing m_PresentRing[1];<br>
> + // generation, updated when resolution change<br>
> + // this is used to detect if a draw command is obsoleted<br>
> + // and should not be executed<br>
> + uint16_t m_DrawGeneration;<br>
> HANDLE m_PresentThread;<br>
> BOOLEAN m_bActive;<br>
> };<br></div></div></blockquote></div></div></div></blockquote><div><br></div></div></div></div></div></blockquote></div><br></div></div>