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

[Simon Glass]

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.

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.

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.

Regards,
Simon

>
> --
> Best regards,
>
> Lukasz Majewski
>
> Samsung Poland R&D Center | Linux Platform Group