Re: [PATCH qxl-wddm-dod v5 4/7] Fixing framebuffer usage logic

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> 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@xxxxxxxxxx>
> Signed-off-by: Sameeh Jubran <sameeh@xxxxxxxxxx>
> ---
>  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.

Acked-by: Frediano Ziglio <fziglio@xxxxxxxxxx>

Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]