Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

viogpudo: fix HWCursor display issues #990

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

osy
Copy link

@osy osy commented Oct 9, 2023

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
Copy link
Collaborator

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

Joelle van Dyne added 3 commits October 9, 2023 08:38
`SetPointerShape` calls `CreateCursor` which copies the shape data to
the host via `TransferToHost2D`. Then it calls `QueueCursor` which sets
the cursor on the host. There is a race condition between these two
functions because `TransferToHost2D` uses the `m_CtrlQueue` and
`QueueCursor` uses the `m_CursorQueue`. Whenever the cursor queue is
handled first by QEMU, the host attempts to set the cursor image by
reading the buffer that is still empty because `TransferToHost2D` had
not yet executed.

The fix here is to wait on `TransferToHost2D` to complete before calling
`QueueCursor`. We achieve this by waiting on an KEVENT and moving some
code around in the IRQ handler to set this event after recieving the
response for the transfer command.

Signed-off-by: Joelle van Dyne <[email protected]>
QEMU checks `cursor->resource_id` and if it is 0, then the cursor
will be hidden.

Signed-off-by: Joelle van Dyne <[email protected]>
Signed-off-by: Joelle van Dyne <[email protected]>
@osy
Copy link
Author

osy commented Oct 9, 2023

@YanVugenfirer Done

@YanVugenfirer
Copy link
Collaborator

@vrozenfe Can you please take a look?

@vrozenfe
Copy link
Collaborator

vrozenfe commented Oct 12, 2023

@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
Copy link
Collaborator

@vrozenfe can I merge?

@YanVugenfirer
Copy link
Collaborator

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

@kostyanf14
Copy link
Member

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

@YanVugenfirer, it fail sometimes not always

@vrozenfe
Copy link
Collaborator

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
cursor

(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
Copy link

gang929 commented Nov 1, 2023

So complex? Can you try this?

@YanVugenfirer
Copy link
Collaborator

@osy Do you want to try and fix the PR?

@osy
Copy link
Author

osy commented Jul 29, 2024

Sorry I've been very busy with other things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants