[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 11:34:15 UTC 2017
On Fri, May 5, 2017 at 2:07 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
>
>
> 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.
>
Driver verifier is active during debugging and certification tests.
No objections to change to PagedPool, do not expect any problem.
>
>> > +}
>> > +
>> > 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/ce2a3a46/attachment-0001.html>
More information about the Spice-devel
mailing list