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

[Lukasz Majewski <l.majewski@majess.pl>]

Hi Simon,

> Hi Lukasz,
> 
> On Wed, Jan 2, 2013 at 8:25 AM, Lukasz Majewski
> <l.majewski@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@samsung.com>
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > Cc: Anatolij Gustschin <agust@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).

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

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