[U-Boot] [PATCH v3 13/18] lcd: Add support for flushing LCD fb from dcache after update

Simon Glass sjg at chromium.org
Thu Oct 18 00:07:56 CEST 2012


Hi Eric,

On Wed, Oct 17, 2012 at 8:34 AM, Eric Nelson
<eric.nelson at 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.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

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


More information about the U-Boot mailing list