Rockbox

Tasklist

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
Task Type Bugs
Category LCD
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

When 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  FS#9890  (5G iPod LCD sleep). There I used a mutex to avoid lockups due to this issue. I see two solutions:
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.
Comment by SadurnĂ­ (sadur) - Tuesday, 14 April 2009, 15:42 GMT
Hi.

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.
Comment by Boris Gjenero (dreamlayers) - Tuesday, 14 April 2009, 16:30 GMT
Sadur, that problem is entirely unrelated to this task. The problem described here can only happen when changing LCD settings. (I mean when you're actually in those settings menus and scrolling or selecting settings.) Based on tests on my 5G 30GB iPod, the "Caption Backlight" problem is due to  FS#8523  ("Disable WPS updating when the backlight is off."), which was committed in r20666.
Comment by Thomas Martitz (kugel.) - Thursday, 23 April 2009, 00:16 GMT
I'm strongly in favor of solution 2.
Comment by Thomas Martitz (kugel.) - Thursday, 23 April 2009, 00:55 GMT
This would be solution 2. It fixes some weirdness when switching from off to on and vice versa (as you said).
Comment by Thomas Martitz (kugel.) - Sunday, 26 April 2009, 19:26 GMT
I see you removed the work around.

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.
Comment by Boris Gjenero (dreamlayers) - Sunday, 26 April 2009, 19:45 GMT
r20793 introduced backlight_lcd_sleep_countdown(true) into the "timeout < 0" part of backlight_update_state(). That code is executed at the start of the fade when selecting "Off" in the current backlight timeout setting menu. So, if "LCD Sleep (After backlight off)" is set to "Always", the LCD will go to sleep at the start of the fade then, effectively bringing back  FS#10129  in 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.
Comment by Thomas Martitz (kugel.) - Sunday, 26 April 2009, 19:50 GMT
Well, it should be called when fading is done. I don't know where this exactly is. That's the reason it's at another place for software fading too.

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?
Comment by Boris Gjenero (dreamlayers) - Sunday, 26 April 2009, 20:17 GMT
> 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.

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().
Comment by Thomas Martitz (kugel.) - Sunday, 26 April 2009, 22:38 GMT
Can't there be a way to determine PWM backlight being turned off regardless of the setting? What's the deal with BACKLIGHT_FADE_FINISH?

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?
Comment by Boris Gjenero (dreamlayers) - Monday, 27 April 2009, 05:53 GMT
> Can't there be a way to determine PWM backlight being turned off regardless of the setting? What's the deal with BACKLIGHT_FADE_FINISH?

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().
Comment by Thomas Martitz (kugel.) - Monday, 27 April 2009, 19:05 GMT
I'd definitely welcome a simplification here.
Comment by Thomas Martitz (kugel.) - Tuesday, 28 April 2009, 08:58 GMT
>> 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().

I did that now. Hopefully it works again.
Comment by Boris Gjenero (dreamlayers) - Wednesday, 29 April 2009, 04:59 GMT
Thanks! The code seems correct. On my 5G 30GB iPod, LCD sleep after backlight off works both at the settings menu and normally, both with fading enabled and disabled.
Comment by Thomas Martitz (kugel.) - Wednesday, 29 April 2009, 06:53 GMT
Ah cool. So we can close this finally? :)
Comment by Boris Gjenero (dreamlayers) - Thursday, 30 April 2009, 03:35 GMT
What about the brightness setting functions? Currently, functions which work directly with the hardware are being called outside of the backlight thread. I don't see any weirdness, but I think they should be called from within the backlight thread. Here's a patch.
Comment by Thomas Martitz (kugel.) - Thursday, 30 April 2009, 08:54 GMT
Seems nice. Noticed no difference. Go for it :)

Loading...