[Spice-devel] [PATCH qxl-wddm-dod v3 2/2] Synchronize display mode change and pushing drawables
Yuri Benditovich
yuri.benditovich at daynix.com
Fri May 5 10:50:17 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.
>
> > +}
> > +
> > 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;
> > };
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170505/6b483850/attachment-0001.html>
More information about the Spice-devel
mailing list