Re: [PATCH 02/12] qxl-wddm-dod: Use rendering offload thread

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

 




On 20 Mar 2017, at 17:37, Christophe de Dinechin <cdupontd@xxxxxxxxxx> wrote:

Side note: the .gitattribute in that project seems all wrong. It makes source files a pain to edit under Linux or macOS. Why not mark the source files as text? 


On 20 Mar 2017, at 13:08, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:


Instead of sending drawable commands down from presentation
callback, collect drawables objects and pass them to dedicated
thread for processing. This reduce peak load of presentation
callback.

Signed-off-by: Javier Celaya <javier.celaya@xxxxxxxxxxx>
Signed-off-by: Yuri Benditovich <yuri.benditovich@xxxxxxxxxx>
---
qxldod/QxlDod.cpp | 43 +++++++++++++++++++++++++++++++++----------
qxldod/QxlDod.h   |  4 ++--
2 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
index b952bf9..01de9b3 100755
--- a/qxldod/QxlDod.cpp
+++ b/qxldod/QxlDod.cpp
@@ -3794,11 +3794,20 @@ QxlDevice::ExecutePresentDisplayOnly(
    SIZE_T sizeRects = NumDirtyRects*sizeof(RECT);
    SIZE_T size = sizeof(DoPresentMemory) + sizeMoves + sizeRects;

+    QXLDrawable **pDrawables = reinterpret_cast<QXLDrawable **>(new
(NonPagedPoolNx) BYTE[sizeof(QXLDrawable *)*(NumDirtyRects + NumMoves +
1)]);

here would be

 QXLDrawable **pDrawables = new (NonPagedPoolNx) QXLDrawable *[NumDirtyRects + NumMoves + 1];

is non paged memory needed? Both functions (producer and consumer) are in paged areas.

+    UINT nIndex = 0;
+
+    if (!pDrawables)
+    {
+        return STATUS_NO_MEMORY;
+    }
+
    DoPresentMemory* ctx = reinterpret_cast<DoPresentMemory*>
                                (new (NonPagedPoolNx) BYTE[size]);


  DoPresentMemory* ctx = new (NonPagedPoolNx) DoPresentMemory;

That would be nicer, but apparently, there is extra stuff padded to it:

    SIZE_T size = sizeof(DoPresentMemory) + sizeMoves + sizeRects;


Also, this part of the code was not changed. It was like this before. But is it necessary ? It’s really borderline with respect to alignment. In one case, we have BYTE-aligned memory, in the other it’s at least sizeof(void *).

You could use placement new. Assuming non-paged pool:

BYTE *storage = new(NonPagedPoolNx) BYTE[size];
DoPresentMemory *ctx = new(storage) DoPresentMemory;

There’s no constructor, apparently. So it does not make much of a difference.


same comments as above

    if (!ctx)
    {
+        delete[] reinterpret_cast<BYTE*>(pDrawables);

delete[] pDrawables;

        return STATUS_NO_MEMORY;
    }

@@ -3828,6 +3837,8 @@ QxlDevice::ExecutePresentDisplayOnly(
        PMDL mdl = IoAllocateMdl((PVOID)SrcAddr, sizeToMap,  FALSE, FALSE,
        NULL);
        if(!mdl)
        {
+            delete[] reinterpret_cast<BYTE*>(ctx);

There were several leaks of ctx in case of failure before. I suggest a separate patch to fix that. There are many other places where the current patch does not fix them, for instance VgaDevice::GetModeList can leak m_ModeInfo if m_ModeNumbers can’t be allocated.


I’m probably wrong on this. They are freed in the destructor, and freed before you can overwrite them. But for local variables like ctx, there are leaks.

Christophe



+            delete[] reinterpret_cast<BYTE*>(pDrawables);

similar to above, in this case "delete ctx;”

There is a risk if, indeed, we store more than an object in it. delete[] and delete are implemented differently by some runtimes (I don’t recall about the Win driver runtime). It is not guaranteed that delete ctx would work reliably if we allocated more than sizeof(DoPresentMemory) bytes. Being able to deal with variable size is the very reason for delete[].



            return STATUS_INSUFFICIENT_RESOURCES;
        }

@@ -3844,6 +3855,8 @@ QxlDevice::ExecutePresentDisplayOnly(
        {
            Status = GetExceptionCode();
            IoFreeMdl(mdl);
+            delete[] reinterpret_cast<BYTE*>(ctx);
+            delete[] reinterpret_cast<BYTE*>(pDrawables);


ditto

            return Status;
        }

@@ -3857,6 +3870,8 @@ QxlDevice::ExecutePresentDisplayOnly(
            Status = STATUS_INSUFFICIENT_RESOURCES;
            MmUnlockPages(mdl);
            IoFreeMdl(mdl);
+            delete[] reinterpret_cast<BYTE*>(ctx);
+            delete[] reinterpret_cast<BYTE*>(pDrawables);

ditto



            return Status;
        }

@@ -3922,7 +3937,9 @@ QxlDevice::ExecutePresentDisplayOnly(
        DbgPrint(TRACE_LEVEL_INFORMATION, ("--- %d SourcePoint.x = %ld,
        SourcePoint.y = %ld, DestRect.bottom = %ld, DestRect.left = %ld,
        DestRect.right = %ld, DestRect.top = %ld\n",
            i , pSourcePoint->x, pSourcePoint->y, pDestRect->bottom,
            pDestRect->left, pDestRect->right, pDestRect->top));

-        CopyBits(*pDestRect, *pSourcePoint);
+        pDrawables[nIndex] = CopyBits(*pDestRect, *pSourcePoint);
+
+        if (pDrawables[nIndex]) nIndex++;
    }

    // Copy all the dirty rects from source image to video frame buffer.
@@ -3936,11 +3953,13 @@ QxlDevice::ExecutePresentDisplayOnly(
        DbgPrint(TRACE_LEVEL_INFORMATION, ("--- %d pDirtyRect->bottom = %ld,
        pDirtyRect->left = %ld, pDirtyRect->right = %ld, pDirtyRect->top =
        %ld\n",
            i, pDirtyRect->bottom, pDirtyRect->left, pDirtyRect->right,
            pDirtyRect->top));

-        BltBits(&DstBltInfo,
+        pDrawables[nIndex] = BltBits(&DstBltInfo,
        &SrcBltInfo,
        1,
        pDirtyRect,
        &sourcePoint);
+
+        if (pDrawables[nIndex]) nIndex++;
    }

    // Unmap unmap and unlock the pages.
@@ -3951,6 +3970,10 @@ QxlDevice::ExecutePresentDisplayOnly(
    }
    delete [] reinterpret_cast<BYTE*>(ctx);

+    pDrawables[nIndex] = NULL;
+
+    PostToWorkerThread(pDrawables);
+
    return STATUS_SUCCESS;
}

@@ -4364,7 +4387,7 @@ VOID QxlDevice::SetImageId(InternalImage *internal,
    }
}

-void QxlDevice::CopyBits(const RECT& rect, const POINT& sourcePoint)
+QXLDrawable *QxlDevice::CopyBits(const RECT& rect, const POINT& sourcePoint)
{

This CopyBits and BltBits are not doing anymore the operation, should
be renamed to something like PrepareCopyBits (better names welcome)

But we still need the driver entry points, right?


    PAGED_CODE();
    QXLDrawable *drawable;
@@ -4373,18 +4396,18 @@ void QxlDevice::CopyBits(const RECT& rect, const
POINT& sourcePoint)

    if (!(drawable = Drawable(QXL_COPY_BITS, &rect, NULL, 0))) {
        DbgPrint(TRACE_LEVEL_ERROR, ("Cannot get Drawable.\n"));
-        return;
+        return NULL;
    }

    drawable->u.copy_bits.src_pos.x = sourcePoint.x;
    drawable->u.copy_bits.src_pos.y = sourcePoint.y;

-    PushDrawable(drawable);
-
    DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
+
+    return drawable;
}

-VOID QxlDevice::BltBits (
+QXLDrawable *QxlDevice::BltBits (
    BLT_INFO* pDst,
    CONST BLT_INFO* pSrc,
    UINT  NumRects,
@@ -4407,7 +4430,7 @@ VOID QxlDevice::BltBits (

    if (!(drawable = Drawable(QXL_DRAW_COPY, pRects, NULL, 0))) {
        DbgPrint(TRACE_LEVEL_ERROR, ("Cannot get Drawable.\n"));
-        return;
+        return NULL;
    }

    CONST RECT* pRect = &pRects[0];
@@ -4480,9 +4503,9 @@ VOID QxlDevice::BltBits (
         drawable->surfaces_rects[0].top,
         drawable->surfaces_rects[0].bottom,
         drawable->u.copy.src_bitmap));

-    PushDrawable(drawable);
-
    DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
+
+    return drawable;
}

VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
index 4a62680..f441f4b 100755
--- a/qxldod/QxlDod.h
+++ b/qxldod/QxlDod.h
@@ -495,12 +495,12 @@ public:
    BOOLEAN IsBIOSCompatible() { return FALSE; }
protected:
    NTSTATUS GetModeList(DXGK_DISPLAY_INFORMATION* pDispInfo);
-    VOID BltBits (BLT_INFO* pDst,
+    QXLDrawable *BltBits (BLT_INFO* pDst,
                    CONST BLT_INFO* pSrc,
                    UINT  NumRects,
                    _In_reads_(NumRects) CONST RECT *pRects,
                    POINT*   pSourcePoint);
-    void CopyBits(const RECT& rect, const POINT& sourcePoint);
+    QXLDrawable *CopyBits(const RECT& rect, const POINT& sourcePoint);
    QXLDrawable *Drawable(UINT8 type,
                    CONST RECT *area,
                    CONST RECT *clip,

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

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

_______________________________________________
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]