On Mon, Sep 26, 2016 at 6:20 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
I saw it now. I was suggesting to remove the DirtyRects/Moves check for NULL>
> Part of source image mapped by PresentDisplayOnly
> should be big enough to cover all rectangles being
> transferred.
>
> Signed-off-by: Sameeh Jubran <sameeh@xxxxxxxxxx>
> Signed-off-by: Dmitry Fleytman <dmitry@xxxxxxxxxx>
> ---
> 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;
> +}
> +
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.
Better safe than sorry :)
I agree with "const RECT&"
With this patch we optimize the mapping size shrinking the bottom part.
Would be worth to shrink even to top part (although more complicated) ?
This patch "expands" and doesn't shrink the mapping size as it calculates the max size
that should be mapped, this is the actual size that should be mapped in memory. This could
prevent rare BSODs when the mapped memory isn't enough for all of the rectangles.
> __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 I would use "static LONG", usually it's the order is used. And it's one
> NumMoves, RECT* DirtyRects, ULONG NumDirtyRects);
> +
> private:
> PUCHAR m_IoBase;
> BOOLEAN m_IoMapped;
C++ gotcha less (cfr "Creative Declaration-Specifier Ordering").
Thanks for noting that!
Frediano
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel