[Spice-devel] [qxl-wddm-dod] Fix for black screen on driver uninstall on ovmf platform
Yuri Benditovich
yuri.benditovich at daynix.com
Thu Jan 17 17:11:08 UTC 2019
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190117/a081f5e8/attachment.html>
More information about the Spice-devel
mailing list