From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Nelson Date: Wed, 17 Oct 2012 08:34:44 -0700 Subject: [U-Boot] [PATCH v3 13/18] lcd: Add support for flushing LCD fb from dcache after update In-Reply-To: <20121017123829.278b8758@amdc308.digital.local> References: <1342106718-3058-1-git-send-email-sjg@chromium.org> <1342106718-3058-14-git-send-email-sjg@chromium.org> <20120809094315.79012bd2@amdc308.digital.local> <20121017123829.278b8758@amdc308.digital.local> Message-ID: <507ED014.1030204@boundarydevices.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 10/17/2012 03:38 AM, Lukasz Majewski wrote: > Hi Simon, > >> Hi, >> >> On Thu, Aug 9, 2012 at 12:43 AM, Lukasz Majewski >> 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 >>>> --- >>>> 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 > > >