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

[Igor Grinberg <grinberg@compulab.co.il>]

Hi Hannes,

On 03/12/15 18:46, Hannes Petermaier wrote:
> 
> On 2015-03-12 13:26, Igor Grinberg wrote:
>> Hi Hannes,
> Hi Igor,
> thanks for response.
>>   #endif
>> -    /* Paint the logo and retrieve LCD base address */
>> -    debug("[LCD] Drawing the logo...\n");
>> -#if defined(CONFIG_LCD_LOGO) && !defined(CONFIG_LCD_INFO_BELOW_LOGO)
>> -    console_rows = (panel_info.vl_row - BMP_LOGO_HEIGHT);
>> -    console_rows /= VIDEO_FONT_HEIGHT;
>> +    /* setup text-console */
>> +    debug("[LCD] setting up console...\n");
>> +#ifdef CONFIG_LCD_ROTATION
>> +    lcd_init_console(lcd_base,
>> +             panel_info.vl_col,
>> +             panel_info.vl_row,
>> +             panel_info.vl_rot);
>>   #else
>> -    console_rows = panel_info.vl_row / VIDEO_FONT_HEIGHT;
>> +    lcd_init_console(lcd_base,
>> +             panel_info.vl_col,
>> +             panel_info.vl_row,
>> +             0);
>>   #endif
>> Please, don't start the #ifdef mess here...
>> just always pass the panel_info.vl_rot.
> This is not possible, because 'vl_rot' does'nt exist if
> CONFIG_LCD_ROTATION is not defined. (have a look into lcd.h).

Of course I did before sending the reply...
What I'm trying to say is - let it exist and default to 0 angle rotation.

> I made this to be compatible to all who have allready initialized a
> panel_info without vl_rot.

This increases the mess and I think is not sensible enough.
Just change the users to initialize the panel_info with vl_rot.
I think also that default initialization of globals is 0, right?
If so, those users that do not initialize the vl_rot explicitly,
should have it initialized to 0 implicitly by compiler.

>>
>>> -    console_cols = panel_info.vl_col / VIDEO_FONT_WIDTH;
>>> -    lcd_init_console(lcd_base, console_rows, console_cols);
>>> +    /* Paint the logo and retrieve LCD base address */
>>> +    debug("[LCD] Drawing the logo...\n");
>>>       if (do_splash) {
>>>           s = getenv("splashimage");
>>>           if (s) {
>>> diff --git a/common/lcd_console.c b/common/lcd_console.c
>>> index cac77be..6199c9a 100644
>>> --- a/common/lcd_console.c
>>> +++ b/common/lcd_console.c
>>> @@ -2,6 +2,7 @@
>>>    * (C) Copyright 2001-2014
>>>    * DENX Software Engineering -- wd at denx.de
>>>    * Compulab Ltd - http://compulab.co.il/
>>> + * Bernecker & Rainer Industrieelektronik GmbH - http://www.br-automation.com
>>>    *
>>>    * SPDX-License-Identifier:    GPL-2.0+
>>>    */
>>> @@ -10,26 +11,27 @@
>>>   #include <lcd.h>
>>>   #include <video_font.h>        /* Get font data, width and height */
>>>   -#define CONSOLE_ROW_SIZE    (VIDEO_FONT_HEIGHT * lcd_line_length)
>>> -#define CONSOLE_ROW_FIRST    cons.lcd_address
>>> -#define CONSOLE_SIZE        (CONSOLE_ROW_SIZE * cons.rows)
>>> +#define PIXLBYTES        (NBYTES(LCD_BPP))
>>> +
>>> +#if LCD_BPP == LCD_COLOR16
>>> +    #define fbptr_t ushort
>>> +#elif LCD_BPP == LCD_COLOR32
>>> +    #define fbptr_t u32
>>> +#else
>>> +    #define fbptr_t uchar
>>> +#endif
>>>     struct console_t {
>>>       short curr_col, curr_row;
>>>       short cols, rows;
>>>       void *lcd_address;
>>> +    u32 lcdsizex, lcdsizey;
>>> +    void (*fp_putc_xy)(ushort x, ushort y, char c);
>>> +    void (*fp_console_moverow)(u32 rowdst, u32 rowsrc);
>>> +    void (*fp_console_setrow)(u32 row, int clr);
>>>   };
>>>   static struct console_t cons;
>>>   -void lcd_init_console(void *address, int rows, int cols)
>>> -{
>>> -    memset(&cons, 0, sizeof(cons));
>>> -    cons.cols = cols;
>>> -    cons.rows = rows;
>>> -    cons.lcd_address = address;
>>> -
>>> -}
>>> -
>>>   void lcd_set_col(short col)
>>>   {
>>>       cons.curr_col = col;
>>> @@ -56,63 +58,221 @@ int lcd_get_screen_columns(void)
>>>       return cons.cols;
>>>   }
>>>   -static void lcd_putc_xy(ushort x, ushort y, char c)
>>> +static void lcd_putc_xy0(ushort x, ushort y, char c)
>>>   {
>>> -    uchar *dest;
>>> -    ushort row;
>>>       int fg_color = lcd_getfgcolor();
>>>       int bg_color = lcd_getbgcolor();
>>> +    int i, row;
>>> +    uchar *dest = (uchar *)(cons.lcd_address +
>>> +                y * cons.lcdsizex * PIXLBYTES +
>>> +                x * PIXLBYTES);
>>> +
>>> +    for (row = 0; row < VIDEO_FONT_HEIGHT; row++) {
>>> +        fbptr_t *d = (fbptr_t *)dest;
>>> +        uchar bits;
>>> +        bits = video_fontdata[c * VIDEO_FONT_HEIGHT + row];
>>> +        for (i = 0; i < 8; ++i) {
>>> +            *d++ = (bits & 0x80) ? fg_color : bg_color;
>>> +            bits <<= 1;
>>> +        }
>>> +        dest += cons.lcdsizex * PIXLBYTES;
>>> +    }
>>> +}
>>> +
>>> +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;
>>> +    for (i = 0; i < (VIDEO_FONT_HEIGHT * cons.lcdsizex); i++)
>>> +        *d++ = clr;
>>> +}
>>>   -    for (row = 0; row < VIDEO_FONT_HEIGHT; ++row, dest += lcd_line_length) {
>>> -#if LCD_BPP == LCD_COLOR16
>>> -        ushort *d = (ushort *)dest;
>>> -#elif LCD_BPP == LCD_COLOR32
>>> -        u32 *d = (u32 *)dest;
>>> -#else
>>> -        uchar *d = dest;
>>> -#endif
>>> +static inline void console_moverow0(u32 rowdst, u32 rowsrc)
>>> +{
>>> +    int i;
>>> +    uchar *dst = (uchar *)(cons.lcd_address +
>>> +                   rowdst * VIDEO_FONT_HEIGHT *
>>> +                   cons.lcdsizex * PIXLBYTES);
>>> +
>>> +    uchar *src = (uchar *)(cons.lcd_address +
>>> +                   rowsrc * VIDEO_FONT_HEIGHT *
>>> +                   cons.lcdsizex * PIXLBYTES);
>>> +
>>> +    fbptr_t *pdst = (fbptr_t *)dst;
>>> +    fbptr_t *psrc = (fbptr_t *)src;
>>> +    for (i = 0; i < (VIDEO_FONT_HEIGHT * cons.lcdsizex); i++)
>>> +        *pdst++ = *psrc++;
>>> +}
>>> +#ifdef CONFIG_LCD_ROTATION
>>> +static void lcd_putc_xy90(ushort x, ushort y, char c)
>>> +{
>>> +    int fg_color = lcd_getfgcolor();
>>> +    int bg_color = lcd_getbgcolor();
>>> +    int i, col;
>>> +    uchar *dest = (uchar *)(cons.lcd_address +
>>> +                cons.lcdsizey * cons.lcdsizex * PIXLBYTES -
>>> +                (x+1) * cons.lcdsizex * PIXLBYTES +
>>> +                y * PIXLBYTES);
>>> +
>>> +    fbptr_t *d = (fbptr_t *)dest;
>>> +    uchar msk = 0x80;
>>> +    uchar *pfont = video_fontdata + c * VIDEO_FONT_HEIGHT;
>>> +    for (col = 0; col < VIDEO_FONT_WIDTH; ++col) {
>>> +        for (i = 0; i < VIDEO_FONT_HEIGHT; ++i)
>>> +            *d++ = (*(pfont + i) & msk) ? fg_color : bg_color;
>>> +        msk >>= 1;
>>> +        d -= (cons.lcdsizex + VIDEO_FONT_HEIGHT);
>>> +    }
>>> +}
>>> +
>>> +static inline void console_setrow90(u32 row, int clr)
>>> +{
>>> +    int i, j;
>>> +    uchar *dst = (uchar *)(cons.lcd_address +
>>> +                   row*VIDEO_FONT_HEIGHT * PIXLBYTES);
>>> +
>>> +    for (j = 0; j < cons.lcdsizey; j++) {
>>> +        fbptr_t *pdst = (fbptr_t *)dst;
>>> +        for (i = 0; i < VIDEO_FONT_HEIGHT; i++)
>>> +            *pdst++ = clr;
>>> +        dst += cons.lcdsizex * PIXLBYTES;
>>> +    }
>>> +}
>>> +
>>> +static inline void console_moverow90(u32 rowdst, u32 rowsrc)
>>> +{
>>> +    int i, j;
>>> +    uchar *dst = (uchar *)(cons.lcd_address +
>>> +                  rowdst*VIDEO_FONT_HEIGHT * PIXLBYTES);
>>> +
>>> +    uchar *src = (uchar *)(cons.lcd_address +
>>> +                  rowsrc*VIDEO_FONT_HEIGHT * PIXLBYTES);
>>> +
>>> +    for (j = 0; j < cons.lcdsizey; j++) {
>>> +        fbptr_t *psrc = (fbptr_t *)src;
>>> +        fbptr_t *pdst = (fbptr_t *)dst;
>>> +        for (i = 0; i < VIDEO_FONT_HEIGHT; i++)
>>> +            *pdst++ = *psrc++;
>>> +        src += cons.lcdsizex * PIXLBYTES;
>>> +        dst += cons.lcdsizex * PIXLBYTES;
>>> +    }
>>> +}
>>> +
>>> +static void lcd_putc_xy180(ushort x, ushort y, char c)
>>> +{
>>> +    int fg_color = lcd_getfgcolor();
>>> +    int bg_color = lcd_getbgcolor();
>>> +    int i, row;
>>> +    uchar *dest = (uchar *)(cons.lcd_address +
>>> +                cons.lcdsizex * PIXLBYTES +
>>> +                cons.lcdsizey * cons.lcdsizex * PIXLBYTES -
>>> +                y * cons.lcdsizex * PIXLBYTES -
>>> +                (x+1) * PIXLBYTES);
>>> +
>>> +    for (row = 0; row < VIDEO_FONT_HEIGHT; row++) {
>>> +        fbptr_t *d = (fbptr_t *)dest;
>>>           uchar bits;
>>>           bits = video_fontdata[c * VIDEO_FONT_HEIGHT + row];
>>>             for (i = 0; i < 8; ++i) {
>>> -            *d++ = (bits & 0x80) ? fg_color : bg_color;
>>> +            *d-- = (bits & 0x80) ? fg_color : bg_color;
>>>               bits <<= 1;
>>>           }
>>> +        dest -= cons.lcdsizex * PIXLBYTES;
>>>       }
>>>   }
>>>   -static void console_scrollup(void)
>>> +static void lcd_putc_xy270(ushort x, ushort y, char c)
>>>   {
>>> -    const int rows = CONFIG_CONSOLE_SCROLL_LINES;
>>> +    int fg_color = lcd_getfgcolor();
>>>       int bg_color = lcd_getbgcolor();
>>> +    int col, i;
>>> +    uchar *dest = (uchar *)(cons.lcd_address +
>>> +                ((x+1) * cons.lcdsizex) * PIXLBYTES -
>>> +                y * PIXLBYTES);
>>> +
>>> +    fbptr_t *d = (fbptr_t *)dest;
>>> +    uchar msk = 0x80;
>>> +    uchar *pfont = video_fontdata + c * VIDEO_FONT_HEIGHT;
>>> +    for (col = 0; col < VIDEO_FONT_WIDTH; ++col) {
>>> +        for (i = 0; i < VIDEO_FONT_HEIGHT; ++i)
>>> +            *d-- = (*(pfont + i) & msk) ? fg_color : bg_color;
>>> +        msk >>= 1;
>>> +        d += (cons.lcdsizex + VIDEO_FONT_HEIGHT);
>>> +    }
>>> +}
>>>   -    /* Copy up rows ignoring those that will be overwritten */
>>> -    memcpy(CONSOLE_ROW_FIRST,
>>> -           cons.lcd_address + CONSOLE_ROW_SIZE * rows,
>>> -           CONSOLE_SIZE - CONSOLE_ROW_SIZE * rows);
>>> +static inline void console_setrow270(u32 row, int clr)
>>> +{
>>> +    int i, j;
>>> +    uchar *dst = (uchar *)(cons.lcd_address +
>>> +                   cons.lcdsizex * PIXLBYTES -
>>> +                   (row*VIDEO_FONT_HEIGHT+1) * PIXLBYTES);
>>> +
>>> +    for (j = 0; j < cons.lcdsizey; j++) {
>>> +        fbptr_t *pdst = (fbptr_t *)dst;
>>> +        for (i = 0; i < VIDEO_FONT_HEIGHT; i++)
>>> +            *pdst-- = clr;
>>> +        dst += cons.lcdsizex * PIXLBYTES;
>>> +    }
>>> +}
>>>   -    /* Clear the last rows */
>>> -#if (LCD_BPP != LCD_COLOR32)
>>> -    memset(lcd_console_address + CONSOLE_SIZE - CONSOLE_ROW_SIZE * rows,
>>> -           bg_color, CONSOLE_ROW_SIZE * rows);
>>> -#else
>>> -    u32 *ppix = cons.lcd_address +
>>> -            CONSOLE_SIZE - CONSOLE_ROW_SIZE * rows;
>>> -    u32 i;
>>> -    for (i = 0;
>>> -        i < (CONSOLE_ROW_SIZE * rows) / NBYTES(panel_info.vl_bpix);
>>> -        i++) {
>>> -        *ppix++ = bg_color;
>>> +static inline void console_moverow270(u32 rowdst, u32 rowsrc)
>>> +{
>>> +    int i, j;
>>> +    uchar *dst = (uchar *)(cons.lcd_address +
>>> +                  cons.lcdsizex * PIXLBYTES -
>>> +                  (rowdst*VIDEO_FONT_HEIGHT+1) * PIXLBYTES);
>>> +
>>> +    uchar *src = (uchar *)(cons.lcd_address +
>>> +                  cons.lcdsizex * PIXLBYTES -
>>> +                  (rowsrc*VIDEO_FONT_HEIGHT+1) * PIXLBYTES);
>>> +
>>> +    for (j = 0; j < cons.lcdsizey; j++) {
>>> +        fbptr_t *psrc = (fbptr_t *)src;
>>> +        fbptr_t *pdst = (fbptr_t *)dst;
>>> +        for (i = 0; i < VIDEO_FONT_HEIGHT; i++)
>>> +            *pdst-- = *psrc--;
>>> +        src += cons.lcdsizex * PIXLBYTES;
>>> +        dst += cons.lcdsizex * PIXLBYTES;
>>>       }
>>> -#endif
>>> -    lcd_sync();
>>> -    cons.curr_row -= rows;
>>>   }
>>>   +static inline void console_setrow180(u32 row, int clr)
>>> +{
>>> +    int i;
>>> +    uchar *dst = (uchar *)(cons.lcd_address +
>>> +                   (cons.rows-row-1) * VIDEO_FONT_HEIGHT *
>>> +                   cons.lcdsizex * PIXLBYTES);
>>> +
>>> +    fbptr_t *d = (fbptr_t *)dst;
>>> +    for (i = 0; i < (VIDEO_FONT_HEIGHT * cons.lcdsizex); i++)
>>> +        *d++ = clr;
>>> +}
>>> +
>>> +static inline void console_moverow180(u32 rowdst, u32 rowsrc)
>>> +{
>>> +    int i;
>>> +    uchar *dst = (uchar *)(cons.lcd_address +
>>> +                   (cons.rows-rowdst-1) * VIDEO_FONT_HEIGHT *
>>> +                   cons.lcdsizex * PIXLBYTES);
>>> +
>>> +    uchar *src = (uchar *)(cons.lcd_address +
>>> +                   (cons.rows-rowsrc-1) * VIDEO_FONT_HEIGHT *
>>> +                   cons.lcdsizex * PIXLBYTES);
>>> +
>>> +    fbptr_t *pdst = (fbptr_t *)dst;
>>> +    fbptr_t *psrc = (fbptr_t *)src;
>>> +    for (i = 0; i < (VIDEO_FONT_HEIGHT * cons.lcdsizex); i++)
>>> +        *pdst++ = *psrc++;
>>> +}
>>> +
>>> +#endif /* CONFIG_LCD_ROTATION */
>> Can't this whole thing go into a separate file?
>> So, the console stuff will only define weak functions which can be overridden
>> by the rotation functionality.
>> This will keep the console code clean (also from ifdefs) and have the rotation
>> functionality cleanly added by a CONFIG_ symbol, which will control the
>> compilation for the separate file.
> Might be possible, which name should we give to it ? lcd_console_rotation.c ?

Sounds good.

> But how we deal with the function-pointer initialization ?

I think the usual method would do...
You call some kind of lcd_console_init_rot() with most of the code
that you currently have in lcd_init_console() that is related to rotation.
If the CONFIG_LCD_ROTATION is not set, then the lcd_init_console() stub
just returns the 0 rotation config.

>>
>>> +
>>>   static inline void console_back(void)
>>>   {
>>>       if (--cons.curr_col < 0) {
>>> @@ -121,26 +281,83 @@ static inline void console_back(void)
>>>               cons.curr_row = 0;
>>>       }
>>>   -    lcd_putc_xy(cons.curr_col * VIDEO_FONT_WIDTH,
>>> -            cons.curr_row * VIDEO_FONT_HEIGHT, ' ');
>>> +    cons.fp_putc_xy(cons.curr_col * VIDEO_FONT_WIDTH,
>>> +            cons.curr_row * VIDEO_FONT_HEIGHT, ' ');
>>>   }
>>>     static inline void console_newline(void)
>>>   {
>>> +    const int rows = CONFIG_CONSOLE_SCROLL_LINES;
>>> +    int bg_color = lcd_getbgcolor();
>>> +    int i;
>>> +
>>>       cons.curr_col = 0;
>>>         /* Check if we need to scroll the terminal */
>>> -    if (++cons.curr_row >= cons.rows)
>>> -        console_scrollup();
>>> -    else
>>> -        lcd_sync();
>>> +    if (++cons.curr_row >= cons.rows) {
>>> +        for (i = 0; i < cons.rows-rows; i++)
>>> +            cons.fp_console_moverow(i, i+rows);
>>> +        for (i = 0; i < rows; i++)
>>> +            cons.fp_console_setrow(cons.rows-i-1, bg_color);
>>> +        cons.curr_row -= rows;
>>> +    }
>>> +    lcd_sync();
>>> +}
>>> +
>>> +static void console_calc_rowcol(int vl_cols, int vl_rows, short *cl, short *rw)
>>> +{
>>> +    *cl = vl_cols / VIDEO_FONT_WIDTH;
>>> +#if defined(CONFIG_LCD_LOGO) && !defined(CONFIG_LCD_INFO_BELOW_LOGO)
>>> +    *rw = (vl_rows - BMP_LOGO_HEIGHT);
>>> +    *rw /= VIDEO_FONT_HEIGHT;
>>> +#else
>>> +    *rw = vl_rows / VIDEO_FONT_HEIGHT;
>>> +#endif
>>> +}
>>> +
>>> +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);
>> This call can be made after the if else structure and written only once.
> No. Have a closer look to it. If display is rotated 0?/180? the
> calculation is not the same if display is rotated 90/270?

Yep. Sorry, I've missed the switch between cols and 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");
>>> +    }
>> I would recommend extracting the whole if else ... structure into
>> a separate function say lcd_setup_console_rot() or something and
>> make the default one doing only the vl_rot == 0 stuff.
> Whats the use of this ?
> Should this also be in a separate file?

Yes, that is how I think it will do better.

Just to explain my points:
1) make the rotation feature an integrative part of the code by always using
   the rotation code: if CONFIG_LCD_ROTATION is *not* set then just rotate it
   to 0 degrees and compile out the rest of the code.
2) make the rotation feature a bit more separate from the console code.
   I believe this way will make it cleaner.

Also, might it be useful to allow specifiying the angle through the
CONFIG_LCD_ROTATION define?
Have you considered using Kconfig for this?

-- 
Regards,
Igor.