[Spice-devel] [PATCH qxl-wddm-dod 1/2] Fixing black-white screen in installation when qxl revision = 3

[Frediano Ziglio]

> 
> When qxl revision is 3, the vga driver is the one that is running. When
> installing the driver the function VgaDevice::HWInit with displayInfo
> structure that is zeroed out. The displayInfo should be intialized using
> DxgkCbAcquirePostDisplayOwnership and thus it should be called before
> calling HWInit.
> 
> Please note that we can't just move the call to
> "DxgkCbAcquirePostDisplayOwnership"
> before calling HWInit as the m_Id isn't iniatilized for QxlDevice untill the
> call
> to HWinit is over.
> 
> This patch fixies a bug similar to the one found here:
> https://bugzilla.redhat.com/show_bug.cgi?id=1202267
> However this one occurs when installing the driver.
> 

Minor typos
intialized -> initialized
iniatilized -> initialized
untill -> until
fixies -> fixes

It's not clear to me the "However this one occurs when installing the driver."
sentence. Do you mean that the bug refer to a problem similar but not
occurring during installation?

> Signed-off-by: Sameeh Jubran <sameeh at daynix.com>
> ---
>  qxldod/QxlDod.cpp | 68
>  +++++++++++++++++++++++++++----------------------------
>  qxldod/QxlDod.h   |  2 ++
>  2 files changed, 36 insertions(+), 34 deletions(-)
> 
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> index 5cfff78..6ef57b8 100755
> --- a/qxldod/QxlDod.cpp
> +++ b/qxldod/QxlDod.cpp
> @@ -124,7 +124,6 @@ NTSTATUS QxlDod::StartDevice(_In_  DXGK_START_INFO*
> pDxgkStartInfo,
>                           _Out_ ULONG*             pNumberOfViews,
>                           _Out_ ULONG*             pNumberOfChildren)
>  {
> -    PHYSICAL_ADDRESS PhysicAddress;
>      PAGED_CODE();
>      QXL_ASSERT(pDxgkStartInfo != NULL);
>      QXL_ASSERT(pDxgkInterface != NULL);
> @@ -176,32 +175,6 @@ NTSTATUS QxlDod::StartDevice(_In_  DXGK_START_INFO*
> pDxgkStartInfo,
>          return Status;
>      }
>  
> -    PhysicAddress.QuadPart =
> m_CurrentModes[0].DispInfo.PhysicAddress.QuadPart;
> -    if (m_pHWDevice->GetId() == 0)
> -    {
> -         Status =
> m_DxgkInterface.DxgkCbAcquirePostDisplayOwnership(m_DxgkInterface.DeviceHandle,
> &(m_CurrentModes[0].DispInfo));
> -    }
> -
> -    if (!NT_SUCCESS(Status) )
> -    {
> -        DbgPrint(TRACE_LEVEL_ERROR, ("DxgkCbAcquirePostDisplayOwnership
> failed with status 0x%X Width = %d\n",
> -                           Status, m_CurrentModes[0].DispInfo.Width));
> -        return STATUS_UNSUCCESSFUL;
> -    }
> -
> -    if (m_CurrentModes[0].DispInfo.Width == 0)
> -    {
> -        m_CurrentModes[0].DispInfo.Width = MIN_WIDTH_SIZE;
> -        m_CurrentModes[0].DispInfo.Height = MIN_HEIGHT_SIZE;
> -        m_CurrentModes[0].DispInfo.Pitch =
> BPPFromPixelFormat(D3DDDIFMT_R8G8B8) / 8;
> -        m_CurrentModes[0].DispInfo.ColorFormat = D3DDDIFMT_R8G8B8;
> -        m_CurrentModes[0].DispInfo.TargetId = 0;
> -        if (PhysicAddress.QuadPart != 0L) {
> -             m_CurrentModes[0].DispInfo.PhysicAddress.QuadPart =
> PhysicAddress.QuadPart;
> -        }
> -
> -    }
> -
>     *pNumberOfViews = MAX_VIEWS;
>     *pNumberOfChildren = MAX_CHILDREN;
>      m_Flags.DriverStarted = TRUE;
> @@ -2488,12 +2461,6 @@ NTSTATUS
> VgaDevice::GetModeList(DXGK_DISPLAY_INFORMATION* pDispInfo)
>  
>      m_CurrentMode = 0;
>      DbgPrint(TRACE_LEVEL_INFORMATION, ("m_ModeInfo = 0x%p, m_ModeNumbers =
>      0x%p\n", m_ModeInfo, m_ModeNumbers));
> -    if (Width == 0 || Height == 0 || BitsPerPixel != VGA_BPP)
> -    {
> -        Width = MIN_WIDTH_SIZE;
> -        Height = MIN_HEIGHT_SIZE;
> -        BitsPerPixel = VGA_BPP;
> -    }
>      for (CurrentMode = 0, SuitableModeCount = 0;
>           CurrentMode < ModeCount;
>           CurrentMode++)
> @@ -2621,7 +2588,7 @@ NTSTATUS VgaDevice::HWInit(PCM_RESOURCE_LIST pResList,
> DXGK_DISPLAY_INFORMATION*
>  
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
>      UNREFERENCED_PARAMETER(pResList);
> -    UNREFERENCED_PARAMETER(pDispInfo);
> +    AcquireDisplayInfo(*(pDispInfo));
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
>      return GetModeList(pDispInfo);
>  }
> @@ -3391,6 +3358,7 @@ NTSTATUS QxlDevice::QxlInit(DXGK_DISPLAY_INFORMATION*
> pDispInfo)
>      CreateMemSlots();
>      InitDeviceMemoryResources();
>      InitMonitorConfig();
> +    Status = AcquireDisplayInfo(*(pDispInfo));
>      return Status;
>  }
>  
> @@ -4835,3 +4803,35 @@ UINT SpiceFromPixelFormat(D3DDDIFORMAT Format)
>          default: QXL_LOG_ASSERTION1("Unknown D3DDDIFORMAT 0x%I64x", Format);
>          return 0;
>      }
>  }
> +
> +NTSTATUS HwDeviceInterface::AcquireDisplayInfo(DXGK_DISPLAY_INFORMATION&
> DispInfo)
> +{
> +    NTSTATUS Status = STATUS_SUCCESS;
> +    PHYSICAL_ADDRESS PhysicAddress;
> +    if (GetId() == 0)
> +    {
> +        Status = m_pQxlDod->AcquireDisplayInfo(DispInfo);
> +    }
> +
> +    if (!NT_SUCCESS(Status))
> +    {
> +        DbgPrint(TRACE_LEVEL_ERROR, ("DxgkCbAcquirePostDisplayOwnership
> failed with status 0x%X Width = %d\n",
> +            Status, DispInfo.Width));

In this function you don't call DxgkCbAcquirePostDisplayOwnership but
QxlDod::AcquireDisplayInfo which calls DxgkCbAcquirePostDisplayOwnership.
I would either change the message or rename QxlDod::AcquireDisplayInfo
to QxlDod::DxgkCbAcquirePostDisplayOwnership.

> +        return STATUS_UNSUCCESSFUL;
> +    }
> +    PhysicAddress.QuadPart = DispInfo.PhysicAddress.QuadPart;
> +
> +    if (DispInfo.Width == 0)
> +    {
> +        DispInfo.Width = MIN_WIDTH_SIZE;
> +        DispInfo.Height = MIN_HEIGHT_SIZE;
> +        DispInfo.Pitch = BPPFromPixelFormat(D3DDDIFMT_R8G8B8) / 8;
> +        DispInfo.ColorFormat = D3DDDIFMT_R8G8B8;
> +        DispInfo.TargetId = 0;
> +        if (PhysicAddress.QuadPart != 0L)
> +        {
> +            DispInfo.PhysicAddress.QuadPart = PhysicAddress.QuadPart;
> +        }
> +    }
> +    return Status;
> +}
> diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> index bf16724..cd2032c 100755
> --- a/qxldod/QxlDod.h
> +++ b/qxldod/QxlDod.h
> @@ -250,6 +250,7 @@ public:
>      virtual NTSTATUS SetPointerShape(_In_ CONST DXGKARG_SETPOINTERSHAPE*
>      pSetPointerShape) = 0;
>      virtual NTSTATUS SetPointerPosition(_In_ CONST
>      DXGKARG_SETPOINTERPOSITION* pSetPointerPosition) = 0;
>      virtual NTSTATUS Escape(_In_ CONST DXGKARG_ESCAPE* pEscap) = 0;
> +    NTSTATUS AcquireDisplayInfo(DXGK_DISPLAY_INFORMATION& DispInfo);
>      ULONG GetId(void) { return m_Id; }
>  protected:
>      virtual NTSTATUS GetModeList(DXGK_DISPLAY_INFORMATION* pDispInfo) = 0;
> @@ -704,6 +705,7 @@ public:
>                                   _In_
>                                   INT
>                                   PositionX,
>                                   _In_
>                                   INT
>                                   PositionY);
>      PDXGKRNL_INTERFACE GetDxgkInterface(void) { return &m_DxgkInterface;}
> +    NTSTATUS AcquireDisplayInfo(DXGK_DISPLAY_INFORMATION& DispInfo) { return
> m_DxgkInterface.DxgkCbAcquirePostDisplayOwnership(m_DxgkInterface.DeviceHandle,
> &(DispInfo)); }

you could use &DispInfo instead of &(DispInfo), feel free to ignore, just
minor style.

>  private:
>      VOID CleanUp(VOID);
>      NTSTATUS CheckHardware();

Frediano