Paul Menzel 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 5:
(14 comments)
https://review.coreboot.org/#/c/33737/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33737/5//COMMIT_MSG@19 PS5, Line 19: functions(0x3 and 0x4f00/1/2) Please add a space before (.
https://review.coreboot.org/#/c/33737/5//COMMIT_MSG@20 PS5, Line 20: list lists
https://review.coreboot.org/#/c/33737/5//COMMIT_MSG@20 PS5, Line 20: Function function
https://review.coreboot.org/#/c/33737/5//COMMIT_MSG@27 PS5, Line 27: Now coreboot will show below msg to user to know there is a potential issue with choosen Please add a blank line above.
https://review.coreboot.org/#/c/33737/5//COMMIT_MSG@28 PS5, Line 28: user users?
https://review.coreboot.org/#/c/33737/5//COMMIT_MSG@9 PS5, Line 9: Existing coreboot oprom implementation relies on user selected vesa : mode through CONFIG_FRAMEBUFFER_VESA_MODE Kconfig option and expects : that all oprom might support user selected vesa mode. : : Take an example: : Enabling AMD external radeon PCIE graphics card on ICLRVP with default : vesa mode 0x118. Unable to get valid X and Y resolution after executing : vbe_get_mode_info() with 0x4118, return data buffer shows 0x0 resolution. : It causes further hang while trying to draw bmpblk image at depthcharge. : : This patch checks for output register AX in all vbe functions(0x3 and 0x4f00/1/2) : and list all supported vesa mode by oprom using Function 0x4F00 (return vbe controller : information). This information might be useful for user to select correct vesa mode : for oprom. : : TEST=Enabling external pcie based graphics card on ICLRVP : : Case 1: with unsupported vesa mode 0x118 : Now coreboot will show below msg to user to know there is a potential issue with choosen : vesa mode and better user know the failure rather going to depthcharge and debug further. Please adhere to the text width limits.
https://review.coreboot.org/#/c/33737/5/src/device/oprom/realmode/x86.c File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/#/c/33737/5/src/device/oprom/realmode/x86.c@241 PS5, Line 241: opron oprom?
https://review.coreboot.org/#/c/33737/5/src/device/oprom/realmode/x86.c@244 PS5, Line 244: are Remove.
https://review.coreboot.org/#/c/33737/5/src/device/oprom/realmode/x86.c@268 PS5, Line 268: Remove
https://review.coreboot.org/#/c/33737/5/src/device/oprom/realmode/x86.c@268 PS5, Line 268: *AH == 0x00 : Function call successful : *AH == 0x01: Function call failed : *AH == 0x02: Function is not supported in the current HW configuration : *AH == 0x03: Functio call invalid in current video mode : * : *Return 0 on success else -1 for failure Please add a space after the asterisk.
https://review.coreboot.org/#/c/33737/5/src/device/oprom/realmode/x86.c@315 PS5, Line 315: die("\nError: In %s function\n", __func__); I’d say `die` is too hard, and the system should try to continue. It does not have to crash in all configurations, but then a likely crash is still better than a certain crash.
https://review.coreboot.org/#/c/33737/5/src/device/oprom/realmode/x86.c@326 PS5, Line 326: /* request clearing of framebuffer */ Separate commit.
https://review.coreboot.org/#/c/33737/5/src/include/vbe.h File src/include/vbe.h:
PS5: Could be a separate commit.
https://review.coreboot.org/#/c/33737/5/src/include/vbe.h@18 PS5, Line 18: Lets Let’s