[Spice-devel] [PATCH qxl-wddm-dod v3 2/2] Synchronize display mode change and pushing drawables

Fri May 5 11:07:27 UTC 2017

> On Fri, May 5, 2017 at 1:36 PM, Frediano Ziglio < fziglio at redhat.com > 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 < fziglio at redhat.com >
> 
> > > ---
> 
> > > 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;
> 
> > > };
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170505/0b437117/attachment-0001.html>