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

[Frediano Ziglio]

> 
> 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 at 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 at redhat.com>

Frediano