<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Mon, Mar 20, 2017 at 2:01 PM, Frediano Ziglio <<a href="mailto:fziglio@redhat.com" target="_blank">fziglio@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">> > Create rendering thread upon device start and terminate it upon > stop. The dedicated thread is normally pending for display commands > to be sent to the host. Currently only single NULL (termination) > command is placed to the ring where the thread consumes events from. > > Signed-off-by: Javier Celaya <<a href="mailto:javier.celaya@flexvdi.com">javier.celaya@flexvdi.com</a>> > Signed-off-by: Yuri Benditovich <<a href="mailto:yuri.benditovich@daynix.com">yuri.benditovich@daynix.com</a>> > --- > qxldod/QxlDod.cpp | 114 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > qxldod/QxlDod.h | 16 ++++++++ > 2 files changed, 130 insertions(+) > > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp > index 9175300..b952bf9 100755 > --- a/qxldod/QxlDod.cpp > +++ b/qxldod/QxlDod.cpp > @@ -3062,6 +3062,7 @@ QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod) > m_CustomMode = 0; > m_FreeOutputs = 0; > m_Pending = 0; > + m_PresentThread = NULL; > } > > QxlDevice::~QxlDevice(void) > @@ -3441,6 +3442,29 @@ NTSTATUS QxlDevice::HWInit(PCM_RESOURCE_LIST pResList, > DXGK_DISPLAY_INFORMATION* > return QxlInit(pDispInfo); > } > > +NTSTATUS QxlDevice::StartPresentThread() > +{ > + PAGED_CODE(); > + OBJECT_ATTRIBUTES ObjectAttributes; > + NTSTATUS Status; > + > + KeClearEvent(&m_PresentThreadReadyEvent); > + > + InitializeObjectAttributes(&ObjectAttributes, NULL, OBJ_KERNEL_HANDLE, > NULL, NULL); > + Status = PsCreateSystemThread( > + &m_PresentThread, > + THREAD_ALL_ACCESS, > + &ObjectAttributes, > + NULL, > + NULL, > + PresentThreadRoutineWrapper, > + this); > + > + WaitForObject(&m_PresentThreadReadyEvent, NULL); > + </div></div>Why we need to wait here? Same above, no much need to clear the event. <div><div class="gmail-h5"> </div></div></blockquote><div>Wait is not mandatory - it is here to have predictable timing and ensure the thread is ready before first 'Present' command.</div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail-h5"> > + return Status; > +} > + > NTSTATUS QxlDevice::QxlInit(DXGK_DISPLAY_INFORMATION* pDispInfo) > { > PAGED_CODE(); > @@ -3467,12 +3491,17 @@ NTSTATUS QxlDevice::QxlInit(DXGK_DISPLAY_INFORMATION* > pDispInfo) > InitDeviceMemoryResources(); > InitMonitorConfig(); > Status = AcquireDisplayInfo(*(pDispInfo)); > + if (NT_SUCCESS(Status)) > + { > + Status = StartPresentThread(); > + } > return Status; > } > > void QxlDevice::QxlClose() > { > PAGED_CODE(); > + StopPresentThread(); > DestroyMemSlots(); > } > > @@ -3609,6 +3638,12 @@ BOOL QxlDevice::CreateEvents() > KeInitializeEvent(&m_IoCmdEvent, > SynchronizationEvent, > FALSE); > + KeInitializeEvent(&m_PresentEvent, > + SynchronizationEvent, > + FALSE); > + KeInitializeEvent(&m_PresentThreadReadyEvent, > + SynchronizationEvent, > + FALSE); > KeInitializeMutex(&m_MemLock,1); > KeInitializeMutex(&m_CmdLock,1); > KeInitializeMutex(&m_IoLock,1); > @@ -3625,6 +3660,7 @@ BOOL QxlDevice::CreateRings() > m_CommandRing = &(m_RamHdr->cmd_ring); > m_CursorRing = &(m_RamHdr->cursor_ring); > m_ReleaseRing = &(m_RamHdr->release_ring); > + SPICE_RING_INIT(m_PresentRing); > DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__)); > return TRUE; > } > @@ -4907,6 +4943,7 @@ UINT SpiceFromPixelFormat(D3DDDIFORMAT Format) > > NTSTATUS HwDeviceInterface::AcquireDisplayInfo(DXGK_DISPLAY_INFORMATION& > DispInfo) > { > + PAGED_CODE(); > NTSTATUS Status = STATUS_SUCCESS; > if (GetId() == 0) > { > @@ -4996,3 +5033,80 @@ QXL_NON_PAGED VOID QxlDod::VsyncTimerProcGate(_In_ > _KDPC *dpc, _In_ PVOID contex > QxlDod* pQxl = reinterpret_cast<QxlDod*>(context); > pQxl->VsyncTimerProc(); > } > + > +void QxlDevice::StopPresentThread() > +{ > + PAGED_CODE(); > + PVOID pDispatcherObject; > + if (m_PresentThread) > + { > + DbgPrint(TRACE_LEVEL_INFORMATION, ("---> %s\n", __FUNCTION__)); > + NTSTATUS Status = ObReferenceObjectByHandle( > + m_PresentThread, 0, NULL, KernelMode, &pDispatcherObject, NULL); > + if (NT_SUCCESS(Status)) > + { > + PostToWorkerThread(NULL); </div></div>In the unluckily event that ObReferenceObjectByHandle failed would be better to do this call anyway, so we have a chance the thread will be destroyed before system potentially free from memory the thread code and data. <div><div class="gmail-h5"> > + WaitForObject(pDispatcherObject, NULL); > + ObDereferenceObject(pDispatcherObject); > + } > + ZwClose(m_PresentThread); > + m_PresentThread = NULL; > + DbgPrint(TRACE_LEVEL_INFORMATION, ("<--- %s\n", __FUNCTION__)); > + } > +} > + > +void QxlDevice::PresentThreadRoutine() > +{ > + PAGED_CODE(); > + int wait; > + int notify; > + QXLDrawable** drawables; > + > + DbgPrint(TRACE_LEVEL_INFORMATION, ("--->%s\n", __FUNCTION__)); > + > + KeSetEvent(&m_PresentThreadReadyEvent, 0, FALSE); > + > + while (1) > + { > + // Pop a drawables list from the ring > + // No need for a mutex, only one consumer thread > + SPICE_RING_CONS_WAIT(m_PresentRing, wait); > + while (wait) { > + WaitForObject(&m_PresentEvent, NULL); > + SPICE_RING_CONS_WAIT(m_PresentRing, wait); > + } > + drawables = *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) > + PushDrawable(drawables[i]); > + delete[] reinterpret_cast<BYTE*>(drawables); </div></div>why all these reinterpret_cast ? Just delete[] drawables; is enough (obviously allocation should the changed too). <div><div class="gmail-h5"> > + } > + else { > + DbgPrint(TRACE_LEVEL_WARNING, ("%s is being terminated\n", > __FUNCTION__)); > + break; > + } > + } > +} > + > +void QxlDevice::PostToWorkerThread(QXLDrawable** drawables) > +{ > + PAGED_CODE(); > + // Push drawables into PresentRing and notify worker thread > + int notify, wait; > + SPICE_RING_PROD_WAIT(m_PresentRing, wait); > + while (wait) { > + WaitForObject(&m_PresentThreadReadyEvent, NULL); > + SPICE_RING_PROD_WAIT(m_PresentRing, wait); > + } > + *SPICE_RING_PROD_ITEM(m_PresentRing) = drawables; > + SPICE_RING_PUSH(m_PresentRing, notify); > + if (notify) { > + KeSetEvent(&m_PresentEvent, 0, FALSE); > + } > + DbgPrint(TRACE_LEVEL_INFORMATION, ("<--- %s\n", __FUNCTION__)); > +} > diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h > index 9cfb6de..4a62680 100755 > --- a/qxldod/QxlDod.h > +++ b/qxldod/QxlDod.h > @@ -456,6 +456,10 @@ typedef struct DpcCbContext { > #define MAX(x, y) (((x) >= (y)) ? (x) : (y)) > #define ALIGN(a, b) (((a) + ((b) - 1)) & ~((b) - 1)) > > +#include "start-packed.h" > +SPICE_RING_DECLARE(QXLPresentOnlyRing, QXLDrawable**, 1024); > +#include "end-packed.h" > + </div></div>Not a regression, however this structure cannot be byte-packed. This would cause potentially problems like 255+1 == 0. <div class="gmail-HOEnZb"><div class="gmail-h5"> </div></div></blockquote><div>I do not see how this can cause problem.</div><div>Packing at byte boundary is not needed here, but the alternative is (ugly) to use #define SPICE_ATTR_PACKED before declaration and #undef SPICE_ATTR_PACKED after.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">> class QxlDevice : > public HwDeviceInterface > { > @@ -559,6 +563,13 @@ private: > NTSTATUS UpdateChildStatus(BOOLEAN connect); > NTSTATUS SetCustomDisplay(QXLEscapeSetCustomDisplay* custom_display); > void SetMonitorConfig(QXLHead* monitor_config); > + NTSTATUS StartPresentThread(); > + void StopPresentThread(); > + void PresentThreadRoutine(); > + static void PresentThreadRoutineWrapper(HANDLE dev) { > + ((QxlDevice *)dev)->PresentThreadRoutine(); > + } > + void PostToWorkerThread(QXLDrawable** drawables); > > static LONG GetMaxSourceMappingHeight(RECT* DirtyRects, ULONG > NumDirtyRects); > > @@ -594,6 +605,8 @@ private: > KEVENT m_DisplayEvent; > KEVENT m_CursorEvent; > KEVENT m_IoCmdEvent; > + KEVENT m_PresentEvent; > + KEVENT m_PresentThreadReadyEvent; > > PUCHAR m_LogPort; > PUCHAR m_LogBuf; > @@ -609,6 +622,9 @@ private: > > QXLMonitorsConfig* m_monitor_config; > QXLPHYSICAL* m_monitor_config_pa; > + > + QXLPresentOnlyRing m_PresentRing[1]; > + HANDLE m_PresentThread; > }; > > class QxlDod { </div></div>Frediano </blockquote></div> </div></div>