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

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

On 03/16/15 10:32, Hannes Petermaier wrote:
>> 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 ?

Well, you need to perform the rows and columns calculation regardless of
the rotation feature, so the console_calc_rowcol() should be there anyway.
It is a "bonus" that the rotation code can use the same function (and it
looks generic enough) for rows and columns calculation, so IMO, a cleaner
solution would be just make it non static.

> 
> 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 ?

Well, since Nikita is working on refactoring the lcd.c stuff and
already examined several aproaches, I think he can provide a better
opinion on this.
Nikita?

> 
>>>> 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.

Sounds good.

> 
>>
>> 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.

Yep. This sounds even better.
Does DT have an already defined property for this?
Have you considered may be using an environment variable for this?


-- 
Regards,
Igor.