[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 4/4] EFI: Do not set text-mode until we actually need it

From: Hans de Goede
Subject: Re: [PATCH 4/4] EFI: Do not set text-mode until we actually need it
Date: Thu, 5 Apr 2018 20:05:47 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0


On 05-04-18 14:34, Daniel Kiper wrote:
On Wed, Mar 28, 2018 at 04:50:28PM +0200, Hans de Goede wrote:
If we're running with a hidden menu we may never need text mode, so do not
change the video-mode to text until we actually need it.

Signed-off-by: Hans de Goede <address@hidden>
  grub-core/term/efi/console.c | 72 +++++++++++++++++++++++-------------
  1 file changed, 47 insertions(+), 25 deletions(-)

diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c
index 02f64ea74..d9fd7cf48 100644
--- a/grub-core/term/efi/console.c
+++ b/grub-core/term/efi/console.c
@@ -24,6 +24,11 @@
  #include <grub/efi/api.h>
  #include <grub/efi/console.h>

+static grub_err_t grub_prepare_for_text_output(struct grub_term_output *term);

Please drop this forward declaration and put the function definition before the 

I did not do that initially because that requires also moving
grub_console_setcolorstate() and grub_console_setcursor() to
higher in the file (or do a forward declaration for those).

I'll add a preparation patch to v2 of the series moving just
those 2 up to higher in the file.

+static int text_mode_available = -1;

Could you use bool type here? If yes please define grub_bool_t as stdbool.h
does (grub/bool.h?) and replace its usage in grub-core/term/tparm.c as separate
patch. If not bool please use enum here.

There are 3 states, so a bool is not going to work I will
switch to an enum for v2.

+static int text_colorstate = -1;

There already is a grub_term_color_state enum for this, which
defines values 0-2, I want to know if setcolorstate has been
called and text_colorstate contains a valid value, so I made
this an int and use -1 to mean not set / invalid.

I can see a number of solutions here:

1) Leave as is
2) Use:

static grub_term_color_state text_colorstate = -1;

Assuming the compiler will not warn about putting -1 in there.

3) Extend the grub_term_color_state like this:

typedef enum
    /* Used for uninitialized grub_term_color_state variables */
    /* The color used to display all text that does not use the
       user defined colors below.  */
    /* The user defined colors for normal text.  */
    /* The user defined colors for highlighted text.  */

Which solution would you prefer?



reply via email to

[Prev in Thread] Current Thread [Next in Thread]