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

Submitted by Yuri Benditovich on March 12, 2017, 8:44 a.m.

Details

Message ID 1489308309-9728-3-git-send-email-yuri.benditovich@daynix.com
State New
Headers show
Series "Set of patches for further support of VSync" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Yuri Benditovich March 12, 2017, 8:44 a.m.
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@flexvdi.com>
Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
---
 qxldod/QxlDod.cpp | 43 +++++++++++++++++++++++++++++++++----------
 qxldod/QxlDod.h   |  4 ++--
 2 files changed, 35 insertions(+), 12 deletions(-)

Patch hide | download patch | download mbox

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)]);
+    UINT nIndex = 0;
+
+    if (!pDrawables)
+    {
+        return STATUS_NO_MEMORY;
+    }
+
     DoPresentMemory* ctx = reinterpret_cast<DoPresentMemory*>
                                 (new (NonPagedPoolNx) BYTE[size]);
 
     if (!ctx)
     {
+        delete[] reinterpret_cast<BYTE*>(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);
+            delete[] reinterpret_cast<BYTE*>(pDrawables);
             return STATUS_INSUFFICIENT_RESOURCES;
         }
 
@@ -3844,6 +3855,8 @@  QxlDevice::ExecutePresentDisplayOnly(
         {
             Status = GetExceptionCode();
             IoFreeMdl(mdl);
+            delete[] reinterpret_cast<BYTE*>(ctx);
+            delete[] reinterpret_cast<BYTE*>(pDrawables);
             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);
             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)
 {
     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,

Comments

> 
> 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@flexvdi.com>
> Signed-off-by: Yuri Benditovich <yuri.benditovich@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;

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);
> +            delete[] reinterpret_cast<BYTE*>(pDrawables);

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

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

>      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
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@redhat.com <mailto:fziglio@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@flexvdi.com <mailto:javier.celaya@flexvdi.com>>
>> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com <mailto:yuri.benditovich@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.

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


>> +            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@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>
> 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@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@flexvdi.com >
> > 
> 
> > > Signed-off-by: Yuri Benditovich < yuri.benditovich@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
>
> On 20 Mar 2017, at 19:03, Frediano Ziglio <fziglio@redhat.com <mailto:fziglio@redhat.com>> 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? 
> To avoid cr/lf conversions.

But we want CR/LF conversions for text files. That’s the whole point.

> 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 did that recently on another project. text=auto does work. I guess there was some other problem at the time. You mention multiple tools. Do you remember which one broke?

> I have no problems with Linux, my editor just handle DOS files as DOS.

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.


Thanks,
Christophe
> 
> On 20 Mar 2017, at 13:08, Frediano Ziglio <fziglio@redhat.com <mailto:fziglio@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@flexvdi.com <mailto:javier.celaya@flexvdi.com>>
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com <mailto:yuri.benditovich@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>
> > On 20 Mar 2017, at 19:03, Frediano Ziglio < fziglio@redhat.com > 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?
> > 
> 

> > To avoid cr/lf conversions.
> 

> But we want CR/LF conversions for text files. That’s the whole point.

The problem is that some files were mostly DOS while others mostly UNIX. 
Instead of picking one style we pick the one (for each file) to avoid to have lot of lines changed but 
to be consistent in each file. 

> > 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 did that recently on another project. text=auto does work. I guess there
> was some other problem at the time. You mention multiple tools. Do you
> remember which one broke?

Even git checkout from both. 

> > I have no problems with Linux, my editor just handle DOS files as DOS.
> 

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

> Thanks,
> Christophe

Frediano 

> > > > On 20 Mar 2017, at 13:08, Frediano Ziglio < fziglio@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@flexvdi.com >
> > > > 
> > > 
> > 
> 
> > > > > Signed-off-by: Yuri Benditovich < yuri.benditovich@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
> > > 
> > 
>
> On 21 Mar 2017, at 11:51, Frediano Ziglio <fziglio@redhat.com <mailto:fziglio@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 <fziglio@redhat.com <mailto:fziglio@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@flexvdi.com <mailto:javier.celaya@flexvdi.com>>
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com <mailto:yuri.benditovich@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>
On Mon, Mar 20, 2017 at 2:08 PM, Frediano Ziglio <fziglio@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@flexvdi.com>
> > Signed-off-by: Yuri Benditovich <yuri.benditovich@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;
>

Current commit intentionally does not change this, to make clear what is
changed.
Without changes in the code we can't just allocate less memory, as trailing
RECT structures are used for
unneeded copy from parameters.
Removal of this completely unneeded 'ctx' is planned for further commits
after all the HCK-related work is done.


>
> 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);
> > +            delete[] reinterpret_cast<BYTE*>(pDrawables);
>
> similar to above, in this case "delete ctx;"
>
> >              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)
>

No problem, prefer to do it on later commits.


>
> >      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
>
> On 21 Mar 2017, at 13:59, Yuri Benditovich <yuri.benditovich@daynix.com> wrote:
> 
> 
> 
> On Mon, Mar 20, 2017 at 2:08 PM, Frediano Ziglio <fziglio@redhat.com <mailto:fziglio@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@flexvdi.com <mailto:javier.celaya@flexvdi.com>>
> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com <mailto:yuri.benditovich@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;
> 
> Current commit intentionally does not change this, to make clear what is changed.

Good.

If we use a non-paged placement new, shouldn’t we use a corresponding placement delete?


> Without changes in the code we can't just allocate less memory, as trailing RECT structures are used for
> unneeded copy from parameters.
> Removal of this completely unneeded 'ctx' is planned for further commits after all the HCK-related work is done.

>  
> 
> 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);
> > +            delete[] reinterpret_cast<BYTE*>(pDrawables);
> 
> similar to above, in this case "delete ctx;"
> 
> >              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)
> 
> No problem, prefer to do it on later commits.
>  
> 
> >      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
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> On 21 Mar 2017, at 14:52, Christophe de Dinechin <cdupontd@redhat.com> wrote:
> 
>> 
>> On 21 Mar 2017, at 13:59, Yuri Benditovich <yuri.benditovich@daynix.com <mailto:yuri.benditovich@daynix.com>> wrote:
>> 
>> 
>> 
>> On Mon, Mar 20, 2017 at 2:08 PM, Frediano Ziglio <fziglio@redhat.com <mailto:fziglio@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@flexvdi.com <mailto:javier.celaya@flexvdi.com>>
>> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com <mailto:yuri.benditovich@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;
>> 
>> Current commit intentionally does not change this, to make clear what is changed.
> 
> Good.
> 
> If we use a non-paged placement new, shouldn’t we use a corresponding placement delete?

Just checked by looking at the Microsoft sample code. They use plain delete. This still seems wrong to me. The underlying pool is not the same. It probably works because the pools share the same internal layout, so the same operator delete works in both cases.



> 
> 
>> Without changes in the code we can't just allocate less memory, as trailing RECT structures are used for
>> unneeded copy from parameters.
>> Removal of this completely unneeded 'ctx' is planned for further commits after all the HCK-related work is done.
> 
>>  
>> 
>> 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);
>> > +            delete[] reinterpret_cast<BYTE*>(pDrawables);
>> 
>> similar to above, in this case "delete ctx;"
>> 
>> >              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)
>> 
>> No problem, prefer to do it on later commits.
>>  
>> 
>> >      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 <mailto:Spice-devel@lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
> On 20 Mar 2017, at 17:37, Christophe de Dinechin <cdupontd@redhat.com <mailto:cdupontd@redhat.com>> 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@redhat.com <mailto:fziglio@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@flexvdi.com <mailto:javier.celaya@flexvdi.com>>
>>> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com <mailto:yuri.benditovich@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.
> 
>> 
>> 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@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 <mailto:Spice-devel@lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>