Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: [WIP]lib/edid_fill_fb: Support multiple framebuffers ......................................................................
[WIP]lib/edid_fill_fb: Support multiple framebuffers
Add an implementation that supports multiple framebuffers. This will allow to compile in multiple graphic drivers and select at runtime which one to choose.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 174 insertions(+), 64 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/1
diff --git a/src/include/edid.h b/src/include/edid.h index a97b99b..7c3e9c8 100644 --- a/src/include/edid.h +++ b/src/include/edid.h @@ -104,11 +104,12 @@ EDID_ABSENT, };
+struct edid_fb_info; /* Defined in src/lib/edid.c */ int decode_edid(unsigned char *edid, int size, struct edid *out); void edid_set_framebuffer_bits_per_pixel(struct edid *edid, int fb_bpp, int row_byte_alignment); -void set_vbe_mode_info_valid(const struct edid *edid, uintptr_t fb_addr); +struct edid_fb_info *set_vbe_mode_info_valid(const struct edid *edid, uintptr_t fb_addr); void set_vbe_framebuffer_orientation(enum lb_fb_orientation orientation); int set_display_mode(struct edid *edid, enum edid_modes mode);
diff --git a/src/include/framebuffer_info.h b/src/include/framebuffer_info.h new file mode 100644 index 0000000..6d2f9a6 --- /dev/null +++ b/src/include/framebuffer_info.h @@ -0,0 +1,45 @@ +/* + * Copyright 2013 Google Inc. + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but without any warranty; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef __FRAMEBUFFER_INFO_H_ +#define __FRAMEBUFFER_INFO_H_ + +#include <stdint.h> +#include <stdbool.h> +#include <commonlib/coreboot_tables.h> + +struct edid_fb_info; +struct edid_fb_info *fb_new_framebuffer_info(void); + +bool fb_fill_framebuffer_info_ex(struct edid_fb_info *info, + uintptr_t fb_addr, uint32_t x_resolution, + uint32_t y_resolution, uint32_t bytes_per_line, + uint8_t bits_per_pixel, uint8_t reserved_mask_pos, + uint8_t reserved_mask_size, uint8_t red_mask_pos, + uint8_t red_mask_size, uint8_t green_mask_pos, + uint8_t green_mask_size, uint8_t blue_mask_pos, + uint8_t blue_mask_size); + +bool fb_fill_framebuffer_info(struct edid_fb_info *info, + uintptr_t fb_addr, uint32_t x_resolution, + uint32_t y_resolution, uint32_t bytes_per_line, + uint8_t bits_per_pixel); + +void fb_fill_framebuffer_set_orientation(struct edid_fb_info *info, + enum lb_fb_orientation orientation); + +#endif /* __FRAMEBUFFER_INFO_H_ */ diff --git a/src/lib/edid_fill_fb.c b/src/lib/edid_fill_fb.c index 1b38ead..3abcdd0 100644 --- a/src/lib/edid_fill_fb.c +++ b/src/lib/edid_fill_fb.c @@ -23,75 +23,137 @@ #include <console/console.h> #include <edid.h> #include <boot/coreboot_tables.h> +#include <framebuffer_info.h> +#include <string.h> +#include <stdlib.h>
-static int fb_valid; -static struct lb_framebuffer edid_fb; +struct edid_fb_info { + bool valid; + struct lb_framebuffer edid_fb; + struct edid_fb_info *next; +}; +static struct edid_fb_info *list; + +/* + * Allocate a new framebuffer info struct on heap. + * Returns NULL on error. + */ +struct edid_fb_info *fb_new_framebuffer_info(void) +{ + struct edid_fb_info *ret; + ret = malloc(sizeof(struct edid_fb_info)); + if (ret) { + memset(ret, 0, sizeof(struct edid_fb_info)); + ret->next = list; + list = ret; + } + return ret; +} + +/* + * Fills a provided framebuffer info struct and sets the valid bit to true. + * Returns false on error. + */ +bool fb_fill_framebuffer_info_ex(struct edid_fb_info *info, + uintptr_t fb_addr, uint32_t x_resolution, + uint32_t y_resolution, uint32_t bytes_per_line, + uint8_t bits_per_pixel, uint8_t reserved_mask_pos, + uint8_t reserved_mask_size, uint8_t red_mask_pos, + uint8_t red_mask_size, uint8_t green_mask_pos, + uint8_t green_mask_size, uint8_t blue_mask_pos, + uint8_t blue_mask_size) +{ + /* Validate */ + if (!info) + return false; + + if (!x_resolution || !y_resolution || !bytes_per_line || !bits_per_pixel) { + printk(BIOS_ERR, "%s: Invalid framebuffer data provided\n", __func__); + return false; + } + + printk(BIOS_INFO, "framebuffer_info: bytes_per_line: %d, bits_per_pixel: %d\n " + " x_res x y_res: %d x %d, size: %d at %p\n", + bytes_per_line, bits_per_pixel, x_resolution, + y_resolution, (bytes_per_line * y_resolution), + (void *)fb_addr); + + /* Update */ + info->edid_fb.physical_address = fb_addr; + info->edid_fb.x_resolution = x_resolution; + info->edid_fb.y_resolution = y_resolution; + info->edid_fb.bytes_per_line = bytes_per_line; + info->edid_fb.bits_per_pixel = bits_per_pixel; + info->edid_fb.reserved_mask_pos = reserved_mask_pos; + info->edid_fb.reserved_mask_size = reserved_mask_size; + info->edid_fb.red_mask_pos = red_mask_pos; + info->edid_fb.red_mask_size = red_mask_size; + info->edid_fb.green_mask_pos = green_mask_pos; + info->edid_fb.green_mask_size = green_mask_size; + info->edid_fb.blue_mask_pos = blue_mask_pos; + info->edid_fb.blue_mask_size = blue_mask_size; + info->valid = true; + + return true; +} + +/* Fills a framebuffer into struct with common values for 32/24/16bpp framebuffers */ +bool fb_fill_framebuffer_info(struct edid_fb_info *info, + uintptr_t fb_addr, uint32_t x_resolution, + uint32_t y_resolution, uint32_t bytes_per_line, + uint8_t bits_per_pixel) +{ + if (!info) + return false; + + switch (bits_per_pixel) { + case 32: + case 24: + /* packed into 4-byte words */ + return fb_fill_framebuffer_info_ex(info, fb_addr, x_resolution, y_resolution, + bytes_per_line, bits_per_pixel, 24, 8, 16, 8, 8, 8, 0, 8); + break; + case 16: + /* packed into 2-byte words */ + return fb_fill_framebuffer_info_ex(info, fb_addr, x_resolution, y_resolution, + bytes_per_line, bits_per_pixel, 0, 0, 11, 5, 5, 6, 0, 5); + break; + default: + printk(BIOS_SPEW, "%s: unsupported BPP %d\n", __func__, bits_per_pixel); + return false; + } + return true; +} + +void fb_fill_framebuffer_set_orientation(struct edid_fb_info *info, + enum lb_fb_orientation orientation) +{ + if (!info) + return; + + info->edid_fb.orientation = orientation; +}
/* * Take an edid, and create a framebuffer. Set fb_valid to 1. */ -void set_vbe_mode_info_valid(const struct edid *edid, uintptr_t fb_addr) +struct edid_fb_info *set_vbe_mode_info_valid(const struct edid *edid, uintptr_t fb_addr) { - edid_fb.physical_address = fb_addr; - edid_fb.x_resolution = edid->x_resolution; - edid_fb.y_resolution = edid->y_resolution; - edid_fb.bytes_per_line = edid->bytes_per_line; - /* In the case of (e.g.) 24 framebuffer bits per pixel, the convention - * nowadays seems to be to round it up to the nearest reasonable - * boundary, because otherwise the byte-packing is hideous. - * So, for example, in RGB with no alpha, the bytes are still - * packed into 32-bit words, the so-called 32bpp-no-alpha mode. - * Or, in 5:6:5 mode, the bytes are also packed into 32-bit words, - * and in 4:4:4 mode, they are packed into 16-bit words. - * Good call on the hardware guys part. - * It's not clear we're covering all cases here, but - * I'm not sure with grahpics you ever can. - */ - edid_fb.bits_per_pixel = edid->framebuffer_bits_per_pixel; - edid_fb.reserved_mask_pos = 0; - edid_fb.reserved_mask_size = 0; - switch (edid->framebuffer_bits_per_pixel) { - case 32: - case 24: - /* packed into 4-byte words */ - edid_fb.reserved_mask_pos = 24; - edid_fb.reserved_mask_size = 8; - edid_fb.red_mask_pos = 16; - edid_fb.red_mask_size = 8; - edid_fb.green_mask_pos = 8; - edid_fb.green_mask_size = 8; - edid_fb.blue_mask_pos = 0; - edid_fb.blue_mask_size = 8; - break; - case 16: - /* packed into 2-byte words */ - edid_fb.red_mask_pos = 11; - edid_fb.red_mask_size = 5; - edid_fb.green_mask_pos = 5; - edid_fb.green_mask_size = 6; - edid_fb.blue_mask_pos = 0; - edid_fb.blue_mask_size = 5; - break; - default: - printk(BIOS_SPEW, "%s: unsupported BPP %d\n", __func__, - edid->framebuffer_bits_per_pixel); - return; - } - - fb_valid = 1; -} - -void set_vbe_framebuffer_orientation(enum lb_fb_orientation orientation) -{ - edid_fb.orientation = orientation; + struct edid_fb_info *info = fb_new_framebuffer_info(); + if (!info) + return NULL; + fb_fill_framebuffer_info(info, fb_addr, edid->x_resolution, edid->y_resolution, + edid->bytes_per_line, edid->framebuffer_bits_per_pixel); + return info; }
int fill_lb_framebuffer(struct lb_framebuffer *framebuffer) { - if (!fb_valid) - return -1; - - *framebuffer = edid_fb; - - return 0; + for (struct edid_fb_info *i = list; i != NULL; i = i->next) { + if (i->valid) { + *framebuffer = i->edid_fb; + return 0; + } + } + return -1; } diff --git a/src/mainboard/google/kukui/mainboard.c b/src/mainboard/google/kukui/mainboard.c index 844496d..4033d51 100644 --- a/src/mainboard/google/kukui/mainboard.c +++ b/src/mainboard/google/kukui/mainboard.c @@ -172,8 +172,10 @@ return false; } mtk_ddp_mode_set(edid); - set_vbe_mode_info_valid(edid, 0); - set_vbe_framebuffer_orientation(panel->s->orientation); + struct edid_fb_info *info = set_vbe_mode_info_valid(edid, 0); + if (info) + fb_fill_framebuffer_set_orientation(info, panel->s->orientation); + return true; }
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#2).
Change subject: [WIP]lib/edid_fill_fb: Support multiple framebuffers ......................................................................
[WIP]lib/edid_fill_fb: Support multiple framebuffers
Add an implementation that supports multiple framebuffers.
This will allow to compile in multiple graphic drivers and select at runtime which one to choose. As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 175 insertions(+), 64 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: [WIP]lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39002/2/src/include/framebuffer_inf... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39002/2/src/include/framebuffer_inf... PS2, Line 25: edid_ EDID is about the modeline (hardware interface) while the framebuffer is just software configuration. I'd try to get rid of it in all the identifiers.
https://review.coreboot.org/c/coreboot/+/39002/2/src/include/framebuffer_inf... PS2, Line 26: struct edid_fb_info *fb_new_framebuffer_info(void); Could we make this part of fb_fill_framebuffer_info*()?
Maybe we could even get rid of the `valid` attribute (if it's not valid, don't add it to the list in the first place).
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#3).
Change subject: [WIP]lib/edid_fill_fb: Support multiple framebuffers ......................................................................
[WIP]lib/edid_fill_fb: Support multiple framebuffers
Add an implementation that supports multiple framebuffers.
This will allow to compile in multiple graphic drivers and select at runtime which one to choose. As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 199 insertions(+), 64 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: [WIP]lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/3/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/3/src/lib/edid_fill_fb.c@131 PS3, Line 131: * Intented for drivers that only supports reporting the current information. 'Intented' may be misspelled - perhaps 'Intended'?
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#4).
Change subject: [TESTME]lib/edid_fill_fb: Support multiple framebuffers ......................................................................
[TESTME]lib/edid_fill_fb: Support multiple framebuffers
Add an implementation that supports multiple framebuffers.
This will allow to compile in multiple graphic drivers and select at runtime which one to choose. As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 199 insertions(+), 64 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/4
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: [TESTME]lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39002/4//COMMIT_MSG@13 PS4, Line 13: for a single graphics driver there's no change in functionality. Can you clarify what the goal is -- is that runtime decision supposed to be made by the payload or by coreboot itself? Right now it looks like you're building a framework to fully use multiple displays at the same time. If all you're trying to do is decide on a single display at runtime, I think you could have that much simpler.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#5).
Change subject: [TESTME]lib/edid_fill_fb: Support multiple framebuffers ......................................................................
[TESTME]lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel+Aspeed on server platforms or Intel+Nvidia on consumer notebooks.
The goal is to remove duplicated fill_fb_framebuffer(), the advertisment of multiple indepent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with it's own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 199 insertions(+), 64 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/5
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: [TESTME]lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39002/4//COMMIT_MSG@13 PS4, Line 13: for a single graphics driver there's no change in functionality.
Can you clarify what the goal is -- is that runtime decision supposed to be made by the payload or b […]
The goal is building a framework to fully use multiple displays at the same time. Even using only one isn't possible as multiple implementations collide when linking.
The primary use case is running Aspeed NGI + Intel graphics on a Supermicro server, but it's useful for other platforms as well (like Nvidia Optimus).
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: [TESTME]lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39002/2/src/include/framebuffer_inf... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39002/2/src/include/framebuffer_inf... PS2, Line 25: edid_
EDID is about the modeline (hardware interface) while the framebuffer […]
Done
https://review.coreboot.org/c/coreboot/+/39002/2/src/include/framebuffer_inf... PS2, Line 26: struct edid_fb_info *fb_new_framebuffer_info(void);
Could we make this part of fb_fill_framebuffer_info*()? […]
Introduced fb_set_framebuffer_info
Hello Naresh Solanki, Alexandru Gagniuc, Subrata Banik, Lee Leahy, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#6).
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel+Aspeed on server platforms or Intel+Nvidia on consumer notebooks.
The goal is to remove duplicated fill_fb_framebuffer(), the advertisment of multiple indepent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with it's own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Tested on: * QEMU VGA cirrus * QEMU VGA bochs * Aspeed AST2500 NGI * Lenovo X220 using libgfxinit * Intel FSP2.0 GOP
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 199 insertions(+), 64 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/6
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 6: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 6: Code-Review+1
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 6: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 6:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/edid.h File src/include/edid.h:
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/edid.h@107 PS6, Line 107: struct fb_info; Shouldn't you #include <framebuffer_info.h> rather than declare this in two places?
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c@36 PS6, Line 36: static struct fb_info *list; Why not use <list.h>?
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c@95 PS6, Line 95: info->edid_fb.blue_mask_size = blue_mask_size; nit: at this point, isn't it simpler to just let the caller fill out the structure directly and then call something like fb_info_validate() to check invariants? Functions with a dozen arguments that are all integers aren't great, it's very easy to mix stuff up and not notice the error.
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c@96 PS6, Line 96: info->valid = true; I'm a bit confused who needs this. Can we ever have a framebuffer in the list that's not valid?
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c@148 PS6, Line 148: fb_fill_framebuffer_set_orientation nit: "fill" makes me think this fills out the whole thing, not just sets a single value. Why not just fb_set_orientation()?
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c@172 PS6, Line 172: for (struct fb_info *i = list; i != NULL; i = i->next) { If this is the only reason for having a list, you don't need a list. You just need a single pointer that you keep updating whenever someone creates a new valid FB.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 6: -Code-Review
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/framebuffer_inf... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/framebuffer_inf... PS6, Line 37: bool fb_fill_framebuffer_info(struct fb_info *info, Does this function really need to be exported separately? I only see it used once with the xgifb stuff, and that looks like it could also just use fb_set_framebuffer_info().
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
PS6: The file is not much about EDID anymore, rename?
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c@172 PS6, Line 172: for (struct fb_info *i = list; i != NULL; i = i->next) {
If this is the only reason for having a list, you don't need a list. […]
A later patch will make use of all registered framebuffers.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/edid.h File src/include/edid.h:
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/edid.h@107 PS6, Line 107: struct fb_info;
Shouldn't you #include <framebuffer_info. […]
I don't see why exposing the internals helps here. The callers doesn't need to know anything but the function prototypes. This way all users are forced to use the API as it's intended to.
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/framebuffer_inf... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/framebuffer_inf... PS6, Line 37: bool fb_fill_framebuffer_info(struct fb_info *info,
Does this function really need to be exported separately? I only see it used once with the xgifb stu […]
It's only used in xgifb_modeset. All methods that could be called multiple times use the *_fill_* methods. It doesn't make much sense to call those methods multiple times, so I'll rework those first.
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c@96 PS6, Line 96: info->valid = true;
I'm a bit confused who needs this. […]
yes, if it doesn't pass the pre check: " if (!x_resolution || !y_resolution || !bytes_per_line || !bits_per_pixel)"
Seems to be a bad design, I'll rework that.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/edid.h File src/include/edid.h:
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/edid.h@107 PS6, Line 107: struct fb_info;
I don't see why exposing the internals helps here. […]
idk, is <framebuffer_info.h> supposed to be a less exposed header than <edid.h>? Looks to me like they were both public APIs. In particular, you can't actually do anything useful with the result of set_vbe_mode_info_valid() unless you #include that one too.
It's just that in general I think our policy in coreboot (and most other projects I've seen) is to just include the header that declares the thing you need, and never forward-declare or double-declare stuff. It just seems messy for no real benefit.
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c@172 PS6, Line 172: for (struct fb_info *i = list; i != NULL; i = i->next) {
A later patch will make use of all registered framebuffers.
Ack
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/edid.h File src/include/edid.h:
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/edid.h@107 PS6, Line 107: struct fb_info;
idk, is <framebuffer_info.h> supposed to be a less exposed header than <edid. […]
Are you maybe talking past each other? `framebuffer_info.h` is the public API and doesn't provide more information (doesn't share internals either). I have no strong preference, though. Only point I can make up: if somebody wanted to change the name of the struct later, this would be one more line to adapt.
Hello Philipp Deppenwiese, build bot (Jenkins), Frans Hendriks, Lee Leahy, Matt DeVillier, Paul Menzel, Subrata Banik, Michael Niewöhner, Naresh Solanki, Alexandru Gagniuc, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#7).
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel+Aspeed on server platforms or Intel+Nvidia on consumer notebooks.
The goal is to remove duplicated fill_fb_framebuffer(), the advertisment of multiple indepent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with it's own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 188 insertions(+), 63 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@102 PS7, Line 102: * Intented for drivers that only supports reporting the current information or have a single 'Intented' may be misspelled - perhaps 'Intended'?
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@147 PS7, Line 147: enum lb_fb_orientation orientation) code indent should use tabs where possible
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/framebuffer_inf... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/framebuffer_inf... PS6, Line 37: bool fb_fill_framebuffer_info(struct fb_info *info,
It's only used in xgifb_modeset. […]
removed.
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
PS6:
The file is not much about EDID anymore, rename?
done in CB:39431
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c@36 PS6, Line 36: static struct fb_info *list;
Why not use <list. […]
Done
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c@96 PS6, Line 96: info->valid = true;
yes, if it doesn't pass the pre check: […]
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 7:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/edid.h File src/include/edid.h:
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/edid.h@107 PS6, Line 107: struct fb_info;
`framebuffer_info.h` is the public API and doesn't provide more information (doesn't share internals either).
Yeah, that's why I'm unclear why we wouldn't just #include it. It does provide the declaration of struct fb_info which is all that's needed here.
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@34 PS7, Line 34: edid_fb nit: Would probably be a good occasion to rename this field since it really has nothing to do with EDID anymore. Just 'fb' would probably be fine?
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@36 PS7, Line 36: static struct fb_info list; This should just be a raw struct list_node.
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@56 PS7, Line 56: struct fb_info *info, Why does this work so differently from fb_set_framebuffer_info()? Why can't you call fb_new_framebuffer_info() internally from here too (so you don't have to export that function at all)? If you're just doing this to share code, you can have a static function that does this, but the exported wrappers should have a consistent interface.
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@63 PS7, Line 63: uint8_t blue_mask_size) I still really don't like this super long function signature. How about passing in a struct lb_framebuffer pointer instead (which is memcpied to the new fb_info) so that the caller can pass the same values, but in a more structured (less error prone) manner?
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@109 PS7, Line 109: set 'set' seems like the wrong verb here, especially in comparison to fb_set_orientation() (which changes an existing fb_info rather than create a new one). Maybe fb_add_framebuffer_info() would be better?
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39002/8/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/8/src/lib/edid_fill_fb.c@102 PS8, Line 102: * Intented for drivers that only supports reporting the current information or have a single 'Intented' may be misspelled - perhaps 'Intended'?
https://review.coreboot.org/c/coreboot/+/39002/8/src/lib/edid_fill_fb.c@147 PS8, Line 147: enum lb_fb_orientation orientation) code indent should use tabs where possible
Hello Philipp Deppenwiese, build bot (Jenkins), Frans Hendriks, Lee Leahy, Matt DeVillier, Paul Menzel, Subrata Banik, Michael Niewöhner, Naresh Solanki, Alexandru Gagniuc, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#9).
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel+Aspeed on server platforms or Intel+Nvidia on consumer notebooks.
The goal is to remove duplicated fill_fb_framebuffer(), the advertisment of multiple indepent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with it's own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 182 insertions(+), 63 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/9
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/9/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/9/src/lib/edid_fill_fb.c@135 PS9, Line 135: if (!info) { braces {} are not necessary for single statement blocks
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 9:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@34 PS7, Line 34: edid_fb
nit: Would probably be a good occasion to rename this field since it really has nothing to do with E […]
Done
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@36 PS7, Line 36: static struct fb_info list;
This should just be a raw struct list_node.
Done
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@56 PS7, Line 56: struct fb_info *info,
Why does this work so differently from fb_set_framebuffer_info()? Why can't you call fb_new_framebuf […]
Done
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@109 PS7, Line 109: set
'set' seems like the wrong verb here, especially in comparison to fb_set_orientation() (which change […]
Done
Hello Philipp Deppenwiese, build bot (Jenkins), Frans Hendriks, Lee Leahy, Matt DeVillier, Paul Menzel, Subrata Banik, Michael Niewöhner, Naresh Solanki, Alexandru Gagniuc, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#10).
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel+Aspeed on server platforms or Intel+Nvidia on consumer notebooks.
The goal is to remove duplicated fill_fb_framebuffer(), the advertisment of multiple indepent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with it's own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 171 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/10
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/10/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/10/src/lib/edid_fill_fb.c@135 PS10, Line 135: if (!info) { braces {} are not necessary for single statement blocks
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@36 PS7, Line 36: static struct fb_info list;
Done
No you didn't?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@36 PS7, Line 36: static struct fb_info list;
No you didn't?
It ended in CB:39005 while rebasing, will fix that.
Hello Philipp Deppenwiese, build bot (Jenkins), Frans Hendriks, Lee Leahy, Matt DeVillier, Paul Menzel, Subrata Banik, Michael Niewöhner, Naresh Solanki, Alexandru Gagniuc, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#11).
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel+Aspeed on server platforms or Intel+Nvidia on consumer notebooks.
The goal is to remove duplicated fill_fb_framebuffer(), the advertisment of multiple indepent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with it's own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 180 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/11
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/11/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/11/src/lib/edid_fill_fb.c@135 PS11, Line 135: if (!info) { braces {} are not necessary for single statement blocks
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 11:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 30: /* Any reason this now slipped into this patch? Seems to me like it belongs in CB:39430.
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 35: static Is there a good reason this is an inline? It's not *that* small...
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 37: uint16_t bpp) Since the use of this is pretty specific to VESA OPROMs, maybe it would be better to just make it take a vbe_mode_info_t* (e.g. call it vbe_get_row_alignment() or something and put it in src/include/vbe.h)?
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 39: DIV_ROUND_UP(bpp, 8) / 8 If bpp is 24 this will be 3 but we need it to be 4. In fact, I think this is also a mistake in CB:39430. Maybe it would help to have a bytes_per_pixel(int bits_per_pixel) helper function to do that translation (should basically be (1 << log2_ceil(bits_per_pixel)) to always get the next-larger power of two).
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 43: /* find first set bit */ We have an __ffs() function for this... but I don't think this is the right way to do this anyway. I think you just want to find the next power of two that's larger than the difference:
row_alignment = 1 << log2_ceil(scanline_bytes - h_active);
https://review.coreboot.org/c/coreboot/+/39002/11/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/11/src/lib/edid_fill_fb.c@163 PS11, Line 163: Odd indentation?
Hello Philipp Deppenwiese, build bot (Jenkins), Frans Hendriks, Lee Leahy, Matt DeVillier, Paul Menzel, Subrata Banik, Michael Niewöhner, Naresh Solanki, Alexandru Gagniuc, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#12).
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel+Aspeed on server platforms or Intel+Nvidia on consumer notebooks.
The goal is to remove duplicated fill_fb_framebuffer(), the advertisment of multiple indepent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with it's own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 166 insertions(+), 53 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/12
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/12/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/12/src/lib/edid_fill_fb.c@143 PS12, Line 143: if (!info) { braces {} are not necessary for single statement blocks
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 12:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 30: /*
Any reason this now slipped into this patch? Seems to me like it belongs in CB:39430.
Done
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 37: uint16_t bpp)
Since the use of this is pretty specific to VESA OPROMs, maybe it would be better to just make it ta […]
Done
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 39: DIV_ROUND_UP(bpp, 8) / 8
If bpp is 24 this will be 3 but we need it to be 4. […]
24bpp is a valid pixel format. If a driver advertises 24bpp, but set up 32bpp (XRGB8) framebuffer that's a driver bug. I'll try to get rid of bpp next, and use an enum instead.
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 43: /* find first set bit */
We have an __ffs() function for this... but I don't think this is the right way to do this anyway. […]
Done
Hello Philipp Deppenwiese, build bot (Jenkins), Frans Hendriks, Lee Leahy, Matt DeVillier, Paul Menzel, Subrata Banik, Michael Niewöhner, Naresh Solanki, Alexandru Gagniuc, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#13).
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel+Aspeed on server platforms or Intel+Nvidia on consumer notebooks.
The goal is to remove duplicated fill_fb_framebuffer(), the advertisment of multiple indepent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with it's own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 166 insertions(+), 53 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/13
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/13/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/13/src/lib/edid_fill_fb.c@143 PS13, Line 143: if (!info) { braces {} are not necessary for single statement blocks
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 13:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/edid.h File src/include/edid.h:
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/edid.h@107 PS6, Line 107: struct fb_info;
`framebuffer_info. […]
Done
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 35: static
Is there a good reason this is an inline? It's not *that* small...
Moved out of this commit and reduced it'S size.
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 39: DIV_ROUND_UP(bpp, 8) / 8
24bpp is a valid pixel format. […]
Started cleaning up the 24bpp mess here: CB:40798 It requires test on all drivers as I can't determine the correct mapping from the source. Marking this as resolved for now.
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@36 PS7, Line 36: static struct fb_info list;
It ended in CB:39005 while rebasing, will fix that.
Done
Hello Philipp Deppenwiese, build bot (Jenkins), Frans Hendriks, Lee Leahy, Matt DeVillier, Paul Menzel, Subrata Banik, Michael Niewöhner, Naresh Solanki, Alexandru Gagniuc, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#14).
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel+Aspeed on server platforms or Intel+Nvidia on consumer notebooks.
The goal is to remove duplicated fill_fb_framebuffer(), the advertisment of multiple indepent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with it's own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 165 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/14
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/11/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/11/src/lib/edid_fill_fb.c@163 PS11, Line 163:
Odd indentation?
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 14:
(5 comments)
LGTM after outstanding comments
https://review.coreboot.org/c/coreboot/+/39002/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39002/4//COMMIT_MSG@13 PS4, Line 13: for a single graphics driver there's no change in functionality.
The goal is building a framework to fully use multiple displays at the same time. […]
Ack
https://review.coreboot.org/c/coreboot/+/39002/14/src/include/edid.h File src/include/edid.h:
https://review.coreboot.org/c/coreboot/+/39002/14/src/include/edid.h@95 PS14, Line 95: #include <framebuffer_info.h> At the top with the other includes, please.
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 39: DIV_ROUND_UP(bpp, 8) / 8
Started cleaning up the 24bpp mess here: CB:40798 […]
Hmm... okay, I think you're right, the bootsplash code actually seems to be written with true 24bpp in mind. It's just the code in set_vbe_mode_info_valid() that has always been wrong then (where case 32 and case 24 map to the same thing). It looks like no platform actually sets bpp to 24 so we're probably good there (all the ones that don't explicitly call edid_set_framebuffer_bits_per_pixel() use the 32 default hardcoded in the EDID decoder, I think).
https://review.coreboot.org/c/coreboot/+/39002/14/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/14/src/lib/edid_fill_fb.c@116 PS14, Line 116: fb_new_framebuffer_info() Comment slightly out of date
https://review.coreboot.org/c/coreboot/+/39002/14/src/lib/edid_fill_fb.c@131 PS14, Line 131: bits_per_pixel Needs to be hardcoded to 32 until you fix the FIXME above.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 14:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39002/14//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39002/14//COMMIT_MSG@17 PS14, Line 17: goal is goals are
https://review.coreboot.org/c/coreboot/+/39002/14//COMMIT_MSG@17 PS14, Line 17: advertisment advertis*e*ment
https://review.coreboot.org/c/coreboot/+/39002/14//COMMIT_MSG@18 PS14, Line 18: indepent independent?
https://review.coreboot.org/c/coreboot/+/39002/14//COMMIT_MSG@22 PS14, Line 22: it's its
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/14/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/14/src/lib/edid_fill_fb.c@78 PS14, Line 78: bits_per_pixel = bpp_mask; I'm not confident about this one. Why not return NULL? If the fields are inconsistent how should we know which is correct?
Also, the actual limit is MAX(*_pos + *_size), iow. the end of the last color component. That might differ because we don't check for gaps between the components. Should we?
Last but not least, we don't check for the new `bits_per_pixel` if `x_resolution * bits_per_pixel / 8 <= bytes_per_line` is (still) true.
Hello Philipp Deppenwiese, build bot (Jenkins), Frans Hendriks, Lee Leahy, Matt DeVillier, Paul Menzel, Subrata Banik, Michael Niewöhner, Naresh Solanki, Alexandru Gagniuc, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#15).
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel+Aspeed on server platforms or Intel+Nvidia on consumer notebooks.
The goals are to remove duplicated fill_fb_framebuffer(), the advertisement of multiple independent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with its own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 164 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/15
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 15:
(8 comments)
https://review.coreboot.org/c/coreboot/+/39002/14//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39002/14//COMMIT_MSG@17 PS14, Line 17: goal is
goals are
Done
https://review.coreboot.org/c/coreboot/+/39002/14//COMMIT_MSG@17 PS14, Line 17: advertisment
advertis*e*ment
Done
https://review.coreboot.org/c/coreboot/+/39002/14//COMMIT_MSG@18 PS14, Line 18: indepent
independent?
Done
https://review.coreboot.org/c/coreboot/+/39002/14//COMMIT_MSG@22 PS14, Line 22: it's
its
Done
https://review.coreboot.org/c/coreboot/+/39002/14/src/include/edid.h File src/include/edid.h:
https://review.coreboot.org/c/coreboot/+/39002/14/src/include/edid.h@95 PS14, Line 95: #include <framebuffer_info.h>
At the top with the other includes, please.
Done
https://review.coreboot.org/c/coreboot/+/39002/14/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/14/src/lib/edid_fill_fb.c@78 PS14, Line 78: bits_per_pixel = bpp_mask;
I'm not confident about this one. Why not return NULL? If the fields […]
Done. What's the definition of bits_per_pixel? does it count gaps, too?
https://review.coreboot.org/c/coreboot/+/39002/14/src/lib/edid_fill_fb.c@116 PS14, Line 116: fb_new_framebuffer_info()
Comment slightly out of date
Done
https://review.coreboot.org/c/coreboot/+/39002/14/src/lib/edid_fill_fb.c@131 PS14, Line 131: bits_per_pixel
Needs to be hardcoded to 32 until you fix the FIXME above.
Done
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@63 PS7, Line 63: uint8_t blue_mask_size)
I still really don't like this super long function signature. […]
that only works if all arguments are fields withing the lb_framebuffer struct, which is not the case in future commits.
Christian Walter has uploaded a new patch set (#17) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel+Aspeed on server platforms or Intel+Nvidia on consumer notebooks.
The goal is to remove duplicated fill_fb_framebuffer(), the advertisment of multiple indepent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with it's own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 165 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/17
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 17: Code-Review-2
This is an olde version that shouldn't be used.
Christian Walter has uploaded a new patch set (#18) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel+Aspeed on server platforms or Intel+Nvidia on consumer notebooks.
The goals are to remove duplicated fill_fb_framebuffer(), the advertisement of multiple independent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with its own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 164 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/18
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 18: -Code-Review
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 18: Code-Review+1
Sorry for not really keeping up with this. I think this patch looks fine to me now from a high level but Nico had the most recent comments so I'll let him +2 when it's done.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 18:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@63 PS7, Line 63: uint8_t blue_mask_size)
that only works if all arguments are fields withing the lb_framebuffer struct, which is not the case […]
Future commits aren't fully reviewed yet, so this should stay open.
https://review.coreboot.org/c/coreboot/+/39002/14/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/14/src/lib/edid_fill_fb.c@78 PS14, Line 78: bits_per_pixel = bpp_mask;
Done. […]
Yes, it's to be used to calculate the location of a pixel in memory, hence has to include gaps.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@63 PS7, Line 63: uint8_t blue_mask_size)
Future commits aren't fully reviewed yet, so this should stay open.
Any updates?
Hello build bot (Jenkins), Frans Hendriks, Matt DeVillier, Paul Menzel, Angel Pons, Julius Werner, Subrata Banik, Arthur Heymans, Michael Niewöhner, Naresh Solanki, Patrick Rudolph, Alexandru Gagniuc, Philipp Deppenwiese, Lee Leahy, Christian Walter, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#21).
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel+Aspeed on server platforms or Intel+Nvidia on consumer notebooks.
The goals are to remove duplicated fill_fb_framebuffer(), the advertisement of multiple independent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with its own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 182 insertions(+), 60 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/21
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 21:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39002/21/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/21/src/lib/edid_fill_fb.c@96 PS21, Line 96: physical_address : fb_addr, space prohibited before that ':' (ctx:WxW)
https://review.coreboot.org/c/coreboot/+/39002/21/src/lib/edid_fill_fb.c@118 PS21, Line 118: physical_address : fb_addr, space prohibited before that ':' (ctx:WxW)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 21: Code-Review+1
(5 comments)
https://review.coreboot.org/c/coreboot/+/39002/21//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39002/21//COMMIT_MSG@17 PS21, Line 17: the advertisement : of advertise
https://review.coreboot.org/c/coreboot/+/39002/21/src/include/framebuffer_in... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39002/21/src/include/framebuffer_in... PS21, Line 2: /* This file is part of the coreboot project. */ Please drop
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@63 PS7, Line 63: uint8_t blue_mask_size)
Any updates?
Latest patchset passes in a pointer to `struct lb_framebuffer`
https://review.coreboot.org/c/coreboot/+/39002/21/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/21/src/lib/edid_fill_fb.c@96 PS21, Line 96: physical_address : fb_addr, This is the old syntax for struct initializers. New syntax would be:
.physical_address = fb_addr,
https://review.coreboot.org/c/coreboot/+/39002/21/src/lib/edid_fill_fb.c@105 PS21, Line 105: 0 Is this supposed to be the same as `red_mask_pos` ?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 21:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39002/21//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39002/21//COMMIT_MSG@14 PS21, Line 14: + add spaces around +
https://review.coreboot.org/c/coreboot/+/39002/21//COMMIT_MSG@15 PS21, Line 15: + add spaces around +
https://review.coreboot.org/c/coreboot/+/39002/21/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/21/src/lib/edid_fill_fb.c@167 PS21, Line 167: //TODO: Add support for advertising all framebuffers in this list TODO
https://review.coreboot.org/c/coreboot/+/39002/21/src/mainboard/google/kukui... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39002/21/src/mainboard/google/kukui... PS21, Line 148: static bool configure_display(void) : { : struct panel_description *panel = get_active_panel(); : if (!panel) : return false; : : mtcmos_display_power_on(); : mtcmos_protect_display_bus(); : configure_panel_backlight(); : power_on_panel(panel); : : struct edid *edid = &panel->s->edid; : edid_set_framebuffer_bits_per_pixel(edid, 32, 0); : mtk_ddp_init(); : u32 mipi_dsi_flags = (MIPI_DSI_MODE_VIDEO | : MIPI_DSI_MODE_VIDEO_SYNC_PULSE | : MIPI_DSI_MODE_LPM); : if (CONFIG(DRIVER_ANALOGIX_ANX7625)) : mipi_dsi_flags |= MIPI_DSI_MODE_EOT_PACKET; : if (mtk_dsi_init(mipi_dsi_flags, MIPI_DSI_FMT_RGB888, 4, edid, : panel->s->init) < 0) { : printk(BIOS_ERR, "%s: Failed in DSI init.\n", __func__); : return false; : } : mtk_ddp_mode_set(edid); : struct fb_info *info = set_vbe_mode_info_valid(edid, 0); : if (info) : fb_set_orientation(info, panel->s->orientation); : : return true; : } : is this really board-specific or should it be moed to some driver?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/21//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39002/21//COMMIT_MSG@14 PS21, Line 14: +
add spaces around +
But this isn't code? IMHO, the `+` here would work like a hyphen on compound adjectives, e.g. `highly-configurable`.
Hello build bot (Jenkins), Frans Hendriks, Matt DeVillier, Paul Menzel, Angel Pons, Julius Werner, Subrata Banik, Arthur Heymans, Michael Niewöhner, Naresh Solanki, Patrick Rudolph, Alexandru Gagniuc, Philipp Deppenwiese, Lee Leahy, Christian Walter, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#22).
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel+Aspeed on server platforms or Intel+Nvidia on consumer notebooks.
The goals are to remove duplicated fill_fb_framebuffer(), the advertisement of multiple independent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with its own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 181 insertions(+), 60 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/22
Hello build bot (Jenkins), Frans Hendriks, Matt DeVillier, Paul Menzel, Angel Pons, Julius Werner, Subrata Banik, Arthur Heymans, Michael Niewöhner, Naresh Solanki, Patrick Rudolph, Alexandru Gagniuc, Philipp Deppenwiese, Lee Leahy, Christian Walter, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#23).
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel and Aspeed on server platforms or Intel and Nvidia on consumer notebooks.
The goals are to remove duplicated fill_fb_framebuffer(), advertise of multiple independent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with its own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 181 insertions(+), 60 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/23
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 22:
(7 comments)
https://review.coreboot.org/c/coreboot/+/39002/21//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39002/21//COMMIT_MSG@14 PS21, Line 14: +
But this isn't code? IMHO, the `+` here would work like a hyphen on compound adjectives, e.g. […]
Done
https://review.coreboot.org/c/coreboot/+/39002/21//COMMIT_MSG@15 PS21, Line 15: +
add spaces around +
Done
https://review.coreboot.org/c/coreboot/+/39002/21//COMMIT_MSG@17 PS21, Line 17: the advertisement : of
advertise
Done
https://review.coreboot.org/c/coreboot/+/39002/21/src/include/framebuffer_in... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39002/21/src/include/framebuffer_in... PS21, Line 2: /* This file is part of the coreboot project. */
Please drop
Done
https://review.coreboot.org/c/coreboot/+/39002/21/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/21/src/lib/edid_fill_fb.c@96 PS21, Line 96: physical_address : fb_addr,
This is the old syntax for struct initializers. New syntax would be: […]
Done
https://review.coreboot.org/c/coreboot/+/39002/21/src/lib/edid_fill_fb.c@105 PS21, Line 105: 0
Is this supposed to be the same as `red_mask_pos` ?
no, done
https://review.coreboot.org/c/coreboot/+/39002/21/src/lib/edid_fill_fb.c@167 PS21, Line 167: //TODO: Add support for advertising all framebuffers in this list
TODO
That's fixed in the following patch. See CB:39005 for details.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/23//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39002/23//COMMIT_MSG@17 PS23, Line 17: The goals are to remove duplicated fill_fb_framebuffer(), advertise of multiple : independent framebuffers in coreboot tables, and better runtime/build time : graphic configuration reflow to 72 chars
Hello build bot (Jenkins), Frans Hendriks, Matt DeVillier, Paul Menzel, Angel Pons, Julius Werner, Subrata Banik, Arthur Heymans, Michael Niewöhner, Naresh Solanki, Patrick Rudolph, Alexandru Gagniuc, Philipp Deppenwiese, Lee Leahy, Christian Walter, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#24).
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel and Aspeed on server platforms or Intel and Nvidia on consumer notebooks.
The goals are to remove duplicated fill_fb_framebuffer(), advertise of multiple independent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with its own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 181 insertions(+), 60 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/24
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 24:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/23//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39002/23//COMMIT_MSG@17 PS23, Line 17: The goals are to remove duplicated fill_fb_framebuffer(), advertise of multiple : independent framebuffers in coreboot tables, and better runtime/build time : graphic configuration
reflow to 72 chars
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 24: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/39002/24//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39002/24//COMMIT_MSG@17 PS24, Line 17: of nit: drop `of`
https://review.coreboot.org/c/coreboot/+/39002/24/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/24/src/lib/edid_fill_fb.c@70 PS24, Line 70: list_insert_after(&info->node, &list); Why not insert the new node in `fb_new_framebuffer_info`?
https://review.coreboot.org/c/coreboot/+/39002/24/src/lib/edid_fill_fb.c@77 PS24, Line 77: supports nit: no `s`: support
https://review.coreboot.org/c/coreboot/+/39002/24/src/mainboard/google/kukui... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39002/24/src/mainboard/google/kukui... PS24, Line 174: if (info) Why not:
if (!info) return false;
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 24:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/24//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39002/24//COMMIT_MSG@17 PS24, Line 17: of
nit: drop `of`
and add a `to` before, as it should be: "The goals are to remove [...], to advertise [...], and better [...] options." Unless you mean the verb "to better" but that reads odd.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 24: Code-Review+1
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 24:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/24/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/24/src/lib/edid_fill_fb.c@70 PS24, Line 70: list_insert_after(&info->node, &list);
Why not insert the new node in `fb_new_framebuffer_info`?
This is a public function called by some graphics drivers.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 24:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39002/24/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/24/src/lib/edid_fill_fb.c@70 PS24, Line 70: list_insert_after(&info->node, &list);
This is a public function called by some graphics drivers.
Ack
Hello build bot (Jenkins), Frans Hendriks, Matt DeVillier, Paul Menzel, Angel Pons, Julius Werner, Subrata Banik, Arthur Heymans, Michael Niewöhner, Naresh Solanki, Patrick Rudolph, Alexandru Gagniuc, Philipp Deppenwiese, Lee Leahy, Christian Walter, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39002
to look at the new patch set (#25).
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel and Aspeed on server platforms or Intel and Nvidia on consumer notebooks.
The goals are to remove duplicated fill_fb_framebuffer(), to advertise multiple independent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with its own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 181 insertions(+), 60 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39002/25
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 24:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39002/24//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39002/24//COMMIT_MSG@17 PS24, Line 17: of
and add a `to` before, as it should be: "The goals are to remove [...], […]
Done
https://review.coreboot.org/c/coreboot/+/39002/24/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/24/src/lib/edid_fill_fb.c@77 PS24, Line 77: supports
nit: no `s`: support
Done
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 25:
Tested on qemu and prodrive/hermes with AST2400 graphics init.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 25: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 25: Code-Review+2
(4 comments)
Integer types in the API seem a bit odd, but won't hurt anyone. There's also some "%d" for unsigned values, but if it doesn't make GCC cry...
https://review.coreboot.org/c/coreboot/+/39002/25/src/include/framebuffer_in... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39002/25/src/include/framebuffer_in... PS25, Line 13: fb_add_framebuffer_info_ex(const struct lb_framebuffer *fb); Nit, line break seems unnecessary.
https://review.coreboot.org/c/coreboot/+/39002/25/src/include/framebuffer_in... PS25, Line 15: uint32_t x_resolution, : uint32_t y_resolution, uint32_t bytes_per_line, : uint8_t bits_per_pixel); Nit, the types seem rather arbitrary. If there is no exact size needed, using just `unsigned int` would probably be more efficient.
https://review.coreboot.org/c/coreboot/+/39002/25/src/include/framebuffer_in... PS25, Line 19: void fb_set_orientation(struct fb_info *info, Nit, line break seems unnecessary.
https://review.coreboot.org/c/coreboot/+/39002/25/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/25/src/lib/edid_fill_fb.c@52 PS25, Line 52: doesn't Nit. `don't match`
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 26:
Works on Asus P8Z77-V LX2 with a dGPU and running the VBIOS either directly or through YABEL
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
lib/edid_fill_fb: Support multiple framebuffers
Currently it's not possible to add multiple graphics driver into one coreboot image. This patch series will fix this issue by providing a single API that multiple graphics driver can use.
This is required for platforms that have two graphic cards, but different graphic drivers, like Intel and Aspeed on server platforms or Intel and Nvidia on consumer notebooks.
The goals are to remove duplicated fill_fb_framebuffer(), to advertise multiple independent framebuffers in coreboot tables, and better runtime/build time graphic configuration options.
Add an implementation in edid_fill_fb that supports registering multiple framebuffers, each with its own configuration.
As the current code is only compiled for a single graphics driver there's no change in functionality.
Change-Id: I7264c2ea2f72f36adfd26f26b00e3ce172133621 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39002 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Nico Huber nico.h@gmx.de --- M src/include/edid.h A src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/kukui/mainboard.c 4 files changed, 181 insertions(+), 60 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/include/edid.h b/src/include/edid.h index 7536a66..3644e6a 100644 --- a/src/include/edid.h +++ b/src/include/edid.h @@ -4,6 +4,7 @@ #define EDID_H
#include <stdint.h> +#include <framebuffer_info.h> #include "commonlib/coreboot_tables.h"
enum edid_modes { @@ -95,7 +96,7 @@ int decode_edid(unsigned char *edid, int size, struct edid *out); void edid_set_framebuffer_bits_per_pixel(struct edid *edid, int fb_bpp, int row_byte_alignment); -void set_vbe_mode_info_valid(const struct edid *edid, uintptr_t fb_addr); +struct fb_info *set_vbe_mode_info_valid(const struct edid *edid, uintptr_t fb_addr); void set_vbe_framebuffer_orientation(enum lb_fb_orientation orientation); int set_display_mode(struct edid *edid, enum edid_modes mode);
diff --git a/src/include/framebuffer_info.h b/src/include/framebuffer_info.h new file mode 100644 index 0000000..5251f29 --- /dev/null +++ b/src/include/framebuffer_info.h @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#ifndef __FRAMEBUFFER_INFO_H_ +#define __FRAMEBUFFER_INFO_H_ + +#include <stdint.h> +#include <stdbool.h> +#include <commonlib/coreboot_tables.h> + +struct fb_info; + +struct fb_info * +fb_add_framebuffer_info_ex(const struct lb_framebuffer *fb); + +struct fb_info *fb_add_framebuffer_info(uintptr_t fb_addr, uint32_t x_resolution, + uint32_t y_resolution, uint32_t bytes_per_line, + uint8_t bits_per_pixel); + +void fb_set_orientation(struct fb_info *info, + enum lb_fb_orientation orientation); + +#endif /* __FRAMEBUFFER_INFO_H_ */ diff --git a/src/lib/edid_fill_fb.c b/src/lib/edid_fill_fb.c index 422feaf..1e8cc5a 100644 --- a/src/lib/edid_fill_fb.c +++ b/src/lib/edid_fill_fb.c @@ -3,75 +3,170 @@ #include <console/console.h> #include <edid.h> #include <boot/coreboot_tables.h> +#include <framebuffer_info.h> +#include <string.h> +#include <stdlib.h> +#include <bootsplash.h> +#include <list.h>
-static int fb_valid; -static struct lb_framebuffer edid_fb; +struct fb_info { + struct list_node node; + struct lb_framebuffer fb; +}; +static struct list_node list;
/* - * Take an edid, and create a framebuffer. Set fb_valid to 1. + * Allocate a new framebuffer info struct on heap. + * Returns NULL on error. */ -void set_vbe_mode_info_valid(const struct edid *edid, uintptr_t fb_addr) +static struct fb_info *fb_new_framebuffer_info(void) { - edid_fb.physical_address = fb_addr; - edid_fb.x_resolution = edid->x_resolution; - edid_fb.y_resolution = edid->y_resolution; - edid_fb.bytes_per_line = edid->bytes_per_line; - /* In the case of (e.g.) 24 framebuffer bits per pixel, the convention - * nowadays seems to be to round it up to the nearest reasonable - * boundary, because otherwise the byte-packing is hideous. - * So, for example, in RGB with no alpha, the bytes are still - * packed into 32-bit words, the so-called 32bpp-no-alpha mode. - * Or, in 5:6:5 mode, the bytes are also packed into 32-bit words, - * and in 4:4:4 mode, they are packed into 16-bit words. - * Good call on the hardware guys part. - * It's not clear we're covering all cases here, but - * I'm not sure with grahpics you ever can. - */ - edid_fb.bits_per_pixel = edid->framebuffer_bits_per_pixel; - edid_fb.reserved_mask_pos = 0; - edid_fb.reserved_mask_size = 0; - switch (edid->framebuffer_bits_per_pixel) { - case 32: - case 24: - /* packed into 4-byte words */ - edid_fb.reserved_mask_pos = 24; - edid_fb.reserved_mask_size = 8; - edid_fb.red_mask_pos = 16; - edid_fb.red_mask_size = 8; - edid_fb.green_mask_pos = 8; - edid_fb.green_mask_size = 8; - edid_fb.blue_mask_pos = 0; - edid_fb.blue_mask_size = 8; - break; - case 16: - /* packed into 2-byte words */ - edid_fb.red_mask_pos = 11; - edid_fb.red_mask_size = 5; - edid_fb.green_mask_pos = 5; - edid_fb.green_mask_size = 6; - edid_fb.blue_mask_pos = 0; - edid_fb.blue_mask_size = 5; - break; - default: - printk(BIOS_SPEW, "%s: unsupported BPP %d\n", __func__, - edid->framebuffer_bits_per_pixel); - return; - } + struct fb_info *ret; + ret = malloc(sizeof(struct fb_info)); + if (ret) + memset(ret, 0, sizeof(struct fb_info));
- fb_valid = 1; + return ret; }
-void set_vbe_framebuffer_orientation(enum lb_fb_orientation orientation) +/* + * Fills a provided framebuffer info struct and adds it to the internal list if it's + * valid. Returns NULL on error. + */ +struct fb_info * +fb_add_framebuffer_info_ex(const struct lb_framebuffer *fb) { - edid_fb.orientation = orientation; + struct fb_info *info; + uint8_t bpp_mask; + + /* Validate input */ + if (!fb || !fb->x_resolution || !fb->y_resolution || !fb->bytes_per_line || + !fb->bits_per_pixel) { + printk(BIOS_ERR, "%s: Invalid framebuffer data provided\n", __func__); + return NULL; + } + + bpp_mask = fb->blue_mask_size + fb->green_mask_size + fb->red_mask_size + + fb->reserved_mask_size; + if (fb->bits_per_pixel != bpp_mask) { + printk(BIOS_ERR, "%s: BPP=%d and channel bit mask=%d doesn't match." + " This is a driver bug.\n", __func__, fb->bits_per_pixel, bpp_mask); + return NULL; + } + + info = fb_new_framebuffer_info(); + if (!info) + return NULL; + + printk(BIOS_INFO, "framebuffer_info: bytes_per_line: %d, bits_per_pixel: %d\n " + " x_res x y_res: %d x %d, size: %d at 0x%llx\n", + fb->bytes_per_line, fb->bits_per_pixel, fb->x_resolution, + fb->y_resolution, (fb->bytes_per_line * fb->y_resolution), + fb->physical_address); + + /* Update */ + info->fb = *fb; + + list_insert_after(&info->node, &list); + + return info; +} + +/* + * Allocates a new framebuffer info struct and fills it for 32/24/16bpp framebuffers. + * Intended for drivers that only support reporting the current information or have a single + * modeset invocation. + * + * Complex drivers should use fb_add_framebuffer_info_ex() instead. + */ +struct fb_info * +fb_add_framebuffer_info(uintptr_t fb_addr, uint32_t x_resolution, + uint32_t y_resolution, uint32_t bytes_per_line, + uint8_t bits_per_pixel) +{ + struct fb_info *info = NULL; + + switch (bits_per_pixel) { + case 32: + case 24: { + /* FIXME: 24 BPP might be RGB8 or XRGB8 */ + /* packed into 4-byte words */ + + const struct lb_framebuffer fb = { + .physical_address = fb_addr, + .x_resolution = x_resolution, + .y_resolution = y_resolution, + .bytes_per_line = bytes_per_line, + .bits_per_pixel = bits_per_pixel, + .red_mask_pos = 16, + .red_mask_size = 8, + .green_mask_pos = 8, + .green_mask_size = 8, + .blue_mask_pos = 0, + .blue_mask_size = 8, + .reserved_mask_pos = 24, + .reserved_mask_size = 8, + .orientation = LB_FB_ORIENTATION_NORMAL, + }; + + info = fb_add_framebuffer_info_ex(&fb); + break; + } + case 16: { + /* packed into 2-byte words */ + const struct lb_framebuffer fb = { + .physical_address = fb_addr, + .x_resolution = x_resolution, + .y_resolution = y_resolution, + .bytes_per_line = bytes_per_line, + .bits_per_pixel = 16, + .red_mask_pos = 11, + .red_mask_size = 5, + .green_mask_pos = 5, + .green_mask_size = 6, + .blue_mask_pos = 0, + .blue_mask_size = 5, + .reserved_mask_pos = 0, + .reserved_mask_size = 0, + .orientation = LB_FB_ORIENTATION_NORMAL, + }; + info = fb_add_framebuffer_info_ex(&fb); + break; + } + default: + printk(BIOS_ERR, "%s: unsupported BPP %d\n", __func__, bits_per_pixel); + } + if (!info) + printk(BIOS_ERR, "%s: failed to add framebuffer info\n", __func__); + + return info; +} + +void fb_set_orientation(struct fb_info *info, enum lb_fb_orientation orientation) +{ + if (!info) + return; + + info->fb.orientation = orientation; +} + +/* + * Take an edid, and create a framebuffer. + */ +struct fb_info *set_vbe_mode_info_valid(const struct edid *edid, uintptr_t fb_addr) +{ + return fb_add_framebuffer_info(fb_addr, edid->x_resolution, edid->y_resolution, + edid->bytes_per_line, edid->framebuffer_bits_per_pixel); }
int fill_lb_framebuffer(struct lb_framebuffer *framebuffer) { - if (!fb_valid) - return -1; + struct fb_info *i;
- *framebuffer = edid_fb; - - return 0; + list_for_each(i, list, node) { + //TODO: Add support for advertising all framebuffers in this list + *framebuffer = i->fb; + return 0; + } + return -1; } diff --git a/src/mainboard/google/kukui/mainboard.c b/src/mainboard/google/kukui/mainboard.c index 4220810d..216b6f4 100644 --- a/src/mainboard/google/kukui/mainboard.c +++ b/src/mainboard/google/kukui/mainboard.c @@ -10,6 +10,7 @@ #include <device/device.h> #include <ec/google/chromeec/ec.h> #include <edid.h> +#include <framebuffer_info.h> #include <gpio.h> #include <soc/ddp.h> #include <soc/dsi.h> @@ -168,8 +169,10 @@ return false; } mtk_ddp_mode_set(edid); - set_vbe_mode_info_valid(edid, 0); - set_vbe_framebuffer_orientation(panel->s->orientation); + struct fb_info *info = set_vbe_mode_info_valid(edid, 0); + if (info) + fb_set_orientation(info, panel->s->orientation); + return true; }
Attention is currently required from: Patrick Rudolph. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 27:
(2 comments)
Patchset:
PS27: Looks like this CL is breaking the framebuffer on picasso devices:
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 41d4 VBE: resolution: 1920x1080@32 VBE: framebuffer: 0xd0000000 VBE: Setting VESA mode 41d4 fb_add_framebuffer_info_ex: blue_mask_size: 8, green_mask_size: 8, red_mask_size: 8, reserved_mask_size: 0 fb_add_framebuffer_info_ex: BPP=32 and channel bit mask=24 doesn't match. This is a driver bug.
File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/comment/19485ad9_f9654f8d PS27, Line 53: " This is a driver bug.\n", __func__, fb->bits_per_pixel, bpp_mask); Looks like this check is breaking the framebuffer on picasso:
Calling Option ROM... ... Option ROM returned. VBE: Getting information about VESA mode 41d4 VBE: resolution: 1920x1080@32 VBE: framebuffer: 0xd0000000 VBE: Setting VESA mode 41d4 fb_add_framebuffer_info_ex: blue_mask_size: 8, green_mask_size: 8, red_mask_size: 8, reserved_mask_size: 0 fb_add_framebuffer_info_ex: BPP=32 and channel bit mask=24 doesn't match. This is a driver bug.
If I comment out the `return NULL` I get the following:
framebuffer_info: bytes_per_line: 7680, bits_per_pixel: 32 x_res x y_res: 1920 x 1080, size: 8294400 at 0xd0000000
How should we go about restoring the picasso framebuffer?
Attention is currently required from: Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 27:
(1 comment)
File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/comment/040aca9f_84a1f14c PS27, Line 53: " This is a driver bug.\n", __func__, fb->bits_per_pixel, bpp_mask);
Looks like this check is breaking the framebuffer on picasso: […]
Can Picasso use 32 bits per pixel? What are the remaining 8 bits used for? Is this using a specific VESA mode?
Attention is currently required from: Patrick Rudolph. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 27:
(1 comment)
File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/comment/1d0a2190_1c017d6d PS27, Line 53: " This is a driver bug.\n", __func__, fb->bits_per_pixel, bpp_mask);
How should we go about restoring the picasso framebuffer?
Ideally, you would convince AMD to contribute to open-source graphics. Blobs will always stay fragile.
Second best option is to ask them for a VGA BIOS that sets sane values.
I can't say that VBE really specifies how things should be. It basically leaves the reserved_mask* fields undocumented :-/ under these circumstances I'd say our VBE driver should sanitize the fields before passing them on to common code?
Attention is currently required from: Patrick Rudolph. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 27:
(1 comment)
File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/comment/2ff81cde_fa30760a PS27, Line 53: " This is a driver bug.\n", __func__, fb->bits_per_pixel, bpp_mask); Here are the supported VESA modes:
Calling Option ROM... ... Option ROM returned. Supported Video Mode list for OpRom: VBE: Getting information about VESA mode 4110 VBE: resolution: 640x480@16 VBE: Getting information about VESA mode 4111 VBE: resolution: 640x480@16 VBE: Getting information about VESA mode 4113 VBE: resolution: 800x600@16 VBE: Getting information about VESA mode 4114 VBE: resolution: 800x600@16 VBE: Getting information about VESA mode 4116 VBE: resolution: 1024x768@16 VBE: Getting information about VESA mode 4117 VBE: resolution: 1024x768@16 VBE: Getting information about VESA mode 4119 VBE: resolution: 1280x1024@16 VBE: Getting information about VESA mode 411a VBE: resolution: 1280x1024@16 VBE: Getting information about VESA mode 4165 VBE: resolution: 1280x960@16 VBE: Getting information about VESA mode 4166 VBE: resolution: 1280x960@32 VBE: Getting information about VESA mode 4121 VBE: resolution: 640x480@32 VBE: Getting information about VESA mode 4122 VBE: resolution: 800x600@32 VBE: Getting information about VESA mode 4123 VBE: resolution: 1024x768@32 VBE: Getting information about VESA mode 4124 VBE: resolution: 1280x1024@32 VBE: Getting information about VESA mode 4145 VBE: resolution: 1400x1050@16 VBE: Getting information about VESA mode 4146 VBE: resolution: 1400x1050@32 VBE: Getting information about VESA mode 4175 VBE: resolution: 1600x1200@16 VBE: Getting information about VESA mode 4176 VBE: resolution: 1600x1200@32 VBE: Getting information about VESA mode 41d2 VBE: resolution: 1920x1080@16 VBE: Getting information about VESA mode 41d4 VBE: resolution: 1920x1080@32 VBE: Getting information about VESA mode 41d4 VBE: resolution: 1920x1080@32 VBE: framebuffer: 0xd0000000 VBE: Setting VESA mode 41d4 fb_add_framebuffer_info_ex: BPP=32 and channel bit mask=24 doesn't match. This is a driver bug.
Ideally, you would convince AMD to contribute to open-source graphics. Blobs
will always stay fragile. Agreed, but that's a battle for a different day 😊
Second best option is to ask them for a VGA BIOS that sets sane values.
If VBE doesn't specify what to do with the reserved fields, I'm not sure I can make the argument to change the VBIOS for picasso. I could maybe make the argument to change it for future revisions though.
I'd say our VBE driver should sanitize the fields before passing them on
to common code? Is the thought that vbe_get_mode_info would do:
if (fb->blue_mask_size + fb->green_mask_size + fb->red_mask_size == 24 && fb->bits_per_pixel == 32) fb->reserved_mask_size = 8;
I think it would be cleaner to relax this check then modify any data.
Attention is currently required from: Patrick Rudolph. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 27:
(1 comment)
File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/comment/7bad5014_a5c764e4 PS27, Line 53: " This is a driver bug.\n", __func__, fb->bits_per_pixel, bpp_mask);
Here are the supported VESA modes: […]
You may be right. Only now that you called it "reserved fields" I noticed that we might have misinterpreted VBE for the last (20?) years. The code always seemed to treat them as fields that describe reserved bits. Never occured to me that they might have been meant as reserved fields. And these fields in coreboot seem to be modeled after VBE...
They are part of an external ABI by now (cf. `coreboot_tables.h`). So I'd still prefer sanitizing the values. Otherwise we'd be incom- patible to our own interface :-/
Relaxing the check seems ok, too, how about only bailing out if the sum is > bits_per_pixel but always logging the error?
Attention is currently required from: Patrick Rudolph. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 27:
(1 comment)
File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/comment/1b0ef143_f01be4ac PS27, Line 53: " This is a driver bug.\n", __func__, fb->bits_per_pixel, bpp_mask); I broke down and looked at the spec, and think I understand 😊
The RedMaskSize, GreenMaskSize, BlueMaskSize, and RsvdMaskSize fields define the size, in bits, of the red, green, and blue components of a direct color pixel. A bit mask can be constructed from the MaskSize fields using simple shift arithmetic. For example, the MaskSize values for a Direct Color 5:6:5 mode would be 5, 6, 5, and 0, for the red, green, blue, and reserved fields, respectively. Note that in the YUV MemoryModel, the red field is used for V, the green field is used for Y, and the blue field is used for U. The MaskSize fields should be set to 0 in modes using a memory model that does not have pixels with component fields.
I have filed a bug with AMD to get the VBIOS fixed. I like your last suggestion. I can send a CL with that change.
Attention is currently required from: Patrick Rudolph. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 27:
(1 comment)
File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/comment/45d24ab6_b7730ea4 PS27, Line 53: " This is a driver bug.\n", __func__, fb->bits_per_pixel, bpp_mask);
I broke down and looked at the spec, and think I understand 😊 […]
https://review.coreboot.org/c/coreboot/+/49404
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Christian Walter (Code Review)
-
Frans Hendriks (Code Review)
-
Julius Werner (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Philipp Deppenwiese (Code Review)
-
Raul Rangel (Code Review)