viogpudo: fix HWCursor display issues

[osy]

This addresses two issues with HWCursor support:

1. Sometimes stale cursor data will be presented to the host due to a race condition.
2. Cursor hiding by guest was not implemented properly which sometimes results in an overlapping cursor when hovering over a text field.

After fixing these issues, it should be good to enable HWCursor by default.

[YanVugenfirer]

Please use a full real name in sign off. Thanks.

[osy]

@YanVugenfirer Done

[YanVugenfirer]

@vrozenfe Can you please take a look?

[vrozenfe]

@osy Thanks a lot for your PR. It looks fine overall. Just jive me some time to run some stress tests on it. I personalty have a different set of fixes for this problems issue, you can see below (just ignore all memory size allocation bits, they are related to the different issue). But if your fix works fine over VNC and under stress, I'm completerly fine with your patch.

diff --git a/viogpu/common/viogpu_queue.cpp b/viogpu/common/viogpu_queue.cpp
index cfe1c30d..cabce99e 100755
--- a/viogpu/common/viogpu_queue.cpp
+++ b/viogpu/common/viogpu_queue.cpp
@@ -843,19 +843,19 @@ BOOLEAN VioGpuMemSegment::Init(_In_ UINT size, _In_opt_ PPHYSICAL_ADDRESS pPAddr

     ASSERT(size);
     PVOID buf = NULL;
-    UINT pages = BYTES_TO_PAGES(size);
-    UINT sglsize = sizeof(SCATTER_GATHER_LIST) + (sizeof(SCATTER_GATHER_ELEMENT) * pages);
-    size = pages * PAGE_SIZE;

     if ((pPAddr == NULL) ||
         pPAddr->QuadPart == 0LL) {
-        m_pVAddr = new (NonPagedPoolNx) BYTE[size];
+        do {
+            m_pVAddr = new (NonPagedPoolNx) BYTE[size];
+            if (!m_pVAddr)
+            {
+                DbgPrint(TRACE_LEVEL_FATAL, ("%s insufficient resources to allocate %x bytes\n", __FUNCTION__, size));
+                VioGpuDbgBreak();
+                size /= 2;
+            }
+        } while (!m_pVAddr);

-        if (!m_pVAddr)
-        {
-            DbgPrint(TRACE_LEVEL_FATAL, ("%s insufficient resources to allocate %x bytes\n", __FUNCTION__, size));
-            return FALSE;
-        }
         RtlZeroMemory(m_pVAddr, size);
         m_bSystemMemory = TRUE;
     }
@@ -868,6 +868,10 @@ BOOLEAN VioGpuMemSegment::Init(_In_ UINT size, _In_opt_ PPHYSICAL_ADDRESS pPAddr
         m_bMapped = TRUE;
     }

+    UINT pages = BYTES_TO_PAGES(size);
+    UINT sglsize = sizeof(SCATTER_GATHER_LIST) + (sizeof(SCATTER_GATHER_ELEMENT) * pages);
+    size = pages * PAGE_SIZE;
+
     m_pMdl = IoAllocateMdl(m_pVAddr, size, FALSE, FALSE, NULL);
     if (!m_pMdl)
     {
diff --git a/viogpu/viogpu.sln b/viogpu/viogpu.sln
index b4381fe3..68705d53 100755
--- a/viogpu/viogpu.sln
+++ b/viogpu/viogpu.sln
@@ -1,7 +1,7 @@
 
 Microsoft Visual Studio Solution File, Format Version 12.00
-# Visual Studio 15
-VisualStudioVersion = 15.0.28307.168
+# Visual Studio Version 16
+VisualStudioVersion = 16.0.33927.289
 MinimumVisualStudioVersion = 10.0.40219.1
 Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "viogpudo", "viogpudo\viogpudo.vcxproj", "{BDE07E9E-2041-4853-8B74-7CF591C255E7}"
    ProjectSection(ProjectDependencies) = postProject
diff --git a/viogpu/viogpudo/trace.h b/viogpu/viogpudo/trace.h
index 76a306a2..943a10bb 100755
--- a/viogpu/viogpudo/trace.h
+++ b/viogpu/viogpudo/trace.h
@@ -1,6 +1,6 @@
 #pragma once

-//#define DBG 1
+#define DBG 1

 #ifndef TRACE_LEVEL_INFORMATION
 #define TRACE_LEVEL_NONE        0   // Tracing is not on
diff --git a/viogpu/viogpudo/viogpudo.cpp b/viogpu/viogpudo/viogpudo.cpp
index a0a5cdc8..cd731340 100755
--- a/viogpu/viogpudo/viogpudo.cpp
+++ b/viogpu/viogpudo/viogpudo.cpp
@@ -49,6 +49,11 @@ m_pHWDevice(NULL)

     DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
     *((UINT*)&m_Flags) = 0;
+#if NTDDI_VERSION > NTDDI_WINBLUE
+    m_dwMemorySize = 0x1000000;
+#else
+    m_dwMemorySize = 0x800000;
+#endif
     RtlZeroMemory(&m_DxgkInterface, sizeof(m_DxgkInterface));
     RtlZeroMemory(&m_DeviceInfo, sizeof(m_DeviceInfo));
     RtlZeroMemory(&m_CurrentMode, sizeof(m_CurrentMode));
@@ -1909,6 +1914,14 @@ NTSTATUS VioGpuDod::GetRegisterInfo(void)
         SetUsePresentProgress(!!value);
     }

+    value = 0;
+    VioGpuDbgBreak();
+    Status = ReadRegistryDWORD(DevInstRegKeyHandle, L"MemorySize", &value);
+    if (NT_SUCCESS(Status))
+    {
+        SetMemorySize(value);
+    }
+
     ZwClose(DevInstRegKeyHandle);
     DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
     return Status;
@@ -1935,6 +1948,12 @@ VioGpuAdapter::VioGpuAdapter(_In_ VioGpuDod* pVioGpuDod) // : IVioGpuAdapter(pVi
     KeInitializeEvent(&m_ConfigUpdateEvent,
         SynchronizationEvent,
         FALSE);
+    KeInitializeEvent(&m_CursorUpdateEvent,
+        SynchronizationEvent,
+        FALSE);
+    InitializeListHead(&m_CursorList);
+    KeInitializeSpinLock(&m_CursorListLock);
+
     m_bStopWorkThread = FALSE;
     m_pWorkThread = NULL;
     m_ResolutionEvent = NULL;
@@ -2326,12 +2345,7 @@ NTSTATUS VioGpuAdapter::HWInit(PCM_RESOURCE_LIST pResList, DXGK_DISPLAY_INFORMAT
     PHYSICAL_ADDRESS fb_pa = m_PciResources.GetPciBar(0)->GetPA();
     UINT fb_size = (UINT)m_PciResources.GetPciBar(0)->GetSize();
     UINT req_size = pDispInfo->Pitch * pDispInfo->Height;
-//FIXME
-#if NTDDI_VERSION > NTDDI_WINBLUE
-    req_size = 0x1000000;
-#else
-    req_size = 0x800000;
-#endif
+    req_size = m_pVioGpuDod->GetMemorySize();
     if (fb_pa.QuadPart != 0LL) {
         pDispInfo->PhysicAddress = fb_pa;
     }
@@ -2584,7 +2598,8 @@ VOID VioGpuAdapter::BlackOutScreen(CURRENT_MODE* pCurrentMod)
 NTSTATUS VioGpuAdapter::SetPointerShape(_In_ CONST DXGKARG_SETPOINTERSHAPE* pSetPointerShape, _In_ CONST CURRENT_MODE* pModeCur)
 {
     PAGED_CODE();
-
+    KIRQL lockHandle;
+    NTSTATUS status = STATUS_UNSUCCESSFUL;
     DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
     DbgPrint(TRACE_LEVEL_INFORMATION, ("<--> %s flag = %d pitch = %d, pixels = %p, id = %d, w = %d, h = %d, x = %d, y = %d\n", __FUNCTION__,
         pSetPointerShape->Flags.Value,
@@ -2598,14 +2613,15 @@ NTSTATUS VioGpuAdapter::SetPointerShape(_In_ CONST DXGKARG_SETPOINTERSHAPE* pSet

     if (pSetPointerShape->Flags.Monochrome) {
         VioGpuDbgBreak();
-        return STATUS_UNSUCCESSFUL;
+        return status;
     }
+    
+    KeAcquireSpinLock(&m_CursorListLock, &lockHandle);

     if (UpdateCursor(pSetPointerShape, pModeCur))
     {
         PGPU_UPDATE_CURSOR crsr;
         PGPU_VBUFFER vbuf;
-        UINT ret = 0;
         crsr = (PGPU_UPDATE_CURSOR)m_CursorQueue.AllocCursor(&vbuf);
         RtlZeroMemory(crsr, sizeof(*crsr));

@@ -2615,25 +2631,27 @@ NTSTATUS VioGpuAdapter::SetPointerShape(_In_ CONST DXGKARG_SETPOINTERSHAPE* pSet
         crsr->pos.y = 0;
         crsr->hot_x = pSetPointerShape->XHot;
         crsr->hot_y = pSetPointerShape->YHot;
-        ret = m_CursorQueue.QueueCursor(vbuf);
-        DbgPrint(TRACE_LEVEL_INFORMATION, ("<--- %s vbuf = %p, ret = %d\n", __FUNCTION__, vbuf, ret));
-        if (ret == 0) {
-            return STATUS_SUCCESS;
-        }
-        VioGpuDbgBreak();
+        PCURSOR_REQUEST cursor_req = new (NonPagedPoolNx) CURSOR_REQUEST;
+        cursor_req->pbuf = vbuf;
+        cursor_req->type = VIRTIO_GPU_CMD_UPDATE_CURSOR;
+        InsertHeadList(&m_CursorList, &cursor_req->ListEntry);
+        KeSetEvent(&m_CursorUpdateEvent, IO_NO_INCREMENT, FALSE);
     }
+    KeReleaseSpinLock(&m_CursorListLock, lockHandle);
     DbgPrint(TRACE_LEVEL_ERROR, ("<--- %s Failed to create cursor\n", __FUNCTION__));
-    return STATUS_UNSUCCESSFUL;
+    return status;
 }

 NTSTATUS VioGpuAdapter::SetPointerPosition(_In_ CONST DXGKARG_SETPOINTERPOSITION* pSetPointerPosition, _In_ CONST CURRENT_MODE* pModeCur)
 {
     PAGED_CODE();
+    KIRQL lockHandle;
+    NTSTATUS status = STATUS_UNSUCCESSFUL;
+    KeAcquireSpinLock(&m_CursorListLock, &lockHandle);
     if (m_pCursorBuf != NULL)
     {
         PGPU_UPDATE_CURSOR crsr;
         PGPU_VBUFFER vbuf;
-        UINT ret = 0;
         crsr = (PGPU_UPDATE_CURSOR)m_CursorQueue.AllocCursor(&vbuf);
         RtlZeroMemory(crsr, sizeof(*crsr));

@@ -2668,14 +2686,15 @@ NTSTATUS VioGpuAdapter::SetPointerPosition(_In_ CONST DXGKARG_SETPOINTERPOSITION
             crsr->pos.x = pSetPointerPosition->X;
             crsr->pos.y = pSetPointerPosition->Y;
         }
-        ret = m_CursorQueue.QueueCursor(vbuf);
-        DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s vbuf = %p, ret = %d\n", __FUNCTION__, vbuf, ret));
-        if (ret == 0) {
-            return STATUS_SUCCESS;
-        }
-        VioGpuDbgBreak();
+        PCURSOR_REQUEST cursor_req = new (NonPagedPoolNx) CURSOR_REQUEST;
+        cursor_req->pbuf = vbuf;
+        cursor_req->type = VIRTIO_GPU_CMD_MOVE_CURSOR;
+        InsertTailList(&m_CursorList, &cursor_req->ListEntry);
+        KeSetEvent(&m_CursorUpdateEvent, IO_NO_INCREMENT, FALSE);
+        status = STATUS_SUCCESS;
     }
-    return STATUS_UNSUCCESSFUL;
+    KeReleaseSpinLock(&m_CursorListLock, lockHandle);
+    return status;
 }

 NTSTATUS VioGpuAdapter::Escape(_In_ CONST DXGKARG_ESCAPE* pEscape)
@@ -2784,7 +2803,7 @@ BOOLEAN VioGpuAdapter::GetEdids(void)
     DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));

     PGPU_VBUFFER vbuf = NULL;
-
+    VioGpuDbgBreak();
     for (UINT32 i = 0; i < m_u32NumScanouts; i++) {
         if (m_CtrlQueue.AskEdidInfo(&vbuf, i) &&
             m_CtrlQueue.GetEdidInfo(vbuf, i, m_EDIDs)) {
@@ -2991,6 +3010,7 @@ NTSTATUS VioGpuAdapter::GetModeList(DXGK_DISPLAY_INFORMATION* pDispInfo)
     m_ModeCount = SuitableModeCount + 1;
     DbgPrint(TRACE_LEVEL_INFORMATION, ("ModeCount filtered %d\n", m_ModeCount));

+    VioGpuDbgBreak();
     GetDisplayInfo();

     for (ULONG idx = 0; idx < ModeCount; idx++)
@@ -3075,21 +3095,57 @@ void VioGpuAdapter::ThreadWork(_In_ PVOID Context)
 void VioGpuAdapter::ThreadWorkRoutine(void)
 {
     KeSetPriorityThread(KeGetCurrentThread(), LOW_REALTIME_PRIORITY);
-
+    NTSTATUS status;
+    PVOID events[2];
+    KIRQL  lockHandle;
+    events[0] = &m_ConfigUpdateEvent;
+    events[1] = &m_CursorUpdateEvent;
     for (;;)
     {
-        KeWaitForSingleObject(&m_ConfigUpdateEvent,
-            Executive,
-            KernelMode,
-            FALSE,
-            NULL);
-
-        if (m_bStopWorkThread) {
-            PsTerminateSystemThread(STATUS_SUCCESS);
-            break;
+        //KeWaitForSingleObject(&m_ConfigUpdateEvent,
+        //    Executive,
+        //    KernelMode,
+        //    FALSE,
+        //    NULL);
+        status = KeWaitForMultipleObjects(2, events, WaitAny, Executive, KernelMode,
+            FALSE, NULL, NULL);
+
+        if (status == STATUS_WAIT_0) {
+            if (m_bStopWorkThread) {
+                PsTerminateSystemThread(STATUS_SUCCESS);
+                break;
+            }
+            ConfigChanged();
+            NotifyResolutionEvent();
+        }
+        else {
+            KeAcquireSpinLock(&m_CursorListLock, &lockHandle);
+            if (!IsListEmpty(&m_CursorList))
+            {
+                PGPU_VBUFFER pbuf = NULL;
+                PLIST_ENTRY le = RemoveHeadList(&m_CursorList);
+                PCURSOR_REQUEST cursor_req = CONTAINING_RECORD(le, CURSOR_REQUEST, ListEntry);
+                if ((cursor_req->type == VIRTIO_GPU_CMD_MOVE_CURSOR) &&
+                    !IsListEmpty(&m_CursorList)) {
+                        pbuf = cursor_req->pbuf;
+                        delete cursor_req;
+                        m_CursorQueue.ReleaseBuffer(pbuf);
+                        le = RemoveTailList(&m_CursorList);
+                        cursor_req = CONTAINING_RECORD(le, CURSOR_REQUEST, ListEntry);
+                }
+                pbuf = cursor_req->pbuf;
+                delete cursor_req;
+                m_CursorQueue.QueueCursor(pbuf);
+                while (!IsListEmpty(&m_CursorList)) {
+                    le = RemoveHeadList(&m_CursorList);
+                    cursor_req = CONTAINING_RECORD(le, CURSOR_REQUEST, ListEntry);
+                    pbuf = cursor_req->pbuf;
+                    delete cursor_req;
+                    m_CursorQueue.ReleaseBuffer(pbuf);
+                }
+            }
+            KeReleaseSpinLock(&m_CursorListLock, lockHandle);
         }
-        ConfigChanged();
-        NotifyResolutionEvent();
     }
 }

diff --git a/viogpu/viogpudo/viogpudo.h b/viogpu/viogpudo/viogpudo.h
index 1fcd3ba8..d4eb573b 100755
--- a/viogpu/viogpudo/viogpudo.h
+++ b/viogpu/viogpudo/viogpudo.h
@@ -70,6 +70,13 @@ typedef struct _CURRENT_MODE
     PVOID FrameBuffer;
 } CURRENT_MODE;

+typedef struct _CURSOR_REQUEST {
+    LIST_ENTRY ListEntry;
+    ULONG type;
+    PGPU_VBUFFER pbuf;
+} CURSOR_REQUEST, * PCURSOR_REQUEST;
+
+
 class VioGpuDod;

 class VioGpuAdapter
@@ -167,6 +174,9 @@ private:
     VioGpuMemSegment m_FrameSegment;
     volatile ULONG m_PendingWorks;
     KEVENT m_ConfigUpdateEvent;
+    KEVENT m_CursorUpdateEvent;
+    LIST_ENTRY m_CursorList;
+    KSPIN_LOCK m_CursorListLock;
     PETHREAD m_pWorkThread;
     BOOLEAN m_bStopWorkThread;
     PKEVENT m_ResolutionEvent;
@@ -182,6 +192,7 @@ private:
     DEVICE_POWER_STATE m_MonitorPowerState;
     DEVICE_POWER_STATE m_AdapterPowerState;
     DRIVER_STATUS_FLAG m_Flags;
+    DWORD m_dwMemorySize;

     CURRENT_MODE m_CurrentMode;

@@ -246,6 +257,14 @@ public:
     {
         m_Flags.UsePresentProgress = enable;
     }
+    void SetMemorySize(DWORD size)
+    {
+        m_dwMemorySize = max(m_dwMemorySize, (size * 1024 * 1024));
+    }
+    DWORD GetMemorySize() const
+    {
+        return m_dwMemorySize;
+    }
 #pragma code_seg(pop)

     NTSTATUS StartDevice(_In_  DXGK_START_INFO*   pDxgkStartInfo,
diff --git a/viogpu/viogpudo/viogpudo.inx b/viogpu/viogpudo/viogpudo.inx
index 11957e55..96c4bc45 100755
--- a/viogpu/viogpudo/viogpudo.inx
+++ b/viogpu/viogpudo/viogpudo.inx
@@ -63,10 +63,11 @@ HKR,,EventMessageFile,%REG_EXPAND_SZ%,"%%SystemRoot%%\System32\IoLogMsg.dll"
 HKR,,TypesSupported,%REG_DWORD%,7

 [VioGpuDod_DeviceSettings]
-HKR,, HWCursor,                    %REG_DWORD%, 0
+HKR,, HWCursor,                    %REG_DWORD%, 1
 HKR,, FlexResolution,              %REG_DWORD%, 1
 HKR,, UsePhysicalMemory,           %REG_DWORD%, 1
 HKR,, UsePresentProgress,          %REG_DWORD%, 1
+HKR,, MemorySize,                  %REG_DWORD%, 32

 [VioGpuDod_PCI_MSIX]
 HKR, "Interrupt Management",, 0x00000010

All the best, Vadim.

[YanVugenfirer]

@vrozenfe can I merge?

[YanVugenfirer]

Wait a second, @kostyanf14 , is "WDDM RotateBlt Window GDI (WoW64)" a new failure?

[kostyanf14]

Wait a second, @kostyanf14 , is "WDDM RotateBlt Window GDI (WoW64)" a new failure?

@YanVugenfirer, it fail sometimes not always

[vrozenfe]

It crashes into KMODE_EXCEPTION_NOT_HANDLED (1E) BSOD almost instantly on my testing setup. Apart from that I saw some issues with the cursor pointer update

(qemu) info version 8.1.0v8.1.0

qemu cli /home/vrozenfe/work/upstream/qemu/build/x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,hv_relaxed,hv_vapic,hv_spinlocks=0x1fff -m 2G -smp 4,maxcpus=4,cores=4,threads=1,sockets=1 -usb -device usb-tablet,id=tablet0 -drive file=/run/media/vrozenfe/elements/images/win10-32-gpu.qcow2,if=none,id=drive-ide0-0-0,cache=off,werror=stop,rerror=stop -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 -netdev tap,id=hostnet0,script=/etc/qemu-ifup -device e1000,netdev=hostnet0,mac=52:83:66:77:88:68,id=net0 -boot c -uuid 5b959a71-e33f-4419-97b4-da6fe8fb7062 -rtc driftfix=slew -global kvm-pit.lost_tick_policy=discard -monitor stdio -name VIOVGA -enable-kvm -device virtio-vga,edid=on,xres=1280,yres=800,max_outputs=1 -serial tcp:127.0.0.1:4445

[gang929]

So complex? Can you try this?