[Spice-devel] [PATCH qxl-wddm-dod v5 5/7] Fix source buffer mapping in PresentDisplayOnly

[Sameeh Jubran]

On Mon, Sep 26, 2016 at 6:20 PM, Frediano Ziglio <fziglio at redhat.com> wrote:

> >
> > Part of source image mapped by PresentDisplayOnly
> > should be big enough to cover all rectangles being
> > transferred.
> >
> > Signed-off-by: Sameeh Jubran <sameeh at daynix.com>
> > Signed-off-by: Dmitry Fleytman <dmitry at daynix.com>
> > ---
> >  qxldod/QxlDod.cpp | 28 +++++++++++++++++++++++-----
> >  qxldod/QxlDod.h   |  2 ++
> >  2 files changed, 25 insertions(+), 5 deletions(-)
> >
> > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> > index ca3f8a3..b6c2494 100755
> > --- a/qxldod/QxlDod.cpp
> > +++ b/qxldod/QxlDod.cpp
> > @@ -3783,12 +3783,11 @@ QxlDevice::ExecutePresentDisplayOnly(
> >      ctx->Mdl              = NULL;
> >      ctx->DisplaySource    = this;
> >
> > -    // Alternate between synch and asynch execution, for demonstrating
> > -    // that a real hardware implementation can do either
> > -
> > +    // Source bitmap is in user mode, must be locked under
> __try/__except
> > +    // and mapped to kernel space before use.
> >      {
> > -        // Map Source into kernel space, as Blt will be executed by
> system
> > worker thread
> > -        UINT sizeToMap = ctx->SrcPitch * ctx->SrcHeight;
> > +        LONG maxHeight = GetMaxSourceMappingHeight(ctx->Moves,
> > ctx->NumMoves, ctx->DirtyRect, ctx->NumDirtyRects);
> > +        UINT sizeToMap = ctx->SrcPitch * maxHeight;
> >
> >          PMDL mdl = IoAllocateMdl((PVOID)SrcAddr, sizeToMap,  FALSE,
> FALSE,
> >          NULL);
> >          if(!mdl)
> > @@ -4699,6 +4698,25 @@ void QxlDevice::SetMonitorConfig(QXLHead *
> > monitor_config)
> >      AsyncIo(QXL_IO_MONITORS_CONFIG_ASYNC, 0);
> >  }
> >
> > +LONG QxlDevice::GetMaxSourceMappingHeight(D3DKMT_MOVE_RECT* Moves,
> ULONG
> > NumMoves, RECT* DirtyRects, ULONG NumDirtyRects)
> > +{
> > +    LONG maxHeight = 0;
> > +    if (Moves != NULL) {
> > +        for (UINT i = 0; i < NumMoves; i++) {
> > +            POINT*   pSourcePoint = &Moves[i].SourcePoint;
> > +            RECT*    pDestRect = &Moves[i].DestRect;
> > +            maxHeight = MAX(maxHeight, pDestRect->bottom -
> pDestRect->top +
> > pSourcePoint->y);
> > +        }
> > +    }
> > +    if (DirtyRects != NULL) {
> > +        for (UINT i = 0; i < NumDirtyRects; i++) {
> > +            RECT*    pDirtyRect = &DirtyRects[i];
> > +            maxHeight = MAX(maxHeight, pDirtyRect->bottom);
> > +        }
> > +    }
> > +    return maxHeight;
> > +}
> > +
>
> I saw it now. I was suggesting to remove the DirtyRects/Moves check for
> NULL
> as NumDirtyRects/NumMoves check should be enough.
> It's not clear however from https://msdn.microsoft.com/en-
> us/library/windows/hardware/hh451288(v=vs.85).aspx
> if is expected you should ignore NumDirtyRects if DirtyRects is NULL.
> It would be a weird specification having NumDirtyRects == 2 and DirtyRects
> == NULL.
> I would also use "const RECT&" but it's just style.
>
Better safe than sorry :)
I agree with "const RECT&"

>
> With this patch we optimize the mapping size shrinking the bottom part.
> Would be worth to shrink even to top part (although more complicated) ?
>
This patch "expands" and doesn't shrink the mapping size as it calculates
the max size
that should be mapped, this is the actual size that should be mapped in
memory. This could
prevent rare BSODs when the mapped memory isn't enough for all of the
rectangles.

>
> >  __declspec(code_seg("PAGE"))
> >  NTSTATUS QxlDevice::Escape(_In_ CONST DXGKARG_ESCAPE* pEscape)
> >  {
> > diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> > index 585b235..2f51328 100755
> > --- a/qxldod/QxlDod.h
> > +++ b/qxldod/QxlDod.h
> > @@ -645,6 +645,8 @@ private:
> >      __declspec(code_seg("PAGE"))
> >      void SetMonitorConfig(QXLHead* monitor_config);
> >
> > +    LONG static GetMaxSourceMappingHeight(D3DKMT_MOVE_RECT* Moves,
> ULONG
> > NumMoves, RECT* DirtyRects, ULONG NumDirtyRects);
> > +
> >  private:
> >      PUCHAR m_IoBase;
> >      BOOLEAN m_IoMapped;
>
> I would use "static LONG", usually it's the order is used. And it's one
> C++ gotcha less (cfr "Creative Declaration-Specifier Ordering").
>
Thanks for noting that!

>
> Frediano
>



-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Junior Software Engineer @ Daynix <http://www.daynix.com>.*
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160928/8c73d56a/attachment-0001.html>