[Spice-devel] [qxl-wddm-dod] Fix for black screen on driver uninstall on ovmf platform
Frediano Ziglio
fziglio at redhat.com
Fri Jan 18 14:02:57 UTC 2019
By the way, the name of the function was not a regression, I acked and merged the patch.
Maybe I'll think about a better name
Frediano
> On Thu, Jan 17, 2019 at 6:20 PM Frediano Ziglio < fziglio at redhat.com > wrote:
> > >
>
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1661147
>
> > > If display miniport driver does not pass valid display info to
>
> > > the basic display driver on disable/uninstall, the basic
>
> > > display driver raises run-time error and stays with yellow
>
> > > bang. This behavior is specific to UEFI machines.
>
> > > Reference:
>
> > > https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/content/dispmprt/nc-dispmprt-dxgkddi_stop_device_and_release_post_display_ownership
>
> > >
>
> > > Signed-off-by: Yuri Benditovich < yuri.benditovich at daynix.com >
>
> > > ---
>
> > > qxldod/QxlDod.cpp | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>
> > > qxldod/QxlDod.h | 6 +++++-
>
> > > 2 files changed, 51 insertions(+), 2 deletions(-)
>
> > >
>
> > > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
>
> > > index b72e12c..dea78e2 100755
>
> > > --- a/qxldod/QxlDod.cpp
>
> > > +++ b/qxldod/QxlDod.cpp
>
> > > @@ -189,6 +189,12 @@ NTSTATUS QxlDod::StartDevice(_In_ DXGK_START_INFO*
>
> > > pDxgkStartInfo,
>
> > > return Status;
>
> > > }
>
> > >
>
> > > + DXGK_DISPLAY_INFORMATION *pInfo = &m_CurrentModes[0].DispInfo;
>
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("%s: Initial display info:\n",
>
> > > __FUNCTION__));
>
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("ACPI/Target Id: %d:%d\n",
>
> > > pInfo->AcpiId, pInfo->TargetId));
>
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("W/H/Pitch/Color:%d:%d:%d:%d\n",
>
> > > pInfo->Width, pInfo->Height, pInfo->Pitch, pInfo->ColorFormat));
>
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("Physical address:%I64x\n",
>
> > > pInfo->PhysicAddress.QuadPart));
>
> > > +
>
> > > Status = RegisterHWInfo(m_pHWDevice->GetId());
>
> > > if (!NT_SUCCESS(Status))
>
> > > {
>
> > > @@ -668,6 +674,12 @@ NTSTATUS
>
> > > QxlDod::StopDeviceAndReleasePostDisplayOwnership(_In_ D3DDDI_VIDEO_PRE
>
> > > m_pHWDevice->BlackOutScreen(&m_CurrentModes[SourceId]);
>
> > >
>
> > > *pDisplayInfo = m_CurrentModes[SourceId].DispInfo;
>
> > > + m_pHWDevice->GetFinalDisplayInfo(pDisplayInfo);
>
> > > +
>
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("%s: Final display info:\n",
>
> > > __FUNCTION__));
>
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("ACPI/Target Id: %d:%d\n",
>
> > > pDisplayInfo->AcpiId, pDisplayInfo->TargetId));
>
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("W/H/Pitch/Color:%d:%d:%d:%d\n",
>
> > > pDisplayInfo->Width, pDisplayInfo->Height, pDisplayInfo->Pitch,
>
> > > pDisplayInfo->ColorFormat));
>
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("Physical address:%I64x\n",
>
> > > pDisplayInfo->PhysicAddress.QuadPart));
>
> > >
>
> > > return StopDevice();
>
> > > }
>
> > > @@ -3055,6 +3067,19 @@ NTSTATUS VgaDevice::Escape(_In_ CONST
> > > DXGKARG_ESCAPE*
>
> > > pEscap)
>
> > > return STATUS_NOT_IMPLEMENTED;
>
> > > }
>
> > >
>
> > > +static BOOLEAN IsUefiMode()
>
> > > +{
>
> > > + PAGED_CODE();
>
> > > + UNICODE_STRING usName;
>
> > > + GUID guid = {};
>
> > > + ULONG res, len = sizeof(res);
>
> > > + RtlInitUnicodeString(&usName, L"dummy");
>
> > > + NTSTATUS status = ExGetFirmwareEnvironmentVariable(&usName, &guid,
> > > &res,
>
> > > &len, NULL);
>
> > > + DbgPrint(TRACE_LEVEL_VERBOSE, ("%s, status %X\n", __FUNCTION__,
>
> > > status));
>
> > > + // on UEFI system the status is STATUS_VARIABLE_NOT_FOUND
>
> > > + return status != STATUS_NOT_IMPLEMENTED;
>
> > > +}
>
> > > +
>
> > > QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod)
>
> > > {
>
> > > PAGED_CODE();
>
> > > @@ -3068,6 +3093,8 @@ QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod)
>
> > > m_Pending = 0;
>
> > > m_PresentThread = NULL;
>
> > > m_bActive = FALSE;
>
> > > + m_bUefiMode = IsUefiMode();
>
> > > + DbgPrint(TRACE_LEVEL_VERBOSE, ("%s, %s mode\n", __FUNCTION__,
>
> > > m_bUefiMode ? "UEFI" : "BIOS"));
>
> > > }
>
> > >
>
> > > QxlDevice::~QxlDevice(void)
>
> > > @@ -3331,6 +3358,17 @@ NTSTATUS
> > > QxlDevice::SetPowerState(DEVICE_POWER_STATE
>
> > > DevicePowerState, DXGK_DISP
>
> > > return STATUS_SUCCESS;
>
> > > }
>
> > >
>
> > > +void QxlDevice::GetFinalDisplayInfo(DXGK_DISPLAY_INFORMATION*
> > > pDisplayInfo)
>
> > > +{
>
> > > + PAGED_CODE();
>
> > > + *pDisplayInfo = m_InitialDisplayInfo;
>
> > > + pDisplayInfo->TargetId = pDisplayInfo->AcpiId = 0;
>
> > > + if (!pDisplayInfo->PhysicAddress.QuadPart)
>
> > > + {
>
> > > + pDisplayInfo->PhysicAddress = m_RamPA;
>
> > > + }
>
> > > +}
>
> > > +
>
> > > NTSTATUS QxlDevice::HWInit(PCM_RESOURCE_LIST pResList,
>
> > > DXGK_DISPLAY_INFORMATION* pDispInfo)
>
> > > {
>
> > > PAGED_CODE();
>
> > > @@ -3530,10 +3568,11 @@ NTSTATUS
> > > QxlDevice::QxlInit(DXGK_DISPLAY_INFORMATION*
>
> > > pDispInfo)
>
> > > DbgPrint(TRACE_LEVEL_ERROR, ("InitMonitorConfig failed with status
>
> > > 0x%X\n", Status));
>
> > > return Status;
>
> > > }
>
> > > - Status = AcquireDisplayInfo(*(pDispInfo));
>
> > > + Status = AcquireDisplayInfo(m_InitialDisplayInfo);
>
> > > if (NT_SUCCESS(Status))
>
> > > {
>
> > > m_bActive = TRUE;
>
> > > + *pDispInfo = m_InitialDisplayInfo;
>
> > > Status = StartPresentThread();
>
> > > }
>
> > > if (!NT_SUCCESS(Status)) {
>
> > > @@ -4728,6 +4767,11 @@ NTSTATUS QxlDevice::HWClose(void)
>
> > > {
>
> > > PAGED_CODE();
>
> > > QxlClose();
>
> > > + if (m_bUefiMode)
>
> > > + {
>
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("%s: Resetting the device\n",
>
> > > __FUNCTION__));
>
> > > + WRITE_PORT_UCHAR((PUCHAR)(m_IoBase + QXL_IO_RESET), 0);
>
> > > + }
>
> > > UnmapMemory();
>
> > > return STATUS_SUCCESS;
>
> > > }
>
> > > @@ -5143,6 +5187,7 @@ NTSTATUS
>
> > > HwDeviceInterface::AcquireDisplayInfo(DXGK_DISPLAY_INFORMATION& DispInf
>
> > >
>
> > > if (DispInfo.Width == 0)
>
> > > {
>
> > > + DbgPrint(TRACE_LEVEL_WARNING, ("QxlDod::AcquireDisplayInfo has zero
>
> > > width!\n"));
>
> > > DispInfo.ColorFormat = D3DDDIFMT_A8R8G8B8;
>
> > > DispInfo.Width = INITIAL_WIDTH;
>
> > > DispInfo.Height = INITIAL_HEIGHT;
>
> > > diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
>
> > > index eb6b78d..d889b9d 100755
>
> > > --- a/qxldod/QxlDod.h
>
> > > +++ b/qxldod/QxlDod.h
>
> > > @@ -285,6 +285,7 @@ public:
>
> > > QXL_NON_PAGED virtual VOID VSyncInterruptPostProcess(_In_
>
> > > PDXGKRNL_INTERFACE) = 0;
>
> > > virtual NTSTATUS AcquireFrameBuffer(CURRENT_BDD_MODE* pCurrentBddMode) {
>
> > > return STATUS_SUCCESS; }
>
> > > virtual NTSTATUS ReleaseFrameBuffer(CURRENT_BDD_MODE* pCurrentBddMode) {
>
> > > return STATUS_SUCCESS; }
>
> > > + virtual void GetFinalDisplayInfo(DXGK_DISPLAY_INFORMATION*
> > > pDisplayInfo)
>
> > > {}
>
> > >
>
> > > ULONG GetModeCount(void) const {return m_ModeCount;}
>
> > > PVIDEO_MODE_INFORMATION GetModeInfo(UINT idx) {return &m_ModeInfo[idx];}
>
> > > @@ -555,7 +556,8 @@ public:
>
> > > NTSTATUS SetPointerShape(_In_ CONST DXGKARG_SETPOINTERSHAPE*
>
> > > pSetPointerShape);
>
> > > NTSTATUS SetPointerPosition(_In_ CONST DXGKARG_SETPOINTERPOSITION*
>
> > > pSetPointerPosition);
>
> > > NTSTATUS Escape(_In_ CONST DXGKARG_ESCAPE* pEscap);
>
> > > - BOOLEAN IsBIOSCompatible() { return FALSE; }
>
> > > + BOOLEAN IsBIOSCompatible() { return m_bUefiMode; }
>
> > So when is UEFI is BIOS compatible and when is BIOS is not BIOS compatible?
>
> > It looks a bit odd, I'm sure it works but is confusing.
>
> Indeed, this looks confusing, but in fact it is not.
> When the system has a BIOS the BDD obtains video modes from the BIOS.
> In MSFT headers the respective field has name 'SupportNonVGA', which does not
> explain
> anything at all, but used by class driver to understand whether the miniport
> will provide on
> leaving meaningful display info to BDD (Basic display driver), so the BDD can
> continue with it.
> For example, QXL driver in 'revision=3' mode uses video modes from the BIOS
> and can report
> exact BIOS-compatible mode when it leaves. So, the name 'BIOS compatible' in
> the code
> reflects the real ability to comply with BIOS-defined set of video modes.
> QXL driver obtains video modes from the device and there is no compatibility
> between device's video modes
> and BIOS ones. So, in case of BIOS system for the QXL driver it is not so
> simple to leave
> the adapter in video mode compatible with BIOS mode list, so we do not make
> such effort
> (according to the documentation this is OK and certification test allows it).
> When the system is UEFI, we do not have a choice, on driver stop we must
> provide
> such data to BDD.
> If you have in mind better name for the field, please write it in response, I
> will change it.
> > > + void GetFinalDisplayInfo(DXGK_DISPLAY_INFORMATION* pDisplayInfo);
>
> > > protected:
>
> > > NTSTATUS GetModeList(DXGK_DISPLAY_INFORMATION* pDispInfo);
>
> > > QXLDrawable *PrepareBltBits (BLT_INFO* pDst,
>
> > > @@ -695,6 +697,8 @@ private:
>
> > > uint16_t m_DrawGeneration;
>
> > > HANDLE m_PresentThread;
>
> > > BOOLEAN m_bActive;
>
> > > + BOOLEAN m_bUefiMode;
>
> > > + DXGK_DISPLAY_INFORMATION m_InitialDisplayInfo;
>
> > > };
>
> > >
>
> > > class QxlDod {
>
> > Frediano
>
More information about the Spice-devel
mailing list