On Mon, Mar 20, 2017 at 2:01 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
Why we need to wait here? Same above, no much need to clear the event.>
> 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 <javier.celaya@xxxxxxxxxxx>
> Signed-off-by: Yuri Benditovich <yuri.benditovich@xxxxxxxxxx>
> ---
> 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);
> +
Wait is not mandatory - it is here to have predictable timing and ensure the thread is ready before first 'Present' command.
In the unluckily event that ObReferenceObjectByHandle failed would> + 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);
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.
why all these reinterpret_cast ?
> + 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);
Just
delete[] drawables;
is enough (obviously allocation should the changed too).
Not a regression, however this structure cannot be byte-packed.
> + }
> + 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"
> +
This would cause potentially problems like 255+1 == 0.
I do not see how this can cause problem.
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.
Frediano> 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 {
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel