[Spice-devel] [PATCH qxl-wddm-dod v5 4/7] Fixing framebuffer usage logic
Sameeh Jubran
sameeh at daynix.com
Wed Sep 28 07:07:47 UTC 2016
On Mon, Sep 26, 2016 at 7:15 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> >
> > This patch fixes 2 issues:
> >
> > 1. Framebuffer should only be used in vga mode,
> > therefore when QxlDevice is active
> > FrameBufferIsActive flag shouldn't be checked;
> > 2. FrameBufferIsActive flag should be set true
> > on successfull frame buffer allocation only.
> >
> > Signed-off-by: Dmitry Fleytman <dmitry at daynix.com>
> > Signed-off-by: Sameeh Jubran <sameeh at daynix.com>
> > ---
> > qxldod/QxlDod.cpp | 116
> > +++++++++++++++++++++++++++---------------------------
> > 1 file changed, 58 insertions(+), 58 deletions(-)
> >
> > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> > index 85d6d94..ca3f8a3 100755
> > --- a/qxldod/QxlDod.cpp
> > +++ b/qxldod/QxlDod.cpp
> > @@ -523,40 +523,37 @@ NTSTATUS QxlDod::PresentDisplayOnly(_In_ CONST
> > DXGKARG_PRESENT_DISPLAYONLY* pPre
> > return STATUS_SUCCESS;
> > }
> >
> > - if
> > (m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].Flags.
> FrameBufferIsActive)
> > - {
> > -
> > - // If actual pixels are coming through, will need to completely
> zero
> > out physical address next time in BlackOutScreen
> > -
> > m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].ZeroedOutStart.
> QuadPart
> > = 0;
> > -
> > m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].ZeroedOutEnd.QuadPart
> > = 0;
> >
> > -
> > - D3DKMDT_VIDPN_PRESENT_PATH_ROTATION RotationNeededByFb =
> > pPresentDisplayOnly->Flags.Rotate ?
> > -
> > m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].Rotation
> > :
> > -
> > D3DKMDT_VPPR_IDENTITY;
> > - BYTE* pDst =
> > (BYTE*)m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].FrameBuffer.
> Ptr;
> > - UINT DstBitPerPixel =
> > BPPFromPixelFormat(m_CurrentModes[pPresentDisplayOnly->
> VidPnSourceId].DispInfo.ColorFormat);
> > - if (m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].Scaling
> ==
> > D3DKMDT_VPPS_CENTERED)
> > - {
> > - UINT CenterShift =
> > (m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].DispInfo.Height -
> > -
> > m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].SrcModeHeight)*
> m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].DispInfo.Pitch;
> > - CenterShift +=
> > (m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].DispInfo.Width -
> > -
> > m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].SrcModeWidth)*
> DstBitPerPixel/8;
> > - pDst += (int)CenterShift/2;
> > - }
> > - Status = m_pHWDevice->ExecutePresentDisplayOnly(
> > - pDst,
> > - DstBitPerPixel,
> > - (BYTE*)pPresentDisplayOnly->pSource,
> > - pPresentDisplayOnly->BytesPerPixel,
> > - pPresentDisplayOnly->Pitch,
> > - pPresentDisplayOnly->NumMoves,
> > - pPresentDisplayOnly->pMoves,
> > - pPresentDisplayOnly->NumDirtyRects,
> > - pPresentDisplayOnly->pDirtyRect,
> > - RotationNeededByFb,
> > - &m_CurrentModes[0]);
> > - }
> > + // If actual pixels are coming through, will need to completely
> zero out
> > physical address next time in BlackOutScreen
> > +
> > m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].ZeroedOutStart.
> QuadPart
> > = 0;
> > + m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].ZeroedOutEnd.
> QuadPart
> > = 0;
> > +
> > +
> > + D3DKMDT_VIDPN_PRESENT_PATH_ROTATION RotationNeededByFb =
> > pPresentDisplayOnly->Flags.Rotate ?
> > +
> > m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].Rotation
> > :
> > +
> > D3DKMDT_VPPR_IDENTITY;
> > + BYTE* pDst =
> > (BYTE*)m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].FrameBuffer.
> Ptr;
> > + UINT DstBitPerPixel =
> > BPPFromPixelFormat(m_CurrentModes[pPresentDisplayOnly->
> VidPnSourceId].DispInfo.ColorFormat);
> > + if (m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].Scaling ==
> > D3DKMDT_VPPS_CENTERED)
> > + {
> > + UINT CenterShift =
> > (m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].DispInfo.Height -
> > +
> > m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].SrcModeHeight)*
> m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].DispInfo.Pitch;
> > + CenterShift +=
> > (m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].DispInfo.Width -
> > +
> > m_CurrentModes[pPresentDisplayOnly->VidPnSourceId].SrcModeWidth)*
> DstBitPerPixel/8;
> > + pDst += (int)CenterShift/2;
> > + }
> > + Status = m_pHWDevice->ExecutePresentDisplayOnly(
> > + pDst,
> > + DstBitPerPixel,
> > + (BYTE*)pPresentDisplayOnly->pSource,
> > + pPresentDisplayOnly->BytesPerPixel,
> > + pPresentDisplayOnly->Pitch,
> > + pPresentDisplayOnly->NumMoves,
> > + pPresentDisplayOnly->pMoves,
> > + pPresentDisplayOnly->NumDirtyRects,
> > + pPresentDisplayOnly->pDirtyRect,
> > + RotationNeededByFb,
> > + &m_CurrentModes[0]);
> > DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
> > return Status;
> > }
> > @@ -1542,12 +1539,11 @@ NTSTATUS QxlDod::SetSourceModeAndPath(CONST
> > D3DKMDT_VIDPN_SOURCE_MODE* pSourceMo
> > pCurrentBddMode->DispInfo.Height =
> > pSourceMode->Format.Graphics.PrimSurfSize.cy;
> > pCurrentBddMode->DispInfo.Pitch =
> > pSourceMode->Format.Graphics.PrimSurfSize.cx *
> > BPPFromPixelFormat(pCurrentBddMode->DispInfo.ColorFormat) /
> > BITS_PER_BYTE;
> >
> > - Status = m_pHWDevice->AcquireFrameBuffer(pCurrentBddMode);
> > + Status = m_pHWDevice->AcquireFrameBuffer(pCurrentBddMode);
> >
>
> Here I think it's just a small mistake, no reason to indent more.
>
> > if (NT_SUCCESS(Status))
> > {
> >
> > - pCurrentBddMode->Flags.FrameBufferIsActive = TRUE;
>
> I would remove the empty line above
>
> > m_pHWDevice->BlackOutScreen(&m_CurrentModes[pPath->
> VidPnSourceId]);
> >
> > // Mark that the next present should be fullscreen so the screen
> > doesn't go from black to actual pixels one dirty rect at a time.
> > @@ -2979,11 +2975,18 @@ VOID VgaDevice::ResetDevice(VOID)
> >
> > NTSTATUS VgaDevice::AcquireFrameBuffer(CURRENT_BDD_MODE*
> pCurrentBddMode)
> > {
> > - // Map the new frame buffer
> > - QXL_ASSERT(pCurrentBddMode->FrameBuffer.Ptr == NULL);
> > - NTSTATUS status =
> > MapFrameBuffer(pCurrentBddMode->DispInfo.PhysicAddress,
> > - pCurrentBddMode->DispInfo.Pitch * pCurrentBddMode->DispInfo.
> Height,
> > - &(pCurrentBddMode->FrameBuffer.Ptr));
> > + NTSTATUS status = STATUS_UNSUCCESSFUL;
> > + if (!pCurrentBddMode->Flags.DoNotMapOrUnmap) {
> > + // Map the new frame buffer
> > + QXL_ASSERT(pCurrentBddMode->FrameBuffer.Ptr == NULL);
> > + status = MapFrameBuffer(pCurrentBddMode->DispInfo.
> PhysicAddress,
> > + pCurrentBddMode->DispInfo.Pitch *
> > pCurrentBddMode->DispInfo.Height,
> > + &(pCurrentBddMode->FrameBuffer.Ptr));
> > + if (NT_SUCCESS(status))
> > + {
> > + pCurrentBddMode->Flags.FrameBufferIsActive = TRUE;
> > + }
> > + }
> > return status;
> > }
> >
>
> Here I would just add a
>
> if (!pCurrentBddMode->Flags.DoNotMapOrUnmap) {
> return STATUS_UNSUCCESSFUL;
> }
>
> just after the function starts.
>
> > @@ -4501,26 +4504,23 @@ VOID QxlDevice::BlackOutScreen(CURRENT_BDD_MODE*
> > pCurrentBddMod)
> > QXLDrawable *drawable;
> > RECT Rect;
> > PAGED_CODE();
> > - if (pCurrentBddMod->Flags.FrameBufferIsActive)
> > + Rect.bottom = pCurrentBddMod->SrcModeHeight;
> > + Rect.top = 0;
> > + Rect.left = 0;
> > + Rect.right = pCurrentBddMod->SrcModeWidth;
> > + if (!(drawable = Drawable(QXL_DRAW_FILL, &Rect, NULL, 0)))
> > {
> > - Rect.bottom = pCurrentBddMod->SrcModeHeight;
> > - Rect.top = 0;
> > - Rect.left = 0;
> > - Rect.right = pCurrentBddMod->SrcModeWidth;
> > - if (!(drawable = Drawable(QXL_DRAW_FILL, &Rect, NULL, 0)))
> > - {
> > - DbgPrint(TRACE_LEVEL_ERROR, ("Cannot get Drawable.\n"));
> > - return;
> > - }
> > - drawable->u.fill.brush.type = SPICE_BRUSH_TYPE_SOLID;
> > - drawable->u.fill.brush.u.color = 0;
> > - drawable->u.fill.rop_descriptor = SPICE_ROPD_OP_PUT;
> > - drawable->u.fill.mask.flags = 0;
> > - drawable->u.fill.mask.pos.x = 0;
> > - drawable->u.fill.mask.pos.y = 0;
> > - drawable->u.fill.mask.bitmap = 0;
> > - PushDrawable(drawable);
> > + DbgPrint(TRACE_LEVEL_ERROR, ("Cannot get Drawable.\n"));
> > + return;
> > }
> > + drawable->u.fill.brush.type = SPICE_BRUSH_TYPE_SOLID;
> > + drawable->u.fill.brush.u.color = 0;
> > + drawable->u.fill.rop_descriptor = SPICE_ROPD_OP_PUT;
> > + drawable->u.fill.mask.flags = 0;
> > + drawable->u.fill.mask.pos.x = 0;
> > + drawable->u.fill.mask.pos.y = 0;
> > + drawable->u.fill.mask.bitmap = 0;
> > + PushDrawable(drawable);
> > }
> >
> > __declspec(code_seg("PAGE"))
>
> All changes I commented are mainly spaces and small style, if you
> agree I can change them, if not reply.
>
Yes, go ahead.
>
> Acked-by: Frediano Ziglio <fziglio at redhat.com>
>
> 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/3555878f/attachment-0001.html>
More information about the Spice-devel
mailing list