[Spice-devel,v2,4/6] qxl-wddm-dod: Simplify interrupt handling for rev4 device

Submitted by Yuri Benditovich on Feb. 15, 2017, noon

Details

Message ID 1487160015-147620-5-git-send-email-yuri.benditovich@daynix.com
State New
Headers show
Series "Preparation to pass HLK tests" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Yuri Benditovich Feb. 15, 2017, noon
Do not clear interrupt mask upon interrupt.
Instead clear pending interrupt status and write
QXL_IO_UPDATE_IRQ register (this drops interrupt level).
There are 3 advantages:
1. We do not need to wake the host to enable interrupt
mask in DPC procedure (1 wake per interrupt instead of 2)
2. The driver is not sensitive to failure when queues DPC, as
already queued DPC will process this interrupt when executed.
3. When we implement VSync interrupt simulation, we do not
need to touch registers neither when notify the OS nor when
process DPC related to this notification.

Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
---
 qxldod/QxlDod.cpp | 54 ++++++++++++------------------------------------------
 qxldod/QxlDod.h   |  4 +---
 2 files changed, 13 insertions(+), 45 deletions(-)

Patch hide | download patch | download mbox

diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
index 2d2a022..cd0a1e5 100755
--- a/qxldod/QxlDod.cpp
+++ b/qxldod/QxlDod.cpp
@@ -4799,14 +4799,14 @@  BOOLEAN QxlDevice::InterruptRoutine(_In_ PDXGKRNL_INTERFACE pDxgkInterface, _In_
     if (!(m_RamHdr->int_pending & m_RamHdr->int_mask)) {
         return FALSE;
     }
-    m_RamHdr->int_mask = 0;
+    m_Pending |= InterlockedExchange((LONG *)&m_RamHdr->int_pending, 0);
     WRITE_PORT_UCHAR((PUCHAR)(m_IoBase + QXL_IO_UPDATE_IRQ), 0);
-    m_Pending |= m_RamHdr->int_pending;
-    m_RamHdr->int_pending = 0;
+    // QXL_IO_UPDATE_IRQ sets interrupt level to m_RamHdr->int_pending & m_RamHdr->int_mask
+    // so it will be dropped if interrupt status is not modified after clear
+
     if (!pDxgkInterface->DxgkCbQueueDpc(pDxgkInterface->DeviceHandle)) {
-        m_RamHdr->int_mask = WIN_QXL_INT_MASK;
-        WRITE_PORT_UCHAR((PUCHAR)(m_IoBase + QXL_IO_UPDATE_IRQ), 0);
-        DbgPrint(TRACE_LEVEL_FATAL, ("---> %s DxgkCbQueueDpc failed\n", __FUNCTION__));
+        // DPC already queued and will process m_Pending when called
+        DbgPrint(TRACE_LEVEL_WARNING, ("---> %s can't queue Dpc for %X\n", __FUNCTION__, m_Pending));
     }
     DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
     return TRUE;
@@ -4815,34 +4815,22 @@  BOOLEAN QxlDevice::InterruptRoutine(_In_ PDXGKRNL_INTERFACE pDxgkInterface, _In_
 QXL_NON_PAGED
 VOID QxlDevice::DpcRoutine(PVOID ptr)
 {
-    PDXGKRNL_INTERFACE pDxgkInterface = (PDXGKRNL_INTERFACE)ptr;
-
+    LONG intStatus = InterlockedExchange(&m_Pending, 0);
+    UNREFERENCED_PARAMETER(ptr);
     DbgPrint(TRACE_LEVEL_INFORMATION, ("---> %s\n", __FUNCTION__));
-    DPC_CB_CONTEXT ctx;
-    BOOLEAN dummy;
-    ctx.ptr = this;
-    NTSTATUS Status = pDxgkInterface->DxgkCbSynchronizeExecution(
-            pDxgkInterface->DeviceHandle,
-            DpcCallbackEx,
-            &ctx,
-            0,
-            &dummy);
-    ASSERT(Status == STATUS_SUCCESS);
-
-    if (ctx.data & QXL_INTERRUPT_DISPLAY) {
+
+    if (intStatus & QXL_INTERRUPT_DISPLAY) {
         DbgPrint(TRACE_LEVEL_INFORMATION, ("---> %s m_DisplayEvent\n", __FUNCTION__));
         KeSetEvent (&m_DisplayEvent, IO_NO_INCREMENT, FALSE);
     }
-    if (ctx.data & QXL_INTERRUPT_CURSOR) {
+    if (intStatus & QXL_INTERRUPT_CURSOR) {
         DbgPrint(TRACE_LEVEL_INFORMATION, ("---> %s m_CursorEvent\n", __FUNCTION__));
         KeSetEvent (&m_CursorEvent, IO_NO_INCREMENT, FALSE);
     }
-    if (ctx.data & QXL_INTERRUPT_IO_CMD) {
+    if (intStatus & QXL_INTERRUPT_IO_CMD) {
         DbgPrint(TRACE_LEVEL_INFORMATION, ("---> %s m_IoCmdEvent\n", __FUNCTION__));
         KeSetEvent (&m_IoCmdEvent, IO_NO_INCREMENT, FALSE);
     }
-    m_RamHdr->int_mask = WIN_QXL_INT_MASK;
-    WRITE_PORT_UCHAR((PUCHAR)(m_IoBase + QXL_IO_UPDATE_IRQ), 0);
 
     DbgPrint(TRACE_LEVEL_INFORMATION, ("<--- %s\n", __FUNCTION__));
 }
@@ -4859,24 +4847,6 @@  VOID QxlDevice::UpdateArea(CONST RECT* area, UINT32 surface_id)
 }
 
 QXL_NON_PAGED
-BOOLEAN QxlDevice:: DpcCallbackEx(PVOID ptr)
-{
-    DbgPrint(TRACE_LEVEL_VERBOSE, ("<--> %s\n", __FUNCTION__));
-    PDPC_CB_CONTEXT ctx = (PDPC_CB_CONTEXT) ptr;
-    QxlDevice* pqxl = (QxlDevice*)ctx->ptr;
-    pqxl->DpcCallback(ctx);
-    return TRUE;
-}
-
-QXL_NON_PAGED
-VOID QxlDevice::DpcCallback(PDPC_CB_CONTEXT ctx)
-{
-    ctx->data = m_Pending;
-    m_Pending = 0;
-
-}
-
-QXL_NON_PAGED
 UINT BPPFromPixelFormat(D3DDDIFORMAT Format)
 {
     switch (Format)
diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
index 4598718..53724e8 100755
--- a/qxldod/QxlDod.h
+++ b/qxldod/QxlDod.h
@@ -549,8 +549,6 @@  private:
     void PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
                             UINT8 **end_ptr, UINT8 *src, int size,
                             size_t alloc_size, uint32_t alignment);
-    QXL_NON_PAGED BOOLEAN static DpcCallbackEx(PVOID);
-    QXL_NON_PAGED void DpcCallback(PDPC_CB_CONTEXT);
     void AsyncIo(UCHAR  Port, UCHAR Value);
     void SyncIo(UCHAR  Port, UCHAR Value);
     NTSTATUS UpdateChildStatus(BOOLEAN connect);
@@ -602,7 +600,7 @@  private:
     MspaceInfo m_MSInfo[NUM_MSPACES];
 
     UINT64 m_FreeOutputs;
-    UINT32 m_Pending;
+    LONG m_Pending;
 
     QXLMonitorsConfig* m_monitor_config;
     QXLPHYSICAL* m_monitor_config_pa;

Comments

> 
> Do not clear interrupt mask upon interrupt.
> Instead clear pending interrupt status and write
> QXL_IO_UPDATE_IRQ register (this drops interrupt level).
> There are 3 advantages:
> 1. We do not need to wake the host to enable interrupt
> mask in DPC procedure (1 wake per interrupt instead of 2)
> 2. The driver is not sensitive to failure when queues DPC, as
> already queued DPC will process this interrupt when executed.
> 3. When we implement VSync interrupt simulation, we do not
> need to touch registers neither when notify the OS nor when
> process DPC related to this notification.
> 
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> ---
>  qxldod/QxlDod.cpp | 54
>  ++++++++++++------------------------------------------
>  qxldod/QxlDod.h   |  4 +---
>  2 files changed, 13 insertions(+), 45 deletions(-)
> 
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> index 2d2a022..cd0a1e5 100755
> --- a/qxldod/QxlDod.cpp
> +++ b/qxldod/QxlDod.cpp
> @@ -4799,14 +4799,14 @@ BOOLEAN QxlDevice::InterruptRoutine(_In_
> PDXGKRNL_INTERFACE pDxgkInterface, _In_
>      if (!(m_RamHdr->int_pending & m_RamHdr->int_mask)) {
>          return FALSE;
>      }
> -    m_RamHdr->int_mask = 0;
> +    m_Pending |= InterlockedExchange((LONG *)&m_RamHdr->int_pending, 0);

I was afraid int_pending could be the wrong type or not properly
aligned but luckily it is so this line works fine (and is supposed
to continue to work).

>      WRITE_PORT_UCHAR((PUCHAR)(m_IoBase + QXL_IO_UPDATE_IRQ), 0);
> -    m_Pending |= m_RamHdr->int_pending;
> -    m_RamHdr->int_pending = 0;
> +    // QXL_IO_UPDATE_IRQ sets interrupt level to m_RamHdr->int_pending &
> m_RamHdr->int_mask
> +    // so it will be dropped if interrupt status is not modified after clear
> +
>      if (!pDxgkInterface->DxgkCbQueueDpc(pDxgkInterface->DeviceHandle)) {
> -        m_RamHdr->int_mask = WIN_QXL_INT_MASK;
> -        WRITE_PORT_UCHAR((PUCHAR)(m_IoBase + QXL_IO_UPDATE_IRQ), 0);
> -        DbgPrint(TRACE_LEVEL_FATAL, ("---> %s DxgkCbQueueDpc failed\n",
> __FUNCTION__));
> +        // DPC already queued and will process m_Pending when called
> +        DbgPrint(TRACE_LEVEL_WARNING, ("---> %s can't queue Dpc for %X\n",
> __FUNCTION__, m_Pending));
>      }
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
>      return TRUE;
> @@ -4815,34 +4815,22 @@ BOOLEAN QxlDevice::InterruptRoutine(_In_
> PDXGKRNL_INTERFACE pDxgkInterface, _In_
>  QXL_NON_PAGED
>  VOID QxlDevice::DpcRoutine(PVOID ptr)
>  {
> -    PDXGKRNL_INTERFACE pDxgkInterface = (PDXGKRNL_INTERFACE)ptr;
> -
> +    LONG intStatus = InterlockedExchange(&m_Pending, 0);
> +    UNREFERENCED_PARAMETER(ptr);

I'd use 

   VOID QxlDevice::DpcRoutine(PVOID)

instead but it's just style

>      DbgPrint(TRACE_LEVEL_INFORMATION, ("---> %s\n", __FUNCTION__));
> -    DPC_CB_CONTEXT ctx;
> -    BOOLEAN dummy;
> -    ctx.ptr = this;
> -    NTSTATUS Status = pDxgkInterface->DxgkCbSynchronizeExecution(
> -            pDxgkInterface->DeviceHandle,
> -            DpcCallbackEx,
> -            &ctx,
> -            0,
> -            &dummy);
> -    ASSERT(Status == STATUS_SUCCESS);
> -
> -    if (ctx.data & QXL_INTERRUPT_DISPLAY) {
> +
> +    if (intStatus & QXL_INTERRUPT_DISPLAY) {
>          DbgPrint(TRACE_LEVEL_INFORMATION, ("---> %s m_DisplayEvent\n",
>          __FUNCTION__));
>          KeSetEvent (&m_DisplayEvent, IO_NO_INCREMENT, FALSE);
>      }
> -    if (ctx.data & QXL_INTERRUPT_CURSOR) {
> +    if (intStatus & QXL_INTERRUPT_CURSOR) {
>          DbgPrint(TRACE_LEVEL_INFORMATION, ("---> %s m_CursorEvent\n",
>          __FUNCTION__));
>          KeSetEvent (&m_CursorEvent, IO_NO_INCREMENT, FALSE);
>      }
> -    if (ctx.data & QXL_INTERRUPT_IO_CMD) {
> +    if (intStatus & QXL_INTERRUPT_IO_CMD) {
>          DbgPrint(TRACE_LEVEL_INFORMATION, ("---> %s m_IoCmdEvent\n",
>          __FUNCTION__));
>          KeSetEvent (&m_IoCmdEvent, IO_NO_INCREMENT, FALSE);
>      }
> -    m_RamHdr->int_mask = WIN_QXL_INT_MASK;
> -    WRITE_PORT_UCHAR((PUCHAR)(m_IoBase + QXL_IO_UPDATE_IRQ), 0);
>  
>      DbgPrint(TRACE_LEVEL_INFORMATION, ("<--- %s\n", __FUNCTION__));
>  }
> @@ -4859,24 +4847,6 @@ VOID QxlDevice::UpdateArea(CONST RECT* area, UINT32
> surface_id)
>  }
>  
>  QXL_NON_PAGED
> -BOOLEAN QxlDevice:: DpcCallbackEx(PVOID ptr)
> -{
> -    DbgPrint(TRACE_LEVEL_VERBOSE, ("<--> %s\n", __FUNCTION__));
> -    PDPC_CB_CONTEXT ctx = (PDPC_CB_CONTEXT) ptr;
> -    QxlDevice* pqxl = (QxlDevice*)ctx->ptr;
> -    pqxl->DpcCallback(ctx);
> -    return TRUE;
> -}
> -
> -QXL_NON_PAGED
> -VOID QxlDevice::DpcCallback(PDPC_CB_CONTEXT ctx)
> -{
> -    ctx->data = m_Pending;
> -    m_Pending = 0;
> -
> -}
> -
> -QXL_NON_PAGED
>  UINT BPPFromPixelFormat(D3DDDIFORMAT Format)
>  {
>      switch (Format)
> diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> index 4598718..53724e8 100755
> --- a/qxldod/QxlDod.h
> +++ b/qxldod/QxlDod.h
> @@ -549,8 +549,6 @@ private:
>      void PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
>                              UINT8 **end_ptr, UINT8 *src, int size,
>                              size_t alloc_size, uint32_t alignment);
> -    QXL_NON_PAGED BOOLEAN static DpcCallbackEx(PVOID);
> -    QXL_NON_PAGED void DpcCallback(PDPC_CB_CONTEXT);
>      void AsyncIo(UCHAR  Port, UCHAR Value);
>      void SyncIo(UCHAR  Port, UCHAR Value);
>      NTSTATUS UpdateChildStatus(BOOLEAN connect);
> @@ -602,7 +600,7 @@ private:
>      MspaceInfo m_MSInfo[NUM_MSPACES];
>  
>      UINT64 m_FreeOutputs;
> -    UINT32 m_Pending;
> +    LONG m_Pending;
>  
>      QXLMonitorsConfig* m_monitor_config;
>      QXLPHYSICAL* m_monitor_config_pa;

Otherwise

Acked-by: Frediano Ziglio <fziglio@redhat.com>

Frediano