FS#10130 - Concurrent backlight function calls from backlight timeout settings menu
Attached to Project:
Rockbox
Opened by Boris Gjenero (dreamlayers) - Tuesday, 14 April 2009, 03:37 GMT
Last edited by Boris Gjenero (dreamlayers) - Friday, 01 May 2009, 04:22 GMT
Opened by Boris Gjenero (dreamlayers) - Tuesday, 14 April 2009, 03:37 GMT
Last edited by Boris Gjenero (dreamlayers) - Friday, 01 May 2009, 04:22 GMT
|
DetailsWhen changing the currently used backlight timeout setting, backlight_update_state() is called from outside backlight_thread(). That function call may lead to target specific backlight on/off and LCD enable/sleep/awake functions. If these functions allow other threads to run (eg. via sleep() or yield()), another concurrent call may happen from backlight_thread().
To see the problem on the 5G iPod, turn the backlight off by scrolling to "Off" on the currently used backlight timeout setting menu, wait for the backlight to fade out, and then turn on the backlight by scrolling down. Over half the time, brightness will be incorrect. For example if brightness is set to 5, the backlight may turn on at brightness 1. This is because of the sleep in _backlight_hw_enable() in backlight-nano_video.c. Other targets have sleep calls in code that runs for lcd_enable(true), and executing multiple copies of that concurrently can't be good. I discovered this while working on 1) Use mutexes or other means to prevent problems in target-specific code 2) Ensure all calls of target-specific code happen from backlight_thread() I prefer 2) because it is less prone to bugs. (Behaviour confirmed with r20696) |
This task depends upon
Closed by Boris Gjenero (dreamlayers)
Friday, 01 May 2009, 04:22 GMT
Reason for closing: Fixed
Additional comments about closing: Fixed in r20778 and r20834.
Friday, 01 May 2009, 04:22 GMT
Reason for closing: Fixed
Additional comments about closing: Fixed in r20778 and r20834.
I don't know if this is related to that I noticed yesterday. I'm not sure from when it's happening (must be since last week), but if the LCD sleep is set to Always or to a number of seconds that leads to the LCD being OFF when the song changes, although the "Caption Backlight" is ON, the LCD won't wake up. If I set the LCD sleep to Never the caption backlight is working as expected.
Anyway, thanks for your great job.
Sadur.
FS#8523("Disable WPS updating when the backlight is off."), which was committed in r20666.I actually noticed weirdness on my e200 too (entirely white screen), when I use the PWM code path for sleeping (which is posting to the queue, instead of calling lcd_sleep directly in backlight_lcd_start_countdown()). This doesn't happen in SVN, but certainly shows that there's something wrong too.
Putting disable_irq() into lcd_sleep seems to help, but I'm still wondering why this. The lcd_sleep of the e200 has a HZ/50 sleep, which means a) that it yields and b) that it runs for more than a tick. That might be related.
FS#10129in that case. The simple solution is attached. Is it good enough? It might be nice to remove backlight_lcd_sleep_countdown(true) calls from PWM fading _backlight_off() code, but I can't think of any nice ways to do that at the moment.I find it confusing that _backlight_on/_off() is used to start fading for PWM, but not for software fading (there, backlight_setup_fade_up/_down() does it) so I'm sorry that I overlooked that.
>> It might be nice to remove backlight_lcd_sleep_countdown(true) calls from PWM fading _backlight_off() code, but I can't think of any nice ways to do that at the moment.
Didn't I do that?
Edit: I guess it's done when BACKLIGHT_FADE_FINISH is posted, so it should probably be inserted into the switch cases for BACKLIGHT_FADE_FINISH, or not?
Yes, that is confusing. PWM fading is a layer in between the platform-independent backlight code which is used on all targets which have a backlight and the target-specific backlight code. That layer offers the same _backlight_on() / _backlight_off() interface that target-specific code offers on targets without fading. It uses different target-specific functions however.
>> It might be nice to remove backlight_lcd_sleep_countdown(true) calls from PWM fading _backlight_off() code, but I can't think of any nice ways to do that at the moment.
>Didn't I do that?
Oh yeah, you removed the call from _backlight_off(). Thanks for pointing that out. It would be a problem if lcd_sleep_after_pwm_fade_when_backlight_always_off.patch was committed.
The backlight_lcd_sleep_countdown(true) in _backlight_off() was used when Rockbox is compiled with PWM fading support and backlight fade-out is disabled.
The backlight_lcd_sleep_countdown(true) in backlight_isr() is used when PWM fading support is compiled in, backlight fade-out is enabled, and the fade-out completes.
The backlight_lcd_sleep_countdown(true) in backlight_switch() is used if timer_register() fails or something else steals the timer interrupt used by PWM fading before the fade-out completes.
The latter two are not in _backlight_off() but they get executed due to a chain of events which start with a call to _backlight_off().
The simplest solution is to add the backlight_lcd_sleep_countdown(true) which was in _backlight_off() before and commit lcd_sleep_after_pwm_fade_when_backlight_always_off.patch. But, that's inconsistent; only that one _backlight_off() calls backlight_lcd_sleep_countdown(true). Maybe the functions which start PWM fading could simply be renamed, maybe to backlight_pwm_fade_on() and backlight_pwm_fade_off().
Oh, and _backlight_off() does not return before fading is finished? If yes, how is the current code not working? If not, how would your proposed changes help at all?
In my previous message, I explained the different ways that the backlight can be turned off. There currently isn't any one common point in backlight.c where all alternatives end. BACKLIGHT_FADE_FINISH is only used when fading is enabled and the fade finishes. BACKLIGHT_FADE_FINISH could be used as a common point, though it would be a bit of an inefficiency and one needs to be careful with it.
> Oh, and _backlight_off() does not return before fading is finished? If yes, how is the current code not working? If not, how would your proposed changes help at all?
When fading is disabled, _backlight_off() calls target-specific code to turn off the backlight.
When fading is enabled _backlight_off() merely sets up PWM fading and returns.
PWM fading _backlight_off() shouldn't behave differently from normal target-specific _backlight_off(). So yeah, I think PWM fading _backlight_off() and _backlight_on() need to be renamed to clarify this.
The problems in r20809 are:
1) When turning the backlight off via the currently used backlight timeout setting menu, with LCD fade-out enabled, the LCD goes to sleep at the start of the fade.
2) When no fading support is present or PWM fading is supported but turned off, the LCD won't go to sleep when after the backlight times out.
lcd_sleep_after_pwm_fade_when_backlight_always_off.patch would fix 1). To fix 2) with PWM fading, backlight_lcd_sleep_countdown(true) could be added to the branch of PWM fading _backlight_off() which is executed when PWM fading is disabled. When there is no fading support, backlight_lcd_sleep_countdown(true) ought to be in do_backlight_off().
>> lcd_sleep_after_pwm_fade_when_backlight_always_off.patch. But, that's inconsistent; only that one _backlight_off() calls backlight_lcd_sleep_countdown(true).
>> Maybe the functions which start PWM fading could simply be renamed, maybe to backlight_pwm_fade_on() and backlight_pwm_fade_off().
I did that now. Hopefully it works again.