> On 21 Mar 2017, at 11:51, Frediano Ziglio <fzig...@redhat.com > <mailto:fzig...@redhat.com>> wrote: > So does Emacs, but it shows additional ^M within the file if CR/LF is > inconsistent throughout the file, as it the case here, presumably because the > .gitattributes was added in the middle of the life of the project without > first making the files consistent. > There's no mix in single files, I have a script to test this issue. > gitattributes was added after making each file consistent but all files are > not consistent (as stated above). > Probably you applied from the mail which tend to strip CRs at the end so you > got the inconsistency.
Not just at the end, it strips all CR. But you are right, only the patched file was inconsistent. Christophe > > Thanks, > Christophe > > Frediano > > On 20 Mar 2017, at 13:08, Frediano Ziglio <fzig...@redhat.com > <mailto:fzig...@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.cel...@flexvdi.com > <mailto:javier.cel...@flexvdi.com>> > Signed-off-by: Yuri Benditovich <yuri.benditov...@daynix.com > <mailto:yuri.benditov...@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 > > _______________________________________________ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org <mailto:Spice-devel@lists.freedesktop.org> > https://lists.freedesktop.org/mailman/listinfo/spice-devel > <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
_______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
