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