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. // -Jordan > The expression does not use Private->Variant, it uses Card->Variant. > > I thought about introducing more fields to Private, but it seemed > overkill; the difference is only in QemuVideoBochsModeSetup(), and that > one can key off a local variable of the caller. > > 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
