Change in ...coreboot[master]: device/oprom: Add vbe return status support as per VBE spec 3.0
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33737
Change subject: device/oprom: Add vbe return status support as per VBE spec 3.0 ......................................................................
device/oprom: Add vbe return status support as per VBE spec 3.0
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.
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4118 VBE: Function call invalid with unsupported video mode 0x118! User to select mode from below list - Supported Video Mode list for OpRom are: 0x110 0x111 0x113 0x114 0x116 0x117 0x119 0x11a 0x165 0x166 0x121 0x122 0x123 0x124 0x145 0x146 0x175 0x176 0x1d2 0x1d4
Error: In vbe_get_mode_info function
Case 2: with supported vesa mode 0x116
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4116 VBE: resolution: 1024x768@16 VBE: framebuffer: a0000000 VBE: Setting VESA mode 4116 VGA Option ROM was run
Change-Id: Iacd2ce468e038a14424f029df3a0adec3e5fa15c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/realmode/x86_asm.S M src/include/vbe.h 3 files changed, 107 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/33737/1
diff --git a/src/device/oprom/realmode/x86.c b/src/device/oprom/realmode/x86.c index a7631a1..d1c7731 100644 --- a/src/device/oprom/realmode/x86.c +++ b/src/device/oprom/realmode/x86.c @@ -221,6 +221,90 @@ return mode_info_valid; }
+static int vbe_check_for_failure(void); + +static vbe_info_t vbe_get_ctrl_info(void) +{ + char *buffer = PTR_TO_REAL_MODE(__realmode_buffer); + vbe_info_t info; + u16 buffer_seg = (((unsigned long)buffer) >> 4) & 0xff00; + u16 buffer_adr = ((unsigned long)buffer) & 0xffff; + realmode_interrupt(0x10, VESA_GET_INFO, 0x0000, 0x0000, 0x0000, + buffer_seg, buffer_adr); + if (vbe_check_for_failure()) + die("\nError: In %s function\n", __func__); + memcpy(&info, buffer, sizeof(vbe_info_t)); + + return info; +} + +static void vbe_opron_list_supported_mode(uint16_t *video_mode_ptr) +{ + uint16_t mode; + printk(BIOS_DEBUG,"Supported Video Mode list for OpRom are:\n"); + do + { + mode = *video_mode_ptr++; + if (mode != 0xffff) + printk(BIOS_DEBUG,"%x\n", mode); + } while (mode != 0xffff); +} + +static void vbe_oprom_supported_mode_list(void) +{ + vbe_info_t info = vbe_get_ctrl_info(); + uint32_t video_mode_ptr = info.video_mode_list[1] << 16 | + info.video_mode_list[0]; + vbe_opron_list_supported_mode((uint16_t *)video_mode_ptr); +} + +/* + * EAX register is used to indicate the completion status upon return from + * VBE function in real mode. + * In x86_asm.S, store EAX into REALMODE_BASE before jumping into protected + * mode. + * If the VBE function completed successfully then 0x0 is returned in the AH + * register. Otherwise the AH register is set with the nature of the failure: + * + *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 + */ +static int vbe_check_for_failure(void) +{ + uint32_t vbe_return_status; + int status; + vbe_return_status = *((unsigned int*)REALMODE_BASE); + + switch ((vbe_return_status & 0xff00) >> 8) { + case 0x0: + status = 0; + break; + case 1: + printk(BIOS_DEBUG, "VBE: Function call failed!\n"); + status = -1; + break; + case 2: + printk(BIOS_DEBUG, "VBE: Function is not supported!\n"); + status = -1; + break; + case 3: + default: + printk(BIOS_DEBUG, "VBE: Function call invalid with" + " unsupported video mode %x!\n", + CONFIG_FRAMEBUFFER_VESA_MODE); + printk(BIOS_DEBUG, "User to select mode from below list - \n"); + vbe_oprom_supported_mode_list(); + status = -1; + break; + } + + return status; +} + static u8 vbe_get_mode_info(vbe_mode_info_t * mi) { printk(BIOS_DEBUG, "VBE: Getting information about VESA mode %04x\n", @@ -230,20 +314,25 @@ u16 buffer_adr = ((unsigned long)buffer) & 0xffff; realmode_interrupt(0x10, VESA_GET_MODE_INFO, 0x0000, mi->video_mode, 0x0000, buffer_seg, buffer_adr); + if (vbe_check_for_failure()) + die("\nError: In %s function\n", __func__); memcpy(mi->mode_info_block, buffer, sizeof(mi->mode_info_block)); mode_info_valid = 1; + return 0; }
static u8 vbe_set_mode(vbe_mode_info_t * mi) { printk(BIOS_DEBUG, "VBE: Setting VESA mode %04x\n", mi->video_mode); - // request linear framebuffer mode + /* request linear framebuffer mode */ mi->video_mode |= (1 << 14); - // request clearing of framebuffer + /* request clearing of framebuffer */ mi->video_mode &= ~(1 << 15); realmode_interrupt(0x10, VESA_SET_MODE, mi->video_mode, 0x0000, 0x0000, 0x0000, 0x0000); + if (vbe_check_for_failure()) + die("\nError: In %s function\n", __func__); return 0; }
@@ -253,6 +342,7 @@ void vbe_set_graphics(void) { mode_info.video_mode = (1 << 14) | CONFIG_FRAMEBUFFER_VESA_MODE; + vbe_get_mode_info(&mode_info); unsigned char *framebuffer = (unsigned char *)mode_info.vesa.phys_base_ptr; @@ -288,6 +378,8 @@ delay(2); realmode_interrupt(0x10, 0x0003, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000); + if (vbe_check_for_failure()) + die("\nError: In %s function\n", __func__); }
int fill_lb_framebuffer(struct lb_framebuffer *framebuffer) diff --git a/src/device/oprom/realmode/x86_asm.S b/src/device/oprom/realmode/x86_asm.S index 87348cd..ed3f897 100644 --- a/src/device/oprom/realmode/x86_asm.S +++ b/src/device/oprom/realmode/x86_asm.S @@ -291,6 +291,13 @@ __intXX_instr = RELOCATED(.) .byte 0xcd, 0x00 /* This becomes intXX */
+ /* + * Here is end of real mode call and time to go back to protected mode. + * Before that its better to store current eax into some memory address + * so that context persist in protected mode too. + */ + mov %eax, REALMODE_BASE + /* Ok, the job is done, now go back to protected mode coreboot */ movl %cr0, %eax orl $PE, %eax diff --git a/src/include/vbe.h b/src/include/vbe.h index 2c40d05..fea55f5 100644 --- a/src/include/vbe.h +++ b/src/include/vbe.h @@ -14,6 +14,10 @@ #define VBE_H
#include <boot/coreboot_tables.h> + +/* Lets hope we never have more than 256 video modes. */ +#define MAX_VBE_FRAMEBUFFER_MODE 256 + // these structs are for input from and output to OF typedef struct { u8 display_type; // 0 = NONE, 1 = analog, 2 = digital @@ -41,10 +45,9 @@ u16 version; u8 *oem_string_ptr; u32 capabilities; - u16 video_mode_list[256]; // lets hope we never have more than - // 256 video modes... + u16 video_mode_list[MAX_VBE_FRAMEBUFFER_MODE]; u16 total_memory; -} vbe_info_t; +} __packed vbe_info_t;
typedef struct { u16 mode_attributes; // 00
build bot (Jenkins) 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 1:
(5 comments)
https://review.coreboot.org/#/c/33737/1/src/device/oprom/realmode/x86.c File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/#/c/33737/1/src/device/oprom/realmode/x86.c@244 PS1, Line 244: printk(BIOS_DEBUG,"Supported Video Mode list for OpRom are:\n"); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/33737/1/src/device/oprom/realmode/x86.c@245 PS1, Line 245: do that open brace { should be on the previous line
https://review.coreboot.org/#/c/33737/1/src/device/oprom/realmode/x86.c@249 PS1, Line 249: printk(BIOS_DEBUG,"%x\n", mode); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/33737/1/src/device/oprom/realmode/x86.c@280 PS1, Line 280: vbe_return_status = *((unsigned int*)REALMODE_BASE); "(foo*)" should be "(foo *)"
https://review.coreboot.org/#/c/33737/1/src/device/oprom/realmode/x86.c@299 PS1, Line 299: printk(BIOS_DEBUG, "User to select mode from below list - \n"); unnecessary whitespace before a quoted newline
Subrata Banik has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/33737 )
Change subject: device/oprom: Add vbe return status support as per VBE spec 3.0 ......................................................................
device/oprom: Add vbe return status support as per VBE spec 3.0
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.
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4118 VBE: Function call invalid with unsupported video mode 0x118! User to select mode from below list - Supported Video Mode list for OpRom are: 0x110 0x111 0x113 0x114 0x116 0x117 0x119 0x11a 0x165 0x166 0x121 0x122 0x123 0x124 0x145 0x146 0x175 0x176 0x1d2 0x1d4
Error: In vbe_get_mode_info function
Case 2: with supported vesa mode 0x116
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4116 VBE: resolution: 1024x768@16 VBE: framebuffer: a0000000 VBE: Setting VESA mode 4116 VGA Option ROM was run
Change-Id: Iacd2ce468e038a14424f029df3a0adec3e5fa15c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/realmode/x86_asm.S M src/include/vbe.h 3 files changed, 106 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/33737/2
Subrata Banik 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 1:
(5 comments)
https://review.coreboot.org/#/c/33737/1/src/device/oprom/realmode/x86.c File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/#/c/33737/1/src/device/oprom/realmode/x86.c@244 PS1, Line 244: printk(BIOS_DEBUG,"Supported Video Mode list for OpRom are:\n");
space required after that ',' (ctx:VxV)
Done
https://review.coreboot.org/#/c/33737/1/src/device/oprom/realmode/x86.c@245 PS1, Line 245: do
that open brace { should be on the previous line
Done
https://review.coreboot.org/#/c/33737/1/src/device/oprom/realmode/x86.c@249 PS1, Line 249: printk(BIOS_DEBUG,"%x\n", mode);
space required after that ',' (ctx:VxV)
Done
https://review.coreboot.org/#/c/33737/1/src/device/oprom/realmode/x86.c@280 PS1, Line 280: vbe_return_status = *((unsigned int*)REALMODE_BASE);
"(foo*)" should be "(foo *)"
Done
https://review.coreboot.org/#/c/33737/1/src/device/oprom/realmode/x86.c@299 PS1, Line 299: printk(BIOS_DEBUG, "User to select mode from below list - \n");
unnecessary whitespace before a quoted newline
Done
build bot (Jenkins) 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 2:
(1 comment)
https://review.coreboot.org/#/c/33737/2/src/device/oprom/realmode/x86.c File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/#/c/33737/2/src/device/oprom/realmode/x86.c@298 PS2, Line 298: printk(BIOS_DEBUG, "User to select mode from below list - \n"); unnecessary whitespace before a quoted newline
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33737
to look at the new patch set (#3).
Change subject: device/oprom: Add vbe return status support as per VBE spec 3.0 ......................................................................
device/oprom: Add vbe return status support as per VBE spec 3.0
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.
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4118 VBE: Function call invalid with unsupported video mode 0x118! User to select mode from below list - Supported Video Mode list for OpRom are: 0x110 0x111 0x113 0x114 0x116 0x117 0x119 0x11a 0x165 0x166 0x121 0x122 0x123 0x124 0x145 0x146 0x175 0x176 0x1d2 0x1d4
Error: In vbe_get_mode_info function
Case 2: with supported vesa mode 0x116
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4116 VBE: resolution: 1024x768@16 VBE: framebuffer: a0000000 VBE: Setting VESA mode 4116 VGA Option ROM was run
Change-Id: Iacd2ce468e038a14424f029df3a0adec3e5fa15c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/realmode/x86_asm.S M src/include/vbe.h 3 files changed, 105 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/33737/3
build bot (Jenkins) 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 3:
(1 comment)
https://review.coreboot.org/#/c/33737/3/src/device/oprom/realmode/x86.c File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/#/c/33737/3/src/device/oprom/realmode/x86.c@298 PS3, Line 298: printk(BIOS_DEBUG, "User to select mode from below list - \n"); unnecessary whitespace before a quoted newline
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33737
to look at the new patch set (#4).
Change subject: device/oprom: Add vbe return status support as per VBE spec 3.0 ......................................................................
device/oprom: Add vbe return status support as per VBE spec 3.0
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.
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4118 VBE: Function call invalid with unsupported video mode 0x118! User to select mode from below list - Supported Video Mode list for OpRom are: 0x110 0x111 0x113 0x114 0x116 0x117 0x119 0x11a 0x165 0x166 0x121 0x122 0x123 0x124 0x145 0x146 0x175 0x176 0x1d2 0x1d4
Error: In vbe_get_mode_info function
Case 2: with supported vesa mode 0x116
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4116 VBE: resolution: 1024x768@16 VBE: framebuffer: a0000000 VBE: Setting VESA mode 4116 VGA Option ROM was run
Change-Id: Iacd2ce468e038a14424f029df3a0adec3e5fa15c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/realmode/x86_asm.S M src/include/vbe.h 3 files changed, 103 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/33737/4
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33737
to look at the new patch set (#5).
Change subject: device/oprom: Add vbe return status support as per VBE spec 3.0 ......................................................................
device/oprom: Add vbe return status support as per VBE spec 3.0
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.
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4118 VBE: Function call invalid with unsupported video mode 0x118! User to select mode from below list - Supported Video Mode list for OpRom are: 0x110 0x111 0x113 0x114 0x116 0x117 0x119 0x11a 0x165 0x166 0x121 0x122 0x123 0x124 0x145 0x146 0x175 0x176 0x1d2 0x1d4
Error: In vbe_get_mode_info function
Case 2: with supported vesa mode 0x116
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4116 VBE: resolution: 1024x768@16 VBE: framebuffer: a0000000 VBE: Setting VESA mode 4116 VGA Option ROM was run
Change-Id: Iacd2ce468e038a14424f029df3a0adec3e5fa15c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/realmode/x86_asm.S M src/include/vbe.h 3 files changed, 102 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/33737/5
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
Subrata Banik 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 (.
Done
https://review.coreboot.org/#/c/33737/5//COMMIT_MSG@20 PS5, Line 20: list
lists
Done
https://review.coreboot.org/#/c/33737/5//COMMIT_MSG@20 PS5, Line 20: Function
function
Done
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.
Done
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.
Done
https://review.coreboot.org/#/c/33737/5//COMMIT_MSG@28 PS5, Line 28: user
users?
Done
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?
Done
https://review.coreboot.org/#/c/33737/5/src/device/oprom/realmode/x86.c@244 PS5, Line 244: are
Remove.
Done
https://review.coreboot.org/#/c/33737/5/src/device/oprom/realmode/x86.c@268 PS5, Line 268:
Remove
Done
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.
Done
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. […]
if we don't die here, it will eventually hang during depthcharge while displaying bmpblk as mentioned in commit msg as well.
Also i think this might be case with any other payload as well because with 0x0 resolution, things might not work.
https://review.coreboot.org/#/c/33737/5/src/device/oprom/realmode/x86.c@326 PS5, Line 326: /* request clearing of framebuffer */
Separate commit.
okay i will push one
https://review.coreboot.org/#/c/33737/5/src/include/vbe.h File src/include/vbe.h:
PS5:
Could be a separate commit.
i will submit another CL
https://review.coreboot.org/#/c/33737/5/src/include/vbe.h@18 PS5, Line 18: Lets
Let’s
Done
Hello Aaron Durbin, Duncan Laurie, Stefan Reinauer, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33737
to look at the new patch set (#6).
Change subject: device/oprom: Add vbe return status support as per VBE spec 3.0 ......................................................................
device/oprom: Add vbe return status support as per VBE spec 3.0
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 function (0x3 and 0x4f00/1/2) and lists 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 users know the failure rather going to depthcharge and debug further.
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4118 VBE: Function call invalid with unsupported video mode 0x118! User to select mode from below list - Supported Video Mode list for OpRom are: 0x110 0x111 0x113 0x114 0x116 0x117 0x119 0x11a 0x165 0x166 0x121 0x122 0x123 0x124 0x145 0x146 0x175 0x176 0x1d2 0x1d4
Error: In vbe_get_mode_info function
Case 2: with supported vesa mode 0x116
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4116 VBE: resolution: 1024x768@16 VBE: framebuffer: a0000000 VBE: Setting VESA mode 4116 VGA Option ROM was run
Change-Id: Iacd2ce468e038a14424f029df3a0adec3e5fa15c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/realmode/x86_asm.S 2 files changed, 94 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/33737/6
Subrata Banik has removed Furquan Shaikh from this change. ( https://review.coreboot.org/c/coreboot/+/33737 )
Change subject: device/oprom: Add vbe return status support as per VBE spec 3.0 ......................................................................
Removed reviewer Furquan Shaikh.
Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, Duncan Laurie, Stefan Reinauer, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33737
to look at the new patch set (#7).
Change subject: device/oprom: Add vbe return status support as per VBE spec 3.0 ......................................................................
device/oprom: Add vbe return status support as per VBE spec 3.0
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 function (0x3 and 0x4f00/1/2) and lists 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 users know the failure rather going to depthcharge and debug further.
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4118 VBE: Function call invalid with unsupported video mode 0x118! User to select mode from below list - Supported Video Mode list for OpRom are: 0x110 0x111 0x113 0x114 0x116 0x117 0x119 0x11a 0x165 0x166 0x121 0x122 0x123 0x124 0x145 0x146 0x175 0x176 0x1d2 0x1d4
Error: In vbe_get_mode_info function
Case 2: with supported vesa mode 0x116
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4116 VBE: resolution: 1024x768@16 VBE: framebuffer: a0000000 VBE: Setting VESA mode 4116 VGA Option ROM was run
Change-Id: Iacd2ce468e038a14424f029df3a0adec3e5fa15c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/realmode/x86_asm.S 2 files changed, 92 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/33737/7
Subrata Banik 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 7:
can someone review this CL?
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 7: Code-Review+1
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:
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
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.
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)
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++; I'm confused by this dereference. Isn't this a segment:offset pointer but we are in protected mode?
Subrata Banik 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)
@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.
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
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++;
I'm confused by this dereference. Isn't this a segment:offset […]
we have already copied it into buffer right ?
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)
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).
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.
Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, Paul Menzel, Duncan Laurie, Stefan Reinauer, build bot (Jenkins), Nico Huber, Furquan Shaikh, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33737
to look at the new patch set (#9).
Change subject: device/oprom: Add vbe return status support as per VBE spec 3.0 ......................................................................
device/oprom: Add vbe return status support as per VBE spec 3.0
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 function (0x3 and 0x4f00/1/2) and lists 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 users know the failure rather going to depthcharge and debug further.
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4118 VBE: Function call invalid with unsupported video mode 0x118! User to select mode from below list - Supported Video Mode list for OpRom are: 0x110 0x111 0x113 0x114 0x116 0x117 0x119 0x11a 0x165 0x166 0x121 0x122 0x123 0x124 0x145 0x146 0x175 0x176 0x1d2 0x1d4
Error: In vbe_get_mode_info function
Case 2: with supported vesa mode 0x116
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4116 VBE: resolution: 1024x768@16 VBE: framebuffer: a0000000 VBE: Setting VESA mode 4116 VGA Option ROM was run
Change-Id: Iacd2ce468e038a14424f029df3a0adec3e5fa15c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/realmode/x86_asm.S M src/include/vbe.h 3 files changed, 105 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/33737/9
Subrata Banik 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 9:
(1 comment)
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++;
What exactly are you referring to? the pointer? the array behind […]
here is the dump of return buffer from int 10, 0x4F00
dump(00000624, 104):
00000624: 56 45 53 41 00 03 58 02 VESA..X. 0000062c: 00 c0 01 00 00 00 68 06 ......h. 00000634: 00 00 00 03 32 0f 73 01 ....2.s. 0000063c: 00 c0 05 01 00 c0 18 48 .......H 00000644: 00 c0 00 00 00 00 00 00 ........ 0000064c: 00 00 00 00 00 00 00 00 ........ 00000654: 00 00 00 00 00 00 00 00 ........ 0000065c: 00 00 00 00 00 00 00 00 ........ 00000664: 00 00 00 00 10 01 11 01 ........ 0000066c: 13 01 14 01 16 01 17 01 ........ 00000674: 19 01 1a 01 65 01 66 01 ....e.f. 0000067c: 21 01 22 01 23 01 24 01 !.".#.$. 00000684: 45 01 46 01 75 01 76 01 E.F.u.v. 0000068c: d2 01 d4 01 ff ff 00 00 ........ 00000694: 00 00 00 00 00 00 00 00 ........ 0000069c: 00 00 00 00 00 00 00 00 ........ 000006a4: 00 00 00 00 00 00 00 00 ........ 000006ac: 00 00 00 00 00 00 00 00 ........ 000006b4: 00 00 00 00 00 00 00 00 ........ 000006bc: 00 00 00 00 00 00 00 00 ........ 000006c4: 00 00 00 00 00 00 00 00 ........ 000006cc: 00 00 00 00 00 00 00 00 ........ 000006d4: 00 00 00 00 00 00 00 00 ........ 000006dc: 00 00 00 00 00 00 00 00 ........ 000006e4: 00 00 00 00 00 00 00 00 ........ 000006ec: 00 00 00 00 00 00 00 00 ........ 000006f4: 00 00 00 00 00 00 00 00 ........ 000006fc: 00 00 00 00 00 00 00 00 ........ 00000704: 00 00 00 00 00 00 00 00 ........ 0000070c: 00 00 00 00 00 00 00 00 ........ 00000714: 00 00 00 00 00 00 00 00 ........ 0000071c: 00 00 00 00 00 00 00 00 ........ 00000724: 60 9c 89 25 `..%
now, i have copied this into vbe_info_t and info.video_mode_ptr refers at 0x00000668, so size of mode list is 16 bit
video_mode_ptr : 00000668 110 video_mode_ptr : 0000066a 111 video_mode_ptr : 0000066c 113 video_mode_ptr : 0000066e 114 video_mode_ptr : 00000670 116 video_mode_ptr : 00000672 117 video_mode_ptr : 00000674 119 video_mode_ptr : 00000676 11a video_mode_ptr : 00000678 165 video_mode_ptr : 0000067a 166 video_mode_ptr : 0000067c 121 video_mode_ptr : 0000067e 122 video_mode_ptr : 00000680 123 video_mode_ptr : 00000682 124 video_mode_ptr : 00000684 145 video_mode_ptr : 00000686 146 video_mode_ptr : 00000688 175 video_mode_ptr : 0000068a 176 video_mode_ptr : 0000068c 1d2 video_mode_ptr : 0000068e 1d4 video_mode_ptr : 00000690
is that clarifies what i'm referring at and what is getting incremented. When we are in protected mode, i have access to entire memory even its below 1 MB, and i can access that memory in 20 bit format. This is what been done here. to access those memory, i don't need to jump into real mode.
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 9:
(4 comments)
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++;
here is the dump of return buffer from int 10, 0x4F00 […]
Thanks for the dump, I think I understand it now. I'm not concerned about the location in memory, but more about the way the address (pointer) is represented. The VBIOS will write a 16-bit offset + 16-bit segment pointer. GCC, however, will expect a linear 32-bit pointer, afaict.
What I could have realized earlier is that we operate solely in segment 0 here. So the offset+segment pointer will match the linear pointer by coin- cidence. However, VBE also allows that the pointer points into ROM (the VBIOS code itself, see below), so we cannot make this assumption.
Maybe an example helps to understand the issue: `oem_string_ptr` in your dump is: 58 02 00 c0. These are two 16-bit numbers in little endian. The offset is 0x0258 and the segment 0xc000. The correct linear address is given by
(segment << 4) + offset
so in this case
(0xc000 << 4) + 0x0258 == 0xc0258
So this is pointing into the VBIOS ROM. You can try to dereference `oem_string_ptr` directly, I'm rather sure it won't work. Hence, the pointers inside the VbeInfoBlock have to be converted, before you can dereference them.
https://review.coreboot.org/c/coreboot/+/33737/9/src/device/oprom/realmode/x... File src/device/oprom/realmode/x86_asm.S:
https://review.coreboot.org/c/coreboot/+/33737/9/src/device/oprom/realmode/x... PS9, Line 299: mov %eax, REALMODE_BASE I don't understand this one: 1. When is the value restored? 2. Isn't our relocated __realmode_idt (see above) at this address?
https://review.coreboot.org/c/coreboot/+/33737/9/src/include/vbe.h File src/include/vbe.h:
https://review.coreboot.org/c/coreboot/+/33737/9/src/include/vbe.h@47 PS9, Line 47: } __packed vbe_info_t; You don't have to place it in a header file if it's going to be used only in a single C file. Also, the VBE spec calls this VbeInfoBlock, maybe `struct vbe_info_block` would be a better name and also avoid the name clash.
https://review.coreboot.org/c/coreboot/+/33737/9/src/include/vbe.h@59 PS9, Line 59: } vbe_info_t; This is also only used in `yabel/vbe.c`. I think it would be best to move both declarations into the respective C file to avoid confusion.
Subrata Banik 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 9:
(4 comments)
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++;
Thanks for the dump, I think I understand it now. I'm not concerned about […]
i will get back on this
https://review.coreboot.org/c/coreboot/+/33737/9/src/device/oprom/realmode/x... File src/device/oprom/realmode/x86_asm.S:
https://review.coreboot.org/c/coreboot/+/33737/9/src/device/oprom/realmode/x... PS9, Line 299: mov %eax, REALMODE_BASE
I don't understand this one: 1. When is the value restored? 2. Isn't […]
This is the return value from int 10, 0x4Fxx any function and value present in %al.
REALMODE_BASE is 0x600 which is application area in real mode. I can tell you the correct __realmode_idt address as well, its not 0x600. i need to see my log.
idea is to hold return value from function call into some memory adress which can exist between real/protected jump.
https://review.coreboot.org/c/coreboot/+/33737/9/src/include/vbe.h File src/include/vbe.h:
https://review.coreboot.org/c/coreboot/+/33737/9/src/include/vbe.h@47 PS9, Line 47: } __packed vbe_info_t;
You don't have to place it in a header file if it's going to be […]
thanks.
https://review.coreboot.org/c/coreboot/+/33737/9/src/include/vbe.h@59 PS9, Line 59: } vbe_info_t;
This is also only used in `yabel/vbe.c`. I think it would […]
thanks
Subrata Banik 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 9:
(1 comment)
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++;
i will get back on this
i have enable depthcharge console and dump 0xc0258 when i create 20 bit address as below.
dpch: mm.l 0xc0258 000c0258: 20444d41 ? 000c025c: 4d4f5441 ? 000c0260: 534f4942 ?
Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, Paul Menzel, Duncan Laurie, Stefan Reinauer, build bot (Jenkins), Nico Huber, Furquan Shaikh, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33737
to look at the new patch set (#10).
Change subject: device/oprom: Add vbe return status support as per VBE spec 3.0 ......................................................................
device/oprom: Add vbe return status support as per VBE spec 3.0
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 function (0x3 and 0x4f00/1/2) and lists 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 users know the failure rather going to depthcharge and debug further.
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4118 VBE: Function call invalid with unsupported video mode 0x118! User to select mode from below list - Supported Video Mode list for OpRom are: 0x110 0x111 0x113 0x114 0x116 0x117 0x119 0x11a 0x165 0x166 0x121 0x122 0x123 0x124 0x145 0x146 0x175 0x176 0x1d2 0x1d4
Error: In vbe_get_mode_info function
Case 2: with supported vesa mode 0x116
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 4116 VBE: resolution: 1024x768@16 VBE: framebuffer: a0000000 VBE: Setting VESA mode 4116 VGA Option ROM was run
Change-Id: Iacd2ce468e038a14424f029df3a0adec3e5fa15c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/realmode/x86_asm.S M src/device/oprom/yabel/vbe.c M src/include/vbe.h 4 files changed, 115 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/33737/10
Subrata Banik 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 10:
@nico, would you like to pay one more look, i know you don't like to review OpRom code but still, this CL is hanging for very long time and i need to get rid of this work
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 10:
(1 comment)
@nico, would you like to pay one more look, i know you don't like to review OpRom code but still, this CL is hanging for very long time and i need to get rid of this work
I already looked at the current patch set yesterday. You didn't fix the address problem I tried to explain. If you need more help to understand the problem, I fear you have to ask another reviewer.
https://review.coreboot.org/c/coreboot/+/33737/9/src/device/oprom/realmode/x... File src/device/oprom/realmode/x86_asm.S:
https://review.coreboot.org/c/coreboot/+/33737/9/src/device/oprom/realmode/x... PS9, Line 299: mov %eax, REALMODE_BASE
This is the return value from int 10, 0x4Fxx any function and value present in %al. […]
Whatever there is at that address, you write to REALMODE_BASE here and I'm asking when is the value stored there read?
Subrata Banik 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 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33737/9/src/device/oprom/realmode/x... File src/device/oprom/realmode/x86_asm.S:
https://review.coreboot.org/c/coreboot/+/33737/9/src/device/oprom/realmode/x... PS9, Line 299: mov %eax, REALMODE_BASE
I'm asking when is the value stored there read?
it will read by caller function after int10h been served
Subrata Banik 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 10:
Did you mean do like below for video_mode_ptr as well ?
(segment << 4) + offset so in this case
(0x0000 << 4) + 0x0668 == 0x00668
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 10:
(1 comment)
Yes. Preferably before the conversion to `u32 *`.
https://review.coreboot.org/c/coreboot/+/33737/9/src/device/oprom/realmode/x... File src/device/oprom/realmode/x86_asm.S:
https://review.coreboot.org/c/coreboot/+/33737/9/src/device/oprom/realmode/x... PS9, Line 299: mov %eax, REALMODE_BASE
I'm asking when is the value stored there read? […]
I see. But why don't you make it a proper return value of the realmode_interrupt() function? Store it away, then restore it before `ret`?
I still see a conflict with __realmode_idt:
$ objdump -t build/cbfs/fallback/ramstage.debug | grep __realmode_idt 00000600 l *ABS* 00000000 __realmode_idt
It would be better to declare a proper symbol for it, how about
__realmode_ret = RELOCATED(.) .long 0
Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, Paul Menzel, Duncan Laurie, Stefan Reinauer, build bot (Jenkins), Nico Huber, Furquan Shaikh, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33737
to look at the new patch set (#11).
Change subject: device/oprom/realmode: Add vbe return status support as per VBE spec 3.0 ......................................................................
device/oprom/realmode: Add vbe return status support as per VBE spec 3.0
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 AH in all vbe function (0x3 and 0x4f00/1/2) and die() if returns error.
Change-Id: Iacd2ce468e038a14424f029df3a0adec3e5fa15c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/realmode/x86.h M src/device/oprom/realmode/x86_asm.S 3 files changed, 79 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/33737/11
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33737 )
Change subject: device/oprom/realmode: Add vbe return status support as per VBE spec 3.0 ......................................................................
Patch Set 11:
yes, taken care
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33737 )
Change subject: device/oprom/realmode: Add vbe return status support as per VBE spec 3.0 ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33737/9/src/device/oprom/realmode/x... File src/device/oprom/realmode/x86_asm.S:
https://review.coreboot.org/c/coreboot/+/33737/9/src/device/oprom/realmode/x... PS9, Line 299: mov %eax, REALMODE_BASE
I see. But why don't you make it a proper return value of the […]
yes, very nice point
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33737 )
Change subject: device/oprom/realmode: Add vbe return status support as per VBE spec 3.0 ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33737/9/src/device/oprom/realmode/x... File src/device/oprom/realmode/x86_asm.S:
https://review.coreboot.org/c/coreboot/+/33737/9/src/device/oprom/realmode/x... PS9, Line 299: mov %eax, REALMODE_BASE
yes, very nice point
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33737 )
Change subject: device/oprom/realmode: Add vbe return status support as per VBE spec 3.0 ......................................................................
Patch Set 11:
can we have any further review comment ?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33737 )
Change subject: device/oprom/realmode: Add vbe return status support as per VBE spec 3.0 ......................................................................
Patch Set 12: Code-Review+1
(2 comments)
A comment is lagging behind somewhat, otherwise I think this is fine.
https://review.coreboot.org/c/coreboot/+/33737/12/src/device/oprom/realmode/... File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/c/coreboot/+/33737/12/src/device/oprom/realmode/... PS12, Line 227: * In x86_asm.S, store EAX into __realmode_ret before jumping into protected I don't think that explanation is still necessary here since __realmode_ret etc is transparent to the realmode_* user.
https://review.coreboot.org/c/coreboot/+/33737/12/src/device/oprom/realmode/... PS12, Line 235: Functio Function
Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, Paul Menzel, Duncan Laurie, Stefan Reinauer, build bot (Jenkins), Nico Huber, Patrick Georgi, Furquan Shaikh, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33737
to look at the new patch set (#13).
Change subject: device/oprom/realmode: Add vbe return status support as per VBE spec 3.0 ......................................................................
device/oprom/realmode: Add vbe return status support as per VBE spec 3.0
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 AH in all vbe function (0x3 and 0x4f00/1/2) and die() if returns error.
Change-Id: Iacd2ce468e038a14424f029df3a0adec3e5fa15c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/realmode/x86.h M src/device/oprom/realmode/x86_asm.S 3 files changed, 78 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/33737/13
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33737 )
Change subject: device/oprom/realmode: Add vbe return status support as per VBE spec 3.0 ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33737/12/src/device/oprom/realmode/... File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/c/coreboot/+/33737/12/src/device/oprom/realmode/... PS12, Line 227: * In x86_asm.S, store EAX into __realmode_ret before jumping into protected
I don't think that explanation is still necessary here since __realmode_ret etc is transparent to th […]
Done
https://review.coreboot.org/c/coreboot/+/33737/12/src/device/oprom/realmode/... PS12, Line 235: Functio
Function
Done
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33737 )
Change subject: device/oprom/realmode: Add vbe return status support as per VBE spec 3.0 ......................................................................
Patch Set 13: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33737 )
Change subject: device/oprom/realmode: Add vbe return status support as per VBE spec 3.0 ......................................................................
Patch Set 13:
(6 comments)
https://review.coreboot.org/c/coreboot/+/33737/5/src/device/oprom/realmode/x... File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/c/coreboot/+/33737/5/src/device/oprom/realmode/x... PS5, Line 315: die("\nError: In %s function\n", __func__);
if we don't die here, it will eventually hang during depthcharge while displaying bmpblk as mentione […]
Done
https://review.coreboot.org/c/coreboot/+/33737/5/src/device/oprom/realmode/x... PS5, Line 326: /* request clearing of framebuffer */
okay i will push one
Done
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++;
i have enable depthcharge console and dump 0xc0258 when i create 20 bit address as below. […]
The follow up commit properly handles segment and offset now (plus, it's in the follow up, with no bearing on this commit anymore)
https://review.coreboot.org/c/coreboot/+/33737/9/src/include/vbe.h File src/include/vbe.h:
https://review.coreboot.org/c/coreboot/+/33737/9/src/include/vbe.h@47 PS9, Line 47: } __packed vbe_info_t;
thanks.
Done
https://review.coreboot.org/c/coreboot/+/33737/9/src/include/vbe.h@59 PS9, Line 59: } vbe_info_t;
thanks
Done
https://review.coreboot.org/c/coreboot/+/33737/5/src/include/vbe.h File src/include/vbe.h:
PS5:
i will submit another CL
Done
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33737 )
Change subject: device/oprom/realmode: Add vbe return status support as per VBE spec 3.0 ......................................................................
device/oprom/realmode: Add vbe return status support as per VBE spec 3.0
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 AH in all vbe function (0x3 and 0x4f00/1/2) and die() if returns error.
Change-Id: Iacd2ce468e038a14424f029df3a0adec3e5fa15c Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33737 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Georgi pgeorgi@google.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/realmode/x86.h M src/device/oprom/realmode/x86_asm.S 3 files changed, 78 insertions(+), 11 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved
diff --git a/src/device/oprom/realmode/x86.c b/src/device/oprom/realmode/x86.c index a7631a1..bf31bab 100644 --- a/src/device/oprom/realmode/x86.c +++ b/src/device/oprom/realmode/x86.c @@ -50,11 +50,11 @@ /* to have a common register file for interrupt handlers */ X86EMU_sysEnv _X86EMU_env;
-void (*realmode_call)(u32 addr, u32 eax, u32 ebx, u32 ecx, u32 edx, +unsigned int (*realmode_call)(u32 addr, u32 eax, u32 ebx, u32 ecx, u32 edx, u32 esi, u32 edi) asmlinkage;
-void (*realmode_interrupt)(u32 intno, u32 eax, u32 ebx, u32 ecx, u32 edx, - u32 esi, u32 edi) asmlinkage; +unsigned int (*realmode_interrupt)(u32 intno, u32 eax, u32 ebx, u32 ecx, + u32 edx, u32 esi, u32 edi) asmlinkage;
static void setup_realmode_code(void) { @@ -221,6 +221,46 @@ return mode_info_valid; }
+/* + * EAX register is used to indicate the completion status upon return from + * VBE function in real mode. + * + * If the VBE function completed successfully then 0x0 is returned in the AH + * register. Otherwise the AH register is set with the nature of the failure: + * + * AH == 0x00: Function call successful + * AH == 0x01: Function call failed + * AH == 0x02: Function is not supported in the current HW configuration + * AH == 0x03: Function call invalid in current video mode + * + * Return 0 on success else -1 for failure + */ +static int vbe_check_for_failure(int ah) +{ + int status; + + switch (ah) { + case 0x0: + status = 0; + break; + case 1: + printk(BIOS_DEBUG, "VBE: Function call failed!\n"); + status = -1; + break; + case 2: + printk(BIOS_DEBUG, "VBE: Function is not supported!\n"); + status = -1; + break; + case 3: + default: + printk(BIOS_DEBUG, "VBE: Unsupported video mode %x!\n", + CONFIG_FRAMEBUFFER_VESA_MODE); + status = -1; + break; + } + + return status; +} static u8 vbe_get_mode_info(vbe_mode_info_t * mi) { printk(BIOS_DEBUG, "VBE: Getting information about VESA mode %04x\n", @@ -228,8 +268,10 @@ char *buffer = PTR_TO_REAL_MODE(__realmode_buffer); u16 buffer_seg = (((unsigned long)buffer) >> 4) & 0xff00; u16 buffer_adr = ((unsigned long)buffer) & 0xffff; - realmode_interrupt(0x10, VESA_GET_MODE_INFO, 0x0000, + X86_EAX = realmode_interrupt(0x10, VESA_GET_MODE_INFO, 0x0000, mi->video_mode, 0x0000, buffer_seg, buffer_adr); + if (vbe_check_for_failure(X86_AH)) + die("\nError: In %s function\n", __func__); memcpy(mi->mode_info_block, buffer, sizeof(mi->mode_info_block)); mode_info_valid = 1; return 0; @@ -242,8 +284,10 @@ mi->video_mode |= (1 << 14); // request clearing of framebuffer mi->video_mode &= ~(1 << 15); - realmode_interrupt(0x10, VESA_SET_MODE, mi->video_mode, + X86_EAX = realmode_interrupt(0x10, VESA_SET_MODE, mi->video_mode, 0x0000, 0x0000, 0x0000, 0x0000); + if (vbe_check_for_failure(X86_AH)) + die("\nError: In %s function\n", __func__); return 0; }
@@ -286,8 +330,10 @@ void vbe_textmode_console(void) { delay(2); - realmode_interrupt(0x10, 0x0003, 0x0000, 0x0000, + X86_EAX = realmode_interrupt(0x10, 0x0003, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000); + if (vbe_check_for_failure(X86_AH)) + die("\nError: In %s function\n", __func__); }
int fill_lb_framebuffer(struct lb_framebuffer *framebuffer) diff --git a/src/device/oprom/realmode/x86.h b/src/device/oprom/realmode/x86.h index b8cc02a..052c9c0 100644 --- a/src/device/oprom/realmode/x86.h +++ b/src/device/oprom/realmode/x86.h @@ -33,11 +33,11 @@ extern unsigned char __realmode_code; extern unsigned int __realmode_code_size;
-extern void (*realmode_call)(u32 addr, u32 eax, u32 ebx, u32 ecx, u32 edx, - u32 esi, u32 edi) asmlinkage; +extern unsigned int (*realmode_call)(u32 addr, u32 eax, u32 ebx, u32 ecx, + u32 edx, u32 esi, u32 edi) asmlinkage;
-extern void (*realmode_interrupt)(u32 intno, u32 eax, u32 ebx, u32 ecx, u32 edx, - u32 esi, u32 edi) asmlinkage; +extern unsigned int (*realmode_interrupt)(u32 intno, u32 eax, u32 ebx, u32 ecx, + u32 edx, u32 esi, u32 edi) asmlinkage;
#define FAKE_MEMORY_SIZE (1024*1024) // only 1MB #define INITIAL_EBDA_SEGMENT 0xF600 diff --git a/src/device/oprom/realmode/x86_asm.S b/src/device/oprom/realmode/x86_asm.S index 87348cd..ec82e53 100644 --- a/src/device/oprom/realmode/x86_asm.S +++ b/src/device/oprom/realmode/x86_asm.S @@ -43,6 +43,10 @@ .globl __realmode_code __realmode_code:
+/* Realmode function return. */ +__realmode_ret = RELOCATED(.) + .long 0 + /* Realmode IDT pointer structure. */ __realmode_idt = RELOCATED(.) .word 1023 /* 16 bit limit */ @@ -167,6 +171,13 @@ .word 0x0000, 0x0000 /* ************************************ */
+ /* + * Here is end of real mode call and time to go back to protected mode. + * Before that its better to store current eax into some memory address + * so that context persist in protected mode too. + */ + mov %eax, __realmode_ret + /* If we got here, we are just about done. * Need to get back to protected mode. */ @@ -196,7 +207,8 @@ popa
/* and exit */ - // TODO return AX from OPROM call + /* return AX from OPROM call */ + mov __realmode_ret, %eax ret
.globl __realmode_interrupt @@ -291,6 +303,13 @@ __intXX_instr = RELOCATED(.) .byte 0xcd, 0x00 /* This becomes intXX */
+ /* + * Here is end of real mode call and time to go back to protected mode. + * Before that its better to store current eax into some memory address + * so that context persist in protected mode too. + */ + mov %eax, __realmode_ret + /* Ok, the job is done, now go back to protected mode coreboot */ movl %cr0, %eax orl $PE, %eax @@ -314,6 +333,8 @@ movl __stack, %esp popf popa + /* return AX from OPROM call */ + mov __realmode_ret, %eax ret
/* This is the 16-bit interrupt entry point called by the IDT stub code.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33737 )
Change subject: device/oprom/realmode: Add vbe return status support as per VBE spec 3.0 ......................................................................
Patch Set 14:
(3 comments)
Sorry for running off, I thought a little distance would be good for the moment. Just leaving some questions and thoughts. As I don't use this code, I don't care too much.
https://review.coreboot.org/c/coreboot/+/33737/14/src/device/oprom/realmode/... File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/c/coreboot/+/33737/14/src/device/oprom/realmode/... PS14, Line 255: default: I'm not sure about the `default`, if VBE ever gets extended or some vendor uses a non-standard code, why would we print the unsupported- mode message?
https://review.coreboot.org/c/coreboot/+/33737/14/src/device/oprom/realmode/... PS14, Line 271: X86_EAX Why use the register file here? wouldn't a local variable be enough? or is this communicating the value elsewhere?
https://review.coreboot.org/c/coreboot/+/33737/14/src/device/oprom/realmode/... PS14, Line 274: die("\nError: In %s function\n", __func__); I don't think we should die(), modesetting errors don't seem generally fatal?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33737 )
Change subject: device/oprom/realmode: Add vbe return status support as per VBE spec 3.0 ......................................................................
Patch Set 14:
(3 comments)
https://review.coreboot.org/c/coreboot/+/33737/14/src/device/oprom/realmode/... File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/c/coreboot/+/33737/14/src/device/oprom/realmode/... PS14, Line 255: default:
I'm not sure about the `default`, if VBE ever gets extended or some […]
It's unlikely that anybody will ever release a new revision of VBE. PCBIOS is dead. As for non-standard codes (or retro-computing fans who might extend VBE), let's go there when we see them - maybe they'd give us a license for Scitech Display Doctor, too.
https://review.coreboot.org/c/coreboot/+/33737/14/src/device/oprom/realmode/... PS14, Line 271: X86_EAX
Why use the register file here? wouldn't a local variable be enough? or […]
I found that rather neat in terms of "that happened, it had an impact on realmode, so make the realmode impact consistent". Overall it won't matter much though, since users will mostly set their registers the way they need them before a call.
So in general it's a pattern that might be interesting to promote in this subsystem, but practically speaking, this subsystem is at the end of its lifetime, and it's hard for me to care about such matters of taste in that case.
https://review.coreboot.org/c/coreboot/+/33737/14/src/device/oprom/realmode/... PS14, Line 274: die("\nError: In %s function\n", __func__);
I don't think we should die(), modesetting errors don't seem generally fatal?
So calling into unknown binaries is okay (VGABIOS hanging was a classic for me, especially pre-YABEL), but complaining loudly when they return an error is not?
It's unlikely that these functions will only sometimes fail (for a given state or input). Under that assumption die() helps with debugging before shipping releases. Again it's a matter of taste (and I might have done it differently, too).
-
build bot (Jenkins) (Code Review) -
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)