[Spice-devel] [PATCH qxl-wddm-dod v2] Synchronize display mode change and pushing drawables
Yuri Benditovich
yuri.benditovich at daynix.com
Fri May 5 10:09:50 UTC 2017
On Tue, May 2, 2017 at 4:33 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 | 95 ++++++++++++++++++++++++++++++
> ++++++++++---------------
> qxldod/QxlDod.h | 20 ++++++++++--
> 2 files changed, 88 insertions(+), 27 deletions(-)
>
> Changes since v1:
> - fix code analizer warnings.
>
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> index 1953688..5168941 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);
> +}
> +
> 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,30 @@ 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]);
> + }
> + }
> + delete[] pDrawables;
> + if (delayed) {
> + DbgPrint(TRACE_LEVEL_WARNING, ("%s: %d delayed chunks\n",
> __FUNCTION__, delayed));
> + }
> + });
> + if (!operation) {
> + 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 +3992,7 @@ QxlDevice::ExecutePresentDisplayOnly(
>
> pDrawables[nIndex] = NULL;
>
> - PostToWorkerThread(pDrawables);
> + PostToWorkerThread(operation);
>
> return STATUS_SUCCESS;
> }
> @@ -5157,6 +5213,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);
> @@ -5240,14 +5300,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) {
> @@ -5255,35 +5313,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
> @@ -5293,7 +5338,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 e865032..2ce3ce5 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 :
> @@ -619,7 +631,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);
>
> @@ -674,6 +686,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;
> };
> --
> 2.9.3
>
>
ACK. Verified in WHCK setup.
There are 2 memory leaks (one is real as I'm able to reproduce the flow),
I'm submitting the patch for them.
> _______________________________________________
> 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/6291e244/attachment-0001.html>
More information about the Spice-devel
mailing list