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
On Thu, 19 Mar 2020 at 15:28, Arvind Sankar <[email protected]> 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
>
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 <[email protected]>
---
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
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 <[email protected]>
---
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 <[email protected]>
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 prototypes and argmap for the Graphics Output Protocol's QueryMode
and SetMode functions.
Signed-off-by: Arvind Sankar <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
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
If the gop doesn't have a framebuffer, there's no point in checking for
con_out support.
Signed-off-by: Arvind Sankar <[email protected]>
---
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
Small cleanup to get rid of conout_found.
Signed-off-by: Arvind Sankar <[email protected]>
---
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
Move extraction of the mode information parameters outside the loop to
find the gop, and eliminate some redundant variables.
Signed-off-by: Arvind Sankar <[email protected]>
---
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
Declare the variables inside the block where they're used.
Get rid of a couple of redundant initializers.
Signed-off-by: Arvind Sankar <[email protected]>
---
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
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 <[email protected]>
---
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 <[email protected]>
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
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 <[email protected]>
---
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
current_fb_base isn't used for anything except assigning to fb_base if
we locate a suitable gop.
Signed-off-by: Arvind Sankar <[email protected]>
---
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
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 <[email protected]>
---
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 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 <[email protected]>
---
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 <[email protected]>
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
Move the loop to find a gop into its own function.
Signed-off-by: Arvind Sankar <[email protected]>
---
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
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 [email protected]):
- 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
On Fri, 20 Mar 2020 at 03:00, Arvind Sankar <[email protected]> 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 [email protected]):
> - 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 [email protected]):
> - 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 <[email protected]> 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 [email protected]):
> > - 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 <[email protected]> wrote:
>
> On Wed, Mar 25, 2020 at 05:41:43PM +0100, Ard Biesheuvel wrote:
> > On Fri, 20 Mar 2020 at 03:00, Arvind Sankar <[email protected]> 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 [email protected]):
> > > - 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 <[email protected]> wrote:
>
> On Wed, 25 Mar 2020 at 23:10, Arvind Sankar <[email protected]> wrote:
> >
> > On Wed, Mar 25, 2020 at 05:41:43PM +0100, Ard Biesheuvel wrote:
> > > On Fri, 20 Mar 2020 at 03:00, Arvind Sankar <[email protected]> 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 [email protected]):
> > > > - 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 <[email protected]> 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.