[U-Boot] [PATCH 4/4] common/lcd_console: introduce display/framebuffer rotation

[Nikita Kiryanov]

Hi Hannes,

I second Grinberg's suggestion of a separate file and 0 degree default (also as a
fallback for invalid rotation value, see below).
Some additional comments:

On 03/11/2015 02:57 PM, Hannes Petermaier wrote:
> From: Hannes Petermaier <hannes.petermaier at br-automation.com>
>
> Sometimes, for example if the display is mounted in portrait mode or even if it
> mounted landscape but rotated by 180 degrees, we need to rotate our content of
> the display respectively the framebuffer, so that user can read the messages
> who are printed out.
>
> For this we introduce the feature called "CONFIG_LCD_ROTATION", this may be
> defined in the board-configuration if needed. After this the lcd_console will
> be initialized with a given rotation from "vl_rot" out of "vidinfo_t" which is
> provided by the board specific code.
>
> If CONFIG_LCD_ROTATION is not defined, the console will be initialized with
> 0 degrees rotation - the screen behaves like the days before.
>
> Signed-off-by: Hannes Petermaier <hannes.petermaier at br-automation.com>
> Signed-off-by: Hannes Petermaier <oe5hpm at oevsv.at>
> ---
>
>   README                |   17 +++
>   common/lcd.c          |   22 ++--
>   common/lcd_console.c  |  333 ++++++++++++++++++++++++++++++++++++++++---------
>   include/lcd.h         |    1 +
>   include/lcd_console.h |    9 +-
>   5 files changed, 309 insertions(+), 73 deletions(-)
>
> diff --git a/README b/README
> index 3c4a2e6..a95b1e8 100644
> --- a/README
> +++ b/README
> @@ -1933,6 +1933,23 @@ CBFS (Coreboot Filesystem) support
>   		the console jump but can help speed up operation when scrolling
>   		is slow.
>
> +		CONFIG_LCD_ROTATION
> +
> +		Sometimes, for example if the display is mounted in portrait
> +		mode or even if it mounted landscape but rotated by 180degree,
> +		we need to rotate our content of the display respectively the
> +		framebuffer, so that user can read the messages who are printed
> +		out.
> +		For this we introduce the feature called "CONFIG_LCD_ROTATION",
> +		this may be defined in the board-configuration if needed. After
> +		this the lcd_console will be initialized with a given rotation
> +		from "vl_rot" out of "vidinfo_t" which is provided by the board
> +		specific code.
> +
> +		If CONFIG_LCD_ROTATION is not defined, the console will be
> +		initialized with 0degree rotation

This is enough. "the screen behaves like the days before" is vague and unnecessary
(days before what?)

  - the screen behaves like the
> +		days before.
> +
>   		CONFIG_LCD_BMP_RLE8
>
>   		Support drawing of RLE8-compressed bitmaps on the LCD.

[...]

> +static inline void console_setrow0(u32 row, int clr)
> +{
>   	int i;
> +	uchar *dst = (uchar *)(cons.lcd_address +
> +			       row * VIDEO_FONT_HEIGHT *
> +			       cons.lcdsizex * PIXLBYTES);
>
> -	dest = (uchar *)(cons.lcd_address +
> -			 y * lcd_line_length + x * NBITS(LCD_BPP) / 8);
> +	fbptr_t *d = (fbptr_t *)dst;

Here you can just create the fbptr variable directly. You have a bunch of
function where this recasting is avoidable.

> +	for (i = 0; i < (VIDEO_FONT_HEIGHT * cons.lcdsizex); i++)
> +		*d++ = clr;
> +}

[...]

> +void lcd_init_console(void *address, int vl_cols, int vl_rows, int vl_rot)
> +{
> +	memset(&cons, 0, sizeof(cons));
> +	cons.lcd_address = address;
> +
> +	cons.lcdsizex = vl_cols;
> +	cons.lcdsizey = vl_rows;
> +
> +	if (vl_rot == 0) {
> +		cons.fp_putc_xy = &lcd_putc_xy0;
> +		cons.fp_console_moverow = &console_moverow0;
> +		cons.fp_console_setrow = &console_setrow0;
> +		console_calc_rowcol(vl_cols, vl_rows, &cons.cols, &cons.rows);
> +#ifdef CONFIG_LCD_ROTATION
> +	} else if (vl_rot == 90) {
> +		cons.fp_putc_xy = &lcd_putc_xy90;
> +		cons.fp_console_moverow = &console_moverow90;
> +		cons.fp_console_setrow = &console_setrow90;
> +		console_calc_rowcol(vl_rows, vl_cols, &cons.cols, &cons.rows);
> +	} else if (vl_rot == 180) {
> +		cons.fp_putc_xy = &lcd_putc_xy180;
> +		cons.fp_console_moverow = &console_moverow180;
> +		cons.fp_console_setrow = &console_setrow180;
> +		console_calc_rowcol(vl_cols, vl_rows, &cons.cols, &cons.rows);
> +	} else if (vl_rot == 270) {
> +		cons.fp_putc_xy = &lcd_putc_xy270;
> +		cons.fp_console_moverow = &console_moverow270;
> +		cons.fp_console_setrow = &console_setrow270;
> +		console_calc_rowcol(vl_rows, vl_cols, &cons.cols, &cons.rows);
> +#endif
> +	} else {
> +		puts("lcd_init_console: invalid framebuffer rotation!\n");

This case leaves the function pointers uninitialized, which would crash the system later on.
I suggest you default to 0 degree rotation both for the generic case and for the fallback behavior
(with the warning message where necessary).

> +	}
> +
> +	debug("lcd_console: have %d/%d col/rws on scr %dx%d (%d deg rotated)\n",
> +	      cons.cols, cons.rows, cons.lcdsizex, cons.lcdsizey, vl_rot);
> +
>   }
>
>   void lcd_putc(const char c)
>   {
>   	if (!lcd_is_enabled) {
>   		serial_putc(c);
> -

This is a cleanup. It should not be in this patch.

>   		return;
>   	}
>
> @@ -150,7 +367,6 @@ void lcd_putc(const char c)
>   		return;
>   	case '\n':
>   		console_newline();
> -

Same here, and in other places...


>   		return;
>   	}
>
> diff --git a/include/lcd.h b/include/lcd.h
> index f049fd3..41e0c11 100644
> --- a/include/lcd.h
> +++ b/include/lcd.h
> @@ -51,6 +51,7 @@ void lcd_set_flush_dcache(int flush);
>   typedef struct vidinfo {
>   	ushort	vl_col;		/* Number of columns (i.e. 160) */
>   	ushort	vl_row;		/* Number of rows (i.e. 100) */
> +	ushort	vl_rot;		/* Rotation of Display (i.e. 90) */
>   	u_char	vl_bpix;	/* Bits per pixel, 0 = 1 */
>   	ushort	*cmap;		/* Pointer to the colormap */
>   	void	*priv;		/* Pointer to driver-specific data */
> diff --git a/include/lcd_console.h b/include/lcd_console.h
> index 429214d..2244b3f 100644
> --- a/include/lcd_console.h
> +++ b/include/lcd_console.h
> @@ -16,11 +16,12 @@
>    * console has.
>    *
>    * @address: Console base address
> - * @rows: Number of rows in the console
> - * @cols: Number of columns in the console
> + * @vl_rows: Number of rows in the console
> + * @vl_cols: Number of columns in the console
> + * @vl_rot: Rotation of display in degree (0 - 90 - 180 - 270) counterlockwise
>    */
> -void lcd_init_console(void *address, int rows, int cols);
> -
> +/*void lcd_init_console(void *address, int rows, int cols); */

Please delete this comment

> +void lcd_init_console(void *address, int vl_cols, int vl_rows, int vl_rot);
>   /**
>    * lcd_set_col() - Set the number of the current lcd console column
>    *
>

-- 
Regards,
Nikita Kiryanov