[U-Boot] [PATCH] video: cfb_console: flush dcache for frame buffer in DRAM

Data cache flushing is required for frame buffer in RAM to fix the distorted console text output. Currently this text distortion is observed with cfb on beageboard and N900 when running with data cache enabled.
Reported-by: Pali Rohár pali.rohar@gmail.com Signed-off-by: Anatolij Gustschin agust@denx.de --- drivers/video/cfb_console.c | 35 +++++++++++++++++++++++++++++++++++ 1 files changed, 35 insertions(+), 0 deletions(-)
diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c index 904caf7..dd7ccb7 100644 --- a/drivers/video/cfb_console.c +++ b/drivers/video/cfb_console.c @@ -360,6 +360,8 @@ void console_cursor(int state); extern void video_get_info_str(int line_number, char *info); #endif
+DECLARE_GLOBAL_DATA_PTR; + /* Locals */ static GraphicDevice *pGD; /* Pointer to Graphic array */
@@ -377,6 +379,8 @@ static int console_row; /* cursor row */
static u32 eorx, fgx, bgx; /* color pats */
+static int cfb_do_flush_cache; + static const int video_font_draw_table8[] = { 0x00000000, 0x000000ff, 0x0000ff00, 0x0000ffff, 0x00ff0000, 0x00ff00ff, 0x00ffff00, 0x00ffffff, @@ -553,6 +557,8 @@ static void video_drawchars(int xx, int yy, unsigned char *s, int count) SWAP32((video_font_draw_table32 [bits & 15][3] & eorx) ^ bgx); } + if (cfb_do_flush_cache) + flush_cache((ulong)dest0, 32); dest0 += VIDEO_FONT_WIDTH * VIDEO_PIXEL_SIZE; s++; } @@ -621,6 +627,8 @@ static void video_invertchar(int xx, int yy) for (x = firstx; x < lastx; x++) { u8 *dest = (u8 *)(video_fb_address) + x + y; *dest = ~*dest; + if (cfb_do_flush_cache) + flush_cache((ulong)dest, 4); } } } @@ -717,6 +725,8 @@ static void console_scrollup(void) #else memsetl(CONSOLE_ROW_LAST, CONSOLE_ROW_SIZE >> 2, CONSOLE_BG_COL); #endif + if (cfb_do_flush_cache) + flush_cache((ulong)CONSOLE_ROW_FIRST, CONSOLE_SIZE); }
static void console_back(void) @@ -1651,6 +1661,29 @@ static void *video_logo(void) } #endif
+static int cfb_fb_is_in_dram(void) +{ + bd_t *bd = gd->bd; + ulong start, end; + int i; + + for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) { +#if defined(CONFIG_ARM) || defined(CONFIG_AVR32) || defined(COFNIG_NDS32) || \ +defined(CONFIG_SANDBOX) || defined(CONFIG_X86) + start = bd->bi_dram[i].start; + end = bd->bi_dram[i].start + bd->bi_dram[i].size - 1; +#else + start = bd->bi_memstart; + end = bd->bi_memsize; +#endif + + if ((ulong)video_fb_address >= start && + (ulong)video_fb_address < end) + return 1; + } + return 0; +} + static int video_init(void) { unsigned char color8; @@ -1664,6 +1697,8 @@ static int video_init(void) video_init_hw_cursor(VIDEO_FONT_WIDTH, VIDEO_FONT_HEIGHT); #endif
+ cfb_do_flush_cache = cfb_fb_is_in_dram() && dcache_status(); + /* Init drawing pats */ switch (VIDEO_DATA_FORMAT) { case GDF__8BIT_INDEX:

On Saturday 28 April 2012 17:04:07 you wrote:
Data cache flushing is required for frame buffer in RAM to fix the distorted console text output. Currently this text distortion is observed with cfb on beageboard and N900 when running with data cache enabled.
Reported-by: Pali Rohár pali.rohar@gmail.com Signed-off-by: Anatolij Gustschin agust@denx.de
Tested-by: Pali Rohár pali.rohar@gmail.com

On Saturday 28 April 2012 11:04:07 Anatolij Gustschin wrote:
+static int cfb_fb_is_in_dram(void) +{
- bd_t *bd = gd->bd;
- ulong start, end;
- int i;
- for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) {
+#if defined(CONFIG_ARM) || defined(CONFIG_AVR32) || defined(COFNIG_NDS32) || \ +defined(CONFIG_SANDBOX) || defined(CONFIG_X86)
start = bd->bi_dram[i].start;
end = bd->bi_dram[i].start + bd->bi_dram[i].size - 1;
+#else
start = bd->bi_memstart;
end = bd->bi_memsize;
+#endif
if ((ulong)video_fb_address >= start &&
(ulong)video_fb_address < end)
return 1;
- }
- return 0;
+}
is this necessary ? the cache funcs should take care of this automatically. -mike

Hi Mike,
On Sat, 28 Apr 2012 14:16:39 -0400 Mike Frysinger vapier@gentoo.org wrote:
On Saturday 28 April 2012 11:04:07 Anatolij Gustschin wrote:
+static int cfb_fb_is_in_dram(void) +{
- bd_t *bd = gd->bd;
- ulong start, end;
- int i;
- for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) {
+#if defined(CONFIG_ARM) || defined(CONFIG_AVR32) || defined(COFNIG_NDS32) || \ +defined(CONFIG_SANDBOX) || defined(CONFIG_X86)
start = bd->bi_dram[i].start;
end = bd->bi_dram[i].start + bd->bi_dram[i].size - 1;
+#else
start = bd->bi_memstart;
end = bd->bi_memsize;
+#endif
if ((ulong)video_fb_address >= start &&
(ulong)video_fb_address < end)
return 1;
- }
- return 0;
+}
is this necessary ? the cache funcs should take care of this automatically.
Currently they don't, or at least on some architectures. Or did you mean that the cache instructions should take care of this?
Thanks, Anatolij

On Monday 30 April 2012 11:32:21 Anatolij Gustschin wrote:
On Sat, 28 Apr 2012 14:16:39 -0400 Mike Frysinger wrote:
On Saturday 28 April 2012 11:04:07 Anatolij Gustschin wrote:
+static int cfb_fb_is_in_dram(void) +{
- bd_t *bd = gd->bd;
- ulong start, end;
- int i;
- for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) {
+#if defined(CONFIG_ARM) || defined(CONFIG_AVR32) || defined(COFNIG_NDS32)
|| \ +defined(CONFIG_SANDBOX) || defined(CONFIG_X86)
start = bd->bi_dram[i].start;
end = bd->bi_dram[i].start + bd->bi_dram[i].size - 1;
+#else
start = bd->bi_memstart;
end = bd->bi_memsize;
+#endif
if ((ulong)video_fb_address >= start &&
(ulong)video_fb_address < end)
return 1;
- }
- return 0;
+}
is this necessary ? the cache funcs should take care of this automatically.
Currently they don't, or at least on some architectures. Or did you mean that the cache instructions should take care of this?
imo, cache flush functions should be safe to call on any memory address (where there is data that could be dma-ed from). this is how Linux works.
otherwise, this function ends up getting duplicated in many places and i can't possibly see how that's an improvement. -mike

Dear Anatolij Gustschin,
Data cache flushing is required for frame buffer in RAM to fix the distorted console text output. Currently this text distortion is observed with cfb on beageboard and N900 when running with data cache enabled.
beagleboard ;-)
Reported-by: Pali Rohár pali.rohar@gmail.com Signed-off-by: Anatolij Gustschin agust@denx.de
drivers/video/cfb_console.c | 35 +++++++++++++++++++++++++++++++++++ 1 files changed, 35 insertions(+), 0 deletions(-)
diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c index 904caf7..dd7ccb7 100644 --- a/drivers/video/cfb_console.c +++ b/drivers/video/cfb_console.c @@ -360,6 +360,8 @@ void console_cursor(int state); extern void video_get_info_str(int line_number, char *info); #endif
+DECLARE_GLOBAL_DATA_PTR;
/* Locals */ static GraphicDevice *pGD; /* Pointer to Graphic array */
@@ -377,6 +379,8 @@ static int console_row; /* cursor row */
static u32 eorx, fgx, bgx; /* color pats */
+static int cfb_do_flush_cache;
static const int video_font_draw_table8[] = { 0x00000000, 0x000000ff, 0x0000ff00, 0x0000ffff, 0x00ff0000, 0x00ff00ff, 0x00ffff00, 0x00ffffff, @@ -553,6 +557,8 @@ static void video_drawchars(int xx, int yy, unsigned char *s, int count) SWAP32((video_font_draw_table32 [bits & 15][3] & eorx) ^ bgx); }
if (cfb_do_flush_cache)
flush_cache((ulong)dest0, 32);
flush_dcache_range() ?
dest0 += VIDEO_FONT_WIDTH * VIDEO_PIXEL_SIZE; s++; }
@@ -621,6 +627,8 @@ static void video_invertchar(int xx, int yy) for (x = firstx; x < lastx; x++) { u8 *dest = (u8 *)(video_fb_address) + x + y; *dest = ~*dest;
if (cfb_do_flush_cache)
flush_cache((ulong)dest, 4);
DTTO
}
} } @@ -717,6 +725,8 @@ static void console_scrollup(void) #else memsetl(CONSOLE_ROW_LAST, CONSOLE_ROW_SIZE >> 2, CONSOLE_BG_COL); #endif
- if (cfb_do_flush_cache)
flush_cache((ulong)CONSOLE_ROW_FIRST, CONSOLE_SIZE);
}
static void console_back(void) @@ -1651,6 +1661,29 @@ static void *video_logo(void) } #endif
+static int cfb_fb_is_in_dram(void) +{
- bd_t *bd = gd->bd;
- ulong start, end;
- int i;
- for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) {
+#if defined(CONFIG_ARM) || defined(CONFIG_AVR32) || defined(COFNIG_NDS32) || \ +defined(CONFIG_SANDBOX) || defined(CONFIG_X86)
start = bd->bi_dram[i].start;
end = bd->bi_dram[i].start + bd->bi_dram[i].size - 1;
+#else
start = bd->bi_memstart;
end = bd->bi_memsize;
+#endif
if ((ulong)video_fb_address >= start &&
(ulong)video_fb_address < end)
return 1;
- }
- return 0;
+}
Can't you have SRAM cached too? ;-)
static int video_init(void) { unsigned char color8; @@ -1664,6 +1697,8 @@ static int video_init(void) video_init_hw_cursor(VIDEO_FONT_WIDTH, VIDEO_FONT_HEIGHT); #endif
- cfb_do_flush_cache = cfb_fb_is_in_dram() && dcache_status();
- /* Init drawing pats */ switch (VIDEO_DATA_FORMAT) { case GDF__8BIT_INDEX:

Hi,
On Sun, Apr 29, 2012 at 7:25 PM, Marek Vasut marex@denx.de wrote:
Dear Anatolij Gustschin,
Data cache flushing is required for frame buffer in RAM to fix the distorted console text output. Currently this text distortion is observed with cfb on beageboard and N900 when running with data cache enabled.
beagleboard ;-)
Regarding this patch, can I suggest also looking at this possibly more generic patch that I originally did for Tegra? The series is here:
http://patchwork.ozlabs.org/user/bundle/2869/
and in particular these patches from the series:
arm: Add control over cachability of memory regions lcd: Add CONFIG_ALIGN_LCD_TO_SECTION to align lcd for MMU lcd: Add support for flushing LCD fb from dcache after update tegra: Align LCD frame buffer to section boundary tegra: Support control of cache settings for LCD tegra: fdt: Add LCD definitions for Seaboard lcd: Add CONSOLE_SCROLL_LINES option to speed console
Regards, Simon
Reported-by: Pali Rohár pali.rohar@gmail.com Signed-off-by: Anatolij Gustschin agust@denx.de
drivers/video/cfb_console.c | 35 +++++++++++++++++++++++++++++++++++ 1 files changed, 35 insertions(+), 0 deletions(-)
diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c index 904caf7..dd7ccb7 100644 --- a/drivers/video/cfb_console.c +++ b/drivers/video/cfb_console.c @@ -360,6 +360,8 @@ void console_cursor(int state); extern void video_get_info_str(int line_number, char *info); #endif
+DECLARE_GLOBAL_DATA_PTR;
/* Locals */ static GraphicDevice *pGD; /* Pointer to Graphic array */
@@ -377,6 +379,8 @@ static int console_row; /* cursor row */
static u32 eorx, fgx, bgx; /* color pats */
+static int cfb_do_flush_cache;
static const int video_font_draw_table8[] = { 0x00000000, 0x000000ff, 0x0000ff00, 0x0000ffff, 0x00ff0000, 0x00ff00ff, 0x00ffff00, 0x00ffffff, @@ -553,6 +557,8 @@ static void video_drawchars(int xx, int yy, unsigned char *s, int count) SWAP32((video_font_draw_table32 [bits & 15][3] & eorx) ^
bgx);
}
if (cfb_do_flush_cache)
flush_cache((ulong)dest0, 32);
flush_dcache_range() ?
dest0 += VIDEO_FONT_WIDTH * VIDEO_PIXEL_SIZE; s++; }
@@ -621,6 +627,8 @@ static void video_invertchar(int xx, int yy) for (x = firstx; x < lastx; x++) { u8 *dest = (u8 *)(video_fb_address) + x + y; *dest = ~*dest;
if (cfb_do_flush_cache)
flush_cache((ulong)dest, 4);
DTTO
} }
} @@ -717,6 +725,8 @@ static void console_scrollup(void) #else memsetl(CONSOLE_ROW_LAST, CONSOLE_ROW_SIZE >> 2, CONSOLE_BG_COL); #endif
if (cfb_do_flush_cache)
flush_cache((ulong)CONSOLE_ROW_FIRST, CONSOLE_SIZE);
}
static void console_back(void) @@ -1651,6 +1661,29 @@ static void *video_logo(void) } #endif
+static int cfb_fb_is_in_dram(void) +{
bd_t *bd = gd->bd;
ulong start, end;
int i;
for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) {
+#if defined(CONFIG_ARM) || defined(CONFIG_AVR32) ||
defined(COFNIG_NDS32)
|| \ +defined(CONFIG_SANDBOX) || defined(CONFIG_X86)
start = bd->bi_dram[i].start;
end = bd->bi_dram[i].start + bd->bi_dram[i].size - 1;
+#else
start = bd->bi_memstart;
end = bd->bi_memsize;
+#endif
if ((ulong)video_fb_address >= start &&
(ulong)video_fb_address < end)
return 1;
}
return 0;
+}
Can't you have SRAM cached too? ;-)
static int video_init(void) { unsigned char color8; @@ -1664,6 +1697,8 @@ static int video_init(void) video_init_hw_cursor(VIDEO_FONT_WIDTH, VIDEO_FONT_HEIGHT); #endif
cfb_do_flush_cache = cfb_fb_is_in_dram() && dcache_status();
/* Init drawing pats */ switch (VIDEO_DATA_FORMAT) { case GDF__8BIT_INDEX:
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Hi Simon,
On Sun, 29 Apr 2012 22:56:01 -0700 Simon Glass sjg@chromium.org wrote: ...
Regarding this patch, can I suggest also looking at this possibly more generic patch that I originally did for Tegra? The series is here:
http://patchwork.ozlabs.org/user/bundle/2869/
and in particular these patches from the series:
arm: Add control over cachability of memory regions lcd: Add CONFIG_ALIGN_LCD_TO_SECTION to align lcd for MMU lcd: Add support for flushing LCD fb from dcache after update tegra: Align LCD frame buffer to section boundary tegra: Support control of cache settings for LCD tegra: fdt: Add LCD definitions for Seaboard lcd: Add CONSOLE_SCROLL_LINES option to speed console
Thanks for the reminder, I completely forgot about these LCD patches. I'll look at them soon.
Thanks, Anatolij

Hi,
On Mon, 30 Apr 2012 04:25:50 +0200 Marek Vasut marex@denx.de wrote: ...
observed with cfb on beageboard and N900 when running with data cache enabled.
beagleboard ;-)
Thanks for catching that!!
...
@@ -553,6 +557,8 @@ static void video_drawchars(int xx, int yy, unsigned char *s, int count) SWAP32((video_font_draw_table32 [bits & 15][3] & eorx) ^ bgx); }
if (cfb_do_flush_cache)
flush_cache((ulong)dest0, 32);
flush_dcache_range() ?
I would have to calculate the end address, then. flush_cache() already does it for me :-)
...
@@ -1651,6 +1661,29 @@ static void *video_logo(void) } #endif
+static int cfb_fb_is_in_dram(void) +{
- bd_t *bd = gd->bd;
- ulong start, end;
- int i;
- for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) {
+#if defined(CONFIG_ARM) || defined(CONFIG_AVR32) || defined(COFNIG_NDS32) || \ +defined(CONFIG_SANDBOX) || defined(CONFIG_X86)
start = bd->bi_dram[i].start;
end = bd->bi_dram[i].start + bd->bi_dram[i].size - 1;
+#else
start = bd->bi_memstart;
end = bd->bi_memsize;
+#endif
if ((ulong)video_fb_address >= start &&
(ulong)video_fb_address < end)
return 1;
- }
- return 0;
+}
Can't you have SRAM cached too? ;-)
I do not know. But who will put the framebuffer into SRAM? It is not big enough.
Thanks, Anatolij

Dear Anatolij Gustschin,
Hi,
On Mon, 30 Apr 2012 04:25:50 +0200 Marek Vasut marex@denx.de wrote: ...
observed with cfb on beageboard and N900 when running with data cache enabled.
beagleboard ;-)
Thanks for catching that!!
...
@@ -553,6 +557,8 @@ static void video_drawchars(int xx, int yy, unsigned char *s, int count) SWAP32((video_font_draw_table32
[bits & 15][3] & eorx) ^ bgx); }
if (cfb_do_flush_cache)
flush_cache((ulong)dest0, 32);
flush_dcache_range() ?
I would have to calculate the end address, then. flush_cache() already does it for me :-)
Well ... that's correct. Maybe someone should rename it to flush_dcache_size() or something ...
...
@@ -1651,6 +1661,29 @@ static void *video_logo(void)
} #endif
+static int cfb_fb_is_in_dram(void) +{
- bd_t *bd = gd->bd;
- ulong start, end;
- int i;
- for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) {
+#if defined(CONFIG_ARM) || defined(CONFIG_AVR32) || defined(COFNIG_NDS32)
|| \ +defined(CONFIG_SANDBOX) || defined(CONFIG_X86)
start = bd->bi_dram[i].start;
end = bd->bi_dram[i].start + bd->bi_dram[i].size - 1;
+#else
start = bd->bi_memstart;
end = bd->bi_memsize;
+#endif
if ((ulong)video_fb_address >= start &&
(ulong)video_fb_address < end)
return 1;
- }
- return 0;
+}
Can't you have SRAM cached too? ;-)
I do not know. But who will put the framebuffer into SRAM? It is not big enough.
Someone who has small LCD and wants to save dram bandwidth (oh, this sentence sounds stupid on its own). But maybe if you want to use LCD in SPL?
Thanks, Anatolij
Best regards, Marek Vasut

Hi,
On Mon, 30 Apr 2012 17:21:51 +0200 Marek Vasut marex@denx.de wrote: ...
Can't you have SRAM cached too? ;-)
I do not know. But who will put the framebuffer into SRAM? It is not big enough.
Someone who has small LCD and wants to save dram bandwidth (oh, this sentence sounds stupid on its own). But maybe if you want to use LCD in SPL?
If someone really needs it, he can always add it later :-)
Thanks, Anatolij

Dear Anatolij Gustschin,
Hi,
On Mon, 30 Apr 2012 17:21:51 +0200 Marek Vasut marex@denx.de wrote: ...
Can't you have SRAM cached too? ;-)
I do not know. But who will put the framebuffer into SRAM? It is not big enough.
Someone who has small LCD and wants to save dram bandwidth (oh, this sentence sounds stupid on its own). But maybe if you want to use LCD in SPL?
If someone really needs it, he can always add it later :-)
I won't argue further :-)
Thanks, Anatolij
Best regards, Marek Vasut

On Sat, 28 Apr 2012 17:04:07 +0200 Anatolij Gustschin agust@denx.de wrote:
Data cache flushing is required for frame buffer in RAM to fix the distorted console text output. Currently this text distortion is observed with cfb on beageboard and N900 when running with data cache enabled.
Reported-by: Pali Rohár pali.rohar@gmail.com Signed-off-by: Anatolij Gustschin agust@denx.de
drivers/video/cfb_console.c | 35 +++++++++++++++++++++++++++++++++++ 1 files changed, 35 insertions(+), 0 deletions(-)
Applied to u-boot-video/master after rebasing and fixing commit log.
Anatolij

On Tuesday 05 June 2012 03:28:40 Anatolij Gustschin wrote:
On Sat, 28 Apr 2012 17:04:07 +0200 Anatolij Gustschin wrote:
Data cache flushing is required for frame buffer in RAM to fix the distorted console text output. Currently this text distortion is observed with cfb on beageboard and N900 when running with data cache enabled.
Applied to u-boot-video/master after rebasing and fixing commit log.
this patch is still wrong for the reasons i cited earlier. your cache functions are broken, not the driver. -mike

Dear Mike Frysinger,
On Tuesday 05 June 2012 03:28:40 Anatolij Gustschin wrote:
On Sat, 28 Apr 2012 17:04:07 +0200 Anatolij Gustschin wrote:
Data cache flushing is required for frame buffer in RAM to fix the distorted console text output. Currently this text distortion is observed with cfb on beageboard and N900 when running with data cache enabled.
Applied to u-boot-video/master after rebasing and fixing commit log.
this patch is still wrong for the reasons i cited earlier. your cache functions are broken, not the driver.
The cache stuff is completely borked on ARM, I have no idea why people enable it.
Best regards, Marek Vasut

On Thursday 19 July 2012 11:49:20 Marek Vasut wrote:
Dear Mike Frysinger,
On Tuesday 05 June 2012 03:28:40 Anatolij Gustschin wrote:
On Sat, 28 Apr 2012 17:04:07 +0200 Anatolij Gustschin wrote:
Data cache flushing is required for frame buffer in RAM to fix the distorted console text output. Currently this text distortion is observed with cfb on beageboard and N900 when running with data cache enabled.
Applied to u-boot-video/master after rebasing and fixing commit log.
this patch is still wrong for the reasons i cited earlier. your cache functions are broken, not the driver.
The cache stuff is completely borked on ARM, I have no idea why people enable it.
in this case, it's making it worse for other arches, and setting bad precedents in the process
i think Simon's solution is a good middle ground: lcd: Add support for flushing LCD fb from dcache after update -mike
participants (6)
-
Anatolij Gustschin
-
Marek Vasut
-
Marek Vasut
-
Mike Frysinger
-
Pali Rohár
-
Simon Glass