Re: [qxl-wddm-dod] Fix for black screen on driver uninstall on ovmf platform

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

 



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@xxxxxxxxxx > 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@xxxxxxxxxx >
> 
> > > ---
> 
> > > 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
> 
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




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