Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33737 )
Change subject: device/oprom: Add vbe return status support as per VBE spec 3.0 ......................................................................
Patch Set 8:
(1 comment)
Patch Set 8:
(1 comment)
Patch Set 8:
Sorry for the late review, I've little time at the moment. And this code for legacy concepts and binary blobs is really not what I like to work on. If you would write modern open-source code for your graphics cards instead, you would have my full attention :D
@Nico: i don't understand what you mean by "your graphics card" ? i was explaining what i would like to achieve here. I've not added any new piece of code of oprom here. I'm just saving developers time to debug issue when coreboot Kconfig forces to set one mode which is not supported by opRom card.
That was just chitchat. Me trying to explain why I didn't look forward to this review. I would love to see new code if it were about actual hardware initialization (and not to support other software doing it).
It took me some time to figure out that the code you edit in the two commits ahead (yabel) and this (realmode) code are mutually exclusive. You are going through a lot of trouble to make the existing `vbe_info_t` compatible to your new code here. It would be much easier to declare a new struct. Or instead of squeezing the yabel code into a new shape, you could make its vbe_info code (vbe_info() and vbe_get_info()) common to both implementations. The existing yabel code looks more comprehen- sive than the mode listing here.
yable and realmode mode both are referring from same vbe spec hence i don't it would matter. But as i said, existing yabel code itself has bug, mentioned here: https://review.coreboot.org/c/coreboot/+/33782/1/src/include/vbe.h#48
There is no existing bug. The yabel code just works differently than you expect, I guess.
https://review.coreboot.org/c/coreboot/+/33737/8/src/device/oprom/realmode/x... File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/c/coreboot/+/33737/8/src/device/oprom/realmode/x... PS8, Line 246: mode = *video_mode_ptr++;
we have already copied it into buffer right ?
What exactly are you referring to? the pointer? the array behind the pointer?
I was wondering why we can dereference a pointer in protected mode that was written by the oprom in real mode. For instance, in lines 230, 231 you convert a linear pointer to a `segment:offset` pointer. But the pointer inside your `vbe_info_t` is never con- verted in the opposite way. And the VBE spec tells us:
When functions are called via the real mode INT 10h software interrupt, a ‘vbeFarPtr’ will be a real mode segment:offset style pointer to a memory location below the 1Mb system memory boundary.
Your results show that it works without that conversion. I just don't understand how.