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

[Lukasz Majewski]

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. 

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



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group