[Spice-devel,qxl-wddm-dod] Implements screen to screen move correctly

Submitted by Frediano Ziglio on Nov. 28, 2016, 11:03 a.m.

Details

Message ID 20161128110311.3365-1-fziglio@redhat.com
State New
Headers show
Series "Implements screen to screen move correctly" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Nov. 28, 2016, 11:03 a.m.
Doing some fast check on Windows 8.1 you could note that
moving windows you got a weird effect were windows were a
bit misaligned.
As documented in DXGKARG_PRESENT_DISPLAYONLY page
NumMoves/pMoves fields point to an array of screen-to-screen
moves while our code implemented them as an image drawing
(the same implementation of dirty rects) causing the weird
effect mentioned.
This patch implement the moves using QXL_COPY_BITS operation
instead of a QXL_DRAW_COPY fixing the issue and avoiding sending
image to the server making the move/scroll operations on the
guest faster (and taking less bandwidth).
It seems that Windows 10 doesn't send the move commands but
instead send only dirty rects so you can't note this problem
using Windows 10.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 qxldod/QxlDod.cpp | 26 +++++++++++++++++++++-----
 qxldod/QxlDod.h   |  1 +
 2 files changed, 22 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
index 68e3383..7adbec4 100755
--- a/qxldod/QxlDod.cpp
+++ b/qxldod/QxlDod.cpp
@@ -3738,11 +3738,7 @@  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));
 
-        BltBits(&DstBltInfo,
-        &SrcBltInfo,
-        1,
-        pDestRect,
-        pSourcePoint);
+        CopyBits(*pDestRect, *pSourcePoint);
     }
 
     // Copy all the dirty rects from source image to video frame buffer.
@@ -4184,6 +4180,26 @@  VOID QxlDevice::SetImageId(InternalImage *internal,
     }
 }
 
+void QxlDevice::CopyBits(const RECT& rect, const POINT& sourcePoint)
+{
+    PAGED_CODE();
+    QXLDrawable *drawable;
+
+    DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s device %d\n", __FUNCTION__,m_Id));
+
+    if (!(drawable = Drawable(QXL_COPY_BITS, &rect, NULL, 0))) {
+        DbgPrint(TRACE_LEVEL_ERROR, ("Cannot get Drawable.\n"));
+        return;
+    }
+
+    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__));
+}
+
 VOID QxlDevice::BltBits (
     BLT_INFO* pDst,
     CONST BLT_INFO* pSrc,
diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
index b151484..324c3d6 100755
--- a/qxldod/QxlDod.h
+++ b/qxldod/QxlDod.h
@@ -487,6 +487,7 @@  protected:
                     UINT  NumRects,
                     _In_reads_(NumRects) CONST RECT *pRects,
                     POINT*   pSourcePoint);
+    void QxlDevice::CopyBits(const RECT& rect, const POINT& sourcePoint);
     QXLDrawable *Drawable(UINT8 type,
                     CONST RECT *area,
                     CONST RECT *clip,

Comments

ping

Maybe this requires some follow ups for VgaDevice and
possibly avoinind taking into account moves during mapping
computation.

Frediano

> 
> Doing some fast check on Windows 8.1 you could note that
> moving windows you got a weird effect were windows were a
> bit misaligned.
> As documented in DXGKARG_PRESENT_DISPLAYONLY page
> NumMoves/pMoves fields point to an array of screen-to-screen
> moves while our code implemented them as an image drawing
> (the same implementation of dirty rects) causing the weird
> effect mentioned.
> This patch implement the moves using QXL_COPY_BITS operation
> instead of a QXL_DRAW_COPY fixing the issue and avoiding sending
> image to the server making the move/scroll operations on the
> guest faster (and taking less bandwidth).
> It seems that Windows 10 doesn't send the move commands but
> instead send only dirty rects so you can't note this problem
> using Windows 10.
> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  qxldod/QxlDod.cpp | 26 +++++++++++++++++++++-----
>  qxldod/QxlDod.h   |  1 +
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> index 68e3383..7adbec4 100755
> --- a/qxldod/QxlDod.cpp
> +++ b/qxldod/QxlDod.cpp
> @@ -3738,11 +3738,7 @@ 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));
>  
> -        BltBits(&DstBltInfo,
> -        &SrcBltInfo,
> -        1,
> -        pDestRect,
> -        pSourcePoint);
> +        CopyBits(*pDestRect, *pSourcePoint);
>      }
>  
>      // Copy all the dirty rects from source image to video frame buffer.
> @@ -4184,6 +4180,26 @@ VOID QxlDevice::SetImageId(InternalImage *internal,
>      }
>  }
>  
> +void QxlDevice::CopyBits(const RECT& rect, const POINT& sourcePoint)
> +{
> +    PAGED_CODE();
> +    QXLDrawable *drawable;
> +
> +    DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s device %d\n",
> __FUNCTION__,m_Id));
> +
> +    if (!(drawable = Drawable(QXL_COPY_BITS, &rect, NULL, 0))) {
> +        DbgPrint(TRACE_LEVEL_ERROR, ("Cannot get Drawable.\n"));
> +        return;
> +    }
> +
> +    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__));
> +}
> +
>  VOID QxlDevice::BltBits (
>      BLT_INFO* pDst,
>      CONST BLT_INFO* pSrc,
> diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> index b151484..324c3d6 100755
> --- a/qxldod/QxlDod.h
> +++ b/qxldod/QxlDod.h
> @@ -487,6 +487,7 @@ protected:
>                      UINT  NumRects,
>                      _In_reads_(NumRects) CONST RECT *pRects,
>                      POINT*   pSourcePoint);
> +    void QxlDevice::CopyBits(const RECT& rect, const POINT& sourcePoint);
>      QXLDrawable *Drawable(UINT8 type,
>                      CONST RECT *area,
>                      CONST RECT *clip,
On Mon, Nov 28, 2016 at 1:03 PM, Frediano Ziglio <fziglio@redhat.com> wrote:

> Doing some fast check on Windows 8.1 you could note that
> moving windows you got a weird effect were windows were a
> bit misaligned.
> As documented in DXGKARG_PRESENT_DISPLAYONLY page
> NumMoves/pMoves fields point to an array of screen-to-screen
> moves while our code implemented them as an image drawing
> (the same implementation of dirty rects) causing the weird
> effect mentioned.
> This patch implement the moves using QXL_COPY_BITS operation
> instead of a QXL_DRAW_COPY fixing the issue and avoiding sending
> image to the server making the move/scroll operations on the
> guest faster (and taking less bandwidth).
> It seems that Windows 10 doesn't send the move commands but
> instead send only dirty rects so you can't note this problem
> using Windows 10.
>
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  qxldod/QxlDod.cpp | 26 +++++++++++++++++++++-----
>  qxldod/QxlDod.h   |  1 +
>  2 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> index 68e3383..7adbec4 100755
> --- a/qxldod/QxlDod.cpp
> +++ b/qxldod/QxlDod.cpp
> @@ -3738,11 +3738,7 @@ 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));
>
> -        BltBits(&DstBltInfo,
> -        &SrcBltInfo,
> -        1,
> -        pDestRect,
> -        pSourcePoint);
> +        CopyBits(*pDestRect, *pSourcePoint);
>      }
>
>      // Copy all the dirty rects from source image to video frame buffer.
> @@ -4184,6 +4180,26 @@ VOID QxlDevice::SetImageId(InternalImage *internal,
>      }
>  }
>
> +void QxlDevice::CopyBits(const RECT& rect, const POINT& sourcePoint)
> +{
> +    PAGED_CODE();
> +    QXLDrawable *drawable;
> +
> +    DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s device %d\n",
> __FUNCTION__,m_Id));
> +
> +    if (!(drawable = Drawable(QXL_COPY_BITS, &rect, NULL, 0))) {
> +        DbgPrint(TRACE_LEVEL_ERROR, ("Cannot get Drawable.\n"));
> +        return;
> +    }
> +
> +    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__));
> +}
> +
>  VOID QxlDevice::BltBits (
>      BLT_INFO* pDst,
>      CONST BLT_INFO* pSrc,
> diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> index b151484..324c3d6 100755
> --- a/qxldod/QxlDod.h
> +++ b/qxldod/QxlDod.h
> @@ -487,6 +487,7 @@ protected:
>                      UINT  NumRects,
>                      _In_reads_(NumRects) CONST RECT *pRects,
>                      POINT*   pSourcePoint);
> +    void QxlDevice::CopyBits(const RECT& rect, const POINT& sourcePoint);
>

I would exclude class name in the declaration


>      QXLDrawable *Drawable(UINT8 type,
>                      CONST RECT *area,
>                      CONST RECT *clip,
> --
> 2.9.3
>
>
Acked-by: Yuri Benditovich <yuri.benditovich@daynix.com>


> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
On Mon, Dec 5, 2016 at 2:51 PM, Frediano Ziglio <fziglio@redhat.com> wrote:

> ping
>
> Maybe this requires some follow ups for VgaDevice and
> possibly avoinind taking into account moves during mapping
> computation.
>
>
Indeed, it seems GetMaxSourceMappingHeight(NULL, 0,...) will be also
correct.
From other side, I think mapping of entire source buffer to kernel space is
redundant,
as we never touch the source memory in context different than OS call.
It looks like the copy operation can use original source address and be
just protected by exception handler.
(the prototype - MSFT BDD example executes the copy in different thread, so
the mapping is required).

Regarding Vga device - I do not see why copying from device memory is
better than copying from source memory.
Does VgaDevice have the same problem of misalign?

Thanks,
Yuri


> Frediano
>
> >
> > Doing some fast check on Windows 8.1 you could note that
> > moving windows you got a weird effect were windows were a
> > bit misaligned.
> > As documented in DXGKARG_PRESENT_DISPLAYONLY page
> > NumMoves/pMoves fields point to an array of screen-to-screen
> > moves while our code implemented them as an image drawing
> > (the same implementation of dirty rects) causing the weird
> > effect mentioned.
> > This patch implement the moves using QXL_COPY_BITS operation
> > instead of a QXL_DRAW_COPY fixing the issue and avoiding sending
> > image to the server making the move/scroll operations on the
> > guest faster (and taking less bandwidth).
> > It seems that Windows 10 doesn't send the move commands but
> > instead send only dirty rects so you can't note this problem
> > using Windows 10.
> >
> > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > ---
> >  qxldod/QxlDod.cpp | 26 +++++++++++++++++++++-----
> >  qxldod/QxlDod.h   |  1 +
> >  2 files changed, 22 insertions(+), 5 deletions(-)
> >
> > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> > index 68e3383..7adbec4 100755
> > --- a/qxldod/QxlDod.cpp
> > +++ b/qxldod/QxlDod.cpp
> > @@ -3738,11 +3738,7 @@ 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));
> >
> > -        BltBits(&DstBltInfo,
> > -        &SrcBltInfo,
> > -        1,
> > -        pDestRect,
> > -        pSourcePoint);
> > +        CopyBits(*pDestRect, *pSourcePoint);
> >      }
> >
> >      // Copy all the dirty rects from source image to video frame buffer.
> > @@ -4184,6 +4180,26 @@ VOID QxlDevice::SetImageId(InternalImage
> *internal,
> >      }
> >  }
> >
> > +void QxlDevice::CopyBits(const RECT& rect, const POINT& sourcePoint)
> > +{
> > +    PAGED_CODE();
> > +    QXLDrawable *drawable;
> > +
> > +    DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s device %d\n",
> > __FUNCTION__,m_Id));
> > +
> > +    if (!(drawable = Drawable(QXL_COPY_BITS, &rect, NULL, 0))) {
> > +        DbgPrint(TRACE_LEVEL_ERROR, ("Cannot get Drawable.\n"));
> > +        return;
> > +    }
> > +
> > +    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__));
> > +}
> > +
> >  VOID QxlDevice::BltBits (
> >      BLT_INFO* pDst,
> >      CONST BLT_INFO* pSrc,
> > diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> > index b151484..324c3d6 100755
> > --- a/qxldod/QxlDod.h
> > +++ b/qxldod/QxlDod.h
> > @@ -487,6 +487,7 @@ protected:
> >                      UINT  NumRects,
> >                      _In_reads_(NumRects) CONST RECT *pRects,
> >                      POINT*   pSourcePoint);
> > +    void QxlDevice::CopyBits(const RECT& rect, const POINT&
> sourcePoint);
> >      QXLDrawable *Drawable(UINT8 type,
> >                      CONST RECT *area,
> >                      CONST RECT *clip,
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>