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));
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
