<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Mon, Sep 26, 2016 at 7:15 PM, Frediano Ziglio <<a href="mailto:fziglio@redhat.com" target="_blank">fziglio@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">> > 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 <<a href="mailto:dmitry@daynix.com">dmitry@daynix.com</a>> > Signed-off-by: Sameeh Jubran <<a href="mailto:sameeh@daynix.com">sameeh@daynix.com</a>> > --- > 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-><a href="http://Format.Graphics.PrimSurfSize.cy" rel="noreferrer" target="_blank">Format.Graphics.PrimSurfSize.cy</a>; > pCurrentBddMode->DispInfo.Pitch = > pSourceMode-><a href="http://Format.Graphics.PrimSurfSize.cx" rel="noreferrer" target="_blank">Format.Graphics.PrimSurfSize.cx</a> * > BPPFromPixelFormat(pCurrentBddMode->DispInfo.ColorFormat) / > BITS_PER_BYTE; > > - Status = m_pHWDevice->AcquireFrameBuffer(pCurrentBddMode); > + Status = m_pHWDevice->AcquireFrameBuffer(pCurrentBddMode); > </div></div>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 <div><div class="h5"> > 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; > } > </div></div>Here I would just add a if (!pCurrentBddMode->Flags.DoNotMapOrUnmap) { return STATUS_UNSUCCESSFUL; } just after the function starts. <div><div class="h5"> > @@ -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")) </div></div>All changes I commented are mainly spaces and small style, if you agree I can change them, if not reply. </blockquote><div>Yes, go ahead. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Acked-by: Frediano Ziglio <<a href="mailto:fziglio@redhat.com">fziglio@redhat.com</a>> Frediano </blockquote></div> <div> </div>-- <div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr">Respectfully, <div style="font-size:12.8px;color:rgb(136,136,136)">Sameeh Jubran</div><div style="font-size:12.8px;color:rgb(136,136,136)"><a href="https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a" title="View public profile" name="UNIQUE_ID_SafeHtmlFilter_UNIQUE_ID_SafeHtmlFilter_UNIQUE_ID_SafeHtmlFilter_UNIQUE_ID_SafeHtmlFilter_14e2c1de96f8c195_UNIQUE_ID_SafeHtmlFilter_SafeHtmlFilter_SafeHtmlFilter_webProfileURL" style="color:rgb(17,85,204);margin:0px;padding:0px;border-width:0px;outline:none;vertical-align:baseline;text-decoration:none" target="_blank">Linkedin</a> </div><div style="font-size:12.8px;color:rgb(136,136,136)">Junior Software Engineer @ <a href="http://www.daynix.com" target="_blank">Daynix</a>.</div></div></div></div></div></div></div> </div></div>