Hi Eric,
On Wed, Oct 17, 2012 at 8:34 AM, Eric Nelson
<eric.nel...@boundarydevices.com> wrote:
> 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.majew...@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<s...@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
Also we might be talking about different things. I have taken the
approach of flushing the whole display, but only after some display
functions. We could flush only what has changed, which is what I was
referring to as 'fine-grained'. You may have meant the idea of
flushing after every function that touches the display, or a
'fine-grained' approach of only flushing after some functions.
My testing shows that flushing is pretty fast, but I was reluctant to
flush after every putc() as it seemed egregiously inefficient.
Regards,
Simon
>
>
>>>
>>> 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
>>
>>
>>
>>
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
