Message ID | 1356246228-26732-3-git-send-email-nikita@compulab.co.il |
---|---|
State | Superseded |
Delegated to: | Tom Rini |
Headers | show |
On 12/23/2012 08:03 AM, Nikita Kiryanov wrote: > Currently there is no logical place to put the code that prepares the > splash image data. The splash image data should be ready in memory > before bmp_display() is called, and after the environment is ready > (since lcd.c looks for the splash image in an address specified by > the environment variable "splashimage"). > > Our window of opportunity in board_init_r() is therefore: between > env_relocate() and bmp_display(), and from the available options > only the lcd related functions in drv_lcd_init() seem appropriate > for such lcd oriented code. > > Add the option to prepare the splash image data in lcd_logo() right > before it is sent to be displayed. > > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > Signed-off-by: Igor Grinberg <grinberg@compulab.co.il> > --- > README | 8 ++++++++ > common/lcd.c | 15 +++++++++++++++ > include/lcd.h | 1 + > 3 files changed, 24 insertions(+) > > diff --git a/README b/README > index 78f40df..cffc016 100644 > --- a/README > +++ b/README > @@ -1541,6 +1541,14 @@ CBFS (Coreboot Filesystem) support > => vertically centered image > at x = dspWidth - bmpWidth - 9 > > + CONFIG_SPLASH_SCREEN_PREPARE > + > + If this option is set then the board_splash_screen_prepare() > + function, which must be defined in your code, is called as part > + of the splash screen display sequence. It gives the board an > + opportunity to prepare the splash image data before it is > + processed and sent to the frame buffer by U-Boot. > + > - Gzip compressed BMP image support: CONFIG_VIDEO_BMP_GZIP > > If this option is set, additionally to standard BMP > diff --git a/common/lcd.c b/common/lcd.c > index 66d4f94..129cf7e 100644 > --- a/common/lcd.c > +++ b/common/lcd.c > @@ -1034,6 +1034,18 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y) > } > #endif > > +#ifdef CONFIG_SPLASH_SCREEN_PREPARE > +static inline int splash_screen_prepare(void) > +{ > + return board_splash_screen_prepare(); > +} > +#else > +static inline int splash_screen_prepare(void) > +{ > + return 0; > +} > +#endif > + > static void *lcd_logo(void) > { > #ifdef CONFIG_SPLASH_SCREEN > @@ -1045,6 +1057,9 @@ static void *lcd_logo(void) > int x = 0, y = 0; > do_splash = 0; > > + if (splash_screen_prepare()) > + return (void *)lcd_base; > + > addr = simple_strtoul (s, NULL, 16); > #ifdef CONFIG_SPLASH_SCREEN_ALIGN > s = getenv("splashpos"); > diff --git a/include/lcd.h b/include/lcd.h > index c24164a..4ac4ddd 100644 > --- a/include/lcd.h > +++ b/include/lcd.h > @@ -47,6 +47,7 @@ extern struct vidinfo panel_info; > > extern void lcd_ctrl_init (void *lcdbase); > extern void lcd_enable (void); > +extern int board_splash_screen_prepare(void); > > /* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ > extern void lcd_setcolreg (ushort regno, Other boards seem to do this in lcd_enable. Perhaps that is also an option. Regards, Jeroen
Hi Jeroen, On 01/20/2013 10:34 PM, Jeroen Hofstee wrote: [...] >> diff --git a/include/lcd.h b/include/lcd.h >> index c24164a..4ac4ddd 100644 >> --- a/include/lcd.h >> +++ b/include/lcd.h >> @@ -47,6 +47,7 @@ extern struct vidinfo panel_info; >> extern void lcd_ctrl_init (void *lcdbase); >> extern void lcd_enable (void); >> +extern int board_splash_screen_prepare(void); >> /* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ >> extern void lcd_setcolreg (ushort regno, > Other boards seem to do this in lcd_enable. Perhaps that is also an option. The problem with doing it in lcd_enable is that it's akin to a side effect, given what the function's name advertises. Preparing the splash image can, however, be a natural part of the process that displays it. > > Regards, > Jeroen
Hello Nikita, On 01/21/2013 08:51 AM, Nikita Kiryanov wrote: > Hi Jeroen, > > On 01/20/2013 10:34 PM, Jeroen Hofstee wrote: > [...] >>> diff --git a/include/lcd.h b/include/lcd.h >>> index c24164a..4ac4ddd 100644 >>> --- a/include/lcd.h >>> +++ b/include/lcd.h >>> @@ -47,6 +47,7 @@ extern struct vidinfo panel_info; >>> extern void lcd_ctrl_init (void *lcdbase); >>> extern void lcd_enable (void); >>> +extern int board_splash_screen_prepare(void); >>> /* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ >>> extern void lcd_setcolreg (ushort regno, >> Other boards seem to do this in lcd_enable. Perhaps that is also an >> option. > > The problem with doing it in lcd_enable is that it's akin to a side > effect, given what the function's name advertises. Preparing the splash > image can, however, be a natural part of the process that displays it. > mmm, I am not so sure I agree that loading a bitmap in lcd_enable is a _problem_, while loading it in show logo and requiring a CONFIG_* is _natural_. But anyway, can't this at least be changed to a __weak function, so the CONFIG and ifdef stuff can be dropped? Regards, Jeroen
On 01/21/2013 09:14 PM, Jeroen Hofstee wrote: > Hello Nikita, > > On 01/21/2013 08:51 AM, Nikita Kiryanov wrote: >> Hi Jeroen, >> >> On 01/20/2013 10:34 PM, Jeroen Hofstee wrote: >> [...] >>>> diff --git a/include/lcd.h b/include/lcd.h >>>> index c24164a..4ac4ddd 100644 >>>> --- a/include/lcd.h >>>> +++ b/include/lcd.h >>>> @@ -47,6 +47,7 @@ extern struct vidinfo panel_info; >>>> extern void lcd_ctrl_init (void *lcdbase); >>>> extern void lcd_enable (void); >>>> +extern int board_splash_screen_prepare(void); >>>> /* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ >>>> extern void lcd_setcolreg (ushort regno, >>> Other boards seem to do this in lcd_enable. Perhaps that is also an >>> option. >> >> The problem with doing it in lcd_enable is that it's akin to a side >> effect, given what the function's name advertises. Preparing the splash >> image can, however, be a natural part of the process that displays it. >> > mmm, I am not so sure I agree that loading a bitmap in lcd_enable is > a _problem_, while loading it in show logo and requiring a CONFIG_* is > _natural_. Well, it is a problem. If we don't respect the abstractions we create then things like function names become meaningless. A function called "lcd_enable" should do just that- enable lcd. Not load stuff from storage to memory or manipulate BMPs. > > But anyway, can't this at least be changed to a __weak function, so the > CONFIG and ifdef stuff can be dropped? The motivation behind the CONFIG was to make it a documentable user setting, rather than an undocumented feature buried in the code. > > Regards, > Jeroen > >
Hello Nikita, On 01/23/2013 09:31 AM, Nikita Kiryanov wrote: > On 01/21/2013 09:14 PM, Jeroen Hofstee wrote: >> Hello Nikita, >> >> On 01/21/2013 08:51 AM, Nikita Kiryanov wrote: >>> Hi Jeroen, >>> >>> On 01/20/2013 10:34 PM, Jeroen Hofstee wrote: >>> [...] >>>>> diff --git a/include/lcd.h b/include/lcd.h >>>>> index c24164a..4ac4ddd 100644 >>>>> --- a/include/lcd.h >>>>> +++ b/include/lcd.h >>>>> @@ -47,6 +47,7 @@ extern struct vidinfo panel_info; >>>>> extern void lcd_ctrl_init (void *lcdbase); >>>>> extern void lcd_enable (void); >>>>> +extern int board_splash_screen_prepare(void); >>>>> /* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ >>>>> extern void lcd_setcolreg (ushort regno, >>>> Other boards seem to do this in lcd_enable. Perhaps that is also an >>>> option. >>> >>> The problem with doing it in lcd_enable is that it's akin to a side >>> effect, given what the function's name advertises. Preparing the splash >>> image can, however, be a natural part of the process that displays it. >>> >> mmm, I am not so sure I agree that loading a bitmap in lcd_enable is >> a _problem_, while loading it in show logo and requiring a CONFIG_* is >> _natural_. > > Well, it is a problem. If we don't respect the abstractions we create > then things like function names become meaningless. A function called > "lcd_enable" should do just that- enable lcd. Not load stuff from > storage to memory or manipulate BMPs. > my point is that lcd_clear will e.g. call lcd_logo. Although I haven't tested it, it seems you're make a side effect of a function only called once a side effect of another function (which could be called multiple times). So you make things even worse (loading an bitmap while the function is just named to display it). >> >> But anyway, can't this at least be changed to a __weak function, so the >> CONFIG and ifdef stuff can be dropped? > > The motivation behind the CONFIG was to make it a documentable user > setting, > rather than an undocumented feature buried in the code. > then document the callback... I don't see the improvement of this patch.. Regards, Jeroen
On 01/24/13 00:13, Jeroen Hofstee wrote: > Hello Nikita, > > On 01/23/2013 09:31 AM, Nikita Kiryanov wrote: >> On 01/21/2013 09:14 PM, Jeroen Hofstee wrote: >>> Hello Nikita, >>> >>> On 01/21/2013 08:51 AM, Nikita Kiryanov wrote: >>>> Hi Jeroen, >>>> >>>> On 01/20/2013 10:34 PM, Jeroen Hofstee wrote: >>>> [...] >>>>>> diff --git a/include/lcd.h b/include/lcd.h >>>>>> index c24164a..4ac4ddd 100644 >>>>>> --- a/include/lcd.h >>>>>> +++ b/include/lcd.h >>>>>> @@ -47,6 +47,7 @@ extern struct vidinfo panel_info; >>>>>> extern void lcd_ctrl_init (void *lcdbase); >>>>>> extern void lcd_enable (void); >>>>>> +extern int board_splash_screen_prepare(void); >>>>>> /* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ >>>>>> extern void lcd_setcolreg (ushort regno, >>>>> Other boards seem to do this in lcd_enable. Perhaps that is also an >>>>> option. >>>> >>>> The problem with doing it in lcd_enable is that it's akin to a side >>>> effect, given what the function's name advertises. Preparing the splash >>>> image can, however, be a natural part of the process that displays it. >>>> >>> mmm, I am not so sure I agree that loading a bitmap in lcd_enable is >>> a _problem_, while loading it in show logo and requiring a CONFIG_* is >>> _natural_. >> >> Well, it is a problem. If we don't respect the abstractions we create >> then things like function names become meaningless. A function called >> "lcd_enable" should do just that- enable lcd. Not load stuff from >> storage to memory or manipulate BMPs. >> > my point is that lcd_clear will e.g. call lcd_logo. Although I haven't tested it, > it seems you're make a side effect of a function only called once a side effect > of another function (which could be called multiple times). So you make things > even worse (loading an bitmap while the function is just named to display it). So what's your point? Do you think we should add a splash screen specific callback inside the board.c U-Boot boot flow? Please, be more specific, as both approaches are not suitable according to what was said above... > >>> >>> But anyway, can't this at least be changed to a __weak function, so the >>> CONFIG and ifdef stuff can be dropped? >> >> The motivation behind the CONFIG was to make it a documentable user setting, >> rather than an undocumented feature buried in the code. >> > then document the callback... Sorry, may be I've missed something, but I can't see any callback being documented in the README file... > > I don't see the improvement of this patch.. What does that suppose to mean? Either be constructive or don't bother... I'd like to hear Anatolij's opinion on this.
Hello Igor, On 01/24/2013 09:35 AM, Igor Grinberg wrote: > On 01/24/13 00:13, Jeroen Hofstee wrote: >> Hello Nikita, >> >> On 01/23/2013 09:31 AM, Nikita Kiryanov wrote: >>> On 01/21/2013 09:14 PM, Jeroen Hofstee wrote: >>>> >>>> mmm, I am not so sure I agree that loading a bitmap in lcd_enable is >>>> a _problem_, while loading it in show logo and requiring a CONFIG_* is >>>> _natural_. >>> Well, it is a problem. If we don't respect the abstractions we create >>> then things like function names become meaningless. A function called >>> "lcd_enable" should do just that- enable lcd. Not load stuff from >>> storage to memory or manipulate BMPs. >>> >> my point is that lcd_clear will e.g. call lcd_logo. Although I haven't tested it, >> it seems you're make a side effect of a function only called once a side effect >> of another function (which could be called multiple times). So you make things >> even worse (loading an bitmap while the function is just named to display it). > So what's your point? Do you think we should add a splash screen specific > callback inside the board.c U-Boot boot flow? no. > Please, be more specific, as both approaches are not suitable according > to what was said above... lets see, drv_lcd_init calls lcd_init. which does lcd_ctrl_init(lcdbase); lcd_is_enabled = 1; lcd_clear(); lcd_enable(); After this patch, lcd_clear calls lcd_logo which calls board_splash_screen_prepare in its turn. In my mind this still leaves allot of side effects. If you want to prepare for displaying and not have it as a side effect of a function which is named to do something else, it should be in between lcd_ctrl_init and lcd_clear in my mind. > >>>> But anyway, can't this at least be changed to a __weak function, so the >>>> CONFIG and ifdef stuff can be dropped? >>> The motivation behind the CONFIG was to make it a documentable user setting, >>> rather than an undocumented feature buried in the code. >>> >> then document the callback... > Sorry, may be I've missed something, but I can't see any callback being > documented in the README file... > >> I don't see the improvement of this patch.. > What does that suppose to mean? Either be constructive or don't bother... This means, as I hopefully explained a bit more clearly now, that the patch makes the loading of a bitmap a side effect of lcd_clear, while the intention was to make it a more natural call sequence. (which can simply be done by putting it somewhere else as mentioned above) btw, I think, loading the image in lcd_enable() won't even work since lcd_enable is actually before lcd_clear. Scanning some boards which load in lcd_enable, they seem to call bmp_display manually. So that makes this patch no longer optional, but is actually required and is an improvement.... > I'd like to hear Anatolij's opinion on this. > yes, me too. I like the __weak far more than requiring a CONFIG_to enable a callback. I cannot think of a reason why these __weak functions cannot be documented. So that's up to Anatolij. Regards, Jeroen
On 01/25/13 00:34, Jeroen Hofstee wrote: > Hello Igor, > > On 01/24/2013 09:35 AM, Igor Grinberg wrote: >> On 01/24/13 00:13, Jeroen Hofstee wrote: >>> Hello Nikita, >>> >>> On 01/23/2013 09:31 AM, Nikita Kiryanov wrote: >>>> On 01/21/2013 09:14 PM, Jeroen Hofstee wrote: >>>>> >>>>> mmm, I am not so sure I agree that loading a bitmap in lcd_enable is >>>>> a _problem_, while loading it in show logo and requiring a CONFIG_* is >>>>> _natural_. >>>> Well, it is a problem. If we don't respect the abstractions we create >>>> then things like function names become meaningless. A function called >>>> "lcd_enable" should do just that- enable lcd. Not load stuff from >>>> storage to memory or manipulate BMPs. >>>> >>> my point is that lcd_clear will e.g. call lcd_logo. Although I haven't tested it, >>> it seems you're make a side effect of a function only called once a side effect >>> of another function (which could be called multiple times). So you make things >>> even worse (loading an bitmap while the function is just named to display it). >> So what's your point? Do you think we should add a splash screen specific >> callback inside the board.c U-Boot boot flow? > no. >> Please, be more specific, as both approaches are not suitable according >> to what was said above... > > lets see, drv_lcd_init calls lcd_init. which does > > lcd_ctrl_init(lcdbase); > lcd_is_enabled = 1; > lcd_clear(); > lcd_enable(); > > After this patch, lcd_clear calls lcd_logo which calls > board_splash_screen_prepare in its turn. That said, lcd_clear() calls lcd_logo()... This is the real source of confusion here - the side effect, and not the fact that lcd_logo() calls the board_splash_screen_prepare()... Being that a problem not directly related to Nikita's patch set, I don't think we should delay it any further. > In my mind this > still leaves allot of side effects. If you want to prepare > for displaying and not have it as a side effect of a function > which is named to do something else, it should be in > between lcd_ctrl_init and lcd_clear in my mind. I think not, lcd_logo() fits perfectly for loading the splash screen. The fact that lcd_logo() is called from lcd_clear(), IMO, would be the one that should be dealt with if you want to remove the confusion ("the side effect"). But that is not related to Nikita's patch set. > >> >>>>> But anyway, can't this at least be changed to a __weak function, so the >>>>> CONFIG and ifdef stuff can be dropped? >>>> The motivation behind the CONFIG was to make it a documentable user setting, >>>> rather than an undocumented feature buried in the code. >>>> >>> then document the callback... >> Sorry, may be I've missed something, but I can't see any callback being >> documented in the README file... >> >>> I don't see the improvement of this patch.. >> What does that suppose to mean? Either be constructive or don't bother... > This means, as I hopefully explained a bit more clearly now, that > the patch makes the loading of a bitmap a side effect of lcd_clear, > while the intention was to make it a more natural call sequence. > (which can simply be done by putting it somewhere else as > mentioned above) As explained above, the patch only uses lcd_logo(), which is meant to display the splash screen, and add the loading functionality. The fact that the lcd_logo() is called from lcd_clear() is the one you should blame. And as already said, not related to this patch. > > btw, I think, loading the image in lcd_enable() won't even work > since lcd_enable is actually before lcd_clear. Scanning some > boards which load in lcd_enable, they seem to call bmp_display > manually. So that makes this patch no longer optional, but is > actually required and is an improvement.... Ok. So we agree that this can't be done in lcd_enable(). >> I'd like to hear Anatolij's opinion on this. >> > yes, me too. I like the __weak far more than requiring a CONFIG_to > enable a callback. I cannot think of a reason why these __weak > functions cannot be documented. So that's up to Anatolij. Usually, I also like the __weak approach, but this time the intention was to document the feature and hopefully stop the lcd_*() API abuse.
Hello Igor, On 01/25/2013 07:45 AM, Igor Grinberg wrote: > On 01/25/13 00:34, Jeroen Hofstee wrote: >> Hello Igor, >> >> On 01/24/2013 09:35 AM, Igor Grinberg wrote: >>> On 01/24/13 00:13, Jeroen Hofstee wrote: >>>> Hello Nikita, >>>> >>>> On 01/23/2013 09:31 AM, Nikita Kiryanov wrote: >>>>> On 01/21/2013 09:14 PM, Jeroen Hofstee wrote: >>>>>> mmm, I am not so sure I agree that loading a bitmap in lcd_enable is >>>>>> a _problem_, while loading it in show logo and requiring a CONFIG_* is >>>>>> _natural_. >>>>> Well, it is a problem. If we don't respect the abstractions we create >>>>> then things like function names become meaningless. A function called >>>>> "lcd_enable" should do just that- enable lcd. Not load stuff from >>>>> storage to memory or manipulate BMPs. >>>>> >>>> my point is that lcd_clear will e.g. call lcd_logo. Although I haven't tested it, >>>> it seems you're make a side effect of a function only called once a side effect >>>> of another function (which could be called multiple times). So you make things >>>> even worse (loading an bitmap while the function is just named to display it). >>> So what's your point? Do you think we should add a splash screen specific >>> callback inside the board.c U-Boot boot flow? >> no. >>> Please, be more specific, as both approaches are not suitable according >>> to what was said above... >> lets see, drv_lcd_init calls lcd_init. which does >> >> lcd_ctrl_init(lcdbase); >> lcd_is_enabled = 1; >> lcd_clear(); >> lcd_enable(); >> >> After this patch, lcd_clear calls lcd_logo which calls >> board_splash_screen_prepare in its turn. > That said, lcd_clear() calls lcd_logo()... > This is the real source of confusion here - the side effect, > and not the fact that lcd_logo() calls the board_splash_screen_prepare()... > Being that a problem not directly related to Nikita's patch set, I don't > think we should delay it any further. > >> In my mind this >> still leaves allot of side effects. If you want to prepare >> for displaying and not have it as a side effect of a function >> which is named to do something else, it should be in >> between lcd_ctrl_init and lcd_clear in my mind. > I think not, lcd_logo() fits perfectly for loading the splash screen. > The fact that lcd_logo() is called from lcd_clear(), IMO, > would be the one that should be dealt with if you want to remove the > confusion ("the side effect"). But that is not related to Nikita's > patch set. > Given that I now know that lcd_enable is not an alternative to this patch, but adding a callback is the only method to load a bitmap and have it shown, I understand now that this patch is not just a formal / cleanup change, but adds an actually missing feature. So I am fine with having it inside lcd_logo. >> btw, I think, loading the image in lcd_enable() won't even work >> since lcd_enable is actually before lcd_clear. Scanning some >> boards which load in lcd_enable, they seem to call bmp_display >> manually. So that makes this patch no longer optional, but is >> actually required and is an improvement.... > Ok. So we agree that this can't be done in lcd_enable(). > yes. >>> I'd like to hear Anatolij's opinion on this. >>> >> yes, me too. I like the __weak far more than requiring a CONFIG_to >> enable a callback. I cannot think of a reason why these __weak >> functions cannot be documented. So that's up to Anatolij. > Usually, I also like the __weak approach, but this time the intention was > to document the feature and hopefully stop the lcd_*() API abuse. > > Regards, Jeroen
diff --git a/README b/README index 78f40df..cffc016 100644 --- a/README +++ b/README @@ -1541,6 +1541,14 @@ CBFS (Coreboot Filesystem) support => vertically centered image at x = dspWidth - bmpWidth - 9 + CONFIG_SPLASH_SCREEN_PREPARE + + If this option is set then the board_splash_screen_prepare() + function, which must be defined in your code, is called as part + of the splash screen display sequence. It gives the board an + opportunity to prepare the splash image data before it is + processed and sent to the frame buffer by U-Boot. + - Gzip compressed BMP image support: CONFIG_VIDEO_BMP_GZIP If this option is set, additionally to standard BMP diff --git a/common/lcd.c b/common/lcd.c index 66d4f94..129cf7e 100644 --- a/common/lcd.c +++ b/common/lcd.c @@ -1034,6 +1034,18 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y) } #endif +#ifdef CONFIG_SPLASH_SCREEN_PREPARE +static inline int splash_screen_prepare(void) +{ + return board_splash_screen_prepare(); +} +#else +static inline int splash_screen_prepare(void) +{ + return 0; +} +#endif + static void *lcd_logo(void) { #ifdef CONFIG_SPLASH_SCREEN @@ -1045,6 +1057,9 @@ static void *lcd_logo(void) int x = 0, y = 0; do_splash = 0; + if (splash_screen_prepare()) + return (void *)lcd_base; + addr = simple_strtoul (s, NULL, 16); #ifdef CONFIG_SPLASH_SCREEN_ALIGN s = getenv("splashpos"); diff --git a/include/lcd.h b/include/lcd.h index c24164a..4ac4ddd 100644 --- a/include/lcd.h +++ b/include/lcd.h @@ -47,6 +47,7 @@ extern struct vidinfo panel_info; extern void lcd_ctrl_init (void *lcdbase); extern void lcd_enable (void); +extern int board_splash_screen_prepare(void); /* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ extern void lcd_setcolreg (ushort regno,