[PATCH 1/2] EFI: console: Do not set colorstate until the first text output

[PATCH 1/2] EFI: console: Do not set colorstate until the first text output

[Hans de Goede]

GRUB_MOD_INIT(normal) does an unconditional:

grub_env_set ("color_normal", "light-gray/black");

which triggers a grub_term_setcolorstate() call. The original version
of the "efi/console: Do not set text-mode until we actually need it" patch:
https://lists.gnu.org/archive/html/grub-devel/2018-03/msg00125.html

Protected against this by caching the requested state in
grub_console_setcolorstate () and then only applying it when the first
text output actually happens. During refactoring to move the
grub_console_setcolorstate () up higher in the grub-core/term/efi/console.c
file the code to cache the color-state + bail early was accidentally
dropped.

Restore the cache the color-state + bail early behavior from the original.

Cc: Javier Martinez Canillas <javierm@redhat.com>
Fixes: 2d7c3abd871f ("efi/console: Do not set text-mode until we actually need it")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 grub-core/term/efi/console.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c
index 2f1ae85ba..c44b2ac31 100644
--- a/grub-core/term/efi/console.c
+++ b/grub-core/term/efi/console.c
@@ -82,6 +82,16 @@ grub_console_setcolorstate (struct grub_term_output *term
 {
   grub_efi_simple_text_output_interface_t *o;
 
+  if (grub_efi_is_finished || text_mode != GRUB_TEXT_MODE_AVAILABLE)
+    {
+      /*
+       * Cache colorstate changes before the first text-output, this avoids
+       * "color_normal" environment writes causing a switch to textmode.
+       */
+      text_colorstate = state;
+      return;
+    }
+
   if (grub_efi_is_finished)
     return;
 
-- 
2.33.1

[PATCH 2/2] EFI: console: Do not set cursor until the first text output

[Hans de Goede]

To allow flickerfree boot the EFI console code does not call
grub_efi_set_text_mode (1) until some text is actually output.

Depending on if the output text is because of an error loading
e.g. the .cfg file; or because of showing the menu the cursor needs
to be on or off when the first text is shown.

So far the cursor was hardcoded to being on, but this is causing
drawing artifacts + slow drawing of the menu as reported here:
https://bugzilla.redhat.com/show_bug.cgi?id=1946969

Handle the cursorstate in the same way as the colorstate to fix this,
when no text has been output yet, just cache the cursorstate and
then use the last set value when the first text is output.

Fixes: 2d7c3abd871f ("efi/console: Do not set text-mode until we actually need it")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 grub-core/term/efi/console.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c
index c44b2ac31..a3622e4fe 100644
--- a/grub-core/term/efi/console.c
+++ b/grub-core/term/efi/console.c
@@ -31,7 +31,15 @@ typedef enum {
 }
 grub_text_mode;
 
+typedef enum {
+    GRUB_CURSOR_MODE_UNDEFINED = -1,
+    GRUB_CURSOR_MODE_OFF = 0,
+    GRUB_CURSUR_MODE_ON
+}
+grub_cursor_mode;
+
 static grub_text_mode text_mode = GRUB_TEXT_MODE_UNDEFINED;
+static grub_cursor_mode cursor_mode = GRUB_CURSOR_MODE_UNDEFINED;
 static grub_term_color_state text_colorstate = GRUB_TERM_COLOR_UNDEFINED;
 
 static grub_uint32_t
@@ -119,8 +127,12 @@ grub_console_setcursor (struct grub_term_output *term __attribute__ ((unused)),
 {
   grub_efi_simple_text_output_interface_t *o;
 
-  if (grub_efi_is_finished)
-    return;
+  if (grub_efi_is_finished || text_mode != GRUB_TEXT_MODE_AVAILABLE)
+    {
+      /* Cache cursor changes before the first text-output */
+      cursor_mode = on;
+      return;
+    }
 
   o = grub_efi_system_table->con_out;
   efi_call_2 (o->enable_cursor, o, on);
@@ -143,7 +155,8 @@ grub_prepare_for_text_output (struct grub_term_output *term)
       return GRUB_ERR_BAD_DEVICE;
     }
 
-  grub_console_setcursor (term, 1);
+  if (cursor_mode != GRUB_CURSOR_MODE_UNDEFINED)
+    grub_console_setcursor (term, cursor_mode);
   if (text_colorstate != GRUB_TERM_COLOR_UNDEFINED)
     grub_console_setcolorstate (term, text_colorstate);
   text_mode = GRUB_TEXT_MODE_AVAILABLE;
-- 
2.33.1

Re: [PATCH 1/2] EFI: console: Do not set colorstate until the first text output

[Javier Martinez Canillas]

Hello Hans,

Thanks for the patch.

On 1/28/22 12:43, Hans de Goede wrote:
> GRUB_MOD_INIT(normal) does an unconditional:
> 
> grub_env_set ("color_normal", "light-gray/black");
> 
> which triggers a grub_term_setcolorstate() call. The original version
> of the "efi/console: Do not set text-mode until we actually need it" patch:
> https://lists.gnu.org/archive/html/grub-devel/2018-03/msg00125.html
> 
> Protected against this by caching the requested state in
> grub_console_setcolorstate () and then only applying it when the first
> text output actually happens. During refactoring to move the
> grub_console_setcolorstate () up higher in the grub-core/term/efi/console.c
> file the code to cache the color-state + bail early was accidentally
> dropped.
> 
> Restore the cache the color-state + bail early behavior from the original.
> 
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Fixes: 2d7c3abd871f ("efi/console: Do not set text-mode until we actually need it")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  grub-core/term/efi/console.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c
> index 2f1ae85ba..c44b2ac31 100644
> --- a/grub-core/term/efi/console.c
> +++ b/grub-core/term/efi/console.c
> @@ -82,6 +82,16 @@ grub_console_setcolorstate (struct grub_term_output *term
>  {
>    grub_efi_simple_text_output_interface_t *o;
>  
> +  if (grub_efi_is_finished || text_mode != GRUB_TEXT_MODE_AVAILABLE)
> +    {
> +      /*
> +       * Cache colorstate changes before the first text-output, this avoids
> +       * "color_normal" environment writes causing a switch to textmode.
> +       */
> +      text_colorstate = state;
> +      return;
> +    }
> +
>    if (grub_efi_is_finished)
>      return;
>

Indeed, sorry for messing this when addressing the concerns raised by
Daniel in your original patch. The fix looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 2/2] EFI: console: Do not set cursor until the first text output

[Javier Martinez Canillas]

On 1/28/22 12:43, Hans de Goede wrote:
> To allow flickerfree boot the EFI console code does not call
> grub_efi_set_text_mode (1) until some text is actually output.
> 
> Depending on if the output text is because of an error loading
> e.g. the .cfg file; or because of showing the menu the cursor needs
> to be on or off when the first text is shown.
> 
> So far the cursor was hardcoded to being on, but this is causing
> drawing artifacts + slow drawing of the menu as reported here:
> https://bugzilla.redhat.com/show_bug.cgi?id=1946969
> 
> Handle the cursorstate in the same way as the colorstate to fix this,
> when no text has been output yet, just cache the cursorstate and
> then use the last set value when the first text is output.
> 
> Fixes: 2d7c3abd871f ("efi/console: Do not set text-mode until we actually need it")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 2/2] EFI: console: Do not set cursor until the first text output

[Robbie Harwood]

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]

Javier Martinez Canillas <javierm@redhat.com> writes:

> On 1/28/22 12:43, Hans de Goede wrote:
>> To allow flickerfree boot the EFI console code does not call
>> grub_efi_set_text_mode (1) until some text is actually output.
>> 
>> Depending on if the output text is because of an error loading
>> e.g. the .cfg file; or because of showing the menu the cursor needs
>> to be on or off when the first text is shown.
>> 
>> So far the cursor was hardcoded to being on, but this is causing
>> drawing artifacts + slow drawing of the menu as reported here:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1946969
>> 
>> Handle the cursorstate in the same way as the colorstate to fix this,
>> when no text has been output yet, just cache the cursorstate and
>> then use the last set value when the first text is output.
>> 
>> Fixes: 2d7c3abd871f ("efi/console: Do not set text-mode until we actually need it")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Robbie Harwood <rharwood@redhat.com>

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH 1/2] EFI: console: Do not set colorstate until the first text output

[Robbie Harwood]

[-- Attachment #1: Type: text/plain, Size: 2257 bytes --]

Javier Martinez Canillas <javierm@redhat.com> writes:

> Hello Hans,
>
> Thanks for the patch.
>
> On 1/28/22 12:43, Hans de Goede wrote:
>> GRUB_MOD_INIT(normal) does an unconditional:
>> 
>> grub_env_set ("color_normal", "light-gray/black");
>> 
>> which triggers a grub_term_setcolorstate() call. The original version
>> of the "efi/console: Do not set text-mode until we actually need it" patch:
>> https://lists.gnu.org/archive/html/grub-devel/2018-03/msg00125.html
>> 
>> Protected against this by caching the requested state in
>> grub_console_setcolorstate () and then only applying it when the first
>> text output actually happens. During refactoring to move the
>> grub_console_setcolorstate () up higher in the grub-core/term/efi/console.c
>> file the code to cache the color-state + bail early was accidentally
>> dropped.
>> 
>> Restore the cache the color-state + bail early behavior from the original.
>> 
>> Cc: Javier Martinez Canillas <javierm@redhat.com>
>> Fixes: 2d7c3abd871f ("efi/console: Do not set text-mode until we actually need it")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  grub-core/term/efi/console.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>> 
>> diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c
>> index 2f1ae85ba..c44b2ac31 100644
>> --- a/grub-core/term/efi/console.c
>> +++ b/grub-core/term/efi/console.c
>> @@ -82,6 +82,16 @@ grub_console_setcolorstate (struct grub_term_output *term
>>  {
>>    grub_efi_simple_text_output_interface_t *o;
>>  
>> +  if (grub_efi_is_finished || text_mode != GRUB_TEXT_MODE_AVAILABLE)
>> +    {
>> +      /*
>> +       * Cache colorstate changes before the first text-output, this avoids
>> +       * "color_normal" environment writes causing a switch to textmode.
>> +       */
>> +      text_colorstate = state;
>> +      return;
>> +    }
>> +
>>    if (grub_efi_is_finished)
>>      return;
>>
>
> Indeed, sorry for messing this when addressing the concerns raised by
> Daniel in your original patch. The fix looks good to me.
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Robbie Harwood <rharwood@redhat.com>

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH 1/2] EFI: console: Do not set colorstate until the first text output

[Hans de Goede]

Hi Daniel,

On 1/28/22 12:43, Hans de Goede wrote:
> GRUB_MOD_INIT(normal) does an unconditional:
> 
> grub_env_set ("color_normal", "light-gray/black");
> 
> which triggers a grub_term_setcolorstate() call. The original version
> of the "efi/console: Do not set text-mode until we actually need it" patch:
> https://lists.gnu.org/archive/html/grub-devel/2018-03/msg00125.html
> 
> Protected against this by caching the requested state in
> grub_console_setcolorstate () and then only applying it when the first
> text output actually happens. During refactoring to move the
> grub_console_setcolorstate () up higher in the grub-core/term/efi/console.c
> file the code to cache the color-state + bail early was accidentally
> dropped.
> 
> Restore the cache the color-state + bail early behavior from the original.
> 
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Fixes: 2d7c3abd871f ("efi/console: Do not set text-mode until we actually need it")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

What is the status of this series? It has been well over a month and both
patches have received 2 Reviewed-by-s and no other comments:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Robbie Harwood <rharwood@redhat.com>

Regards,

Hans



> ---
>  grub-core/term/efi/console.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c
> index 2f1ae85ba..c44b2ac31 100644
> --- a/grub-core/term/efi/console.c
> +++ b/grub-core/term/efi/console.c
> @@ -82,6 +82,16 @@ grub_console_setcolorstate (struct grub_term_output *term
>  {
>    grub_efi_simple_text_output_interface_t *o;
>  
> +  if (grub_efi_is_finished || text_mode != GRUB_TEXT_MODE_AVAILABLE)
> +    {
> +      /*
> +       * Cache colorstate changes before the first text-output, this avoids
> +       * "color_normal" environment writes causing a switch to textmode.
> +       */
> +      text_colorstate = state;
> +      return;
> +    }
> +
>    if (grub_efi_is_finished)
>      return;
>  

Re: [PATCH 1/2] EFI: console: Do not set colorstate until the first text output

[Daniel Kiper]

On Fri, Mar 04, 2022 at 11:30:13AM +0100, Hans de Goede wrote:
> Hi Daniel,
>
> On 1/28/22 12:43, Hans de Goede wrote:
> > GRUB_MOD_INIT(normal) does an unconditional:
> >
> > grub_env_set ("color_normal", "light-gray/black");
> >
> > which triggers a grub_term_setcolorstate() call. The original version
> > of the "efi/console: Do not set text-mode until we actually need it" patch:
> > https://lists.gnu.org/archive/html/grub-devel/2018-03/msg00125.html
> >
> > Protected against this by caching the requested state in
> > grub_console_setcolorstate () and then only applying it when the first
> > text output actually happens. During refactoring to move the
> > grub_console_setcolorstate () up higher in the grub-core/term/efi/console.c
> > file the code to cache the color-state + bail early was accidentally
> > dropped.
> >
> > Restore the cache the color-state + bail early behavior from the original.
> >
> > Cc: Javier Martinez Canillas <javierm@redhat.com>
> > Fixes: 2d7c3abd871f ("efi/console: Do not set text-mode until we actually need it")
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> What is the status of this series? It has been well over a month and both
> patches have received 2 Reviewed-by-s and no other comments:
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Robbie Harwood <rharwood@redhat.com>

I will take a look at them in a week or two. Sorry for delay...

Daniel

Re: [PATCH 1/2] EFI: console: Do not set colorstate until the first text output

[Daniel Kiper]

On Fri, Mar 04, 2022 at 11:30:13AM +0100, Hans de Goede wrote:
> Hi Daniel,
>
> On 1/28/22 12:43, Hans de Goede wrote:
> > GRUB_MOD_INIT(normal) does an unconditional:
> >
> > grub_env_set ("color_normal", "light-gray/black");
> >
> > which triggers a grub_term_setcolorstate() call. The original version
> > of the "efi/console: Do not set text-mode until we actually need it" patch:
> > https://lists.gnu.org/archive/html/grub-devel/2018-03/msg00125.html
> >
> > Protected against this by caching the requested state in
> > grub_console_setcolorstate () and then only applying it when the first
> > text output actually happens. During refactoring to move the
> > grub_console_setcolorstate () up higher in the grub-core/term/efi/console.c
> > file the code to cache the color-state + bail early was accidentally
> > dropped.
> >
> > Restore the cache the color-state + bail early behavior from the original.
> >
> > Cc: Javier Martinez Canillas <javierm@redhat.com>
> > Fixes: 2d7c3abd871f ("efi/console: Do not set text-mode until we actually need it")
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> What is the status of this series? It has been well over a month and both
> patches have received 2 Reviewed-by-s and no other comments:
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Robbie Harwood <rharwood@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH 1/2] EFI: console: Do not set colorstate until the first text output

[Daniel Kiper]

On Fri, Mar 04, 2022 at 11:30:13AM +0100, Hans de Goede wrote:
> Hi Daniel,
>
> On 1/28/22 12:43, Hans de Goede wrote:
> > GRUB_MOD_INIT(normal) does an unconditional:
> >
> > grub_env_set ("color_normal", "light-gray/black");
> >
> > which triggers a grub_term_setcolorstate() call. The original version
> > of the "efi/console: Do not set text-mode until we actually need it" patch:
> > https://lists.gnu.org/archive/html/grub-devel/2018-03/msg00125.html
> >
> > Protected against this by caching the requested state in
> > grub_console_setcolorstate () and then only applying it when the first
> > text output actually happens. During refactoring to move the
> > grub_console_setcolorstate () up higher in the grub-core/term/efi/console.c
> > file the code to cache the color-state + bail early was accidentally
> > dropped.
> >
> > Restore the cache the color-state + bail early behavior from the original.
> >
> > Cc: Javier Martinez Canillas <javierm@redhat.com>
> > Fixes: 2d7c3abd871f ("efi/console: Do not set text-mode until we actually need it")
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> What is the status of this series? It has been well over a month and both
> patches have received 2 Reviewed-by-s and no other comments:
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Robbie Harwood <rharwood@redhat.com>

Argh... By mistake I forgot to add Javier's and Robbie's RB. Sorry about that...

Daniel