[Spice-devel] [qxl-wddm-dod] Fix for black screen on driver uninstall on ovmf platform

Mon Jan 21 07:13:11 UTC 2019

On Fri, Jan 18, 2019 at 4:02 PM Frediano Ziglio <fziglio at redhat.com> wrote:

> By the way, the name of the function was not a regression, I acked and
> merged the patch.

It looks like the patch is not merged, BTW
If I'm mistaken, this means the repository migrated somewhere.

>
>
> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190121/7ee61e04/attachment.html>