[Spice-devel] [PATCH qxl-wddm-dod v5 5/7] Fix source buffer mapping in PresentDisplayOnly

[Frediano Ziglio]

> 
> Part of source image mapped by PresentDisplayOnly
> should be big enough to cover all rectangles being
> transferred.
> 
> Signed-off-by: Sameeh Jubran <sameeh at daynix.com>
> Signed-off-by: Dmitry Fleytman <dmitry at daynix.com>
> ---
>  qxldod/QxlDod.cpp | 28 +++++++++++++++++++++++-----
>  qxldod/QxlDod.h   |  2 ++
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> index ca3f8a3..b6c2494 100755
> --- a/qxldod/QxlDod.cpp
> +++ b/qxldod/QxlDod.cpp
> @@ -3783,12 +3783,11 @@ QxlDevice::ExecutePresentDisplayOnly(
>      ctx->Mdl              = NULL;
>      ctx->DisplaySource    = this;
>  
> -    // Alternate between synch and asynch execution, for demonstrating
> -    // that a real hardware implementation can do either
> -
> +    // Source bitmap is in user mode, must be locked under __try/__except
> +    // and mapped to kernel space before use.
>      {
> -        // Map Source into kernel space, as Blt will be executed by system
> worker thread
> -        UINT sizeToMap = ctx->SrcPitch * ctx->SrcHeight;
> +        LONG maxHeight = GetMaxSourceMappingHeight(ctx->Moves,
> ctx->NumMoves, ctx->DirtyRect, ctx->NumDirtyRects);
> +        UINT sizeToMap = ctx->SrcPitch * maxHeight;
>  
>          PMDL mdl = IoAllocateMdl((PVOID)SrcAddr, sizeToMap,  FALSE, FALSE,
>          NULL);
>          if(!mdl)
> @@ -4699,6 +4698,25 @@ void QxlDevice::SetMonitorConfig(QXLHead *
> monitor_config)
>      AsyncIo(QXL_IO_MONITORS_CONFIG_ASYNC, 0);
>  }
>  
> +LONG QxlDevice::GetMaxSourceMappingHeight(D3DKMT_MOVE_RECT* Moves, ULONG
> NumMoves, RECT* DirtyRects, ULONG NumDirtyRects)
> +{
> +    LONG maxHeight = 0;
> +    if (Moves != NULL) {
> +        for (UINT i = 0; i < NumMoves; i++) {
> +            POINT*   pSourcePoint = &Moves[i].SourcePoint;
> +            RECT*    pDestRect = &Moves[i].DestRect;
> +            maxHeight = MAX(maxHeight, pDestRect->bottom - pDestRect->top +
> pSourcePoint->y);
> +        }
> +    }
> +    if (DirtyRects != NULL) {
> +        for (UINT i = 0; i < NumDirtyRects; i++) {
> +            RECT*    pDirtyRect = &DirtyRects[i];
> +            maxHeight = MAX(maxHeight, pDirtyRect->bottom);
> +        }
> +    }
> +    return maxHeight;
> +}
> +

I saw it now. I was suggesting to remove the DirtyRects/Moves check for NULL
as NumDirtyRects/NumMoves check should be enough.
It's not clear however from https://msdn.microsoft.com/en-us/library/windows/hardware/hh451288(v=vs.85).aspx
if is expected you should ignore NumDirtyRects if DirtyRects is NULL.
It would be a weird specification having NumDirtyRects == 2 and DirtyRects == NULL.
I would also use "const RECT&" but it's just style.

With this patch we optimize the mapping size shrinking the bottom part.
Would be worth to shrink even to top part (although more complicated) ?

>  __declspec(code_seg("PAGE"))
>  NTSTATUS QxlDevice::Escape(_In_ CONST DXGKARG_ESCAPE* pEscape)
>  {
> diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> index 585b235..2f51328 100755
> --- a/qxldod/QxlDod.h
> +++ b/qxldod/QxlDod.h
> @@ -645,6 +645,8 @@ private:
>      __declspec(code_seg("PAGE"))
>      void SetMonitorConfig(QXLHead* monitor_config);
>  
> +    LONG static GetMaxSourceMappingHeight(D3DKMT_MOVE_RECT* Moves, ULONG
> NumMoves, RECT* DirtyRects, ULONG NumDirtyRects);
> +
>  private:
>      PUCHAR m_IoBase;
>      BOOLEAN m_IoMapped;

I would use "static LONG", usually it's the order is used. And it's one
C++ gotcha less (cfr "Creative Declaration-Specifier Ordering").

Frediano