Change in ...coreboot[master]: device/oprom: Update vbe_info_t as per vbe specification 3.0
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33806
Change subject: device/oprom: Update vbe_info_t as per vbe specification 3.0 ......................................................................
device/oprom: Update vbe_info_t as per vbe specification 3.0
This patch fixes unable to get correct value from TotalMemory variable using existing vbe_info_t structure.
New vbe_info_t structure follows vbe spec 3.0. http://www.vesa.org/public/VBE/vbe3.pdf
Change-Id: I12db01280aec480f1f42332ebe97d92539dfdc1c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/oprom/yabel/vbe.c M src/include/vbe.h 2 files changed, 16 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/33806/1
diff --git a/src/device/oprom/yabel/vbe.c b/src/device/oprom/yabel/vbe.c index 682bf00..104b4c2 100644 --- a/src/device/oprom/yabel/vbe.c +++ b/src/device/oprom/yabel/vbe.c @@ -66,6 +66,9 @@ u8 *biosmem; u32 biosmem_size;
+/* Let's hope we never have more than 256 video modes. */ +#define MAX_VBE_FRAMEBUFFER_MODE 256 + #if CONFIG(FRAMEBUFFER_SET_VESA_MODE) static inline u8 vbe_prepare(void) @@ -133,20 +136,9 @@ info->capabilities = in32le(vbe_info_buffer + 10);
// offset 14: 32 bit le containing segment:offset of supported video mode table - u16 *video_mode_ptr; - video_mode_ptr = - (u16 *) (biosmem + - ((in16le(vbe_info_buffer + 16) << 4) + - in16le(vbe_info_buffer + 14))); - u32 i = 0; - do { - info->video_mode_list[i] = in16le(video_mode_ptr + i); - i++; - } - while ((i < - (sizeof(info->video_mode_list) / - sizeof(info->video_mode_list[0]))) - && (info->video_mode_list[i - 1] != 0xFFFF)); + info->video_mode_ptr = + (u32 *)(biosmem + ((in16le(vbe_info_buffer + 16) << 4) + + in16le(vbe_info_buffer + 14)));
//offset 18: 16bit le total memory in 64KB blocks info->total_memory = in16le(vbe_info_buffer + 18); @@ -441,7 +433,7 @@ vbe_get_info(void) { u8 rval; - int i; + int i = 0;
// XXX FIXME these need to be filled with sane values
@@ -536,12 +528,17 @@ DEBUG_PRINTF_VBE("DDC: found display type %d\n", output->display_type); memcpy(output->edid_block_zero, ddc_info.edid_block_zero, sizeof(ddc_info.edid_block_zero)); - i = 0; vbe_mode_info_t mode_info; vbe_mode_info_t best_mode_info; + u16 video_mode_list[MAX_VBE_FRAMEBUFFER_MODE]; // initialize best_mode to 0 memset(&best_mode_info, 0, sizeof(best_mode_info)); - while ((mode_info.video_mode = info.video_mode_list[i]) != 0xFFFF) { + do { + video_mode_list[i] = in16le(info.video_mode_ptr + i); + i++; + } while (video_mode_list[i - 1] != 0xFFFF); + i = 0; + while ((mode_info.video_mode = video_mode_list[i]) != 0xFFFF) { //DEBUG_PRINTF_VBE("%x: Mode: %04x\n", i, mode_info.video_mode); vbe_get_mode_info(&mode_info);
diff --git a/src/include/vbe.h b/src/include/vbe.h index 2aa45a8..af7bfa1 100644 --- a/src/include/vbe.h +++ b/src/include/vbe.h @@ -41,9 +41,9 @@ u16 version; u8 *oem_string_ptr; u32 capabilities; - u16 video_mode_list[256]; // lets hope we never have more than - // 256 video modes... + u32 *video_mode_ptr; u16 total_memory; + char reserved[236]; } __packed vbe_info_t;
typedef struct {
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33806 )
Change subject: device/oprom: Update vbe_info_t as per vbe specification 3.0 ......................................................................
Patch Set 2:
(1 comment)
I don't see why this change is needed.
https://review.coreboot.org/c/coreboot/+/33806/2/src/device/oprom/yabel/vbe.... File src/device/oprom/yabel/vbe.c:
https://review.coreboot.org/c/coreboot/+/33806/2/src/device/oprom/yabel/vbe.... PS2, Line 143: I see you are deferring this conversion to vbe_get_info() below. But why?
It seems the original code did all conversions here in a common place, using `vbe_info_t` as an interface to later steps that won't require any conversion. Please maintain such coherences.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33806 )
Change subject: device/oprom: Update vbe_info_t as per vbe specification 3.0 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33806/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33806/2//COMMIT_MSG@13 PS2, Line 13: http://www.vesa.org/public/VBE/vbe3.pdf The whole code is written such that `vbe_info_t` isn't (and doesn't have to be) a direct representation of the ABI structure. That's good programming practice, please don't change that.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33806 )
Change subject: device/oprom: Update vbe_info_t as per vbe specification 3.0 ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33806/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33806/2//COMMIT_MSG@13 PS2, Line 13: http://www.vesa.org/public/VBE/vbe3.pdf
The whole code is written such that `vbe_info_t` isn't (and doesn't […]
vbe_info_t structure has existing bug, Please refer to comment from Arthur as well.
https://review.coreboot.org/c/coreboot/+/33782/1/src/include/vbe.h#48
this structure was not compliant to vbe spec. By approach here to make it proper.
I can achieve my purpose without changing this code but then i will keep existing bug open where current vbe_info_t structure can't provide correct total_memory value.
https://review.coreboot.org/c/coreboot/+/33806/2/src/device/oprom/yabel/vbe.... File src/device/oprom/yabel/vbe.c:
https://review.coreboot.org/c/coreboot/+/33806/2/src/device/oprom/yabel/vbe.... PS2, Line 143:
I see you are deferring this conversion to vbe_get_info() below. But why? […]
This logic developed based on wrong vbe_info_t structure which is not complaint to spec
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33806 )
Change subject: device/oprom: Update vbe_info_t as per vbe specification 3.0 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33806/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33806/2//COMMIT_MSG@13 PS2, Line 13: http://www.vesa.org/public/VBE/vbe3.pdf
vbe_info_t structure has existing bug, Please refer to comment from Arthur as well.
https://review.coreboot.org/c/coreboot/+/33782/1/src/include/vbe.h#48
Arthur wrote: "If you you memcpy the return value buffer into this struct, the total_memory value is not correct.". He was referring to your patch. The existing code does not use memcpy() that way, you did. The existing code looks fine.
this structure was not compliant to vbe spec.
Yes, on purpose.
By approach here to make it proper.
I think it was proper already.
I can achieve my purpose without changing this code but then i will keep existing bug open where current vbe_info_t structure can't provide correct total_memory value.
There is no existing bug. See above.
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33806 )
Change subject: device/oprom: Update vbe_info_t as per vbe specification 3.0 ......................................................................
Abandoned
-
Nico Huber (Code Review) -
Subrata Banik (Code Review)