[U-Boot] [PATCH RESEND] video:cache:fix: Buffer alignment and dcache flush for lcd subsystem

[Simon Glass]

Hi Lukasz,

On Sun, Jan 6, 2013 at 12:03 AM, Lukasz Majewski <l.majewski at majess.pl> wrote:
> Hi Simon,
>
>> Hi Lukasz,
>>
>> On Wed, Jan 2, 2013 at 8:25 AM, Lukasz Majewski
>> <l.majewski at samsung.com> wrote:
>> > This commit makes the video subsystem code cache aware.
>> > Memory allocated for decompressed BMP memory is now cache line
>> > aligned.
>> >
>> > Flushing of the dcache is also performed after copying BMP data to
>> > fb address (it is done for 32 BPP bitmap used on Trats board
>> > (Exynos4210)).
>> >
>>
>> Sorry if I have this wrong, just have a few comments.
>>
>> >
>> > Tested-by: Lukasz Majewski <l.majewski at samsung.com>
>> > Signed-off-by: Lukasz Majewski <l.majewski at samsung.com>
>> > Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
>> > Cc: Anatolij Gustschin <agust at denx.de>
>> > ---
>> >  common/cmd_bmp.c |    2 +-
>> >  common/lcd.c     |    3 +++
>> >  2 files changed, 4 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/common/cmd_bmp.c b/common/cmd_bmp.c
>> > index 5a52edd..57f3eb5 100644
>> > --- a/common/cmd_bmp.c
>> > +++ b/common/cmd_bmp.c
>> > @@ -55,7 +55,7 @@ bmp_image_t *gunzip_bmp(unsigned long addr,
>> > unsigned long *lenp)
>> >          * Decompress bmp image
>> >          */
>> >         len = CONFIG_SYS_VIDEO_LOGO_MAX_SIZE;
>> > -       dst = malloc(CONFIG_SYS_VIDEO_LOGO_MAX_SIZE);
>> > +       dst = memalign(CONFIG_SYS_CACHELINE_SIZE, len);
>>
>> Why do you need to align this one? It is just returned to the caller,
>> isn't it?
>
> Yes, it is returned to the caller, but afterwards lcd_display_bitmap
> takes this pointer (and thereof probably unaligned buffer) and works on
> it. (At least in the case of trats the bmp is gzipped).

OK, so it is directly used as a frame buffer? In that case it looks
right to me. I doubt you want to be able to control the cache features
for this area, since you only write it once. But if you did, then the
memory would currently need to be section-aligned.

>
>>
>> >         if (dst == NULL) {
>> >                 puts("Error: malloc in gunzip failed!\n");
>> >                 return NULL;
>> > diff --git a/common/lcd.c b/common/lcd.c
>> > index 4778655..784d1fb 100644
>> > --- a/common/lcd.c
>> > +++ b/common/lcd.c
>> > @@ -1023,6 +1023,9 @@ int lcd_display_bitmap(ulong bmp_image, int
>> > x, int y) }
>> >                         fb  -= (lcd_line_length + width * (bpix /
>> > 8)); }
>> > +               flush_dcache_range((unsigned long) fb,
>> > +                                  (unsigned long) fb +
>> > +                                  (lcd_line_length * height));
>>
>> I'm not sure this is needed - there is a call to lcd_sync() at the
>> bottom of this function now which should do the same thing,
>>
>> Please can you take a look at currently mainline and see if you need
>> to do anything more?
>
> Ok, I've checked. You are right, there is lcd_sync. Thanks for pointing.
>
> I will force trats board to use it, and prepare patches.

OK.

>
>>
>> >                 break;
>> >  #endif /* CONFIG_BMP_32BPP */
>> >         default:
>> > --
>> > 1.7.2.3
>> >
>>
>> Regards,
>> Simon
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>

Regards,
Simon