Re: [PATCH 01/12] qxl-wddm-dod: Prepare system thread for rendering

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On Mon, Mar 20, 2017 at 2:01 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>
> 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);
> +

Why we need to wait here? Same above, no much need to clear the event.

Wait is not mandatory - it is here to have predictable timing and ensure the thread is ready before first 'Present' command.

 
> +    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);

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.

> +            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);

why all these reinterpret_cast ?
Just

  delete[] drawables;

is enough (obviously allocation should the changed too).

> +        }
> +        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"
> +

Not a regression, however this structure cannot be byte-packed.
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.
 
>  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 {

Frediano

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]