[U-Boot] [PATCH v3 13/18] lcd: Add support for flushing LCD fb from dcache after update
Eric Nelson
eric.nelson at boundarydevices.com
Wed Oct 17 17:34:44 CEST 2012
On 10/17/2012 03:38 AM, Lukasz Majewski wrote:
> Hi Simon,
>
>> Hi,
>>
>> On Thu, Aug 9, 2012 at 12:43 AM, Lukasz Majewski
>> <l.majewski at samsung.com> wrote:
>>> Hi Simon,
>>>
>>>> This provides an option for the LCD to flush the dcache after each
>>>> update (puts, scroll or clear).
>>>>
>>>> Signed-off-by: Simon Glass<sjg at chromium.org>
>>>> ---
>>>> Changes in v2:
>>>> - Put the LCD cache flush logic into lcd_putc() instead of
>>>> lcd_puts()
>>>>
>>>> Changes in v3:
>>>> - Put the LCD cache flush logic back into lcd_puts()
>>>>
>>>> common/lcd.c | 46
>>>> +++++++++++++++++++++++++++++++++++++++------- include/lcd.h |
>>>> 8 ++++++++ 2 files changed, 47 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/common/lcd.c b/common/lcd.c
>>>> index 18525a7..f7514a4 100644
>>>> --- a/common/lcd.c
>>>> +++ b/common/lcd.c
>>>> @@ -97,6 +97,9 @@ static void lcd_setbgcolor (int color);
>>>>
>>>> char lcd_is_enabled = 0;
>>>>
>>>> +static char lcd_flush_dcache; /* 1 to flush dcache after
>>>> each lcd update */ +
>>>> +
>>>> #ifdef NOT_USED_SO_FAR
>>>> static void lcd_getcolreg (ushort regno,
>>>> ushort *red, ushort *green, ushort
>>>> *blue); @@ -105,6 +108,28 @@ static int lcd_getfgcolor (void);
>>>>
>>>> /************************************************************************/
>>>>
>>>> +/* Flush LCD activity to the caches */
>>>> +void lcd_sync(void)
>>>> +{
>>>> + /*
>>>> + * flush_dcache_range() is declared in common.h but it seems
>>>> that some
>>>> + * architectures do not actually implement it. Is there a
>>>> way to find
>>>> + * out whether it exists? For now, ARM is safe.
>>>> + */
>>>> +#ifdef CONFIG_ARM
>>>> + int line_length;
>>>> +
>>>> + if (lcd_flush_dcache)
>>>> + flush_dcache_range((u32)lcd_base,
>>>> + (u32)(lcd_base +
>>>> lcd_get_size(&line_length))); +#endif
>>>
>>> I'm struggling with a similar problem - but not in console putc, but
>>> at lcd_display_bitmap().
>>>
>>> The solution (in mine case) is:
>>> flush_dcache_range((unsigned long) fb,
>>> (unsigned long) fb +
>>> (lcd_line_length * height));
>>> which takes the "real" image range (as it is defined by fb).
>>>
>>> Flushing the lcd_base based range is a bit overkill for me.
>>>
>>>> +}
>>>> +
>>>> +void lcd_set_flush_dcache(int flush)
>>>> +{
>>>> + lcd_flush_dcache = (flush != 0);
>>>> +}
>>>> +
>>>
>>> I'm wondering if this flush_dcache_range cannot be added directly to
>>> relevant places in the code?
>>>
>>> flush_dcache_* calls are either defined (for a relevant - cache
>>> aware archs) or are dummy.
>>>
>>>> /*----------------------------------------------------------------------*/
>>>>
>>>> static void console_scrollup (void)
>>>> @@ -114,6 +139,7 @@ static void console_scrollup (void)
>>>>
>>>> /* Clear the last one */
>>>> memset (CONSOLE_ROW_LAST, COLOR_MASK(lcd_color_bg),
>>>> CONSOLE_ROW_SIZE);
>>>> + lcd_sync();
>>>> }
>>>>
>>>> /*----------------------------------------------------------------------*/
>>>> @@ -144,6 +170,8 @@ static inline void console_newline (void)
>>>> /* Scroll everything up */
>>>> console_scrollup () ;
>>>> --console_row;
>>>> + } else {
>>>> + lcd_sync();
>>>> }
>>>> }
>>>>
>>>> @@ -198,6 +226,7 @@ void lcd_puts (const char *s)
>>>> while (*s) {
>>>> lcd_putc (*s++);
>>>> }
>>>> + lcd_sync();
>>>> }
>>>>
>>>> /*----------------------------------------------------------------------*/
>>>> @@ -365,13 +394,6 @@ int drv_lcd_init (void)
>>>> }
>>>>
>>>> /*----------------------------------------------------------------------*/
>>>> -static
>>>> -int do_lcd_clear(cmd_tbl_t *cmdtp, int flag, int argc, char *const
>>>> argv[]) -{
>>>> - lcd_clear();
>>>> - return 0;
>>>> -}
>>>> -
>>>> void lcd_clear(void)
>>>> {
>>>> #if LCD_BPP == LCD_MONOCHROME
>>>> @@ -413,6 +435,14 @@ void lcd_clear(void)
>>>>
>>>> console_col = 0;
>>>> console_row = 0;
>>>> + lcd_sync();
>>>> +}
>>>> +
>>>> +static int do_lcd_clear(cmd_tbl_t *cmdtp, int flag, int argc,
>>>> + char *const argv[])
>>>> +{
>>>> + lcd_clear();
>>>> + return 0;
>>>> }
>>>>
>>>> U_BOOT_CMD(
>>>> @@ -607,6 +637,7 @@ void bitmap_plot (int x, int y)
>>>> }
>>>>
>>>> WATCHDOG_RESET();
>>>> + lcd_sync();
>>>> }
>>>> #else
>>>> static inline void bitmap_plot(int x, int y) {}
>>>> @@ -820,6 +851,7 @@ int lcd_display_bitmap(ulong bmp_image, int x,
>>>> int y) break;
>>>> };
>>>>
>>>> + lcd_sync();
>>>> return (0);
>>>> }
>>>> #endif
>>>> diff --git a/include/lcd.h b/include/lcd.h
>>>> index 26f6d83..4363131 100644
>>>> --- a/include/lcd.h
>>>> +++ b/include/lcd.h
>>>> @@ -57,6 +57,14 @@ extern void lcd_initcolregs (void);
>>>> extern struct bmp_image *gunzip_bmp(unsigned long addr, unsigned
>>>> long *lenp); extern int bmp_display(ulong addr, int x, int y);
>>>>
>>>> +/**
>>>> + * Set whether we need to flush the dcache when changing the LCD
>>>> image. This
>>>> + * defaults to off.
>>>> + *
>>>> + * @param flush non-zero to flush cache after update,
>>>> 0 to skip
>>>> + */
>>>> +void lcd_set_flush_dcache(int flush);
>>>> +
>>>> #if defined CONFIG_MPC823
>>>> /*
>>>> * LCD controller stucture for MPC823 CPU
>>>
>>> Anyway, I'm looking forward for v4 version of this patch.
>>
>> Sorry I don't think I replied to this.
>>
> Yes, this is still open issue. (not fixed I mean) I maintain the patch
> locally.
>
>> Certainly we could make the flushing more fine grained. My reasons for
>> not (so far) are:
>>
>> 1. It is simpler to flush everything (no need to figure out what part
>> of display has changed)
>> 2. The performance difference is likely to be small since flushing an
>> already-flushed range should be fast.
>
> From the sake of "SW engineering" I would opt for fine grained
> flushing. But if it turns out, that it costs too much, we can flush
> everything.
>
Is anybody else in a position to get some numbers about how/if
performance is better when flushing at the more granular level?
Before deciding it wasn't worth the code, I implemented granular
flushing and didn't see any noticeable degradation when just
flushing at the end of all public functions as in this patch.
http://lists.denx.de/pipermail/u-boot/2012-September/134979.html
It seems that performance is the only reason for fine-grained
cache flush operations
>>
>> Certainly we could enhance this code. I wonder though whether a
>> generic flushing mechanism may need to be added to support LCD and
>> also video drivers.
>
> We can add a generic mechanism to LCD and video.
>
> Simon, do you plan to post some code in a near future? Or we are now
> just "gathering requirements"?
>
>>
>> Regards,
>> Simon
>>
>>>
>>> --
>>> Best regards,
>>>
>>> Lukasz Majewski
>>>
>>> Samsung Poland R&D Center | Linux Platform Group
>
>
>
More information about the U-Boot
mailing list