This series is against tip:efi/core. Patches 1-9 are small cleanups and refactoring of the code in libstub/gop.c. The rest of the patches add the ability to use a command-line option to switch the gop's display mode. The options supported are: video=efifb:mode=n Choose a specific mode number video=efifb:<xres>x<yres>[-(rgb|bgr|<bpp>)] Specify mode by resolution and optionally color depth video=efifb:auto Let the EFI stub choose the highest resolution mode available. The mode-setting additions increase code size of gop.o by about 3k on x86-64 with EFI_MIXED enabled. Arvind Sankar (14): efi/gop: Remove redundant current_fb_base efi/gop: Move check for framebuffer before con_out efi/gop: Get mode information outside the loop efi/gop: Factor out locating the gop into a function efi/gop: Slightly re-arrange logic of find_gop efi/gop: Move variable declarations into loop block efi/gop: Use helper macros for populating lfb_base efi/gop: Use helper macros for find_bits efi/gop: Remove unreachable code from setup_pixel_info efi/gop: Add prototypes for query_mode and set_mode efi/gop: Allow specifying mode number on command line efi/gop: Allow specifying mode by <xres>x<yres> efi/gop: Allow specifying depth as well as resolution efi/gop: Allow automatically choosing the best mode Documentation/fb/efifb.rst | 33 +- arch/x86/include/asm/efi.h | 4 + .../firmware/efi/libstub/efi-stub-helper.c | 3 + drivers/firmware/efi/libstub/efistub.h | 8 +- drivers/firmware/efi/libstub/gop.c | 489 ++++++++++++++---- 5 files changed, 428 insertions(+), 109 deletions(-) -- 2.24.1
current_fb_base isn't used for anything except assigning to fb_base if we locate a suitable gop. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 55e6b3f286fe..f40d535dccb8 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -108,7 +108,6 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID; bool conout_found = false; void *dummy = NULL; - efi_physical_addr_t current_fb_base; status = efi_bs_call(handle_protocol, h, proto, (void **)&gop); if (status != EFI_SUCCESS) @@ -120,7 +119,6 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, mode = efi_table_attr(gop, mode); info = efi_table_attr(mode, info); - current_fb_base = efi_table_attr(mode, frame_buffer_base); if ((!first_gop || conout_found) && info->pixel_format != PIXEL_BLT_ONLY) { @@ -136,7 +134,7 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, pixel_format = info->pixel_format; pixel_info = info->pixel_information; pixels_per_scan_line = info->pixels_per_scan_line; - fb_base = current_fb_base; + fb_base = efi_table_attr(mode, frame_buffer_base); /* * Once we've found a GOP supporting ConOut, -- 2.24.1
If the gop doesn't have a framebuffer, there's no point in checking for con_out support. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index f40d535dccb8..201b66970b2b 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -113,15 +113,16 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, if (status != EFI_SUCCESS) continue; + mode = efi_table_attr(gop, mode); + info = efi_table_attr(mode, info); + if (info->pixel_format == PIXEL_BLT_ONLY) + continue; + status = efi_bs_call(handle_protocol, h, &conout_proto, &dummy); if (status == EFI_SUCCESS) conout_found = true; - mode = efi_table_attr(gop, mode); - info = efi_table_attr(mode, info); - - if ((!first_gop || conout_found) && - info->pixel_format != PIXEL_BLT_ONLY) { + if (!first_gop || conout_found) { /* * Systems that use the UEFI Console Splitter may * provide multiple GOP devices, not all of which are -- 2.24.1
Move extraction of the mode information parameters outside the loop to find the gop, and eliminate some redundant variables. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 38 +++++++++++------------------- 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 201b66970b2b..d692b8c65813 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -89,12 +89,9 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, unsigned long size, void **handles) { efi_graphics_output_protocol_t *gop, *first_gop; - u16 width, height; - u32 pixels_per_scan_line; - u32 ext_lfb_base; + efi_graphics_output_protocol_mode_t *mode; + efi_graphics_output_mode_info_t *info = NULL; efi_physical_addr_t fb_base; - efi_pixel_bitmask_t pixel_info; - int pixel_format; efi_status_t status; efi_handle_t h; int i; @@ -103,8 +100,6 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, gop = NULL; for_each_efi_handle(h, handles, size, i) { - efi_graphics_output_protocol_mode_t *mode; - efi_graphics_output_mode_info_t *info = NULL; efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID; bool conout_found = false; void *dummy = NULL; @@ -129,15 +124,7 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, * backed by real hardware. The workaround is to search * for a GOP implementing the ConOut protocol, and if * one isn't found, to just fall back to the first GOP. - */ - width = info->horizontal_resolution; - height = info->vertical_resolution; - pixel_format = info->pixel_format; - pixel_info = info->pixel_information; - pixels_per_scan_line = info->pixels_per_scan_line; - fb_base = efi_table_attr(mode, frame_buffer_base); - - /* + * * Once we've found a GOP supporting ConOut, * don't bother looking any further. */ @@ -152,21 +139,24 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, return EFI_NOT_FOUND; /* EFI framebuffer */ + mode = efi_table_attr(first_gop, mode); + info = efi_table_attr(mode, info); + si->orig_video_isVGA = VIDEO_TYPE_EFI; - si->lfb_width = width; - si->lfb_height = height; - si->lfb_base = fb_base; + si->lfb_width = info->horizontal_resolution; + si->lfb_height = info->vertical_resolution; - ext_lfb_base = (u64)(unsigned long)fb_base >> 32; - if (ext_lfb_base) { + fb_base = efi_table_attr(mode, frame_buffer_base); + si->lfb_base = fb_base; + si->ext_lfb_base = (u64)(unsigned long)fb_base >> 32; + if (si->ext_lfb_base) si->capabilities |= VIDEO_CAPABILITY_64BIT_BASE; - si->ext_lfb_base = ext_lfb_base; - } si->pages = 1; - setup_pixel_info(si, pixels_per_scan_line, pixel_info, pixel_format); + setup_pixel_info(si, info->pixels_per_scan_line, + info->pixel_information, info->pixel_format); si->lfb_size = si->lfb_linelength * si->lfb_height; -- 2.24.1
Move the loop to find a gop into its own function. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index d692b8c65813..92abcf558845 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -85,19 +85,17 @@ setup_pixel_info(struct screen_info *si, u32 pixels_per_scan_line, } } -static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, - unsigned long size, void **handles) +static efi_graphics_output_protocol_t * +find_gop(efi_guid_t *proto, unsigned long size, void **handles) { efi_graphics_output_protocol_t *gop, *first_gop; efi_graphics_output_protocol_mode_t *mode; efi_graphics_output_mode_info_t *info = NULL; - efi_physical_addr_t fb_base; efi_status_t status; efi_handle_t h; int i; first_gop = NULL; - gop = NULL; for_each_efi_handle(h, handles, size, i) { efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID; @@ -134,12 +132,25 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, } } + return first_gop; +} + +static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, + unsigned long size, void **handles) +{ + efi_graphics_output_protocol_t *gop; + efi_graphics_output_protocol_mode_t *mode; + efi_graphics_output_mode_info_t *info = NULL; + efi_physical_addr_t fb_base; + + gop = find_gop(proto, size, handles); + /* Did we find any GOPs? */ - if (!first_gop) + if (!gop) return EFI_NOT_FOUND; /* EFI framebuffer */ - mode = efi_table_attr(first_gop, mode); + mode = efi_table_attr(gop, mode); info = efi_table_attr(mode, info); si->orig_video_isVGA = VIDEO_TYPE_EFI; -- 2.24.1
Small cleanup to get rid of conout_found. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 92abcf558845..a7d3efe36c78 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -99,7 +99,6 @@ find_gop(efi_guid_t *proto, unsigned long size, void **handles) for_each_efi_handle(h, handles, size, i) { efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID; - bool conout_found = false; void *dummy = NULL; status = efi_bs_call(handle_protocol, h, proto, (void **)&gop); @@ -111,25 +110,22 @@ find_gop(efi_guid_t *proto, unsigned long size, void **handles) if (info->pixel_format == PIXEL_BLT_ONLY) continue; + /* + * Systems that use the UEFI Console Splitter may + * provide multiple GOP devices, not all of which are + * backed by real hardware. The workaround is to search + * for a GOP implementing the ConOut protocol, and if + * one isn't found, to just fall back to the first GOP. + * + * Once we've found a GOP supporting ConOut, + * don't bother looking any further. + */ status = efi_bs_call(handle_protocol, h, &conout_proto, &dummy); if (status == EFI_SUCCESS) - conout_found = true; - - if (!first_gop || conout_found) { - /* - * Systems that use the UEFI Console Splitter may - * provide multiple GOP devices, not all of which are - * backed by real hardware. The workaround is to search - * for a GOP implementing the ConOut protocol, and if - * one isn't found, to just fall back to the first GOP. - * - * Once we've found a GOP supporting ConOut, - * don't bother looking any further. - */ + return gop; + + if (!first_gop) first_gop = gop; - if (conout_found) - break; - } } return first_gop; -- 2.24.1
Declare the variables inside the block where they're used. Get rid of a couple of redundant initializers. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index a7d3efe36c78..0d195060a370 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -88,16 +88,19 @@ setup_pixel_info(struct screen_info *si, u32 pixels_per_scan_line, static efi_graphics_output_protocol_t * find_gop(efi_guid_t *proto, unsigned long size, void **handles) { - efi_graphics_output_protocol_t *gop, *first_gop; - efi_graphics_output_protocol_mode_t *mode; - efi_graphics_output_mode_info_t *info = NULL; - efi_status_t status; + efi_graphics_output_protocol_t *first_gop; efi_handle_t h; int i; first_gop = NULL; for_each_efi_handle(h, handles, size, i) { + efi_status_t status; + + efi_graphics_output_protocol_t *gop; + efi_graphics_output_protocol_mode_t *mode; + efi_graphics_output_mode_info_t *info; + efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID; void *dummy = NULL; @@ -136,7 +139,7 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, { efi_graphics_output_protocol_t *gop; efi_graphics_output_protocol_mode_t *mode; - efi_graphics_output_mode_info_t *info = NULL; + efi_graphics_output_mode_info_t *info; efi_physical_addr_t fb_base; gop = find_gop(proto, size, handles); -- 2.24.1
Use the lower/upper_32_bits macros from kernel.h to initialize si->lfb_base and si->ext_lfb_base. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 0d195060a370..7b0baf9a912f 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -158,8 +158,8 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, si->lfb_height = info->vertical_resolution; fb_base = efi_table_attr(mode, frame_buffer_base); - si->lfb_base = fb_base; - si->ext_lfb_base = (u64)(unsigned long)fb_base >> 32; + si->lfb_base = lower_32_bits(fb_base); + si->ext_lfb_base = upper_32_bits(fb_base); if (si->ext_lfb_base) si->capabilities |= VIDEO_CAPABILITY_64BIT_BASE; -- 2.24.1
Use the __ffs/__fls macros to calculate the position and size of the mask. Correct type of mask to u32 instead of unsigned long. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 7b0baf9a912f..8bf424f35759 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -5,6 +5,7 @@ * * ----------------------------------------------------------------------- */ +#include <linux/bitops.h> #include <linux/efi.h> #include <linux/screen_info.h> #include <asm/efi.h> @@ -12,27 +13,16 @@ #include "efistub.h" -static void find_bits(unsigned long mask, u8 *pos, u8 *size) +static void find_bits(u32 mask, u8 *pos, u8 *size) { - u8 first, len; - - first = 0; - len = 0; - - if (mask) { - while (!(mask & 0x1)) { - mask = mask >> 1; - first++; - } - - while (mask & 0x1) { - mask = mask >> 1; - len++; - } + if (!mask) { + *pos = *size = 0; + return; } - *pos = first; - *size = len; + /* UEFI spec guarantees that the set bits are contiguous */ + *pos = __ffs(mask); + *size = __fls(mask) - *pos + 1; } static void -- 2.24.1
pixel_format must be one of PIXEL_RGB_RESERVED_8BIT_PER_COLOR PIXEL_BGR_RESERVED_8BIT_PER_COLOR PIXEL_BIT_MASK since we skip PIXEL_BLT_ONLY when finding a gop. Remove the redundant code and add another check in find_gop to skip any pixel formats that we don't know about, in case a later version of the UEFI spec adds one. Reformat the code a little. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 66 ++++++++++++------------------ 1 file changed, 26 insertions(+), 40 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 8bf424f35759..2d91699e3061 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -29,49 +29,34 @@ static void setup_pixel_info(struct screen_info *si, u32 pixels_per_scan_line, efi_pixel_bitmask_t pixel_info, int pixel_format) { - if (pixel_format == PIXEL_RGB_RESERVED_8BIT_PER_COLOR) { - si->lfb_depth = 32; - si->lfb_linelength = pixels_per_scan_line * 4; - si->red_size = 8; - si->red_pos = 0; - si->green_size = 8; - si->green_pos = 8; - si->blue_size = 8; - si->blue_pos = 16; - si->rsvd_size = 8; - si->rsvd_pos = 24; - } else if (pixel_format == PIXEL_BGR_RESERVED_8BIT_PER_COLOR) { - si->lfb_depth = 32; - si->lfb_linelength = pixels_per_scan_line * 4; - si->red_size = 8; - si->red_pos = 16; - si->green_size = 8; - si->green_pos = 8; - si->blue_size = 8; - si->blue_pos = 0; - si->rsvd_size = 8; - si->rsvd_pos = 24; - } else if (pixel_format == PIXEL_BIT_MASK) { - find_bits(pixel_info.red_mask, &si->red_pos, &si->red_size); - find_bits(pixel_info.green_mask, &si->green_pos, - &si->green_size); - find_bits(pixel_info.blue_mask, &si->blue_pos, &si->blue_size); - find_bits(pixel_info.reserved_mask, &si->rsvd_pos, - &si->rsvd_size); + if (pixel_format == PIXEL_BIT_MASK) { + find_bits(pixel_info.red_mask, + &si->red_pos, &si->red_size); + find_bits(pixel_info.green_mask, + &si->green_pos, &si->green_size); + find_bits(pixel_info.blue_mask, + &si->blue_pos, &si->blue_size); + find_bits(pixel_info.reserved_mask, + &si->rsvd_pos, &si->rsvd_size); si->lfb_depth = si->red_size + si->green_size + si->blue_size + si->rsvd_size; si->lfb_linelength = (pixels_per_scan_line * si->lfb_depth) / 8; } else { - si->lfb_depth = 4; - si->lfb_linelength = si->lfb_width / 2; - si->red_size = 0; - si->red_pos = 0; - si->green_size = 0; - si->green_pos = 0; - si->blue_size = 0; - si->blue_pos = 0; - si->rsvd_size = 0; - si->rsvd_pos = 0; + if (pixel_format == PIXEL_RGB_RESERVED_8BIT_PER_COLOR) { + si->red_pos = 0; + si->blue_pos = 16; + } else /* PIXEL_BGR_RESERVED_8BIT_PER_COLOR */ { + si->blue_pos = 0; + si->red_pos = 16; + } + + si->green_pos = 8; + si->rsvd_pos = 24; + si->red_size = si->green_size = + si->blue_size = si->rsvd_size = 8; + + si->lfb_depth = 32; + si->lfb_linelength = pixels_per_scan_line * 4; } } @@ -100,7 +85,8 @@ find_gop(efi_guid_t *proto, unsigned long size, void **handles) mode = efi_table_attr(gop, mode); info = efi_table_attr(mode, info); - if (info->pixel_format == PIXEL_BLT_ONLY) + if (info->pixel_format == PIXEL_BLT_ONLY || + info->pixel_format >= PIXEL_FORMAT_MAX) continue; /* -- 2.24.1
Add prototypes and argmap for the Graphics Output Protocol's QueryMode and SetMode functions. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- arch/x86/include/asm/efi.h | 4 ++++ drivers/firmware/efi/libstub/efistub.h | 6 ++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index cdcf48d52a12..781170d36f50 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -305,6 +305,10 @@ static inline u32 efi64_convert_status(efi_status_t status) #define __efi64_argmap_load_file(protocol, path, policy, bufsize, buf) \ ((protocol), (path), (policy), efi64_zero_upper(bufsize), (buf)) +/* Graphics Output Protocol */ +#define __efi64_argmap_query_mode(gop, mode, size, info) \ + ((gop), (mode), efi64_zero_upper(size), efi64_zero_upper(info)) + /* * The macros below handle the plumbing for the argument mapping. To add a * mapping for a specific EFI method, simply define a macro diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h index cc90a748bcf0..c400fd88fe38 100644 --- a/drivers/firmware/efi/libstub/efistub.h +++ b/drivers/firmware/efi/libstub/efistub.h @@ -298,8 +298,10 @@ typedef union efi_graphics_output_protocol efi_graphics_output_protocol_t; union efi_graphics_output_protocol { struct { - void *query_mode; - void *set_mode; + efi_status_t (__efiapi *query_mode)(efi_graphics_output_protocol_t *, + u32, unsigned long *, + efi_graphics_output_mode_info_t **); + efi_status_t (__efiapi *set_mode) (efi_graphics_output_protocol_t *, u32); void *blt; efi_graphics_output_protocol_mode_t *mode; }; -- 2.24.1
Add the ability to choose a video mode for the selected gop by using a command-line argument of the form video=efifb:mode=<n> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- Documentation/fb/efifb.rst | 20 +++- .../firmware/efi/libstub/efi-stub-helper.c | 3 + drivers/firmware/efi/libstub/efistub.h | 2 + drivers/firmware/efi/libstub/gop.c | 107 ++++++++++++++++++ 4 files changed, 129 insertions(+), 3 deletions(-) diff --git a/Documentation/fb/efifb.rst b/Documentation/fb/efifb.rst index 04840331a00e..367fbda2f4da 100644 --- a/Documentation/fb/efifb.rst +++ b/Documentation/fb/efifb.rst @@ -2,8 +2,10 @@ What is efifb? ============== -This is a generic EFI platform driver for Intel based Apple computers. -efifb is only for EFI booted Intel Macs. +This is a generic EFI platform driver for systems with UEFI firmware. The +system must be booted via the EFI stub for this to be usable. efifb supports +both firmware with Graphics Output Protocol (GOP) displays as well as older +systems with only Universal Graphics Adapter (UGA) displays. Supported Hardware ================== @@ -12,11 +14,14 @@ Supported Hardware - Macbook - Macbook Pro 15"/17" - MacMini +- ARM/ARM64/X86 systems with UEFI firmware How to use it? ============== -efifb does not have any kind of autodetection of your machine. +For UGA displays, efifb does not have any kind of autodetection of your +machine. + You have to add the following kernel parameters in your elilo.conf:: Macbook : @@ -28,6 +33,9 @@ You have to add the following kernel parameters in your elilo.conf:: Macbook Pro 17", iMac 20" : video=efifb:i20 +For GOP displays, efifb can autodetect the display's resolution and framebuffer +address, so these should work out of the box without any special parameters. + Accepted options: ======= =========================================================== @@ -36,4 +44,10 @@ nowc Don't map the framebuffer write combined. This can be used when large amounts of console data are written. ======= =========================================================== +Options for GOP displays: + +mode=n + The EFI stub will set the mode of the display to mode number n if + possible. + Edgar Hucek <gimli@dark-green.com> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index 9f34c7242939..c6092b6038cf 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -105,6 +105,9 @@ efi_status_t efi_parse_options(char const *cmdline) efi_disable_pci_dma = true; if (parse_option_str(val, "no_disable_early_pci_dma")) efi_disable_pci_dma = false; + } else if (!strcmp(param, "video") && + val && strstarts(val, "efifb:")) { + efi_parse_option_graphics(val + strlen("efifb:")); } } efi_bs_call(free_pool, buf); diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h index c400fd88fe38..4844c3bd40df 100644 --- a/drivers/firmware/efi/libstub/efistub.h +++ b/drivers/firmware/efi/libstub/efistub.h @@ -650,6 +650,8 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr, efi_status_t efi_parse_options(char const *cmdline); +void efi_parse_option_graphics(char *option); + efi_status_t efi_setup_gop(struct screen_info *si, efi_guid_t *proto, unsigned long size); diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 2d91699e3061..34b55715d372 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -8,11 +8,115 @@ #include <linux/bitops.h> #include <linux/efi.h> #include <linux/screen_info.h> +#include <linux/string.h> #include <asm/efi.h> #include <asm/setup.h> #include "efistub.h" +enum efi_cmdline_option { + EFI_CMDLINE_NONE, + EFI_CMDLINE_MODE_NUM, +}; + +static struct { + enum efi_cmdline_option option; + u32 mode; +} __efistub_global cmdline = { .option = EFI_CMDLINE_NONE }; + +static bool parse_modenum(char *option, char **next) +{ + u32 m; + + if (!strstarts(option, "mode=")) + return false; + option += strlen("mode="); + m = simple_strtoull(option, &option, 0); + if (*option && *option++ != ',') + return false; + cmdline.option = EFI_CMDLINE_MODE_NUM; + cmdline.mode = m; + + *next = option; + return true; +} + +void efi_parse_option_graphics(char *option) +{ + while (*option) { + if (parse_modenum(option, &option)) + continue; + + while (*option && *option++ != ',') + ; + } +} + +static u32 choose_mode_modenum(efi_graphics_output_protocol_t *gop) +{ + efi_status_t status; + + efi_graphics_output_protocol_mode_t *mode; + efi_graphics_output_mode_info_t *info; + unsigned long info_size; + + u32 max_mode, cur_mode; + int pf; + + mode = efi_table_attr(gop, mode); + + cur_mode = efi_table_attr(mode, mode); + if (cmdline.mode == cur_mode) + return cur_mode; + + max_mode = efi_table_attr(mode, max_mode); + if (cmdline.mode >= max_mode) { + efi_printk("Requested mode is invalid\n"); + return cur_mode; + } + + status = efi_call_proto(gop, query_mode, cmdline.mode, + &info_size, &info); + if (status != EFI_SUCCESS) { + efi_printk("Couldn't get mode information\n"); + return cur_mode; + } + + pf = info->pixel_format; + + efi_bs_call(free_pool, info); + + if (pf == PIXEL_BLT_ONLY || pf >= PIXEL_FORMAT_MAX) { + efi_printk("Invalid PixelFormat\n"); + return cur_mode; + } + + return cmdline.mode; +} + +static void set_mode(efi_graphics_output_protocol_t *gop) +{ + efi_graphics_output_protocol_mode_t *mode; + u32 cur_mode, new_mode; + + switch (cmdline.option) { + case EFI_CMDLINE_NONE: + return; + case EFI_CMDLINE_MODE_NUM: + new_mode = choose_mode_modenum(gop); + break; + } + + mode = efi_table_attr(gop, mode); + cur_mode = efi_table_attr(mode, mode); + + if (new_mode == cur_mode) + return; + + if (efi_call_proto(gop, set_mode, new_mode) != EFI_SUCCESS) + efi_printk("Failed to set requested mode\n"); +} + static void find_bits(u32 mask, u8 *pos, u8 *size) { if (!mask) { @@ -124,6 +228,9 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, if (!gop) return EFI_NOT_FOUND; + /* Change mode if requested */ + set_mode(gop); + /* EFI framebuffer */ mode = efi_table_attr(gop, mode); info = efi_table_attr(mode, info); -- 2.24.1
Add the ability to choose a video mode using a command-line argument of the form video=efifb:<xres>x<yres> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- Documentation/fb/efifb.rst | 5 ++ drivers/firmware/efi/libstub/gop.c | 84 +++++++++++++++++++++++++++++- 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/Documentation/fb/efifb.rst b/Documentation/fb/efifb.rst index 367fbda2f4da..635275071307 100644 --- a/Documentation/fb/efifb.rst +++ b/Documentation/fb/efifb.rst @@ -50,4 +50,9 @@ mode=n The EFI stub will set the mode of the display to mode number n if possible. +<xres>x<yres> + The EFI stub will search for a display mode that matches the specified + horizontal and vertical resolution, and set the mode of the display to + it if one is found. + Edgar Hucek <gimli@dark-green.com> diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 34b55715d372..2d56efaa1600 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -6,6 +6,7 @@ * ----------------------------------------------------------------------- */ #include <linux/bitops.h> +#include <linux/ctype.h> #include <linux/efi.h> #include <linux/screen_info.h> #include <linux/string.h> @@ -17,11 +18,17 @@ enum efi_cmdline_option { EFI_CMDLINE_NONE, EFI_CMDLINE_MODE_NUM, + EFI_CMDLINE_RES }; static struct { enum efi_cmdline_option option; - u32 mode; + union { + u32 mode; + struct { + u32 width, height; + } res; + }; } __efistub_global cmdline = { .option = EFI_CMDLINE_NONE }; static bool parse_modenum(char *option, char **next) @@ -41,11 +48,33 @@ static bool parse_modenum(char *option, char **next) return true; } +static bool parse_res(char *option, char **next) +{ + u32 w, h; + + if (!isdigit(*option)) + return false; + w = simple_strtoull(option, &option, 10); + if (*option++ != 'x' || !isdigit(*option)) + return false; + h = simple_strtoull(option, &option, 10); + if (*option && *option++ != ',') + return false; + cmdline.option = EFI_CMDLINE_RES; + cmdline.res.width = w; + cmdline.res.height = h; + + *next = option; + return true; +} + void efi_parse_option_graphics(char *option) { while (*option) { if (parse_modenum(option, &option)) continue; + if (parse_res(option, &option)) + continue; while (*option && *option++ != ',') ; @@ -94,6 +123,56 @@ static u32 choose_mode_modenum(efi_graphics_output_protocol_t *gop) return cmdline.mode; } +static u32 choose_mode_res(efi_graphics_output_protocol_t *gop) +{ + efi_status_t status; + + efi_graphics_output_protocol_mode_t *mode; + efi_graphics_output_mode_info_t *info; + unsigned long info_size; + + u32 max_mode, cur_mode; + int pf; + u32 m, w, h; + + mode = efi_table_attr(gop, mode); + + cur_mode = efi_table_attr(mode, mode); + info = efi_table_attr(mode, info); + w = info->horizontal_resolution; + h = info->vertical_resolution; + + if (w == cmdline.res.width && h == cmdline.res.height) + return cur_mode; + + max_mode = efi_table_attr(mode, max_mode); + + for (m = 0; m < max_mode; m++) { + if (m == cur_mode) + continue; + + status = efi_call_proto(gop, query_mode, m, + &info_size, &info); + if (status != EFI_SUCCESS) + continue; + + pf = info->pixel_format; + w = info->horizontal_resolution; + h = info->vertical_resolution; + + efi_bs_call(free_pool, info); + + if (pf == PIXEL_BLT_ONLY || pf >= PIXEL_FORMAT_MAX) + continue; + if (w == cmdline.res.width && h == cmdline.res.height) + return m; + } + + efi_printk("Couldn't find requested mode\n"); + + return cur_mode; +} + static void set_mode(efi_graphics_output_protocol_t *gop) { efi_graphics_output_protocol_mode_t *mode; @@ -105,6 +184,9 @@ static void set_mode(efi_graphics_output_protocol_t *gop) case EFI_CMDLINE_MODE_NUM: new_mode = choose_mode_modenum(gop); break; + case EFI_CMDLINE_RES: + new_mode = choose_mode_res(gop); + break; } mode = efi_table_attr(gop, mode); -- 2.24.1
Extend the video mode argument to handle an optional color depth specification of the form video=efifb:<xres>x<yres>[-(rgb|bgr|<bpp>)] Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- Documentation/fb/efifb.rst | 8 +++-- drivers/firmware/efi/libstub/gop.c | 48 ++++++++++++++++++++++++++---- 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/Documentation/fb/efifb.rst b/Documentation/fb/efifb.rst index 635275071307..eca38466487a 100644 --- a/Documentation/fb/efifb.rst +++ b/Documentation/fb/efifb.rst @@ -50,9 +50,11 @@ mode=n The EFI stub will set the mode of the display to mode number n if possible. -<xres>x<yres> +<xres>x<yres>[-(rgb|bgr|<bpp>)] The EFI stub will search for a display mode that matches the specified - horizontal and vertical resolution, and set the mode of the display to - it if one is found. + horizontal and vertical resolution, and optionally bit depth, and set + the mode of the display to it if one is found. The bit depth can either + "rgb" or "bgr" to match specifically those pixel formats, or a number + for a mode with matching bits per pixel. Edgar Hucek <gimli@dark-green.com> diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 2d56efaa1600..671f812e0b5a 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -27,6 +27,8 @@ static struct { u32 mode; struct { u32 width, height; + int format; + u8 depth; } res; }; } __efistub_global cmdline = { .option = EFI_CMDLINE_NONE }; @@ -50,7 +52,8 @@ static bool parse_modenum(char *option, char **next) static bool parse_res(char *option, char **next) { - u32 w, h; + u32 w, h, d = 0; + int pf = -1; if (!isdigit(*option)) return false; @@ -58,11 +61,26 @@ static bool parse_res(char *option, char **next) if (*option++ != 'x' || !isdigit(*option)) return false; h = simple_strtoull(option, &option, 10); + if (*option == '-') { + option++; + if (strstarts(option, "rgb")) { + option += strlen("rgb"); + pf = PIXEL_RGB_RESERVED_8BIT_PER_COLOR; + } else if (strstarts(option, "bgr")) { + option += strlen("bgr"); + pf = PIXEL_BGR_RESERVED_8BIT_PER_COLOR; + } else if (isdigit(*option)) + d = simple_strtoull(option, &option, 10); + else + return false; + } if (*option && *option++ != ',') return false; cmdline.option = EFI_CMDLINE_RES; cmdline.res.width = w; cmdline.res.height = h; + cmdline.res.format = pf; + cmdline.res.depth = d; *next = option; return true; @@ -123,6 +141,18 @@ static u32 choose_mode_modenum(efi_graphics_output_protocol_t *gop) return cmdline.mode; } +static u8 pixel_bpp(int pixel_format, efi_pixel_bitmask_t pixel_info) +{ + if (pixel_format == PIXEL_BIT_MASK) { + u32 mask = pixel_info.red_mask | pixel_info.green_mask | + pixel_info.blue_mask | pixel_info.reserved_mask; + if (!mask) + return 0; + return __fls(mask) - __ffs(mask) + 1; + } else + return 32; +} + static u32 choose_mode_res(efi_graphics_output_protocol_t *gop) { efi_status_t status; @@ -133,16 +163,21 @@ static u32 choose_mode_res(efi_graphics_output_protocol_t *gop) u32 max_mode, cur_mode; int pf; + efi_pixel_bitmask_t pi; u32 m, w, h; mode = efi_table_attr(gop, mode); cur_mode = efi_table_attr(mode, mode); info = efi_table_attr(mode, info); - w = info->horizontal_resolution; - h = info->vertical_resolution; + pf = info->pixel_format; + pi = info->pixel_information; + w = info->horizontal_resolution; + h = info->vertical_resolution; - if (w == cmdline.res.width && h == cmdline.res.height) + if (w == cmdline.res.width && h == cmdline.res.height && + (cmdline.res.format < 0 || cmdline.res.format == pf) && + (!cmdline.res.depth || cmdline.res.depth == pixel_bpp(pf, pi))) return cur_mode; max_mode = efi_table_attr(mode, max_mode); @@ -157,6 +192,7 @@ static u32 choose_mode_res(efi_graphics_output_protocol_t *gop) continue; pf = info->pixel_format; + pi = info->pixel_information; w = info->horizontal_resolution; h = info->vertical_resolution; @@ -164,7 +200,9 @@ static u32 choose_mode_res(efi_graphics_output_protocol_t *gop) if (pf == PIXEL_BLT_ONLY || pf >= PIXEL_FORMAT_MAX) continue; - if (w == cmdline.res.width && h == cmdline.res.height) + if (w == cmdline.res.width && h == cmdline.res.height && + (cmdline.res.format < 0 || cmdline.res.format == pf) && + (!cmdline.res.depth || cmdline.res.depth == pixel_bpp(pf, pi))) return m; } -- 2.24.1
Add the ability to automatically pick the highest resolution video mode (defined as the product of vertical and horizontal resolution) by using a command-line argument of the form video=efifb:auto If there are multiple modes with the highest resolution, pick one with the highest color depth. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- Documentation/fb/efifb.rst | 6 +++ drivers/firmware/efi/libstub/gop.c | 81 +++++++++++++++++++++++++++++- 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/Documentation/fb/efifb.rst b/Documentation/fb/efifb.rst index eca38466487a..519550517fd4 100644 --- a/Documentation/fb/efifb.rst +++ b/Documentation/fb/efifb.rst @@ -57,4 +57,10 @@ mode=n "rgb" or "bgr" to match specifically those pixel formats, or a number for a mode with matching bits per pixel. +auto + The EFI stub will choose the mode with the highest resolution (product + of horizontal and vertical resolution). If there are multiple modes + with the highest resolution, it will choose one with the highest color + depth. + Edgar Hucek <gimli@dark-green.com> diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 671f812e0b5a..affdcb6cca9a 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -18,7 +18,8 @@ enum efi_cmdline_option { EFI_CMDLINE_NONE, EFI_CMDLINE_MODE_NUM, - EFI_CMDLINE_RES + EFI_CMDLINE_RES, + EFI_CMDLINE_AUTO }; static struct { @@ -86,6 +87,19 @@ static bool parse_res(char *option, char **next) return true; } +static bool parse_auto(char *option, char **next) +{ + if (!strstarts(option, "auto")) + return false; + option += strlen("auto"); + if (*option && *option++ != ',') + return false; + cmdline.option = EFI_CMDLINE_AUTO; + + *next = option; + return true; +} + void efi_parse_option_graphics(char *option) { while (*option) { @@ -93,6 +107,8 @@ void efi_parse_option_graphics(char *option) continue; if (parse_res(option, &option)) continue; + if (parse_auto(option, &option)) + continue; while (*option && *option++ != ',') ; @@ -211,6 +227,66 @@ static u32 choose_mode_res(efi_graphics_output_protocol_t *gop) return cur_mode; } +static u32 choose_mode_auto(efi_graphics_output_protocol_t *gop) +{ + efi_status_t status; + + efi_graphics_output_protocol_mode_t *mode; + efi_graphics_output_mode_info_t *info; + unsigned long info_size; + + u32 max_mode, cur_mode, best_mode, area; + u8 depth; + int pf; + efi_pixel_bitmask_t pi; + u32 m, w, h, a; + u8 d; + + mode = efi_table_attr(gop, mode); + + cur_mode = efi_table_attr(mode, mode); + max_mode = efi_table_attr(mode, max_mode); + + info = efi_table_attr(mode, info); + + pf = info->pixel_format; + pi = info->pixel_information; + w = info->horizontal_resolution; + h = info->vertical_resolution; + + best_mode = cur_mode; + area = w * h; + depth = pixel_bpp(pf, pi); + + for (m = 0; m < max_mode; m++) { + status = efi_call_proto(gop, query_mode, m, + &info_size, &info); + if (status != EFI_SUCCESS) + continue; + + pf = info->pixel_format; + pi = info->pixel_information; + w = info->horizontal_resolution; + h = info->vertical_resolution; + + efi_bs_call(free_pool, info); + + if (pf == PIXEL_BLT_ONLY || pf >= PIXEL_FORMAT_MAX) + continue; + a = w * h; + if (a < area) + continue; + d = pixel_bpp(pf, pi); + if (a > area || d > depth) { + best_mode = m; + area = a; + depth = d; + } + } + + return best_mode; +} + static void set_mode(efi_graphics_output_protocol_t *gop) { efi_graphics_output_protocol_mode_t *mode; @@ -225,6 +301,9 @@ static void set_mode(efi_graphics_output_protocol_t *gop) case EFI_CMDLINE_RES: new_mode = choose_mode_res(gop); break; + case EFI_CMDLINE_AUTO: + new_mode = choose_mode_auto(gop); + break; } mode = efi_table_attr(gop, mode); -- 2.24.1
On Thu, 19 Mar 2020 at 15:28, Arvind Sankar <nivedita@alum.mit.edu> wrote: > > This series is against tip:efi/core. > > Patches 1-9 are small cleanups and refactoring of the code in > libstub/gop.c. > > The rest of the patches add the ability to use a command-line option to > switch the gop's display mode. > > The options supported are: > video=efifb:mode=n > Choose a specific mode number > video=efifb:<xres>x<yres>[-(rgb|bgr|<bpp>)] > Specify mode by resolution and optionally color depth > video=efifb:auto > Let the EFI stub choose the highest resolution mode available. > > The mode-setting additions increase code size of gop.o by about 3k on > x86-64 with EFI_MIXED enabled. > > Arvind Sankar (14): > efi/gop: Remove redundant current_fb_base > efi/gop: Move check for framebuffer before con_out > efi/gop: Get mode information outside the loop > efi/gop: Factor out locating the gop into a function > efi/gop: Slightly re-arrange logic of find_gop > efi/gop: Move variable declarations into loop block > efi/gop: Use helper macros for populating lfb_base > efi/gop: Use helper macros for find_bits > efi/gop: Remove unreachable code from setup_pixel_info > efi/gop: Add prototypes for query_mode and set_mode > efi/gop: Allow specifying mode number on command line > efi/gop: Allow specifying mode by <xres>x<yres> > efi/gop: Allow specifying depth as well as resolution > efi/gop: Allow automatically choosing the best mode > Thanks for this! I like it a lot. Adding Hans to cc as he has been working on seamless fb handover. I will review this somewhere next week. > Documentation/fb/efifb.rst | 33 +- > arch/x86/include/asm/efi.h | 4 + > .../firmware/efi/libstub/efi-stub-helper.c | 3 + > drivers/firmware/efi/libstub/efistub.h | 8 +- > drivers/firmware/efi/libstub/gop.c | 489 ++++++++++++++---- > 5 files changed, 428 insertions(+), 109 deletions(-) > > -- > 2.24.1 >
[-- Attachment #1: Type: text/plain, Size: 2113 bytes --] Hi Arvind, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on efi/next] [also build test WARNING on next-20200319] [cannot apply to linux/master linus/master v5.6-rc6] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Arvind-Sankar/efi-gop-Refactoring-mode-setting-feature/20200320-044605 base: https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next config: arm-multi_v7_defconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (GCC) 9.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=9.2.0 make.cross ARCH=arm If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> drivers/firmware/efi/libstub/gop.c:25:1: warning: 'section' attribute does not apply to types [-Wattributes] 25 | } __efistub_global cmdline = { .option = EFI_CMDLINE_NONE }; | ^ drivers/firmware/efi/libstub/gop.c: In function 'efi_setup_gop': drivers/firmware/efi/libstub/gop.c:116:21: warning: 'new_mode' may be used uninitialized in this function [-Wmaybe-uninitialized] 116 | if (efi_call_proto(gop, set_mode, new_mode) != EFI_SUCCESS) | ^~~ drivers/firmware/efi/libstub/gop.c:100:16: note: 'new_mode' was declared here 100 | u32 cur_mode, new_mode; | ^~~~~~~~ vim +/section +25 drivers/firmware/efi/libstub/gop.c 21 22 static struct { 23 enum efi_cmdline_option option; 24 u32 mode; > 25 } __efistub_global cmdline = { .option = EFI_CMDLINE_NONE }; 26 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 49915 bytes --]
[-- Attachment #1: Type: text/plain, Size: 2542 bytes --] Hi Arvind, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on efi/next] [also build test WARNING on next-20200319] [cannot apply to linux/master linus/master v5.6-rc6] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Arvind-Sankar/efi-gop-Refactoring-mode-setting-feature/20200320-044605 base: https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next config: arm64-defconfig (attached as .config) compiler: aarch64-linux-gcc (GCC) 9.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=9.2.0 make.cross ARCH=arm64 If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> Note: it may well be a FALSE warning. FWIW you are at least aware of it now. http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings All warnings (new ones prefixed by >>): drivers/firmware/efi/libstub/gop.c: In function 'efi_setup_gop': >> drivers/firmware/efi/libstub/gop.c:116:21: warning: 'new_mode' may be used uninitialized in this function [-Wmaybe-uninitialized] 116 | if (efi_call_proto(gop, set_mode, new_mode) != EFI_SUCCESS) | ^~~ drivers/firmware/efi/libstub/gop.c:100:16: note: 'new_mode' was declared here 100 | u32 cur_mode, new_mode; | ^~~~~~~~ vim +/new_mode +116 drivers/firmware/efi/libstub/gop.c 96 97 static void set_mode(efi_graphics_output_protocol_t *gop) 98 { 99 efi_graphics_output_protocol_mode_t *mode; 100 u32 cur_mode, new_mode; 101 102 switch (cmdline.option) { 103 case EFI_CMDLINE_NONE: 104 return; 105 case EFI_CMDLINE_MODE_NUM: 106 new_mode = choose_mode_modenum(gop); 107 break; 108 } 109 110 mode = efi_table_attr(gop, mode); 111 cur_mode = efi_table_attr(mode, mode); 112 113 if (new_mode == cur_mode) 114 return; 115 > 116 if (efi_call_proto(gop, set_mode, new_mode) != EFI_SUCCESS) 117 efi_printk("Failed to set requested mode\n"); 118 } 119 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 47131 bytes --]
This series is against tip:efi/core. Patches 1-9 are small cleanups and refactoring of the code in libstub/gop.c. The rest of the patches add the ability to use a command-line option to switch the gop's display mode. The options supported are: video=efifb:mode=n Choose a specific mode number video=efifb:<xres>x<yres>[-(rgb|bgr|<bpp>)] Specify mode by resolution and optionally color depth video=efifb:auto Let the EFI stub choose the highest resolution mode available. The mode-setting additions increase code size of gop.o by about 3k on x86-64 with EFI_MIXED enabled. Changes in v2 (HT lkp@intel.com): - Fix __efistub_global attribute to be after the variable. (NB: bunch of other places should ideally be fixed, those I guess don't matter as they are scalars?) - Silence -Wmaybe-uninitialized warning in set_mode function. Arvind Sankar (14): efi/gop: Remove redundant current_fb_base efi/gop: Move check for framebuffer before con_out efi/gop: Get mode information outside the loop efi/gop: Factor out locating the gop into a function efi/gop: Slightly re-arrange logic of find_gop efi/gop: Move variable declarations into loop block efi/gop: Use helper macros for populating lfb_base efi/gop: Use helper macros for find_bits efi/gop: Remove unreachable code from setup_pixel_info efi/gop: Add prototypes for query_mode and set_mode efi/gop: Allow specifying mode number on command line efi/gop: Allow specifying mode by <xres>x<yres> efi/gop: Allow specifying depth as well as resolution efi/gop: Allow automatically choosing the best mode Documentation/fb/efifb.rst | 33 +- arch/x86/include/asm/efi.h | 4 + .../firmware/efi/libstub/efi-stub-helper.c | 3 + drivers/firmware/efi/libstub/efistub.h | 8 +- drivers/firmware/efi/libstub/gop.c | 489 ++++++++++++++---- 5 files changed, 428 insertions(+), 109 deletions(-) base-commit: d5528d5e91041e68e8eab9792ce627705a0ed273 -- 2.24.1
current_fb_base isn't used for anything except assigning to fb_base if we locate a suitable gop. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 55e6b3f286fe..f40d535dccb8 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -108,7 +108,6 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID; bool conout_found = false; void *dummy = NULL; - efi_physical_addr_t current_fb_base; status = efi_bs_call(handle_protocol, h, proto, (void **)&gop); if (status != EFI_SUCCESS) @@ -120,7 +119,6 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, mode = efi_table_attr(gop, mode); info = efi_table_attr(mode, info); - current_fb_base = efi_table_attr(mode, frame_buffer_base); if ((!first_gop || conout_found) && info->pixel_format != PIXEL_BLT_ONLY) { @@ -136,7 +134,7 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, pixel_format = info->pixel_format; pixel_info = info->pixel_information; pixels_per_scan_line = info->pixels_per_scan_line; - fb_base = current_fb_base; + fb_base = efi_table_attr(mode, frame_buffer_base); /* * Once we've found a GOP supporting ConOut, -- 2.24.1
If the gop doesn't have a framebuffer, there's no point in checking for con_out support. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index f40d535dccb8..201b66970b2b 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -113,15 +113,16 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, if (status != EFI_SUCCESS) continue; + mode = efi_table_attr(gop, mode); + info = efi_table_attr(mode, info); + if (info->pixel_format == PIXEL_BLT_ONLY) + continue; + status = efi_bs_call(handle_protocol, h, &conout_proto, &dummy); if (status == EFI_SUCCESS) conout_found = true; - mode = efi_table_attr(gop, mode); - info = efi_table_attr(mode, info); - - if ((!first_gop || conout_found) && - info->pixel_format != PIXEL_BLT_ONLY) { + if (!first_gop || conout_found) { /* * Systems that use the UEFI Console Splitter may * provide multiple GOP devices, not all of which are -- 2.24.1
Move extraction of the mode information parameters outside the loop to find the gop, and eliminate some redundant variables. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 38 +++++++++++------------------- 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 201b66970b2b..d692b8c65813 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -89,12 +89,9 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, unsigned long size, void **handles) { efi_graphics_output_protocol_t *gop, *first_gop; - u16 width, height; - u32 pixels_per_scan_line; - u32 ext_lfb_base; + efi_graphics_output_protocol_mode_t *mode; + efi_graphics_output_mode_info_t *info = NULL; efi_physical_addr_t fb_base; - efi_pixel_bitmask_t pixel_info; - int pixel_format; efi_status_t status; efi_handle_t h; int i; @@ -103,8 +100,6 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, gop = NULL; for_each_efi_handle(h, handles, size, i) { - efi_graphics_output_protocol_mode_t *mode; - efi_graphics_output_mode_info_t *info = NULL; efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID; bool conout_found = false; void *dummy = NULL; @@ -129,15 +124,7 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, * backed by real hardware. The workaround is to search * for a GOP implementing the ConOut protocol, and if * one isn't found, to just fall back to the first GOP. - */ - width = info->horizontal_resolution; - height = info->vertical_resolution; - pixel_format = info->pixel_format; - pixel_info = info->pixel_information; - pixels_per_scan_line = info->pixels_per_scan_line; - fb_base = efi_table_attr(mode, frame_buffer_base); - - /* + * * Once we've found a GOP supporting ConOut, * don't bother looking any further. */ @@ -152,21 +139,24 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, return EFI_NOT_FOUND; /* EFI framebuffer */ + mode = efi_table_attr(first_gop, mode); + info = efi_table_attr(mode, info); + si->orig_video_isVGA = VIDEO_TYPE_EFI; - si->lfb_width = width; - si->lfb_height = height; - si->lfb_base = fb_base; + si->lfb_width = info->horizontal_resolution; + si->lfb_height = info->vertical_resolution; - ext_lfb_base = (u64)(unsigned long)fb_base >> 32; - if (ext_lfb_base) { + fb_base = efi_table_attr(mode, frame_buffer_base); + si->lfb_base = fb_base; + si->ext_lfb_base = (u64)(unsigned long)fb_base >> 32; + if (si->ext_lfb_base) si->capabilities |= VIDEO_CAPABILITY_64BIT_BASE; - si->ext_lfb_base = ext_lfb_base; - } si->pages = 1; - setup_pixel_info(si, pixels_per_scan_line, pixel_info, pixel_format); + setup_pixel_info(si, info->pixels_per_scan_line, + info->pixel_information, info->pixel_format); si->lfb_size = si->lfb_linelength * si->lfb_height; -- 2.24.1
Move the loop to find a gop into its own function. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index d692b8c65813..92abcf558845 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -85,19 +85,17 @@ setup_pixel_info(struct screen_info *si, u32 pixels_per_scan_line, } } -static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, - unsigned long size, void **handles) +static efi_graphics_output_protocol_t * +find_gop(efi_guid_t *proto, unsigned long size, void **handles) { efi_graphics_output_protocol_t *gop, *first_gop; efi_graphics_output_protocol_mode_t *mode; efi_graphics_output_mode_info_t *info = NULL; - efi_physical_addr_t fb_base; efi_status_t status; efi_handle_t h; int i; first_gop = NULL; - gop = NULL; for_each_efi_handle(h, handles, size, i) { efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID; @@ -134,12 +132,25 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, } } + return first_gop; +} + +static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, + unsigned long size, void **handles) +{ + efi_graphics_output_protocol_t *gop; + efi_graphics_output_protocol_mode_t *mode; + efi_graphics_output_mode_info_t *info = NULL; + efi_physical_addr_t fb_base; + + gop = find_gop(proto, size, handles); + /* Did we find any GOPs? */ - if (!first_gop) + if (!gop) return EFI_NOT_FOUND; /* EFI framebuffer */ - mode = efi_table_attr(first_gop, mode); + mode = efi_table_attr(gop, mode); info = efi_table_attr(mode, info); si->orig_video_isVGA = VIDEO_TYPE_EFI; -- 2.24.1
Small cleanup to get rid of conout_found. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 92abcf558845..a7d3efe36c78 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -99,7 +99,6 @@ find_gop(efi_guid_t *proto, unsigned long size, void **handles) for_each_efi_handle(h, handles, size, i) { efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID; - bool conout_found = false; void *dummy = NULL; status = efi_bs_call(handle_protocol, h, proto, (void **)&gop); @@ -111,25 +110,22 @@ find_gop(efi_guid_t *proto, unsigned long size, void **handles) if (info->pixel_format == PIXEL_BLT_ONLY) continue; + /* + * Systems that use the UEFI Console Splitter may + * provide multiple GOP devices, not all of which are + * backed by real hardware. The workaround is to search + * for a GOP implementing the ConOut protocol, and if + * one isn't found, to just fall back to the first GOP. + * + * Once we've found a GOP supporting ConOut, + * don't bother looking any further. + */ status = efi_bs_call(handle_protocol, h, &conout_proto, &dummy); if (status == EFI_SUCCESS) - conout_found = true; - - if (!first_gop || conout_found) { - /* - * Systems that use the UEFI Console Splitter may - * provide multiple GOP devices, not all of which are - * backed by real hardware. The workaround is to search - * for a GOP implementing the ConOut protocol, and if - * one isn't found, to just fall back to the first GOP. - * - * Once we've found a GOP supporting ConOut, - * don't bother looking any further. - */ + return gop; + + if (!first_gop) first_gop = gop; - if (conout_found) - break; - } } return first_gop; -- 2.24.1
Declare the variables inside the block where they're used. Get rid of a couple of redundant initializers. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index a7d3efe36c78..0d195060a370 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -88,16 +88,19 @@ setup_pixel_info(struct screen_info *si, u32 pixels_per_scan_line, static efi_graphics_output_protocol_t * find_gop(efi_guid_t *proto, unsigned long size, void **handles) { - efi_graphics_output_protocol_t *gop, *first_gop; - efi_graphics_output_protocol_mode_t *mode; - efi_graphics_output_mode_info_t *info = NULL; - efi_status_t status; + efi_graphics_output_protocol_t *first_gop; efi_handle_t h; int i; first_gop = NULL; for_each_efi_handle(h, handles, size, i) { + efi_status_t status; + + efi_graphics_output_protocol_t *gop; + efi_graphics_output_protocol_mode_t *mode; + efi_graphics_output_mode_info_t *info; + efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID; void *dummy = NULL; @@ -136,7 +139,7 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, { efi_graphics_output_protocol_t *gop; efi_graphics_output_protocol_mode_t *mode; - efi_graphics_output_mode_info_t *info = NULL; + efi_graphics_output_mode_info_t *info; efi_physical_addr_t fb_base; gop = find_gop(proto, size, handles); -- 2.24.1
Use the lower/upper_32_bits macros from kernel.h to initialize si->lfb_base and si->ext_lfb_base. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 0d195060a370..7b0baf9a912f 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -158,8 +158,8 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, si->lfb_height = info->vertical_resolution; fb_base = efi_table_attr(mode, frame_buffer_base); - si->lfb_base = fb_base; - si->ext_lfb_base = (u64)(unsigned long)fb_base >> 32; + si->lfb_base = lower_32_bits(fb_base); + si->ext_lfb_base = upper_32_bits(fb_base); if (si->ext_lfb_base) si->capabilities |= VIDEO_CAPABILITY_64BIT_BASE; -- 2.24.1
Use the __ffs/__fls macros to calculate the position and size of the mask. Correct type of mask to u32 instead of unsigned long. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 7b0baf9a912f..8bf424f35759 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -5,6 +5,7 @@ * * ----------------------------------------------------------------------- */ +#include <linux/bitops.h> #include <linux/efi.h> #include <linux/screen_info.h> #include <asm/efi.h> @@ -12,27 +13,16 @@ #include "efistub.h" -static void find_bits(unsigned long mask, u8 *pos, u8 *size) +static void find_bits(u32 mask, u8 *pos, u8 *size) { - u8 first, len; - - first = 0; - len = 0; - - if (mask) { - while (!(mask & 0x1)) { - mask = mask >> 1; - first++; - } - - while (mask & 0x1) { - mask = mask >> 1; - len++; - } + if (!mask) { + *pos = *size = 0; + return; } - *pos = first; - *size = len; + /* UEFI spec guarantees that the set bits are contiguous */ + *pos = __ffs(mask); + *size = __fls(mask) - *pos + 1; } static void -- 2.24.1
pixel_format must be one of PIXEL_RGB_RESERVED_8BIT_PER_COLOR PIXEL_BGR_RESERVED_8BIT_PER_COLOR PIXEL_BIT_MASK since we skip PIXEL_BLT_ONLY when finding a gop. Remove the redundant code and add another check in find_gop to skip any pixel formats that we don't know about, in case a later version of the UEFI spec adds one. Reformat the code a little. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- drivers/firmware/efi/libstub/gop.c | 66 ++++++++++++------------------ 1 file changed, 26 insertions(+), 40 deletions(-) diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 8bf424f35759..2d91699e3061 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -29,49 +29,34 @@ static void setup_pixel_info(struct screen_info *si, u32 pixels_per_scan_line, efi_pixel_bitmask_t pixel_info, int pixel_format) { - if (pixel_format == PIXEL_RGB_RESERVED_8BIT_PER_COLOR) { - si->lfb_depth = 32; - si->lfb_linelength = pixels_per_scan_line * 4; - si->red_size = 8; - si->red_pos = 0; - si->green_size = 8; - si->green_pos = 8; - si->blue_size = 8; - si->blue_pos = 16; - si->rsvd_size = 8; - si->rsvd_pos = 24; - } else if (pixel_format == PIXEL_BGR_RESERVED_8BIT_PER_COLOR) { - si->lfb_depth = 32; - si->lfb_linelength = pixels_per_scan_line * 4; - si->red_size = 8; - si->red_pos = 16; - si->green_size = 8; - si->green_pos = 8; - si->blue_size = 8; - si->blue_pos = 0; - si->rsvd_size = 8; - si->rsvd_pos = 24; - } else if (pixel_format == PIXEL_BIT_MASK) { - find_bits(pixel_info.red_mask, &si->red_pos, &si->red_size); - find_bits(pixel_info.green_mask, &si->green_pos, - &si->green_size); - find_bits(pixel_info.blue_mask, &si->blue_pos, &si->blue_size); - find_bits(pixel_info.reserved_mask, &si->rsvd_pos, - &si->rsvd_size); + if (pixel_format == PIXEL_BIT_MASK) { + find_bits(pixel_info.red_mask, + &si->red_pos, &si->red_size); + find_bits(pixel_info.green_mask, + &si->green_pos, &si->green_size); + find_bits(pixel_info.blue_mask, + &si->blue_pos, &si->blue_size); + find_bits(pixel_info.reserved_mask, + &si->rsvd_pos, &si->rsvd_size); si->lfb_depth = si->red_size + si->green_size + si->blue_size + si->rsvd_size; si->lfb_linelength = (pixels_per_scan_line * si->lfb_depth) / 8; } else { - si->lfb_depth = 4; - si->lfb_linelength = si->lfb_width / 2; - si->red_size = 0; - si->red_pos = 0; - si->green_size = 0; - si->green_pos = 0; - si->blue_size = 0; - si->blue_pos = 0; - si->rsvd_size = 0; - si->rsvd_pos = 0; + if (pixel_format == PIXEL_RGB_RESERVED_8BIT_PER_COLOR) { + si->red_pos = 0; + si->blue_pos = 16; + } else /* PIXEL_BGR_RESERVED_8BIT_PER_COLOR */ { + si->blue_pos = 0; + si->red_pos = 16; + } + + si->green_pos = 8; + si->rsvd_pos = 24; + si->red_size = si->green_size = + si->blue_size = si->rsvd_size = 8; + + si->lfb_depth = 32; + si->lfb_linelength = pixels_per_scan_line * 4; } } @@ -100,7 +85,8 @@ find_gop(efi_guid_t *proto, unsigned long size, void **handles) mode = efi_table_attr(gop, mode); info = efi_table_attr(mode, info); - if (info->pixel_format == PIXEL_BLT_ONLY) + if (info->pixel_format == PIXEL_BLT_ONLY || + info->pixel_format >= PIXEL_FORMAT_MAX) continue; /* -- 2.24.1
Add prototypes and argmap for the Graphics Output Protocol's QueryMode and SetMode functions. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- arch/x86/include/asm/efi.h | 4 ++++ drivers/firmware/efi/libstub/efistub.h | 6 ++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index cdcf48d52a12..781170d36f50 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -305,6 +305,10 @@ static inline u32 efi64_convert_status(efi_status_t status) #define __efi64_argmap_load_file(protocol, path, policy, bufsize, buf) \ ((protocol), (path), (policy), efi64_zero_upper(bufsize), (buf)) +/* Graphics Output Protocol */ +#define __efi64_argmap_query_mode(gop, mode, size, info) \ + ((gop), (mode), efi64_zero_upper(size), efi64_zero_upper(info)) + /* * The macros below handle the plumbing for the argument mapping. To add a * mapping for a specific EFI method, simply define a macro diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h index cc90a748bcf0..c400fd88fe38 100644 --- a/drivers/firmware/efi/libstub/efistub.h +++ b/drivers/firmware/efi/libstub/efistub.h @@ -298,8 +298,10 @@ typedef union efi_graphics_output_protocol efi_graphics_output_protocol_t; union efi_graphics_output_protocol { struct { - void *query_mode; - void *set_mode; + efi_status_t (__efiapi *query_mode)(efi_graphics_output_protocol_t *, + u32, unsigned long *, + efi_graphics_output_mode_info_t **); + efi_status_t (__efiapi *set_mode) (efi_graphics_output_protocol_t *, u32); void *blt; efi_graphics_output_protocol_mode_t *mode; }; -- 2.24.1
Add the ability to choose a video mode for the selected gop by using a command-line argument of the form video=efifb:mode=<n> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- Documentation/fb/efifb.rst | 20 +++- .../firmware/efi/libstub/efi-stub-helper.c | 3 + drivers/firmware/efi/libstub/efistub.h | 2 + drivers/firmware/efi/libstub/gop.c | 107 ++++++++++++++++++ 4 files changed, 129 insertions(+), 3 deletions(-) diff --git a/Documentation/fb/efifb.rst b/Documentation/fb/efifb.rst index 04840331a00e..367fbda2f4da 100644 --- a/Documentation/fb/efifb.rst +++ b/Documentation/fb/efifb.rst @@ -2,8 +2,10 @@ What is efifb? ============== -This is a generic EFI platform driver for Intel based Apple computers. -efifb is only for EFI booted Intel Macs. +This is a generic EFI platform driver for systems with UEFI firmware. The +system must be booted via the EFI stub for this to be usable. efifb supports +both firmware with Graphics Output Protocol (GOP) displays as well as older +systems with only Universal Graphics Adapter (UGA) displays. Supported Hardware ================== @@ -12,11 +14,14 @@ Supported Hardware - Macbook - Macbook Pro 15"/17" - MacMini +- ARM/ARM64/X86 systems with UEFI firmware How to use it? ============== -efifb does not have any kind of autodetection of your machine. +For UGA displays, efifb does not have any kind of autodetection of your +machine. + You have to add the following kernel parameters in your elilo.conf:: Macbook : @@ -28,6 +33,9 @@ You have to add the following kernel parameters in your elilo.conf:: Macbook Pro 17", iMac 20" : video=efifb:i20 +For GOP displays, efifb can autodetect the display's resolution and framebuffer +address, so these should work out of the box without any special parameters. + Accepted options: ======= =========================================================== @@ -36,4 +44,10 @@ nowc Don't map the framebuffer write combined. This can be used when large amounts of console data are written. ======= =========================================================== +Options for GOP displays: + +mode=n + The EFI stub will set the mode of the display to mode number n if + possible. + Edgar Hucek <gimli@dark-green.com> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index 9f34c7242939..c6092b6038cf 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -105,6 +105,9 @@ efi_status_t efi_parse_options(char const *cmdline) efi_disable_pci_dma = true; if (parse_option_str(val, "no_disable_early_pci_dma")) efi_disable_pci_dma = false; + } else if (!strcmp(param, "video") && + val && strstarts(val, "efifb:")) { + efi_parse_option_graphics(val + strlen("efifb:")); } } efi_bs_call(free_pool, buf); diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h index c400fd88fe38..4844c3bd40df 100644 --- a/drivers/firmware/efi/libstub/efistub.h +++ b/drivers/firmware/efi/libstub/efistub.h @@ -650,6 +650,8 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr, efi_status_t efi_parse_options(char const *cmdline); +void efi_parse_option_graphics(char *option); + efi_status_t efi_setup_gop(struct screen_info *si, efi_guid_t *proto, unsigned long size); diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 2d91699e3061..a32b784b4577 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -8,11 +8,115 @@ #include <linux/bitops.h> #include <linux/efi.h> #include <linux/screen_info.h> +#include <linux/string.h> #include <asm/efi.h> #include <asm/setup.h> #include "efistub.h" +enum efi_cmdline_option { + EFI_CMDLINE_NONE, + EFI_CMDLINE_MODE_NUM, +}; + +static struct { + enum efi_cmdline_option option; + u32 mode; +} cmdline __efistub_global = { .option = EFI_CMDLINE_NONE }; + +static bool parse_modenum(char *option, char **next) +{ + u32 m; + + if (!strstarts(option, "mode=")) + return false; + option += strlen("mode="); + m = simple_strtoull(option, &option, 0); + if (*option && *option++ != ',') + return false; + cmdline.option = EFI_CMDLINE_MODE_NUM; + cmdline.mode = m; + + *next = option; + return true; +} + +void efi_parse_option_graphics(char *option) +{ + while (*option) { + if (parse_modenum(option, &option)) + continue; + + while (*option && *option++ != ',') + ; + } +} + +static u32 choose_mode_modenum(efi_graphics_output_protocol_t *gop) +{ + efi_status_t status; + + efi_graphics_output_protocol_mode_t *mode; + efi_graphics_output_mode_info_t *info; + unsigned long info_size; + + u32 max_mode, cur_mode; + int pf; + + mode = efi_table_attr(gop, mode); + + cur_mode = efi_table_attr(mode, mode); + if (cmdline.mode == cur_mode) + return cur_mode; + + max_mode = efi_table_attr(mode, max_mode); + if (cmdline.mode >= max_mode) { + efi_printk("Requested mode is invalid\n"); + return cur_mode; + } + + status = efi_call_proto(gop, query_mode, cmdline.mode, + &info_size, &info); + if (status != EFI_SUCCESS) { + efi_printk("Couldn't get mode information\n"); + return cur_mode; + } + + pf = info->pixel_format; + + efi_bs_call(free_pool, info); + + if (pf == PIXEL_BLT_ONLY || pf >= PIXEL_FORMAT_MAX) { + efi_printk("Invalid PixelFormat\n"); + return cur_mode; + } + + return cmdline.mode; +} + +static void set_mode(efi_graphics_output_protocol_t *gop) +{ + efi_graphics_output_protocol_mode_t *mode; + u32 cur_mode, new_mode; + + switch (cmdline.option) { + case EFI_CMDLINE_MODE_NUM: + new_mode = choose_mode_modenum(gop); + break; + default: + return; + } + + mode = efi_table_attr(gop, mode); + cur_mode = efi_table_attr(mode, mode); + + if (new_mode == cur_mode) + return; + + if (efi_call_proto(gop, set_mode, new_mode) != EFI_SUCCESS) + efi_printk("Failed to set requested mode\n"); +} + static void find_bits(u32 mask, u8 *pos, u8 *size) { if (!mask) { @@ -124,6 +228,9 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto, if (!gop) return EFI_NOT_FOUND; + /* Change mode if requested */ + set_mode(gop); + /* EFI framebuffer */ mode = efi_table_attr(gop, mode); info = efi_table_attr(mode, info); -- 2.24.1
Add the ability to choose a video mode using a command-line argument of the form video=efifb:<xres>x<yres> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- Documentation/fb/efifb.rst | 5 ++ drivers/firmware/efi/libstub/gop.c | 84 +++++++++++++++++++++++++++++- 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/Documentation/fb/efifb.rst b/Documentation/fb/efifb.rst index 367fbda2f4da..635275071307 100644 --- a/Documentation/fb/efifb.rst +++ b/Documentation/fb/efifb.rst @@ -50,4 +50,9 @@ mode=n The EFI stub will set the mode of the display to mode number n if possible. +<xres>x<yres> + The EFI stub will search for a display mode that matches the specified + horizontal and vertical resolution, and set the mode of the display to + it if one is found. + Edgar Hucek <gimli@dark-green.com> diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index a32b784b4577..cc84e6a82f54 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -6,6 +6,7 @@ * ----------------------------------------------------------------------- */ #include <linux/bitops.h> +#include <linux/ctype.h> #include <linux/efi.h> #include <linux/screen_info.h> #include <linux/string.h> @@ -17,11 +18,17 @@ enum efi_cmdline_option { EFI_CMDLINE_NONE, EFI_CMDLINE_MODE_NUM, + EFI_CMDLINE_RES }; static struct { enum efi_cmdline_option option; - u32 mode; + union { + u32 mode; + struct { + u32 width, height; + } res; + }; } cmdline __efistub_global = { .option = EFI_CMDLINE_NONE }; static bool parse_modenum(char *option, char **next) @@ -41,11 +48,33 @@ static bool parse_modenum(char *option, char **next) return true; } +static bool parse_res(char *option, char **next) +{ + u32 w, h; + + if (!isdigit(*option)) + return false; + w = simple_strtoull(option, &option, 10); + if (*option++ != 'x' || !isdigit(*option)) + return false; + h = simple_strtoull(option, &option, 10); + if (*option && *option++ != ',') + return false; + cmdline.option = EFI_CMDLINE_RES; + cmdline.res.width = w; + cmdline.res.height = h; + + *next = option; + return true; +} + void efi_parse_option_graphics(char *option) { while (*option) { if (parse_modenum(option, &option)) continue; + if (parse_res(option, &option)) + continue; while (*option && *option++ != ',') ; @@ -94,6 +123,56 @@ static u32 choose_mode_modenum(efi_graphics_output_protocol_t *gop) return cmdline.mode; } +static u32 choose_mode_res(efi_graphics_output_protocol_t *gop) +{ + efi_status_t status; + + efi_graphics_output_protocol_mode_t *mode; + efi_graphics_output_mode_info_t *info; + unsigned long info_size; + + u32 max_mode, cur_mode; + int pf; + u32 m, w, h; + + mode = efi_table_attr(gop, mode); + + cur_mode = efi_table_attr(mode, mode); + info = efi_table_attr(mode, info); + w = info->horizontal_resolution; + h = info->vertical_resolution; + + if (w == cmdline.res.width && h == cmdline.res.height) + return cur_mode; + + max_mode = efi_table_attr(mode, max_mode); + + for (m = 0; m < max_mode; m++) { + if (m == cur_mode) + continue; + + status = efi_call_proto(gop, query_mode, m, + &info_size, &info); + if (status != EFI_SUCCESS) + continue; + + pf = info->pixel_format; + w = info->horizontal_resolution; + h = info->vertical_resolution; + + efi_bs_call(free_pool, info); + + if (pf == PIXEL_BLT_ONLY || pf >= PIXEL_FORMAT_MAX) + continue; + if (w == cmdline.res.width && h == cmdline.res.height) + return m; + } + + efi_printk("Couldn't find requested mode\n"); + + return cur_mode; +} + static void set_mode(efi_graphics_output_protocol_t *gop) { efi_graphics_output_protocol_mode_t *mode; @@ -103,6 +182,9 @@ static void set_mode(efi_graphics_output_protocol_t *gop) case EFI_CMDLINE_MODE_NUM: new_mode = choose_mode_modenum(gop); break; + case EFI_CMDLINE_RES: + new_mode = choose_mode_res(gop); + break; default: return; } -- 2.24.1
Extend the video mode argument to handle an optional color depth specification of the form video=efifb:<xres>x<yres>[-(rgb|bgr|<bpp>)] Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- Documentation/fb/efifb.rst | 8 +++-- drivers/firmware/efi/libstub/gop.c | 48 ++++++++++++++++++++++++++---- 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/Documentation/fb/efifb.rst b/Documentation/fb/efifb.rst index 635275071307..eca38466487a 100644 --- a/Documentation/fb/efifb.rst +++ b/Documentation/fb/efifb.rst @@ -50,9 +50,11 @@ mode=n The EFI stub will set the mode of the display to mode number n if possible. -<xres>x<yres> +<xres>x<yres>[-(rgb|bgr|<bpp>)] The EFI stub will search for a display mode that matches the specified - horizontal and vertical resolution, and set the mode of the display to - it if one is found. + horizontal and vertical resolution, and optionally bit depth, and set + the mode of the display to it if one is found. The bit depth can either + "rgb" or "bgr" to match specifically those pixel formats, or a number + for a mode with matching bits per pixel. Edgar Hucek <gimli@dark-green.com> diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index cc84e6a82f54..848cb605a9c4 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -27,6 +27,8 @@ static struct { u32 mode; struct { u32 width, height; + int format; + u8 depth; } res; }; } cmdline __efistub_global = { .option = EFI_CMDLINE_NONE }; @@ -50,7 +52,8 @@ static bool parse_modenum(char *option, char **next) static bool parse_res(char *option, char **next) { - u32 w, h; + u32 w, h, d = 0; + int pf = -1; if (!isdigit(*option)) return false; @@ -58,11 +61,26 @@ static bool parse_res(char *option, char **next) if (*option++ != 'x' || !isdigit(*option)) return false; h = simple_strtoull(option, &option, 10); + if (*option == '-') { + option++; + if (strstarts(option, "rgb")) { + option += strlen("rgb"); + pf = PIXEL_RGB_RESERVED_8BIT_PER_COLOR; + } else if (strstarts(option, "bgr")) { + option += strlen("bgr"); + pf = PIXEL_BGR_RESERVED_8BIT_PER_COLOR; + } else if (isdigit(*option)) + d = simple_strtoull(option, &option, 10); + else + return false; + } if (*option && *option++ != ',') return false; cmdline.option = EFI_CMDLINE_RES; cmdline.res.width = w; cmdline.res.height = h; + cmdline.res.format = pf; + cmdline.res.depth = d; *next = option; return true; @@ -123,6 +141,18 @@ static u32 choose_mode_modenum(efi_graphics_output_protocol_t *gop) return cmdline.mode; } +static u8 pixel_bpp(int pixel_format, efi_pixel_bitmask_t pixel_info) +{ + if (pixel_format == PIXEL_BIT_MASK) { + u32 mask = pixel_info.red_mask | pixel_info.green_mask | + pixel_info.blue_mask | pixel_info.reserved_mask; + if (!mask) + return 0; + return __fls(mask) - __ffs(mask) + 1; + } else + return 32; +} + static u32 choose_mode_res(efi_graphics_output_protocol_t *gop) { efi_status_t status; @@ -133,16 +163,21 @@ static u32 choose_mode_res(efi_graphics_output_protocol_t *gop) u32 max_mode, cur_mode; int pf; + efi_pixel_bitmask_t pi; u32 m, w, h; mode = efi_table_attr(gop, mode); cur_mode = efi_table_attr(mode, mode); info = efi_table_attr(mode, info); - w = info->horizontal_resolution; - h = info->vertical_resolution; + pf = info->pixel_format; + pi = info->pixel_information; + w = info->horizontal_resolution; + h = info->vertical_resolution; - if (w == cmdline.res.width && h == cmdline.res.height) + if (w == cmdline.res.width && h == cmdline.res.height && + (cmdline.res.format < 0 || cmdline.res.format == pf) && + (!cmdline.res.depth || cmdline.res.depth == pixel_bpp(pf, pi))) return cur_mode; max_mode = efi_table_attr(mode, max_mode); @@ -157,6 +192,7 @@ static u32 choose_mode_res(efi_graphics_output_protocol_t *gop) continue; pf = info->pixel_format; + pi = info->pixel_information; w = info->horizontal_resolution; h = info->vertical_resolution; @@ -164,7 +200,9 @@ static u32 choose_mode_res(efi_graphics_output_protocol_t *gop) if (pf == PIXEL_BLT_ONLY || pf >= PIXEL_FORMAT_MAX) continue; - if (w == cmdline.res.width && h == cmdline.res.height) + if (w == cmdline.res.width && h == cmdline.res.height && + (cmdline.res.format < 0 || cmdline.res.format == pf) && + (!cmdline.res.depth || cmdline.res.depth == pixel_bpp(pf, pi))) return m; } -- 2.24.1
Add the ability to automatically pick the highest resolution video mode (defined as the product of vertical and horizontal resolution) by using a command-line argument of the form video=efifb:auto If there are multiple modes with the highest resolution, pick one with the highest color depth. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- Documentation/fb/efifb.rst | 6 +++ drivers/firmware/efi/libstub/gop.c | 81 +++++++++++++++++++++++++++++- 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/Documentation/fb/efifb.rst b/Documentation/fb/efifb.rst index eca38466487a..519550517fd4 100644 --- a/Documentation/fb/efifb.rst +++ b/Documentation/fb/efifb.rst @@ -57,4 +57,10 @@ mode=n "rgb" or "bgr" to match specifically those pixel formats, or a number for a mode with matching bits per pixel. +auto + The EFI stub will choose the mode with the highest resolution (product + of horizontal and vertical resolution). If there are multiple modes + with the highest resolution, it will choose one with the highest color + depth. + Edgar Hucek <gimli@dark-green.com> diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 848cb605a9c4..0882c07a9f54 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -18,7 +18,8 @@ enum efi_cmdline_option { EFI_CMDLINE_NONE, EFI_CMDLINE_MODE_NUM, - EFI_CMDLINE_RES + EFI_CMDLINE_RES, + EFI_CMDLINE_AUTO }; static struct { @@ -86,6 +87,19 @@ static bool parse_res(char *option, char **next) return true; } +static bool parse_auto(char *option, char **next) +{ + if (!strstarts(option, "auto")) + return false; + option += strlen("auto"); + if (*option && *option++ != ',') + return false; + cmdline.option = EFI_CMDLINE_AUTO; + + *next = option; + return true; +} + void efi_parse_option_graphics(char *option) { while (*option) { @@ -93,6 +107,8 @@ void efi_parse_option_graphics(char *option) continue; if (parse_res(option, &option)) continue; + if (parse_auto(option, &option)) + continue; while (*option && *option++ != ',') ; @@ -211,6 +227,66 @@ static u32 choose_mode_res(efi_graphics_output_protocol_t *gop) return cur_mode; } +static u32 choose_mode_auto(efi_graphics_output_protocol_t *gop) +{ + efi_status_t status; + + efi_graphics_output_protocol_mode_t *mode; + efi_graphics_output_mode_info_t *info; + unsigned long info_size; + + u32 max_mode, cur_mode, best_mode, area; + u8 depth; + int pf; + efi_pixel_bitmask_t pi; + u32 m, w, h, a; + u8 d; + + mode = efi_table_attr(gop, mode); + + cur_mode = efi_table_attr(mode, mode); + max_mode = efi_table_attr(mode, max_mode); + + info = efi_table_attr(mode, info); + + pf = info->pixel_format; + pi = info->pixel_information; + w = info->horizontal_resolution; + h = info->vertical_resolution; + + best_mode = cur_mode; + area = w * h; + depth = pixel_bpp(pf, pi); + + for (m = 0; m < max_mode; m++) { + status = efi_call_proto(gop, query_mode, m, + &info_size, &info); + if (status != EFI_SUCCESS) + continue; + + pf = info->pixel_format; + pi = info->pixel_information; + w = info->horizontal_resolution; + h = info->vertical_resolution; + + efi_bs_call(free_pool, info); + + if (pf == PIXEL_BLT_ONLY || pf >= PIXEL_FORMAT_MAX) + continue; + a = w * h; + if (a < area) + continue; + d = pixel_bpp(pf, pi); + if (a > area || d > depth) { + best_mode = m; + area = a; + depth = d; + } + } + + return best_mode; +} + static void set_mode(efi_graphics_output_protocol_t *gop) { efi_graphics_output_protocol_mode_t *mode; @@ -223,6 +299,9 @@ static void set_mode(efi_graphics_output_protocol_t *gop) case EFI_CMDLINE_RES: new_mode = choose_mode_res(gop); break; + case EFI_CMDLINE_AUTO: + new_mode = choose_mode_auto(gop); + break; default: return; } -- 2.24.1
Hi Arvind, Thank you for the patch! Perhaps something to improve: url: https://github.com/0day-ci/linux/commits/Arvind-Sankar/efi-gop-Refactoring-mode-setting-feature/20200320-044605 base: https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> New smatch warnings: drivers/firmware/efi/libstub/gop.c:113 set_mode() error: uninitialized symbol 'new_mode'. # https://github.com/0day-ci/linux/commit/af85e496c9f577df9743784171b1cda94220dd8f git remote add linux-review https://github.com/0day-ci/linux git remote update linux-review git checkout af85e496c9f577df9743784171b1cda94220dd8f vim +/info +85 drivers/firmware/efi/libstub/gop.c af85e496c9f577 Arvind Sankar 2020-03-19 97 static void set_mode(efi_graphics_output_protocol_t *gop) af85e496c9f577 Arvind Sankar 2020-03-19 98 { af85e496c9f577 Arvind Sankar 2020-03-19 99 efi_graphics_output_protocol_mode_t *mode; af85e496c9f577 Arvind Sankar 2020-03-19 100 u32 cur_mode, new_mode; af85e496c9f577 Arvind Sankar 2020-03-19 101 af85e496c9f577 Arvind Sankar 2020-03-19 102 switch (cmdline.option) { af85e496c9f577 Arvind Sankar 2020-03-19 103 case EFI_CMDLINE_NONE: af85e496c9f577 Arvind Sankar 2020-03-19 104 return; af85e496c9f577 Arvind Sankar 2020-03-19 105 case EFI_CMDLINE_MODE_NUM: af85e496c9f577 Arvind Sankar 2020-03-19 106 new_mode = choose_mode_modenum(gop); af85e496c9f577 Arvind Sankar 2020-03-19 107 break; No default case? af85e496c9f577 Arvind Sankar 2020-03-19 108 } af85e496c9f577 Arvind Sankar 2020-03-19 109 af85e496c9f577 Arvind Sankar 2020-03-19 110 mode = efi_table_attr(gop, mode); af85e496c9f577 Arvind Sankar 2020-03-19 111 cur_mode = efi_table_attr(mode, mode); af85e496c9f577 Arvind Sankar 2020-03-19 112 af85e496c9f577 Arvind Sankar 2020-03-19 @113 if (new_mode == cur_mode) af85e496c9f577 Arvind Sankar 2020-03-19 114 return; af85e496c9f577 Arvind Sankar 2020-03-19 115 af85e496c9f577 Arvind Sankar 2020-03-19 116 if (efi_call_proto(gop, set_mode, new_mode) != EFI_SUCCESS) af85e496c9f577 Arvind Sankar 2020-03-19 117 efi_printk("Failed to set requested mode\n"); af85e496c9f577 Arvind Sankar 2020-03-19 118 } --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Fri, Mar 20, 2020 at 05:36:04PM +0300, Dan Carpenter wrote: > Hi Arvind, > > Thank you for the patch! Perhaps something to improve: > > url: https://github.com/0day-ci/linux/commits/Arvind-Sankar/efi-gop-Refactoring-mode-setting-feature/20200320-044605 > base: https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next > > If you fix the issue, kindly add following tag > Reported-by: kbuild test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > New smatch warnings: > drivers/firmware/efi/libstub/gop.c:113 set_mode() error: uninitialized symbol 'new_mode'. > > # https://github.com/0day-ci/linux/commit/af85e496c9f577df9743784171b1cda94220dd8f > git remote add linux-review https://github.com/0day-ci/linux > git remote update linux-review > git checkout af85e496c9f577df9743784171b1cda94220dd8f > vim +/info +85 drivers/firmware/efi/libstub/gop.c > > af85e496c9f577 Arvind Sankar 2020-03-19 97 static void set_mode(efi_graphics_output_protocol_t *gop) > af85e496c9f577 Arvind Sankar 2020-03-19 98 { > af85e496c9f577 Arvind Sankar 2020-03-19 99 efi_graphics_output_protocol_mode_t *mode; > af85e496c9f577 Arvind Sankar 2020-03-19 100 u32 cur_mode, new_mode; > af85e496c9f577 Arvind Sankar 2020-03-19 101 > af85e496c9f577 Arvind Sankar 2020-03-19 102 switch (cmdline.option) { > af85e496c9f577 Arvind Sankar 2020-03-19 103 case EFI_CMDLINE_NONE: > af85e496c9f577 Arvind Sankar 2020-03-19 104 return; > af85e496c9f577 Arvind Sankar 2020-03-19 105 case EFI_CMDLINE_MODE_NUM: > af85e496c9f577 Arvind Sankar 2020-03-19 106 new_mode = choose_mode_modenum(gop); > af85e496c9f577 Arvind Sankar 2020-03-19 107 break; > > No default case? Yeah, it's an enum with the only two values covered by the switch cases, so it's really a bogus warning. I posted a v2 with a default case instead anyway to silence it. > > af85e496c9f577 Arvind Sankar 2020-03-19 108 } > af85e496c9f577 Arvind Sankar 2020-03-19 109 > af85e496c9f577 Arvind Sankar 2020-03-19 110 mode = efi_table_attr(gop, mode); > af85e496c9f577 Arvind Sankar 2020-03-19 111 cur_mode = efi_table_attr(mode, mode); > af85e496c9f577 Arvind Sankar 2020-03-19 112 > af85e496c9f577 Arvind Sankar 2020-03-19 @113 if (new_mode == cur_mode) > af85e496c9f577 Arvind Sankar 2020-03-19 114 return; > af85e496c9f577 Arvind Sankar 2020-03-19 115 > af85e496c9f577 Arvind Sankar 2020-03-19 116 if (efi_call_proto(gop, set_mode, new_mode) != EFI_SUCCESS) > af85e496c9f577 Arvind Sankar 2020-03-19 117 efi_printk("Failed to set requested mode\n"); > af85e496c9f577 Arvind Sankar 2020-03-19 118 } > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Fri, 20 Mar 2020 at 03:00, Arvind Sankar <nivedita@alum.mit.edu> wrote: > > This series is against tip:efi/core. > > Patches 1-9 are small cleanups and refactoring of the code in > libstub/gop.c. > > The rest of the patches add the ability to use a command-line option to > switch the gop's display mode. > > The options supported are: > video=efifb:mode=n > Choose a specific mode number > video=efifb:<xres>x<yres>[-(rgb|bgr|<bpp>)] > Specify mode by resolution and optionally color depth > video=efifb:auto > Let the EFI stub choose the highest resolution mode available. > > The mode-setting additions increase code size of gop.o by about 3k on > x86-64 with EFI_MIXED enabled. > > Changes in v2 (HT lkp@intel.com): > - Fix __efistub_global attribute to be after the variable. > (NB: bunch of other places should ideally be fixed, those I guess > don't matter as they are scalars?) > - Silence -Wmaybe-uninitialized warning in set_mode function. > These look good to me. The only question I have is whether it would be possible to use the existing next_arg() and parse_option_str() functions to replace some of the open code parsing that goes on in patches 11 - 14. > Arvind Sankar (14): > efi/gop: Remove redundant current_fb_base > efi/gop: Move check for framebuffer before con_out > efi/gop: Get mode information outside the loop > efi/gop: Factor out locating the gop into a function > efi/gop: Slightly re-arrange logic of find_gop > efi/gop: Move variable declarations into loop block > efi/gop: Use helper macros for populating lfb_base > efi/gop: Use helper macros for find_bits > efi/gop: Remove unreachable code from setup_pixel_info > efi/gop: Add prototypes for query_mode and set_mode > efi/gop: Allow specifying mode number on command line > efi/gop: Allow specifying mode by <xres>x<yres> > efi/gop: Allow specifying depth as well as resolution > efi/gop: Allow automatically choosing the best mode > > Documentation/fb/efifb.rst | 33 +- > arch/x86/include/asm/efi.h | 4 + > .../firmware/efi/libstub/efi-stub-helper.c | 3 + > drivers/firmware/efi/libstub/efistub.h | 8 +- > drivers/firmware/efi/libstub/gop.c | 489 ++++++++++++++---- > 5 files changed, 428 insertions(+), 109 deletions(-) > > > base-commit: d5528d5e91041e68e8eab9792ce627705a0ed273 > -- > 2.24.1 >
Hi, On 3/20/20 3:00 AM, Arvind Sankar wrote: > This series is against tip:efi/core. > > Patches 1-9 are small cleanups and refactoring of the code in > libstub/gop.c. > > The rest of the patches add the ability to use a command-line option to > switch the gop's display mode. > > The options supported are: > video=efifb:mode=n > Choose a specific mode number > video=efifb:<xres>x<yres>[-(rgb|bgr|<bpp>)] > Specify mode by resolution and optionally color depth > video=efifb:auto > Let the EFI stub choose the highest resolution mode available. > > The mode-setting additions increase code size of gop.o by about 3k on > x86-64 with EFI_MIXED enabled. Thank you for adding me to the Cc. I will add these to my personal tree, which I test semi-regular on various hardware. I've only looked at patches 10 - 14 and a quick glance these look good to me. I was worried that you would maybe always enumerate the modes or some such, but I see that you have structured things in such a way that if the new kernel cmdline options are not used no extra EFI calls are made, which make me very happy! This way we do not need to worry about this patch-set tripping up buggy firmware (which is quite likely to be out there somewhere) by making new, previously unused, EFI calls. Regards, Hans > > Changes in v2 (HT lkp@intel.com): > - Fix __efistub_global attribute to be after the variable. > (NB: bunch of other places should ideally be fixed, those I guess > don't matter as they are scalars?) > - Silence -Wmaybe-uninitialized warning in set_mode function. > > Arvind Sankar (14): > efi/gop: Remove redundant current_fb_base > efi/gop: Move check for framebuffer before con_out > efi/gop: Get mode information outside the loop > efi/gop: Factor out locating the gop into a function > efi/gop: Slightly re-arrange logic of find_gop > efi/gop: Move variable declarations into loop block > efi/gop: Use helper macros for populating lfb_base > efi/gop: Use helper macros for find_bits > efi/gop: Remove unreachable code from setup_pixel_info > efi/gop: Add prototypes for query_mode and set_mode > efi/gop: Allow specifying mode number on command line > efi/gop: Allow specifying mode by <xres>x<yres> > efi/gop: Allow specifying depth as well as resolution > efi/gop: Allow automatically choosing the best mode > > Documentation/fb/efifb.rst | 33 +- > arch/x86/include/asm/efi.h | 4 + > .../firmware/efi/libstub/efi-stub-helper.c | 3 + > drivers/firmware/efi/libstub/efistub.h | 8 +- > drivers/firmware/efi/libstub/gop.c | 489 ++++++++++++++---- > 5 files changed, 428 insertions(+), 109 deletions(-) > > > base-commit: d5528d5e91041e68e8eab9792ce627705a0ed273 >
On Wed, Mar 25, 2020 at 05:41:43PM +0100, Ard Biesheuvel wrote:
> On Fri, 20 Mar 2020 at 03:00, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > This series is against tip:efi/core.
> >
> > Patches 1-9 are small cleanups and refactoring of the code in
> > libstub/gop.c.
> >
> > The rest of the patches add the ability to use a command-line option to
> > switch the gop's display mode.
> >
> > The options supported are:
> > video=efifb:mode=n
> > Choose a specific mode number
> > video=efifb:<xres>x<yres>[-(rgb|bgr|<bpp>)]
> > Specify mode by resolution and optionally color depth
> > video=efifb:auto
> > Let the EFI stub choose the highest resolution mode available.
> >
> > The mode-setting additions increase code size of gop.o by about 3k on
> > x86-64 with EFI_MIXED enabled.
> >
> > Changes in v2 (HT lkp@intel.com):
> > - Fix __efistub_global attribute to be after the variable.
> > (NB: bunch of other places should ideally be fixed, those I guess
> > don't matter as they are scalars?)
> > - Silence -Wmaybe-uninitialized warning in set_mode function.
> >
>
> These look good to me. The only question I have is whether it would be
> possible to use the existing next_arg() and parse_option_str()
> functions to replace some of the open code parsing that goes on in
> patches 11 - 14.
>
I don't think so -- next_arg is for parsing space-separated param=value
pairs, so efi_parse_options can use it, but it doesn't work for the
comma-separated options we'll have within the value.
parse_option_str would only work for the "auto" option, but it scans the
entire option string and just returns whether it was there or not, so it
wouldn't be too useful either, since we have to check for the other
possibilities anyway.
It would be nice to have a more generic library for cmdline parsing,
there are a lot of places that have to open-code option parsing like
this.
There's one thing I noticed while working at this, btw. The Makefile
specifies -ffreestanding, but at least x86 builds without having to
specify that. With -ffreestanding, the compiler doesn't optimize string
functions -- strlen(string literal) into a compile-time constant, for
eg. A couple hundred bytes or so can be saved by removing that option,
if it also works for ARM.
On Wed, Mar 25, 2020 at 05:50:19PM +0100, Hans de Goede wrote:
> Hi,
>
> On 3/20/20 3:00 AM, Arvind Sankar wrote:
> > This series is against tip:efi/core.
> >
> > Patches 1-9 are small cleanups and refactoring of the code in
> > libstub/gop.c.
> >
> > The rest of the patches add the ability to use a command-line option to
> > switch the gop's display mode.
> >
> > The options supported are:
> > video=efifb:mode=n
> > Choose a specific mode number
> > video=efifb:<xres>x<yres>[-(rgb|bgr|<bpp>)]
> > Specify mode by resolution and optionally color depth
> > video=efifb:auto
> > Let the EFI stub choose the highest resolution mode available.
> >
> > The mode-setting additions increase code size of gop.o by about 3k on
> > x86-64 with EFI_MIXED enabled.
>
> Thank you for adding me to the Cc. I will add these to my personal
> tree, which I test semi-regular on various hardware.
>
> I've only looked at patches 10 - 14 and a quick glance these look
> good to me.
>
> I was worried that you would maybe always enumerate the modes or
> some such, but I see that you have structured things in such a way
> that if the new kernel cmdline options are not used no extra EFI
> calls are made, which make me very happy!
>
> This way we do not need to worry about this patch-set tripping up
> buggy firmware (which is quite likely to be out there somewhere)
> by making new, previously unused, EFI calls.
>
Yep. Also, if the option is specified, it does enumerate the modes, but
if the currently set mode matches what the cmdline option wants, it
won't reset the mode, so it shouldn't interfere with seamless boot as
long as the mode doesn't have to be changed.
On Wed, 25 Mar 2020 at 23:10, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Wed, Mar 25, 2020 at 05:41:43PM +0100, Ard Biesheuvel wrote:
> > On Fri, 20 Mar 2020 at 03:00, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > This series is against tip:efi/core.
> > >
> > > Patches 1-9 are small cleanups and refactoring of the code in
> > > libstub/gop.c.
> > >
> > > The rest of the patches add the ability to use a command-line option to
> > > switch the gop's display mode.
> > >
> > > The options supported are:
> > > video=efifb:mode=n
> > > Choose a specific mode number
> > > video=efifb:<xres>x<yres>[-(rgb|bgr|<bpp>)]
> > > Specify mode by resolution and optionally color depth
> > > video=efifb:auto
> > > Let the EFI stub choose the highest resolution mode available.
> > >
> > > The mode-setting additions increase code size of gop.o by about 3k on
> > > x86-64 with EFI_MIXED enabled.
> > >
> > > Changes in v2 (HT lkp@intel.com):
> > > - Fix __efistub_global attribute to be after the variable.
> > > (NB: bunch of other places should ideally be fixed, those I guess
> > > don't matter as they are scalars?)
> > > - Silence -Wmaybe-uninitialized warning in set_mode function.
> > >
> >
> > These look good to me. The only question I have is whether it would be
> > possible to use the existing next_arg() and parse_option_str()
> > functions to replace some of the open code parsing that goes on in
> > patches 11 - 14.
> >
>
> I don't think so -- next_arg is for parsing space-separated param=value
> pairs, so efi_parse_options can use it, but it doesn't work for the
> comma-separated options we'll have within the value.
>
> parse_option_str would only work for the "auto" option, but it scans the
> entire option string and just returns whether it was there or not, so it
> wouldn't be too useful either, since we have to check for the other
> possibilities anyway.
>
> It would be nice to have a more generic library for cmdline parsing,
> there are a lot of places that have to open-code option parsing like
> this.
>
> There's one thing I noticed while working at this, btw. The Makefile
> specifies -ffreestanding, but at least x86 builds without having to
> specify that. With -ffreestanding, the compiler doesn't optimize string
> functions -- strlen(string literal) into a compile-time constant, for
> eg. A couple hundred bytes or so can be saved by removing that option,
> if it also works for ARM.
Yes, -ffreestanding implies -fno-builtin, which means that the
compiler cannot assume it knows (and can optimize away) the behavior
of strlen(), memset(), etc.
On Thu, 26 Mar 2020 at 00:36, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 25 Mar 2020 at 23:10, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Wed, Mar 25, 2020 at 05:41:43PM +0100, Ard Biesheuvel wrote:
> > > On Fri, 20 Mar 2020 at 03:00, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > >
> > > > This series is against tip:efi/core.
> > > >
> > > > Patches 1-9 are small cleanups and refactoring of the code in
> > > > libstub/gop.c.
> > > >
> > > > The rest of the patches add the ability to use a command-line option to
> > > > switch the gop's display mode.
> > > >
> > > > The options supported are:
> > > > video=efifb:mode=n
> > > > Choose a specific mode number
> > > > video=efifb:<xres>x<yres>[-(rgb|bgr|<bpp>)]
> > > > Specify mode by resolution and optionally color depth
> > > > video=efifb:auto
> > > > Let the EFI stub choose the highest resolution mode available.
> > > >
> > > > The mode-setting additions increase code size of gop.o by about 3k on
> > > > x86-64 with EFI_MIXED enabled.
> > > >
> > > > Changes in v2 (HT lkp@intel.com):
> > > > - Fix __efistub_global attribute to be after the variable.
> > > > (NB: bunch of other places should ideally be fixed, those I guess
> > > > don't matter as they are scalars?)
> > > > - Silence -Wmaybe-uninitialized warning in set_mode function.
> > > >
> > >
> > > These look good to me. The only question I have is whether it would be
> > > possible to use the existing next_arg() and parse_option_str()
> > > functions to replace some of the open code parsing that goes on in
> > > patches 11 - 14.
> > >
> >
> > I don't think so -- next_arg is for parsing space-separated param=value
> > pairs, so efi_parse_options can use it, but it doesn't work for the
> > comma-separated options we'll have within the value.
> >
> > parse_option_str would only work for the "auto" option, but it scans the
> > entire option string and just returns whether it was there or not, so it
> > wouldn't be too useful either, since we have to check for the other
> > possibilities anyway.
> >
> > It would be nice to have a more generic library for cmdline parsing,
> > there are a lot of places that have to open-code option parsing like
> > this.
> >
OK, I have queued these up now in the EFI next branch, but this will
obviously have to wait for v5.8
Thanks
On Thu, Mar 26, 2020 at 12:36:25AM +0100, Ard Biesheuvel wrote:
>
> Yes, -ffreestanding implies -fno-builtin, which means that the
> compiler cannot assume it knows (and can optimize away) the behavior
> of strlen(), memset(), etc.
Yes exactly. Do you foresee any problems with removing it?
Thanks.
On Fri, 27 Mar 2020 at 00:56, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Thu, Mar 26, 2020 at 12:36:25AM +0100, Ard Biesheuvel wrote:
> >
> > Yes, -ffreestanding implies -fno-builtin, which means that the
> > compiler cannot assume it knows (and can optimize away) the behavior
> > of strlen(), memset(), etc.
>
> Yes exactly. Do you foresee any problems with removing it?
>
I'm not sure why it is there in the first place, but I wouldn't extend
huge issues when we remove it.
Ard, can you replace the previous version with v3? I skipped calling query_mode for cur_mode in choose_mode_res but forgot to do it for choose_mode_auto. Thanks.
Add the ability to automatically pick the highest resolution video mode (defined as the product of vertical and horizontal resolution) by using a command-line argument of the form video=efifb:auto If there are multiple modes with the highest resolution, pick one with the highest color depth. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- Documentation/fb/efifb.rst | 6 +++ drivers/firmware/efi/libstub/gop.c | 84 +++++++++++++++++++++++++++++- 2 files changed, 89 insertions(+), 1 deletion(-) diff --git a/Documentation/fb/efifb.rst b/Documentation/fb/efifb.rst index eca38466487a..519550517fd4 100644 --- a/Documentation/fb/efifb.rst +++ b/Documentation/fb/efifb.rst @@ -57,4 +57,10 @@ mode=n "rgb" or "bgr" to match specifically those pixel formats, or a number for a mode with matching bits per pixel. +auto + The EFI stub will choose the mode with the highest resolution (product + of horizontal and vertical resolution). If there are multiple modes + with the highest resolution, it will choose one with the highest color + depth. + Edgar Hucek <gimli@dark-green.com> diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c index 848cb605a9c4..fa05a0b0adfd 100644 --- a/drivers/firmware/efi/libstub/gop.c +++ b/drivers/firmware/efi/libstub/gop.c @@ -18,7 +18,8 @@ enum efi_cmdline_option { EFI_CMDLINE_NONE, EFI_CMDLINE_MODE_NUM, - EFI_CMDLINE_RES + EFI_CMDLINE_RES, + EFI_CMDLINE_AUTO }; static struct { @@ -86,6 +87,19 @@ static bool parse_res(char *option, char **next) return true; } +static bool parse_auto(char *option, char **next) +{ + if (!strstarts(option, "auto")) + return false; + option += strlen("auto"); + if (*option && *option++ != ',') + return false; + cmdline.option = EFI_CMDLINE_AUTO; + + *next = option; + return true; +} + void efi_parse_option_graphics(char *option) { while (*option) { @@ -93,6 +107,8 @@ void efi_parse_option_graphics(char *option) continue; if (parse_res(option, &option)) continue; + if (parse_auto(option, &option)) + continue; while (*option && *option++ != ',') ; @@ -211,6 +227,69 @@ static u32 choose_mode_res(efi_graphics_output_protocol_t *gop) return cur_mode; } +static u32 choose_mode_auto(efi_graphics_output_protocol_t *gop) +{ + efi_status_t status; + + efi_graphics_output_protocol_mode_t *mode; + efi_graphics_output_mode_info_t *info; + unsigned long info_size; + + u32 max_mode, cur_mode, best_mode, area; + u8 depth; + int pf; + efi_pixel_bitmask_t pi; + u32 m, w, h, a; + u8 d; + + mode = efi_table_attr(gop, mode); + + cur_mode = efi_table_attr(mode, mode); + max_mode = efi_table_attr(mode, max_mode); + + info = efi_table_attr(mode, info); + + pf = info->pixel_format; + pi = info->pixel_information; + w = info->horizontal_resolution; + h = info->vertical_resolution; + + best_mode = cur_mode; + area = w * h; + depth = pixel_bpp(pf, pi); + + for (m = 0; m < max_mode; m++) { + if (m == cur_mode) + continue; + + status = efi_call_proto(gop, query_mode, m, + &info_size, &info); + if (status != EFI_SUCCESS) + continue; + + pf = info->pixel_format; + pi = info->pixel_information; + w = info->horizontal_resolution; + h = info->vertical_resolution; + + efi_bs_call(free_pool, info); + + if (pf == PIXEL_BLT_ONLY || pf >= PIXEL_FORMAT_MAX) + continue; + a = w * h; + if (a < area) + continue; + d = pixel_bpp(pf, pi); + if (a > area || d > depth) { + best_mode = m; + area = a; + depth = d; + } + } + + return best_mode; +} + static void set_mode(efi_graphics_output_protocol_t *gop) { efi_graphics_output_protocol_mode_t *mode; @@ -223,6 +302,9 @@ static void set_mode(efi_graphics_output_protocol_t *gop) case EFI_CMDLINE_RES: new_mode = choose_mode_res(gop); break; + case EFI_CMDLINE_AUTO: + new_mode = choose_mode_auto(gop); + break; default: return; } -- 2.24.1
On Sat, 28 Mar 2020 at 17:06, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> Ard, can you replace the previous version with v3? I skipped calling
> query_mode for cur_mode in choose_mode_res but forgot to do it for
> choose_mode_auto.
>
Done.