Hello Julius Werner,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/38537
to review the following change.
Change subject: libpayload/corebootfb: Keep local copy of framebuffer info ......................................................................
libpayload/corebootfb: Keep local copy of framebuffer info
Keeping a local copy of the framebuffer info allows us to make changes, e.g. add offsets. It also avoids trouble with relocation.
Change-Id: I852c4eb229dd0724114acb302ab2ed7164712b64 Signed-off-by: Nico Huber nico.huber@secunet.com --- M payloads/libpayload/drivers/video/corebootfb.c 1 file changed, 6 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/38537/1
diff --git a/payloads/libpayload/drivers/video/corebootfb.c b/payloads/libpayload/drivers/video/corebootfb.c index 11397ba..c4b5048 100644 --- a/payloads/libpayload/drivers/video/corebootfb.c +++ b/payloads/libpayload/drivers/video/corebootfb.c @@ -61,13 +61,12 @@ (0xFF << 16) | (0xFF << 8) | 0xFF, };
-/* Addresses for the various components */ -static unsigned long fbinfo; -static unsigned long fbaddr; +struct cb_framebuffer fbinfo; static unsigned short *chars;
-#define FI ((struct cb_framebuffer *) phys_to_virt(fbinfo)) -#define FB ((unsigned char *) phys_to_virt(fbaddr)) +/* Addresses for the various components */ +#define FI (&fbinfo) +#define FB ((unsigned char *) phys_to_virt(FI->physical_address)) #define CHARS (chars)
static void corebootfb_scroll_up(void) @@ -230,12 +229,9 @@ if (lib_sysinfo.framebuffer == NULL) return -1;
- /* We might have been called before relocation (like FILO does). So - just keep the physical address which won't break on relocation. */ - fbinfo = virt_to_phys(lib_sysinfo.framebuffer); + fbinfo = *lib_sysinfo.framebuffer;
- fbaddr = FI->physical_address; - if (fbaddr == 0) + if (FI->physical_address == 0) return -1;
font_init(FI->x_resolution);
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38537 )
Change subject: libpayload/corebootfb: Keep local copy of framebuffer info ......................................................................
Patch Set 1: Code-Review+1
Has it been tested?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38537 )
Change subject: libpayload/corebootfb: Keep local copy of framebuffer info ......................................................................
Patch Set 1:
Has it been tested?
Yes, at the end of the patch train only, though.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38537 )
Change subject: libpayload/corebootfb: Keep local copy of framebuffer info ......................................................................
Patch Set 1: Code-Review+2
Patch Set 1:
Has it been tested?
Yes, at the end of the patch train only, though.
Ack
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38537 )
Change subject: libpayload/corebootfb: Keep local copy of framebuffer info ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38537/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/corebootfb.c:
https://review.coreboot.org/c/coreboot/+/38537/1/payloads/libpayload/drivers... PS1, Line 234: if (FI->physical_address == 0) Can we also remove FI-> now and just replace it with fbinfo. everywhere?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38537 )
Change subject: libpayload/corebootfb: Keep local copy of framebuffer info ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38537/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/corebootfb.c:
https://review.coreboot.org/c/coreboot/+/38537/1/payloads/libpayload/drivers... PS1, Line 234: if (FI->physical_address == 0)
Can we also remove FI-> now and just replace it with fbinfo. […]
Finally done in CB:39379.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38537 )
Change subject: libpayload/corebootfb: Keep local copy of framebuffer info ......................................................................
libpayload/corebootfb: Keep local copy of framebuffer info
Keeping a local copy of the framebuffer info allows us to make changes, e.g. add offsets. It also avoids trouble with relocation.
Change-Id: I852c4eb229dd0724114acb302ab2ed7164712b64 Signed-off-by: Nico Huber nico.huber@secunet.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38537 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M payloads/libpayload/drivers/video/corebootfb.c 1 file changed, 6 insertions(+), 10 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/payloads/libpayload/drivers/video/corebootfb.c b/payloads/libpayload/drivers/video/corebootfb.c index 11397ba..c4b5048 100644 --- a/payloads/libpayload/drivers/video/corebootfb.c +++ b/payloads/libpayload/drivers/video/corebootfb.c @@ -61,13 +61,12 @@ (0xFF << 16) | (0xFF << 8) | 0xFF, };
-/* Addresses for the various components */ -static unsigned long fbinfo; -static unsigned long fbaddr; +struct cb_framebuffer fbinfo; static unsigned short *chars;
-#define FI ((struct cb_framebuffer *) phys_to_virt(fbinfo)) -#define FB ((unsigned char *) phys_to_virt(fbaddr)) +/* Addresses for the various components */ +#define FI (&fbinfo) +#define FB ((unsigned char *) phys_to_virt(FI->physical_address)) #define CHARS (chars)
static void corebootfb_scroll_up(void) @@ -230,12 +229,9 @@ if (lib_sysinfo.framebuffer == NULL) return -1;
- /* We might have been called before relocation (like FILO does). So - just keep the physical address which won't break on relocation. */ - fbinfo = virt_to_phys(lib_sysinfo.framebuffer); + fbinfo = *lib_sysinfo.framebuffer;
- fbaddr = FI->physical_address; - if (fbaddr == 0) + if (FI->physical_address == 0) return -1;
font_init(FI->x_resolution);
-
Angel Pons (Code Review) -
Julius Werner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)