[U-Boot] [PATCH v3] efi_loader: console: Correctly report modes

[Emmanuel Vadot]

On Fri, 28 Oct 2016 11:33:19 +0200
Alexander Graf <agraf at suse.de> wrote:

> On 10/28/2016 10:04 AM, Emmanuel Vadot wrote:
> > Add support for EFI console modes.
> > Mode 0 is always 80x25 and present by EFI specification.
> > Mode 1 is always 80x50 and not mandatory.
> > Mode 2 and above is freely usable.
> >
> > If the terminal can handle mode 1, we mark it as supported.
> > If the terminal size is greater than mode 0 and different than mode 1,
> > we install it as mode 2.
> >
> > Modes can be switch with cout_set_mode.
> >
> > Changes in V3:
> >   Valid mode are 0 to EFIMode-1
> >   Fix style
> >
> > Changes in V2:
> >   Add mode switch
> >   Report only the modes that we support
> >
> > Signed-off-by: Emmanuel Vadot <manu at bidouilliste.com>
> > ---
> >   lib/efi_loader/efi_console.c | 84 +++++++++++++++++++++++++++++++++++---------
> >   1 file changed, 68 insertions(+), 16 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> > index 2e0228c..eabf54f 100644
> > --- a/lib/efi_loader/efi_console.c
> > +++ b/lib/efi_loader/efi_console.c
> > @@ -9,11 +9,38 @@
> >   #include <common.h>
> >   #include <efi_loader.h>
> >   
> > -/* If we can't determine the console size, default to 80x24 */
> > -static int console_columns = 80;
> > -static int console_rows = 24;
> >   static bool console_size_queried;
> >   
> > +#define EFI_COUT_MODE_2 2
> > +#define EFI_MAX_COUT_MODE 3
> > +
> > +struct cout_mode {
> > +	unsigned long columns;
> > +	unsigned long rows;
> > +	int present;
> > +};
> > +
> > +static struct cout_mode efi_cout_modes[] = {
> > +	/* EFI Mode 0 is 80x25 and always present */
> > +	{
> > +		.columns = 80,
> > +		.rows = 25,
> > +		.present = 1,
> > +	},
> > +	/* EFI Mode 1 is always 80x50 */
> > +	{
> > +		.columns = 80,
> > +		.rows = 50,
> > +		.present = 0,
> > +	},
> > +	/* Value are unknown until we query the console */
> > +	{
> > +		.columns = 0,
> > +		.rows = 0,
> > +		.present = 0,
> > +	},
> > +};
> > +
> >   const efi_guid_t efi_guid_console_control = CONSOLE_CONTROL_GUID;
> >   
> >   #define cESC '\x1b'
> > @@ -56,8 +83,9 @@ const struct efi_console_control_protocol efi_console_control = {
> >   	.lock_std_in = efi_cin_lock_std_in,
> >   };
> >   
> > +/* Default to mode 0 */
> >   static struct simple_text_output_mode efi_con_mode = {
> > -	.max_mode = 0,
> > +	.max_mode = 1,
> >   	.mode = 0,
> >   	.attribute = 0,
> >   	.cursor_column = 0,
> > @@ -131,8 +159,10 @@ static efi_status_t EFIAPI efi_cout_output_string(
> >   			struct efi_simple_text_output_protocol *this,
> >   			const unsigned short *string)
> >   {
> > +	struct cout_mode *mode;
> >   	u16 ch;
> >   
> > +	mode = &efi_cout_modes[efi_con_mode.mode];
> >   	EFI_ENTRY("%p, %p", this, string);
> >   	for (;(ch = *string); string++) {
> >   		print_unicode_in_utf8(ch);
> > @@ -140,13 +170,12 @@ static efi_status_t EFIAPI efi_cout_output_string(
> >   		if (ch == '\n') {
> >   			efi_con_mode.cursor_column = 1;
> >   			efi_con_mode.cursor_row++;
> > -		} else if (efi_con_mode.cursor_column > console_columns) {
> > +		} else if (efi_con_mode.cursor_column > mode->columns) {
> >   			efi_con_mode.cursor_column = 1;
> >   			efi_con_mode.cursor_row++;
> >   		}
> > -		if (efi_con_mode.cursor_row > console_rows) {
> > -			efi_con_mode.cursor_row = console_rows;
> > -		}
> > +		if (efi_con_mode.cursor_row > mode->rows)
> > +			efi_con_mode.cursor_row = mode->rows;
> >   	}
> >   
> >   	return EFI_EXIT(EFI_SUCCESS);
> > @@ -191,15 +220,36 @@ static efi_status_t EFIAPI efi_cout_query_mode(
> >   			goto out;
> >   		}
> >   
> > -		console_columns = n[2];
> > -		console_rows = n[1];
> > +		/* Test if we can have Mode 1 */
> > +		if (n[2] >= 80 && n[1] >= 50) {
> 
> This is going to be very tedious to read for someone who isn't familiar 
> with the return semantics of term_read_reply(). Please put n[x] into 
> local col/row variables and use them in all conditionals afterwards.

 Will do.

> > +			efi_cout_modes[1].present = 1;
> > +			efi_con_mode.max_mode = 2;
> > +		}
> > +
> > +		/*
> > +		 * Install our mode as mode 2 if it is different
> > +		 * than mode 0 or 1 and set it to the default one
> 
> I guess you want to say "set it as the currently selected mode"?

 Yes, I'll change this.

> > +		 */
> > +		if ((n[2] != 80 && n[1] != 25) || (n[2] != 80 && n[1] != 50)) {
> > +			efi_cout_modes[EFI_COUT_MODE_2].columns = n[2];
> > +			efi_cout_modes[EFI_COUT_MODE_2].rows = n[1];
> > +			efi_cout_modes[EFI_COUT_MODE_2].present = 1;
> > +			efi_con_mode.max_mode = EFI_MAX_COUT_MODE;
> > +			efi_con_mode.mode = EFI_COUT_MODE_2;
> > +		}
> >   	}
> >   
> > +	if (mode_number >= efi_con_mode.max_mode)
> > +		return EFI_EXIT(EFI_UNSUPPORTED);
> > +
> > +	if (efi_cout_modes[mode_number].present != 1)
> > +		return EFI_EXIT(EFI_UNSUPPORTED);
> > +
> >   out:
> >   	if (columns)
> > -		*columns = console_columns;
> > +		*columns = efi_cout_modes[mode_number].columns;
> >   	if (rows)
> > -		*rows = console_rows;
> > +		*rows = efi_cout_modes[mode_number].rows;
> >   
> >   	return EFI_EXIT(EFI_SUCCESS);
> >   }
> > @@ -210,11 +260,13 @@ static efi_status_t EFIAPI efi_cout_set_mode(
> >   {
> >   	EFI_ENTRY("%p, %ld", this, mode_number);
> >   
> > -	/* We only support text output for now */
> > -	if (mode_number == EFI_CONSOLE_MODE_TEXT)
> > -		return EFI_EXIT(EFI_SUCCESS);
> 
> You're dropping the MODE_TEXT check - is that intentional?
> 
> 
> Alex

 Yes.
 Set mode switch between cout modes, the supplied mode number just have
to exists.
 After re-reading the spec I see that the cursor needs to be
set to 0x0, I'll update that too.

> >   
> > -	return EFI_EXIT(EFI_UNSUPPORTED);
> > +	if (mode_number > efi_con_mode.max_mode)
> > +		return EFI_EXIT(EFI_UNSUPPORTED);
> > +
> > +	efi_con_mode.mode = mode_number;
> > +
> > +	return EFI_EXIT(EFI_SUCCESS);
> >   }
> >   
> >   static efi_status_t EFIAPI efi_cout_set_attribute(


-- 
Emmanuel Vadot <manu at bidouilliste.com> <manu at freebsd.org>