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

[Frediano Ziglio]

> 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?

To avoid cr/lf conversions. 
If you can find good flags to make git Windows and git Unix to work you are welcome. 
Got mad to try, you fix a tool and you break another. 
I have no problems with Linux, my editor just handle DOS files as DOS. 

> > On 20 Mar 2017, at 13:08, Frediano Ziglio < fziglio at redhat.com > 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 at flexvdi.com >
> > 
> 
> > > Signed-off-by: Yuri Benditovich < yuri.benditovich at daynix.com >
> > 
> 
> > > ---
> > 
> 
> > > 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.

Looking at the code it looks quite weird, a ctx is allocated in the function, then at the end freed, 
I found these comments: 

// Alternate between synch and asynch execution, for demonstrating 
// that a real hardware implementation can do either 
... 

// Save Mdl to unmap and unlock the pages in worker thread 
... 

// copy moves and update pointer 
... 

// copy dirty rects and update pointer 

apparently this is all due to the initial code (from a Microsoft example) that was marshalling this 
call to a worker thread. 
However now that we are going to introduce a worker thread this looks misleading. 
I would remove comments and code. For instance there's no need to copy Moves & DirtyRect 
and DoPresentMemory can be allocated just in the stack (or even better removed). 

> > 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.

Can you write some patches about these? These specifically seem not really related. 

> > > + 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?

These are method, not really related to entry points. 

> > > 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
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170320/f0caa727/attachment-0001.html>