Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
edid_fill_fb: Add row_alignment argument
Calculate bytes_per_line internally if not set by using a new row_alignment argument in bytes. If row_alignment is set to 0 the bytes_per_line will not be aligned.
Future patches will get rid of externally provided bytes_per_line.
Change-Id: I4ce88193c5e324a2a7b04e0a7e3438172afca038 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/yabel/vbe.c M src/drivers/aspeed/common/ast_mode_corebootfb.c M src/drivers/emulation/qemu/bochs.c M src/drivers/emulation/qemu/cirrus.c M src/drivers/intel/fsp1_1/fsp_gop.c M src/drivers/intel/fsp2_0/graphics.c M src/drivers/intel/gma/gma-gfx_init.ads M src/drivers/intel/gma/hires_fb/gma-gfx_init.adb M src/drivers/xgi/common/xgi_coreboot.c M src/include/edid.h M src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/google/daisy/mainboard.c M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/oak/mainboard.c M src/mainboard/google/peach_pit/mainboard.c M src/northbridge/intel/i945/gma.c M src/soc/nvidia/tegra124/display.c M src/soc/rockchip/rk3288/display.c M src/soc/rockchip/rk3399/display.c 21 files changed, 56 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39430/1
diff --git a/src/device/oprom/realmode/x86.c b/src/device/oprom/realmode/x86.c index 660b5f0..fe15c48 100644 --- a/src/device/oprom/realmode/x86.c +++ b/src/device/oprom/realmode/x86.c @@ -377,6 +377,7 @@ le16_to_cpu(mode_info.vesa.y_resolution), le16_to_cpu(mode_info.vesa.bytes_per_scanline), mode_info.vesa.bits_per_pixel, + 0, mode_info.vesa.reserved_mask_pos, mode_info.vesa.reserved_mask_size, mode_info.vesa.red_mask_pos, diff --git a/src/device/oprom/yabel/vbe.c b/src/device/oprom/yabel/vbe.c index e7faf9a..cb7d5b6 100644 --- a/src/device/oprom/yabel/vbe.c +++ b/src/device/oprom/yabel/vbe.c @@ -761,6 +761,7 @@ le16_to_cpu(mode_info.vesa.y_resolution), le16_to_cpu(mode_info.vesa.bytes_per_scanline), mode_info.vesa.bits_per_pixel, + 0, mode_info.vesa.reserved_mask_pos, mode_info.vesa.reserved_mask_size, mode_info.vesa.red_mask_pos, diff --git a/src/drivers/aspeed/common/ast_mode_corebootfb.c b/src/drivers/aspeed/common/ast_mode_corebootfb.c index 2ec85ac..f9a02e3 100644 --- a/src/drivers/aspeed/common/ast_mode_corebootfb.c +++ b/src/drivers/aspeed/common/ast_mode_corebootfb.c @@ -247,7 +247,7 @@ ast_hide_cursor(&crtc);
/* Advertise new mode */ - set_vbe_mode_info_valid(&edid, fb.mmio_addr); + set_vbe_mode_info_valid(&edid, fb.mmio_addr, 8);
/* Clear display */ memset((void *)fb.mmio_addr, 0, edid.bytes_per_line * edid.y_resolution); diff --git a/src/drivers/emulation/qemu/bochs.c b/src/drivers/emulation/qemu/bochs.c index d9e4ce1..91aa413 100644 --- a/src/drivers/emulation/qemu/bochs.c +++ b/src/drivers/emulation/qemu/bochs.c @@ -118,7 +118,7 @@ edid.panel_bits_per_color = 8; edid.panel_bits_per_pixel = 24; edid_set_framebuffer_bits_per_pixel(&edid, 32, 0); - set_vbe_mode_info_valid(&edid, addr); + set_vbe_mode_info_valid(&edid, addr, 0); }
static void bochs_init_text_mode(struct device *dev) diff --git a/src/drivers/emulation/qemu/cirrus.c b/src/drivers/emulation/qemu/cirrus.c index 6b1968c..4968613 100644 --- a/src/drivers/emulation/qemu/cirrus.c +++ b/src/drivers/emulation/qemu/cirrus.c @@ -318,7 +318,7 @@ edid.panel_bits_per_color = 8; edid.panel_bits_per_pixel = 24; edid_set_framebuffer_bits_per_pixel(&edid, 32, 0); - set_vbe_mode_info_valid(&edid, addr); + set_vbe_mode_info_valid(&edid, addr, 0); }
static void cirrus_init_text_mode(struct device *dev) diff --git a/src/drivers/intel/fsp1_1/fsp_gop.c b/src/drivers/intel/fsp1_1/fsp_gop.c index e2c6336..7fcb1c3 100644 --- a/src/drivers/intel/fsp1_1/fsp_gop.c +++ b/src/drivers/intel/fsp1_1/fsp_gop.c @@ -36,7 +36,8 @@ vbt_gop->GraphicsMode.HorizontalResolution, vbt_gop->GraphicsMode.VerticalResolution, vbt_gop->GraphicsMode.PixelsPerScanLine * 4, - 32); + 32, + 0); }
BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, fill_framebuffer_info, NULL); diff --git a/src/drivers/intel/fsp2_0/graphics.c b/src/drivers/intel/fsp2_0/graphics.c index 3d06336..f5a2d2e 100644 --- a/src/drivers/intel/fsp2_0/graphics.c +++ b/src/drivers/intel/fsp2_0/graphics.c @@ -110,6 +110,7 @@ ginfo->vertical_resolution, ginfo->pixels_per_scanline * 4, 32, + 0, fbinfo->rsvd.pos, fbinfo->rsvd.size, fbinfo->red.pos, diff --git a/src/drivers/intel/gma/gma-gfx_init.ads b/src/drivers/intel/gma/gma-gfx_init.ads index 31ab5cf..f9e0fe4 100644 --- a/src/drivers/intel/gma/gma-gfx_init.ads +++ b/src/drivers/intel/gma/gma-gfx_init.ads @@ -16,7 +16,8 @@ x_resolution : word32; y_resolution : word32; bytes_per_line : word32; - bits_per_pixel : word8) + bits_per_pixel : word8; + row_alignment : word32) return Interfaces.C.int;
pragma import (C, c_fb_set_framebuffer_info, "fb_set_framebuffer_info"); diff --git a/src/drivers/intel/gma/hires_fb/gma-gfx_init.adb b/src/drivers/intel/gma/hires_fb/gma-gfx_init.adb index 0ecf14f..2980232 100644 --- a/src/drivers/intel/gma/hires_fb/gma-gfx_init.adb +++ b/src/drivers/intel/gma/hires_fb/gma-gfx_init.adb @@ -21,7 +21,8 @@ x_resolution : word32; y_resolution : word32; bytes_per_line : word32; - bits_per_pixel : word8) + bits_per_pixel : word8; + row_alignment : word32) is use type word32; ignored : Interfaces.C.int; @@ -30,7 +31,8 @@ x_resolution, y_resolution, bytes_per_line, - bits_per_pixel + bits_per_pixel, + row_alignment ); end fb_set_framebuffer_info;
@@ -102,7 +104,8 @@ word32(fb.Width), word32(fb.Height), 4 * word32 (fb.Stride), - 32); + 32, + 16); end if; end if; end if; diff --git a/src/drivers/xgi/common/xgi_coreboot.c b/src/drivers/xgi/common/xgi_coreboot.c index 9c60e3a..b52cad0 100644 --- a/src/drivers/xgi/common/xgi_coreboot.c +++ b/src/drivers/xgi/common/xgi_coreboot.c @@ -363,7 +363,8 @@ xgifb_info->video_width, xgifb_info->video_height, xgifb_info->video_width * xgifb_info->video_bpp, - xgifb_info->video_bpp); + xgifb_info->video_bpp, + 0); } else { /* * FIXME diff --git a/src/include/edid.h b/src/include/edid.h index 70d21eb..b630b14 100644 --- a/src/include/edid.h +++ b/src/include/edid.h @@ -109,7 +109,8 @@ 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); -struct fb_info *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, + uint32_t row_alignment); 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 index 70ec11c..1bdffe0 100644 --- a/src/include/framebuffer_info.h +++ b/src/include/framebuffer_info.h @@ -28,15 +28,15 @@ bool fb_fill_framebuffer_info_ex(struct 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); + uint8_t bits_per_pixel, uint32_t row_alignment, + 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);
struct fb_info *fb_set_framebuffer_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 bits_per_pixel, uint32_t row_alignment);
void fb_set_orientation(struct fb_info *info, enum lb_fb_orientation orientation); diff --git a/src/lib/edid_fill_fb.c b/src/lib/edid_fill_fb.c index 8a1613e..89a62b2 100644 --- a/src/lib/edid_fill_fb.c +++ b/src/lib/edid_fill_fb.c @@ -56,11 +56,11 @@ bool fb_fill_framebuffer_info_ex(struct 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) + uint8_t bits_per_pixel, uint32_t row_alignment, + 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) @@ -77,6 +77,14 @@ y_resolution, (bytes_per_line * y_resolution), (void *)fb_addr);
+ if (!row_alignment) + row_alignment = 1; + + if (!bytes_per_line) + bytes_per_line = x_resolution * bits_per_pixel / 8; + + bytes_per_line = ALIGN_UP(bytes_per_line, row_alignment); + /* Update */ info->edid_fb.physical_address = fb_addr; info->edid_fb.x_resolution = x_resolution; @@ -107,8 +115,8 @@ */ struct fb_info * fb_set_framebuffer_info(uintptr_t fb_addr, uint32_t x_resolution, - uint32_t y_resolution, uint32_t bytes_per_line, - uint8_t bits_per_pixel) + uint32_t y_resolution, uint32_t bytes_per_line, + uint8_t bits_per_pixel, uint32_t row_alignment) { struct fb_info *info = fb_new_framebuffer_info(); if (!info) @@ -119,7 +127,8 @@ case 24: /* packed into 4-byte words */ if (!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)) { + bytes_per_line, bits_per_pixel, row_alignment, 24, 8, 16, 8, 8, 8, + 0, 8)) { printk(BIOS_ERR, "%s: failed to add framebuffer info\n", __func__); free(info); return NULL; @@ -128,7 +137,8 @@ case 16: /* packed into 2-byte words */ if (!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)) { + bytes_per_line, bits_per_pixel, row_alignment, 0, 0, 11, 5, 5, 6, + 0, 5)) { printk(BIOS_ERR, "%s: failed to add framebuffer info\n", __func__); free(info); return NULL; @@ -155,10 +165,11 @@ /* * Take an edid, and create a framebuffer. Set fb_valid to 1. */ -struct fb_info *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, + uint32_t row_alignment) { return fb_set_framebuffer_info(fb_addr, edid->x_resolution, edid->y_resolution, - edid->bytes_per_line, edid->framebuffer_bits_per_pixel); + edid->bytes_per_line, edid->framebuffer_bits_per_pixel, row_alignment); }
int fill_lb_framebuffer(struct lb_framebuffer *framebuffer) diff --git a/src/mainboard/google/daisy/mainboard.c b/src/mainboard/google/daisy/mainboard.c index 30f8805..05479ed 100644 --- a/src/mainboard/google/daisy/mainboard.c +++ b/src/mainboard/google/daisy/mainboard.c @@ -277,7 +277,7 @@
sdmmc_vdd();
- set_vbe_mode_info_valid(&edid, (uintptr_t)fb_addr); + set_vbe_mode_info_valid(&edid, (uintptr_t)fb_addr, 0);
lcd_vdd();
diff --git a/src/mainboard/google/kukui/mainboard.c b/src/mainboard/google/kukui/mainboard.c index 47aa40c..436be30 100644 --- a/src/mainboard/google/kukui/mainboard.c +++ b/src/mainboard/google/kukui/mainboard.c @@ -173,7 +173,7 @@ return false; } mtk_ddp_mode_set(edid); - struct fb_info *info = set_vbe_mode_info_valid(edid, 0); + struct fb_info *info = set_vbe_mode_info_valid(edid, 0, 0); if (info) fb_set_orientation(info, panel->s->orientation);
diff --git a/src/mainboard/google/oak/mainboard.c b/src/mainboard/google/oak/mainboard.c index 421826c..7fe0408 100644 --- a/src/mainboard/google/oak/mainboard.c +++ b/src/mainboard/google/oak/mainboard.c @@ -238,7 +238,7 @@ }
mtk_ddp_mode_set(&edid); - set_vbe_mode_info_valid(&edid, (uintptr_t)0); + set_vbe_mode_info_valid(&edid, (uintptr_t)0, 0); }
static void mainboard_init(struct device *dev) diff --git a/src/mainboard/google/peach_pit/mainboard.c b/src/mainboard/google/peach_pit/mainboard.c index 553c2ad..3b1c8b0 100644 --- a/src/mainboard/google/peach_pit/mainboard.c +++ b/src/mainboard/google/peach_pit/mainboard.c @@ -416,7 +416,7 @@
sdmmc_vdd();
- set_vbe_mode_info_valid(&edid, (uintptr_t)fb_addr); + set_vbe_mode_info_valid(&edid, (uintptr_t)fb_addr, 0);
/* * The reset value for FIMD SYSMMU register MMU_CTRL:0x14640000 diff --git a/src/northbridge/intel/i945/gma.c b/src/northbridge/intel/i945/gma.c index 98e30e7..f1d20f2 100644 --- a/src/northbridge/intel/i945/gma.c +++ b/src/northbridge/intel/i945/gma.c @@ -386,7 +386,7 @@ (void *)pgfx, hactive * vactive * 4); memset((void *)pgfx, 0x00, hactive * vactive * 4);
- set_vbe_mode_info_valid(&edid, pgfx); + set_vbe_mode_info_valid(&edid, pgfx, 64); } else { vga_misc_write(0x67);
diff --git a/src/soc/nvidia/tegra124/display.c b/src/soc/nvidia/tegra124/display.c index 6fa3bdf..d54c3f0 100644 --- a/src/soc/nvidia/tegra124/display.c +++ b/src/soc/nvidia/tegra124/display.c @@ -330,5 +330,5 @@ edid.mode.ha = config->xres; edid_set_framebuffer_bits_per_pixel(&edid, config->framebuffer_bits_per_pixel, 32); - set_vbe_mode_info_valid(&edid, (uintptr_t)(framebuffer_base_mb*MiB)); + set_vbe_mode_info_valid(&edid, (uintptr_t)(framebuffer_base_mb*MiB), 32); } diff --git a/src/soc/rockchip/rk3288/display.c b/src/soc/rockchip/rk3288/display.c index a66b2d4..54aac15 100644 --- a/src/soc/rockchip/rk3288/display.c +++ b/src/soc/rockchip/rk3288/display.c @@ -138,5 +138,5 @@ break; }
- set_vbe_mode_info_valid(&edid, (uintptr_t)lcdbase); + set_vbe_mode_info_valid(&edid, (uintptr_t)lcdbase, 0); } diff --git a/src/soc/rockchip/rk3399/display.c b/src/soc/rockchip/rk3399/display.c index 9cd4053..401c917 100644 --- a/src/soc/rockchip/rk3399/display.c +++ b/src/soc/rockchip/rk3399/display.c @@ -174,7 +174,7 @@ break; } mainboard_power_on_backlight(); - set_vbe_mode_info_valid(&edid, (uintptr_t)0); + set_vbe_mode_info_valid(&edid, (uintptr_t)0, 0);
return; }
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 1:
(1 comment)
Somewhat con
https://review.coreboot.org/c/coreboot/+/39430/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39430/1//COMMIT_MSG@13 PS1, Line 13: Future patches will get rid of externally provided bytes_per_line. Well... when is this gonna happen? I think it only makes sense to do this all at once, otherwise we have even more confusing redundancies floating around.
Hello build bot (Jenkins), Lee Leahy, Julius Werner, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39430
to look at the new patch set (#2).
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
edid_fill_fb: Add row_alignment argument
Calculate bytes_per_line internally by using a new row_alignment argument in bytes. If row_alignment is set to 0 the bytes_per_line will be aligned to bits_per_pixel/8.
Drop bytes_per_line argument as it's now obsolete.
Change-Id: I4ce88193c5e324a2a7b04e0a7e3438172afca038 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/yabel/vbe.c M src/drivers/aspeed/common/ast_mode_corebootfb.c M src/drivers/emulation/qemu/bochs.c M src/drivers/emulation/qemu/cirrus.c M src/drivers/intel/fsp1_1/fsp_gop.c M src/drivers/intel/fsp2_0/graphics.c M src/drivers/intel/gma/gma-gfx_init.ads M src/drivers/intel/gma/hires_fb/gma-gfx_init.adb M src/drivers/xgi/common/xgi_coreboot.c M src/include/edid.h M src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/emulation/qemu-armv7/mainboard.c M src/mainboard/google/daisy/mainboard.c M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/oak/mainboard.c M src/mainboard/google/peach_pit/mainboard.c M src/northbridge/intel/i945/gma.c M src/soc/nvidia/tegra124/display.c M src/soc/nvidia/tegra210/dc.c M src/soc/rockchip/rk3288/display.c M src/soc/rockchip/rk3399/display.c 23 files changed, 61 insertions(+), 54 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39430/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/39430/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39430/2//COMMIT_MSG@13 PS2, Line 13: Drop bytes_per_line argument as it's now obsolete. Please also remove it from struct edid and everything accessing it there.
Hello build bot (Jenkins), Lee Leahy, Julius Werner, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39430
to look at the new patch set (#3).
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
edid_fill_fb: Add row_alignment argument
Calculate bytes_per_line internally by using a new row_alignment argument in bytes. If row_alignment is set to 0 the bytes_per_line will be aligned to bits_per_pixel/8.
Drop bytes_per_line argument as it's now obsolete.
Change-Id: I4ce88193c5e324a2a7b04e0a7e3438172afca038 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/yabel/vbe.c M src/drivers/aspeed/common/ast_mode_corebootfb.c M src/drivers/emulation/qemu/bochs.c M src/drivers/emulation/qemu/cirrus.c M src/drivers/intel/fsp1_1/fsp_gop.c M src/drivers/intel/fsp2_0/graphics.c M src/drivers/intel/gma/gma-gfx_init.ads M src/drivers/intel/gma/hires_fb/gma-gfx_init.adb M src/drivers/xgi/common/xgi_coreboot.c M src/include/edid.h M src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/emulation/qemu-armv7/mainboard.c M src/mainboard/google/daisy/mainboard.c M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/oak/mainboard.c M src/mainboard/google/peach_pit/mainboard.c M src/northbridge/intel/i945/gma.c M src/soc/nvidia/tegra124/display.c M src/soc/nvidia/tegra210/dc.c M src/soc/rockchip/rk3288/display.c M src/soc/rockchip/rk3399/display.c 23 files changed, 83 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39430/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39430/3/src/device/oprom/realmode/x... File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/c/coreboot/+/39430/3/src/device/oprom/realmode/x... PS3, Line 370: uint32_t row_alignment = fb_helper_get_row_alignement( 'alignement' may be misspelled - perhaps 'alignment'?
https://review.coreboot.org/c/coreboot/+/39430/3/src/device/oprom/yabel/vbe.... File src/device/oprom/yabel/vbe.c:
https://review.coreboot.org/c/coreboot/+/39430/3/src/device/oprom/yabel/vbe.... PS3, Line 754: uint32_t row_alignment = fb_helper_get_row_alignement( 'alignement' may be misspelled - perhaps 'alignment'?
https://review.coreboot.org/c/coreboot/+/39430/3/src/include/framebuffer_inf... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39430/3/src/include/framebuffer_inf... PS3, Line 45: * To be used by drivers that don't know the hardware dependend alignment, for example the 'dependend' may be misspelled - perhaps 'dependent'?
https://review.coreboot.org/c/coreboot/+/39430/3/src/include/framebuffer_inf... PS3, Line 48: static inline uint32_t fb_helper_get_row_alignement(uint32_t scanline_bytes, 'alignement' may be misspelled - perhaps 'alignment'?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39430/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39430/2//COMMIT_MSG@13 PS2, Line 13: Drop bytes_per_line argument as it's now obsolete.
Please also remove it from struct edid and everything accessing it there.
Done in CB:39773
Hello build bot (Jenkins), Lee Leahy, Julius Werner, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39430
to look at the new patch set (#4).
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
edid_fill_fb: Add row_alignment argument
Calculate bytes_per_line internally by using a new row_alignment argument in bytes. If row_alignment is set to 0 the bytes_per_line will be aligned to bits_per_pixel/8.
Drop bytes_per_line argument as it's now obsolete.
Change-Id: I4ce88193c5e324a2a7b04e0a7e3438172afca038 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/yabel/vbe.c M src/drivers/aspeed/common/ast_mode_corebootfb.c M src/drivers/emulation/qemu/bochs.c M src/drivers/emulation/qemu/cirrus.c M src/drivers/intel/fsp1_1/fsp_gop.c M src/drivers/intel/fsp2_0/graphics.c M src/drivers/intel/gma/gma-gfx_init.ads M src/drivers/intel/gma/hires_fb/gma-gfx_init.adb M src/drivers/xgi/common/xgi_coreboot.c M src/include/edid.h M src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/emulation/qemu-armv7/mainboard.c M src/mainboard/google/daisy/mainboard.c M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/oak/mainboard.c M src/mainboard/google/peach_pit/mainboard.c M src/northbridge/intel/i945/gma.c M src/soc/nvidia/tegra124/display.c M src/soc/nvidia/tegra210/dc.c M src/soc/rockchip/rk3288/display.c M src/soc/rockchip/rk3399/display.c 23 files changed, 83 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39430/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39430/4/src/device/oprom/realmode/x... File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/c/coreboot/+/39430/4/src/device/oprom/realmode/x... PS4, Line 370: uint32_t row_alignment = fb_helper_get_row_alignement( 'alignement' may be misspelled - perhaps 'alignment'?
https://review.coreboot.org/c/coreboot/+/39430/4/src/device/oprom/yabel/vbe.... File src/device/oprom/yabel/vbe.c:
https://review.coreboot.org/c/coreboot/+/39430/4/src/device/oprom/yabel/vbe.... PS4, Line 754: uint32_t row_alignment = fb_helper_get_row_alignement( 'alignement' may be misspelled - perhaps 'alignment'?
https://review.coreboot.org/c/coreboot/+/39430/4/src/include/framebuffer_inf... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39430/4/src/include/framebuffer_inf... PS4, Line 45: * To be used by drivers that don't know the hardware dependend alignment, for example the 'dependend' may be misspelled - perhaps 'dependent'?
https://review.coreboot.org/c/coreboot/+/39430/4/src/include/framebuffer_inf... PS4, Line 48: static inline uint32_t fb_helper_get_row_alignement(uint32_t scanline_bytes, 'alignement' may be misspelled - perhaps 'alignment'?
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39430/5/src/device/oprom/realmode/x... File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/c/coreboot/+/39430/5/src/device/oprom/realmode/x... PS5, Line 370: uint32_t row_alignment = fb_helper_get_row_alignement( 'alignement' may be misspelled - perhaps 'alignment'?
https://review.coreboot.org/c/coreboot/+/39430/5/src/device/oprom/yabel/vbe.... File src/device/oprom/yabel/vbe.c:
https://review.coreboot.org/c/coreboot/+/39430/5/src/device/oprom/yabel/vbe.... PS5, Line 754: uint32_t row_alignment = fb_helper_get_row_alignement( 'alignement' may be misspelled - perhaps 'alignment'?
https://review.coreboot.org/c/coreboot/+/39430/5/src/include/framebuffer_inf... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39430/5/src/include/framebuffer_inf... PS5, Line 45: * To be used by drivers that don't know the hardware dependend alignment, for example the 'dependend' may be misspelled - perhaps 'dependent'?
https://review.coreboot.org/c/coreboot/+/39430/5/src/include/framebuffer_inf... PS5, Line 48: static inline uint32_t fb_helper_get_row_alignement(uint32_t scanline_bytes, 'alignement' may be misspelled - perhaps 'alignment'?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39430/5/src/include/framebuffer_inf... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39430/5/src/include/framebuffer_inf... PS5, Line 64: return row_alignment; Sorry, where's this function used? I don't see it.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39430/5/src/include/framebuffer_inf... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39430/5/src/include/framebuffer_inf... PS5, Line 64: return row_alignment;
Sorry, where's this function used? I don't see it.
src/device/oprom/* in vbe_set_graphics()
Hello build bot (Jenkins), Lee Leahy, Julius Werner, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39430
to look at the new patch set (#6).
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
edid_fill_fb: Add row_alignment argument
Calculate bytes_per_line internally by using a new row_alignment argument in bytes. If row_alignment is set to 0 the bytes_per_line will be aligned to bits_per_pixel/8.
Drop bytes_per_line argument as it's now obsolete.
Change-Id: I4ce88193c5e324a2a7b04e0a7e3438172afca038 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/yabel/vbe.c M src/drivers/aspeed/common/ast_mode_corebootfb.c M src/drivers/emulation/qemu/bochs.c M src/drivers/emulation/qemu/cirrus.c M src/drivers/intel/fsp1_1/fsp_gop.c M src/drivers/intel/fsp2_0/graphics.c M src/drivers/intel/gma/gma-gfx_init.ads M src/drivers/intel/gma/hires_fb/gma-gfx_init.adb M src/drivers/xgi/common/xgi_coreboot.c M src/include/edid.h M src/include/framebuffer_info.h M src/lib/edid_fill_fb.c M src/mainboard/emulation/qemu-armv7/mainboard.c M src/mainboard/google/daisy/mainboard.c M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/oak/mainboard.c M src/mainboard/google/peach_pit/mainboard.c M src/northbridge/intel/i945/gma.c M src/soc/nvidia/tegra124/display.c M src/soc/nvidia/tegra210/dc.c M src/soc/rockchip/rk3288/display.c M src/soc/rockchip/rk3399/display.c 23 files changed, 60 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39430/6
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39430/5/src/include/framebuffer_inf... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39430/5/src/include/framebuffer_inf... PS5, Line 64: return row_alignment;
src/device/oprom/* in vbe_set_graphics()
Ack
https://review.coreboot.org/c/coreboot/+/39430/6/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39430/6/src/lib/edid_fill_fb.c@81 PS6, Line 81: bytes_per_line = ALIGN_UP(x_resolution * bits_per_pixel / 8, row_alignment); Like I mentioned in CB:39002 I think this is a problem. bits_per_pixel the way we usually use it can be 24, but in that case the framebuffer pixels are still 32 bits apart.
The way you get the real bytes-per-pixel here is by adding up red_mask_size, greeen_mask_size, blue_mask_size and reserved_mask_size. (Should probably assert() that that result is divisible by 8, too.)
Hello build bot (Jenkins), Lee Leahy, Julius Werner, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39430
to look at the new patch set (#7).
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
edid_fill_fb: Add row_alignment argument
Calculate bytes_per_line internally by using a new row_alignment argument in bytes. If row_alignment is set to 0 the bytes_per_line will be aligned to bits_per_pixel/8.
Drop bytes_per_line argument as it's now obsolete.
Change-Id: I4ce88193c5e324a2a7b04e0a7e3438172afca038 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/yabel/vbe.c M src/drivers/aspeed/common/ast_mode_corebootfb.c M src/drivers/emulation/qemu/bochs.c M src/drivers/emulation/qemu/cirrus.c M src/drivers/intel/fsp1_1/fsp_gop.c M src/drivers/intel/fsp2_0/graphics.c M src/drivers/intel/gma/gma-gfx_init.ads M src/drivers/intel/gma/hires_fb/gma-gfx_init.adb M src/drivers/xgi/common/xgi_coreboot.c M src/include/edid.h M src/include/framebuffer_info.h M src/include/vbe.h M src/lib/edid_fill_fb.c M src/mainboard/emulation/qemu-armv7/mainboard.c M src/mainboard/google/daisy/mainboard.c M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/oak/mainboard.c M src/mainboard/google/peach_pit/mainboard.c M src/northbridge/intel/i945/gma.c M src/soc/nvidia/tegra124/display.c M src/soc/nvidia/tegra210/dc.c M src/soc/rockchip/rk3288/display.c M src/soc/rockchip/rk3399/display.c 24 files changed, 85 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39430/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39430/7/src/include/vbe.h File src/include/vbe.h:
https://review.coreboot.org/c/coreboot/+/39430/7/src/include/vbe.h@125 PS7, Line 125: if (scanline_bytes > h_active) { braces {} are not necessary for single statement blocks
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39430/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39430/7//COMMIT_MSG@11 PS7, Line 11: to bits_per_pixel/8. Please re-flow for 72/75 characters per line.
Hello build bot (Jenkins), Lee Leahy, Julius Werner, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39430
to look at the new patch set (#8).
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
edid_fill_fb: Add row_alignment argument
Calculate bytes_per_line internally by using a new row_alignment argument in bytes. If row_alignment is set to 0 the bytes_per_line will be aligned to bits_per_pixel/8.
Drop bytes_per_line argument as it's now obsolete.
Change-Id: I4ce88193c5e324a2a7b04e0a7e3438172afca038 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/yabel/vbe.c M src/drivers/aspeed/common/ast_mode_corebootfb.c M src/drivers/emulation/qemu/bochs.c M src/drivers/emulation/qemu/cirrus.c M src/drivers/intel/fsp1_1/fsp_gop.c M src/drivers/intel/fsp2_0/graphics.c M src/drivers/intel/gma/gma-gfx_init.ads M src/drivers/intel/gma/hires_fb/gma-gfx_init.adb M src/drivers/xgi/common/xgi_coreboot.c M src/include/edid.h M src/include/framebuffer_info.h M src/include/vbe.h M src/lib/edid_fill_fb.c M src/mainboard/emulation/qemu-armv7/mainboard.c M src/mainboard/google/daisy/mainboard.c M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/oak/mainboard.c M src/mainboard/google/peach_pit/mainboard.c M src/northbridge/intel/i945/gma.c M src/soc/nvidia/tegra124/display.c M src/soc/nvidia/tegra210/dc.c M src/soc/rockchip/rk3288/display.c M src/soc/rockchip/rk3399/display.c 24 files changed, 85 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39430/8
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39430/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39430/7//COMMIT_MSG@11 PS7, Line 11: to bits_per_pixel/8.
Please re-flow for 72/75 characters per line.
Done
https://review.coreboot.org/c/coreboot/+/39430/6/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39430/6/src/lib/edid_fill_fb.c@81 PS6, Line 81: bytes_per_line = ALIGN_UP(x_resolution * bits_per_pixel / 8, row_alignment);
Like I mentioned in CB:39002 I think this is a problem. […]
Added an check to compare bits_per_pixel and the accumulated mask_sizes. If it's not equal an error is printed and the accumulated mask_size is used. Note: This break the RGB888 pixel format, but I guess that's OK for now as 24bpp in coreboot actually means XRGB8.
Hello build bot (Jenkins), Lee Leahy, Julius Werner, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39430
to look at the new patch set (#10).
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
edid_fill_fb: Add row_alignment argument
Calculate bytes_per_line internally by using a new row_alignment argument in bytes. If row_alignment is set to 0 the bytes_per_line will be aligned to bits_per_pixel/8.
Drop bytes_per_line argument as it's now obsolete.
Change-Id: I4ce88193c5e324a2a7b04e0a7e3438172afca038 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/yabel/vbe.c M src/drivers/aspeed/common/ast_mode_corebootfb.c M src/drivers/emulation/qemu/bochs.c M src/drivers/emulation/qemu/cirrus.c M src/drivers/intel/fsp2_0/graphics.c M src/drivers/intel/gma/gma-gfx_init.ads M src/drivers/intel/gma/hires_fb/gma-gfx_init.adb M src/include/edid.h M src/include/framebuffer_info.h M src/include/vbe.h M src/lib/edid_fill_fb.c M src/mainboard/emulation/qemu-armv7/mainboard.c M src/mainboard/google/daisy/mainboard.c M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/oak/mainboard.c M src/mainboard/google/peach_pit/mainboard.c M src/northbridge/intel/i945/gma.c M src/soc/nvidia/tegra124/display.c M src/soc/nvidia/tegra210/dc.c M src/soc/rockchip/rk3288/display.c M src/soc/rockchip/rk3399/display.c 22 files changed, 77 insertions(+), 34 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39430/10
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 10: Code-Review-1
(3 comments)
Sorry for not looking earlier into this. I don't understand it.
What does it fix? What does it provide?
All I see is a positive diffstat (comlexitiy added?), and a working, generic concept (bytes_per_line) replaced by a broken, abstract one that doesn't always apply (bytes_per_line can be arbitrary so far, there is no power-of-2 alignment rule).
(ignore the left-unitialized comments, those are fixed in the next change)
https://review.coreboot.org/c/coreboot/+/39430/10/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-armv7/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39430/10/src/mainboard/emulation/qe... PS10, Line 15: struct edid edid; Left uninitialized?
https://review.coreboot.org/c/coreboot/+/39430/10/src/soc/nvidia/tegra124/di... File src/soc/nvidia/tegra124/display.c:
https://review.coreboot.org/c/coreboot/+/39430/10/src/soc/nvidia/tegra124/di... PS10, Line 315: struct edid edid; Left uninitialized?
https://review.coreboot.org/c/coreboot/+/39430/10/src/soc/nvidia/tegra210/dc... File src/soc/nvidia/tegra210/dc.c:
https://review.coreboot.org/c/coreboot/+/39430/10/src/soc/nvidia/tegra210/dc... PS10, Line 216: struct edid edid; Left uninitialized?
Hello build bot (Jenkins), Nico Huber, Lee Leahy, Julius Werner, Huang Jin, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39430
to look at the new patch set (#11).
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
edid_fill_fb: Add row_alignment argument
Calculate bytes_per_line internally by using a new row_alignment argument in bytes. If row_alignment is set to 0 the bytes_per_line will be aligned to bits_per_pixel/8.
Drop bytes_per_line argument as it's now obsolete.
Change-Id: I4ce88193c5e324a2a7b04e0a7e3438172afca038 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/yabel/vbe.c M src/drivers/aspeed/common/ast_mode_corebootfb.c M src/drivers/emulation/qemu/bochs.c M src/drivers/emulation/qemu/cirrus.c M src/drivers/intel/fsp2_0/graphics.c M src/drivers/intel/gma/gma-gfx_init.ads M src/drivers/intel/gma/hires_fb/gma-gfx_init.adb M src/include/edid.h M src/include/framebuffer_info.h M src/include/vbe.h M src/lib/edid_fill_fb.c M src/mainboard/emulation/qemu-armv7/mainboard.c M src/mainboard/google/daisy/mainboard.c M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/oak/mainboard.c M src/mainboard/google/peach_pit/mainboard.c M src/northbridge/intel/i945/gma.c M src/soc/nvidia/tegra124/display.c M src/soc/nvidia/tegra210/dc.c M src/soc/rockchip/rk3288/display.c M src/soc/rockchip/rk3399/display.c 22 files changed, 77 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39430/11
Christian Walter has uploaded a new patch set (#12) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
edid_fill_fb: Add row_alignment argument
Calculate bytes_per_line internally by using a new row_alignment argument in bytes. If row_alignment is set to 0 the bytes_per_line will be aligned to bits_per_pixel/8.
Drop bytes_per_line argument as it's now obsolete.
Change-Id: I4ce88193c5e324a2a7b04e0a7e3438172afca038 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/yabel/vbe.c M src/drivers/aspeed/common/ast_mode_corebootfb.c M src/drivers/emulation/qemu/bochs.c M src/drivers/emulation/qemu/cirrus.c M src/drivers/intel/fsp1_1/fsp_gop.c M src/drivers/intel/fsp2_0/graphics.c M src/drivers/intel/gma/gma-gfx_init.ads M src/drivers/intel/gma/hires_fb/gma-gfx_init.adb M src/include/edid.h M src/include/framebuffer_info.h M src/include/vbe.h M src/lib/edid_fill_fb.c M src/mainboard/emulation/qemu-armv7/mainboard.c M src/mainboard/google/daisy/mainboard.c M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/oak/mainboard.c M src/mainboard/google/peach_pit/mainboard.c M src/northbridge/intel/i945/gma.c M src/soc/nvidia/tegra124/display.c M src/soc/nvidia/tegra210/dc.c M src/soc/rockchip/rk3288/display.c M src/soc/rockchip/rk3399/display.c 23 files changed, 86 insertions(+), 40 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39430/12
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 12:
(7 comments)
https://review.coreboot.org/c/coreboot/+/39430/12/src/drivers/emulation/qemu... File src/drivers/emulation/qemu/bochs.c:
https://review.coreboot.org/c/coreboot/+/39430/12/src/drivers/emulation/qemu... PS12, Line 148: <<<<<<< HEAD spaces required around that '<' (ctx:OxW)
https://review.coreboot.org/c/coreboot/+/39430/12/src/drivers/emulation/qemu... PS12, Line 150: ======= spaces required around that '==' (ctx:ExO)
https://review.coreboot.org/c/coreboot/+/39430/12/src/drivers/emulation/qemu... PS12, Line 150: ======= spaces required around that '==' (ctx:OxO)
https://review.coreboot.org/c/coreboot/+/39430/12/src/drivers/emulation/qemu... PS12, Line 150: ======= spaces required around that '==' (ctx:OxO)
https://review.coreboot.org/c/coreboot/+/39430/12/src/drivers/emulation/qemu... PS12, Line 150: ======= spaces required around that '=' (ctx:OxE)
https://review.coreboot.org/c/coreboot/+/39430/12/src/drivers/emulation/qemu... PS12, Line 152: >>>>>>> 7dbdc2ac5b... edid_fill_fb: Add row_alignment argument spaces required around that '>' (ctx:OxW)
https://review.coreboot.org/c/coreboot/+/39430/12/src/drivers/emulation/qemu... PS12, Line 152: >>>>>>> 7dbdc2ac5b... edid_fill_fb: Add row_alignment argument spaces required around that ':' (ctx:VxW)
Christian Walter has uploaded a new patch set (#13) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
edid_fill_fb: Add row_alignment argument
Calculate bytes_per_line internally by using a new row_alignment argument in bytes. If row_alignment is set to 0 the bytes_per_line will be aligned to bits_per_pixel/8.
Drop bytes_per_line argument as it's now obsolete.
Change-Id: I4ce88193c5e324a2a7b04e0a7e3438172afca038 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/device/oprom/realmode/x86.c M src/device/oprom/yabel/vbe.c M src/drivers/aspeed/common/ast_mode_corebootfb.c M src/drivers/emulation/qemu/bochs.c M src/drivers/emulation/qemu/cirrus.c M src/drivers/intel/fsp2_0/graphics.c M src/drivers/intel/gma/gma-gfx_init.ads M src/drivers/intel/gma/hires_fb/gma-gfx_init.adb M src/include/edid.h M src/include/framebuffer_info.h M src/include/vbe.h M src/lib/edid_fill_fb.c M src/mainboard/emulation/qemu-armv7/mainboard.c M src/mainboard/google/daisy/mainboard.c M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/oak/mainboard.c M src/mainboard/google/peach_pit/mainboard.c M src/northbridge/intel/i945/gma.c M src/soc/nvidia/tegra124/display.c M src/soc/nvidia/tegra210/dc.c M src/soc/rockchip/rk3288/display.c M src/soc/rockchip/rk3399/display.c 22 files changed, 77 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39430/13
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 13:
What does it fix? What does it provide?
All I see is a positive diffstat (comlexitiy added?), and a working, generic concept (bytes_per_line) replaced by a broken, abstract one that doesn't always apply (bytes_per_line can be arbitrary so far, there is no power-of-2 alignment rule).
I don't really remember but I think I suggested this somewhere hoping that we could then get rid of edid_set_framebuffer_bits_per_pixel() and the redundant x_resolution/y_resolution in struct edid eventually (just because storing the same thing redundantly in multiple forms is never great). Looks like these patches don't quite get there yet.
Fundamentally, the framebuffer layout is defined by the X and Y resolution of the chosen display mode (which is already in edid->mode.ha, so doesn't really need to be slightly adjusted in edid->x_resolution anymore) and the "gap" the display controller expects between the end of one row and the start of the next. In practice I think this gap is always caused by a hardwired alignment (at least all cases I've seen suggest that) so that's why we use row_alignment to model that (which we have been doing with edid_set_framebuffer_bits_per_pixel() for a while already). I think the bytes_per_line is just a hold-over from VBE modes but it's weird because it is always dependent on the display mode (which is decided by the panel, not a fundamental property of the display controller hardware). So when writing native graphics init the thing the SoC display code wants to provide is really just that alignment, and right now the SoC code always has to call this function to translate it into a resolution-dependent value after the panel resolution is known, which is a bit weird and unnecessarily roundabout.
I would kinda like to separate the stuff that comes from the panel (like resolution) which belongs in struct edid from the stuff that comes from display controller hardware (like that alignment) which probably belongs somewhere else. Maybe this patch series isn't the right place to shoehorn that in, I don't know... but in general struct edid has some redundancies and some confusion of purpose (storing info that's really not related to the EDID) that would be nice to get rid of.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 13:
(1 comment)
To be honest, I knew the edid struct is messed up and just avoided it. But that struct shouldn't affect drivers that don't use it anyway.
I would prefer to keep an API for `bytes_per_line`. Maybe just take both arguments and use the maximum or bail out if both aren't 0? I know there is only one case where we don't know an alignment (VBE), but the solution looks too hack'ish for my taste.
If you feel strong about it and want to keep vbe_get_row_ alignment(), please add an elaborate comment so no future reader has to have doubts why it's working.
https://review.coreboot.org/c/coreboot/+/39430/13/src/include/vbe.h File src/include/vbe.h:
https://review.coreboot.org/c/coreboot/+/39430/13/src/include/vbe.h@114 PS13, Line 114: * Calculate row alignment from given parameters. It's not actually calculating the row alignment. It just gives a number that is good enough to reconstruct `bytes_per_line` later. For instance if the row alignment is 64 and `scanline_bytes` is 640, it would just return 1 (which is ok given the context where this is used, but only there).
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 13:
I think the bytes_per_line is just a hold-over from VBE modes but it's weird because it is always dependent on the display mode (which is decided by the panel, not a fundamental property of the display controller hardware). So when writing native graphics init the thing the SoC display code wants to provide is really just that alignment, and right now the SoC code always has to call this function to translate it into a resolution-dependent value after the panel resolution is known, which is a bit weird and unnecessarily roundabout.
It's more than that. The framebuffer resolution is not always the same as the panel resolution (e.g. scaling for high-reso- lution panels, multiple displays). There are cases where the controller can't even handle the panel resolution. Not seen in coreboot, though (e.g. the odd 1366x768 panels, some con- trollers can only do multiples of 8 pixels or so). And there are controllers that want the bytes-per-line programmed, so we also can't say that every driver would calculate it only for coreboot.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39430/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39430/1//COMMIT_MSG@13 PS1, Line 13: Future patches will get rid of externally provided bytes_per_line.
Well... […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 14:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39430/10/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-armv7/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39430/10/src/mainboard/emulation/qe... PS10, Line 15: struct edid edid;
Left uninitialized?
Ack
https://review.coreboot.org/c/coreboot/+/39430/10/src/soc/nvidia/tegra124/di... File src/soc/nvidia/tegra124/display.c:
https://review.coreboot.org/c/coreboot/+/39430/10/src/soc/nvidia/tegra124/di... PS10, Line 315: struct edid edid;
Left uninitialized?
Ack
https://review.coreboot.org/c/coreboot/+/39430/10/src/soc/nvidia/tegra210/dc... File src/soc/nvidia/tegra210/dc.c:
https://review.coreboot.org/c/coreboot/+/39430/10/src/soc/nvidia/tegra210/dc... PS10, Line 216: struct edid edid;
Left uninitialized?
Ack
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39430/13/src/include/vbe.h File src/include/vbe.h:
https://review.coreboot.org/c/coreboot/+/39430/13/src/include/vbe.h@114 PS13, Line 114: * Calculate row alignment from given parameters.
It's not actually calculating the row alignment. It just gives a number […]
As there seems to be no progress here, I tried to document this myself. But then realized that we make assumptions here that we can't control. We're dealing with blobs and don't know what they will do. Two counter-examples came to mind: * The blob might not choose a power-of-2 alignment. * It might use longer lines even without alignment, e.g. `h_active = 512`, `scanline_bytes = 768`, how to ever reconstruct that by aligning? The only possible alignment would be 768, but we can't use that with `ALIGN_UP()` as it's not a power of 2. So we'd either have to support arbitrary alignments, then we could simply return `scanline_bytes` here, or keep the current `bytes_per_line` inter- face as alternative to the `row_alignment` one.
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Christian Walter (Code Review)
-
Julius Werner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)