On 08/26/14 01:41, Jordan Justen wrote: > On Mon, Aug 25, 2014 at 2:56 PM, Laszlo Ersek <[email protected]> wrote: >> On 08/25/14 23:42, Jordan Justen wrote: >>> On Mon, Aug 25, 2014 at 2:21 PM, Laszlo Ersek <[email protected]> >>> wrote: >>>> On 08/25/14 14:43, Gerd Hoffmann wrote: >>>>> Hi, >>>>> >>>>>> The current calculation is correct for stdvga, because on stdvga >>>>>> the value returned by VBE_DISPI_INDEX_VIDEO_MEMORY_64K, ie. the >>>>>> full video RAM, is usable for drawing. >>>>> >>>>> Value returned by VBE_DISPI_INDEX_VIDEO_MEMORY_64K being wrong is a >>>>> qemu bug. We have releases with the bug in the wild though, so >>>>> adding this as (temporary?) workaround is sensible IMO. The comment >>>>> should be updated saying so though. >>>> >>>> Thanks for the review! >>>> >>>> If the workaround is stable in your opinion (ie. it can be relied >>>> upon even after the qemu problem with >>>> VBE_DISPI_INDEX_VIDEO_MEMORY_64K is fixed), then I'd probably leave >>>> it in, even in the long term. >>>> >>>> Jordan, is it enough if I only reword the commit message below, or do >>>> you want me to resubmit the patch? >>> >>> Why not use Private->Variant, rather than 'IsQxl'? >> >> I wondered if you would ask that question! :) >> >> The reason is that at that point Private->Variant does not >> *necessarily* distinguish QXL from stdvga. Originally, yes, >> Private->Variant distinguishes QXL (QEMU_VIDEO_BOCHS) from stdvga >> (QEMU_VIDEO_BOCHS_MMIO), but please see this in >> QemuVideoControllerDriverStart(): >> >> // >> // Check whenever the qemu stdvga mmio bar is present (qemu 1.3+). >> // >> if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO) { >> EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *MmioDesc; >> >> Status = Private->PciIo->GetBarAttributes ( >> Private->PciIo, >> PCI_BAR_IDX2, >> NULL, >> (VOID**) &MmioDesc >> ); >> if (EFI_ERROR (Status) || >> MmioDesc->ResType != ACPI_ADDRESS_SPACE_TYPE_MEM) { >> DEBUG ((EFI_D_INFO, "QemuVideo: No mmio bar, fallback to port io\n")); >> Private->Variant = QEMU_VIDEO_BOCHS; >> } else { >> DEBUG ((EFI_D_INFO, "QemuVideo: Using mmio bar @ 0x%lx\n", >> MmioDesc->AddrRangeMin)); >> } >> >> if (!EFI_ERROR (Status)) { >> FreePool (MmioDesc); >> } >> } >> >> There's a path here where Private->Variant changes from >> QEMU_VIDEO_BOCHS_MMIO to QEMU_VIDEO_BOCHS, ie. an stdvga card would >> appear as QXL. >> >> For this reason, I added the IsQxl parameter, and I fill it in from: >> >> case QEMU_VIDEO_BOCHS: >> Status = QemuVideoBochsModeSetup (Private, >> (BOOLEAN)(Card->Variant == QEMU_VIDEO_BOCHS)); > > Hmm, I see. > > How about, for clarity, above, near where you first assign > Private->Variant, you also set a local IsQxl variable? > > And a comment might not hurt: > // > // IsQxl is based on the detected Card->Variant, which at a > // later point might not match Private->Variant. > //
Will do, thanks! Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
