[Spice-devel] [PATCH qxl-wddm-dod v5 4/7] Fixing framebuffer usage logic

[Frediano Ziglio]

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

Well...

    if (pCurrentBddMode->Flags.DoNotMapOrUnmap) {
        return STATUS_UNSUCCESSFUL;
    }

Frediano