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

[Hannes Petermaier]

> Hi Hannes,
Hi Igor,

> >> +    /* 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.
Yes, thats a good idea. I will check if the compiler really initializes 
the global
struct panel_info with zero and change this.
 
[...]
> >>>   }
> >>>   +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.

I just started to move rotation specific functions into own file, called 
lcd_console_rotation.c and
ran into some trouble.

1) 
I need during initialization the console_calc_rowcol(...) function, which 
is provided by lcd.c.
A possible solution might be to "un-static" it - but i am not happy with 
this. 
Another way could be to take up vl_rot into console_t structure and pass 
only a pointer to structure to this function and decide inside the 
function.
But this would create a little mix between 0 degree and rotation code.
Yet another idea is to have (also having pointer to console_t in call) in 
lcd_console_rotation also such a calc function which overrides the values 
calculated before.

or maybe you've another solution ?

2)
I need in almost every "paint-function" the framebuffer base 
(cons.lcd_address) and the screen dimension.
This information is stored in the static structure within lcd.c - i don't 
like to make this public.
A possible solution could be to change all painting function to work 
without some global variable and pass
a third argument, pointer to console_t, and take informations from there. 
This will consume one more register
on function call, runtime is equal i think.

Whats your opinion around this ?

> >> 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.
Actually (within new code) i do initialize the pointers and things around 
with 0 degree rotation and
call afterwards the new function lcd_init_console_rot which is implemented 
in lcd_console_rotation.c
and "__weak" in lcd_console.c which does re-initialze fps and col/row 
stuff.

> 
> Also, might it be useful to allow specifiying the angle through the
> CONFIG_LCD_ROTATION define?
> Have you considered using Kconfig for this?
First i thought about this to specifiy rotation angle with a value defined 
CONFIG_LCD_ROTATION - but this is only usefull to one of my boards (where 
display is rotated 180degrees). In another board i have one U-Boot for a 
bunch of variants (16 at this time) where all rotation angles are needed. 
I want to take there this information out of device-tree and write it to 
vl_rot.
> 
> -- 
> Regards,
> Igor.
best regards,
Hannes