753842 - SetScreenEnabled suspends the device preventing proximity events from firing

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Etienne Segonzac (:etienne)]

Encountered while implementing this:
https://github.com/andreasgal/gaia/pull/1374

Details:
*  The call is not ended, everything is fine on this side
*  We still get logs like

I/Sensors ( 1835): Proximity :: raw = 1  distance = 5.0
I/Sensors ( 1835): Proximity :: raw = 0  distance = 0.0

Comment 1

[Michael Wu [:mwu]]

A new nexus s kernel has landed which supposedly fixes this issue. Etienne, can you get a new build and check if it's fixed? Both the boot and system partitions will need to be flashed.

Comment 2

[Etienne Segonzac (:etienne)]

(In reply to Michael Wu [:mwu] from comment #1)
> A new nexus s kernel has landed which supposedly fixes this issue. Etienne,
> can you get a new build and check if it's fixed? Both the boot and system
> partitions will need to be flashed.

No luck. I flashed a Nexus S today and I have the exact same behavior.

Comment 3

[Andrew Overholt [:overholt]]

Are we seeing this with Otoro devices?

Comment 4

[Etienne Segonzac (:etienne)]

Yes, just tested.

Comment 5

[Etienne Segonzac (:etienne)]

FYI we have a Gaia workaround for this.

Comment 6

[Jason Smith [:jsmith]]

Could you clarify more details of the bug? What's the work-around? Is this worth blocking ship for v1?

Comment 7

[Etienne Segonzac (:etienne)]

When a userproximity event tell us that a face is near (during a call) we turn the screen off using setScreenEnabled.

But once the screen if off we don't get any userpoximity events. So we don't see that the user has remove the phone from his face, and we never turn the screen back on.

The ugly workaround is to display a black div on screen instead of turning it off.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Leaving the screen on is bad for battery.

Do we perhaps need to grab some sort of wake lock to keep getting sensor readings?  Or is the gonk widget backend disabling the notifications when the screen goes off, maybe?

Comment 9

[Doug Turner (:dougt)]

i can take a look soon if no one gets there before me.

Comment 10

[Doug Turner (:dougt)]

it looks like what we are doing is turning off the cpu and all sensor devices.  quickly looking, i didn't find any wake-lock available for sensors.  we might be able to add one?  or maybe there is something else we can do.

Comment 11

[Doug Turner (:dougt)]

cc'ing tzimmermann.  tzimmermann, any ideas?

Comment 12

[Doug Turner (:dougt)]

We are calling:
   http://mxr.mozilla.org/mozilla-central/source/hal/gonk/GonkHal.cpp#411

Comment 13

[Dave Hylands [:dhylands]]

wakelocks prevent the phone from suspending, so that's probably not what you want. Generally speaking, once the phone is suspended, only an interrupt can wake it up.

So you either need an interrupt from your sensor or you need to schedule a HW timer to go off, which will generate the required interrupt.

Comment 14

[Doug Turner (:dougt)]

I don't know if we want to phone to suspend during a phone call, or if we just want the screen to power off.

Comment 15

[Dave Hylands [:dhylands]]

In an ideal world, the phone should suspend whenever possible. I've seen some devices which suspend during the vertical blanking period on the LCD update.

The real secret is ensuring that the HW generates the appropriate interrupts to resume. There are really only 2 reasons not to suspend. One is that you don't have an appropriate interrupt to wake you up. The second is that the overhead with suspending/resuming introduces enough latency to impact normal behavior.

The notion of suspending is not related in any way to the appearance of the phone. It may be possible to suspend even while the screen is visible. And you can suspend pieces of the phone without suspending the whole phone. This is normally heavily kernel and kernel driver related and is mostly transparent to the user-mode software.

Comment 16

[Kan-Ru Chen [:kanru] (UTC+9)]

@Etienne

To make sure the phone is not suspended during the call, grab the "cpu" wake lock before call SetScreenEnabled. Note that the phone is never suspended while the usb is plugged-in, so you might see different result with or without the usb plugged-in.

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Doug Turner (:dougt) from comment #11)
> cc'ing tzimmermann.  tzimmermann, any ideas?

Only what dhylands said in comment 13.

But from the discussion, it's not clear to me if the proximity sensor is disabled because the screen has been turned off, or because the CPU has been suspended?

Comment 18

[Doug Turner (:dougt)]

It looks like all sensors are disabled when we turn off the screen. From reading the code, we call set_screen_state() which writes 'off' to /sys/power/state (see power.c).  This causes the cpu and sensors to be suspended.  There are no wake locks for just the sensors, so maybe what kanru's right... We should grap the general wake lock before calling set_screen_state(), then release it when we set the screen back on.

Comment 19

[Dave Hylands [:dhylands]]

You can just grab the wake_lock when you start the call and release it at the end, and the cpu won't suspend (this should be independant of whether the backlight is on or not).

The logic (about writing to /sys/power/state) seems incorrect to me.

When we turn the backlight on we should grab a wake_lock. When we turn the backlight off we should release the wake_lock. When we start a call we can grab a wake_lock. When we finish the call we release the wake_lock.

The kernel should suspend automatically when nobody is holding any wake_locks.

Comment 20

[Dave Hylands [:dhylands]]

The power management stuff on a phone should be setup such that every time the kernel goes idle it should try to suspend the cpu, unless a wake-lock is being held, in which case the idle process will spin and consume 100% of the CPU (which is what happens when the cpu isn't being suspended).

Interrupts from any source then take it out of suspend mode until the next time the kernel goes idle.

Comment 21

[Michael Wu [:mwu]]

(In reply to Dave Hylands [:dhylands] from comment #19)
> You can just grab the wake_lock when you start the call and release it at
> the end, and the cpu won't suspend (this should be independant of whether
> the backlight is on or not).
> 
> The logic (about writing to /sys/power/state) seems incorrect to me.
> 
> When we turn the backlight on we should grab a wake_lock. When we turn the
> backlight off we should release the wake_lock. When we start a call we can
> grab a wake_lock. When we finish the call we release the wake_lock.
> 
> The kernel should suspend automatically when nobody is holding any
> wake_locks.

AFAIK what we do is what Android does, and is implemented in hardware/libhardware_legacy. I agree that it is insane (what does the power state have to do with the screen being on?), but we do as Android does..

Comment 22

[Doug Turner (:dougt)]

dave, do you want to take this bug?

Comment 23

[Kan-Ru Chen [:kanru] (UTC+9)]

We found that on Otoro the light sensor is sharing the power source with the screen, maybe the proximity too?

Comment 24

[Marco Chen [:mchen]]

1. For comment 18, the string write to /sys/power/state for suspend should be 'mem' not 'off'.
   
2. For comment 20, even there is a wake lock here, when kernel detect idle state CPU didn't spin and consume full of cpu.
   Instead of that CPU will go into WFI (wait for interrupt) state. 
       a. In this state, all of irq are all enabled as usual.
       b. This state didn't go through device_suspend path so all driver should work normally.

3. If device really go into suspend state, then only power key can wake phone or other wake up source. It means that if you want to adjust the volume via volume key it is impossible. 

4. So Kan-Ru should be right. Acquire a wake lock for preventing device from going suspend state due to time out from power management on user space.

Comment 25

[Etienne Segonzac (:etienne)]

FYI I tried to request a cpu lock before turning off the screen and we still don't get anymore events after that.

Comment 26

[Marco Chen [:mchen]]

Hi Etienne,

Thanks for your follow up.
The issue you met now (Otoro) is the one on aspect of kernel driver.
And we already notice the Vendor to solve it.

But it would be workable on another reference phone without proximity issue on kernel driver.

Comment 27

[Marco Chen [:mchen]]

Update Status.

Kernel Bug: The proximity sensor didn't fire event after turning off the screen on Otoro device.

The Kernel Bug above is fixed by our ODM partner and would be integrated into next kernel version.

Comment 28

[Marco Chen [:mchen]]

Hi Etienne,

I got the fixed from ODM partner for proximity issue.
So maybe you can give me the version of grabbing wakelock then I can test it in advance.

Thanks.

Comment 29

[Etienne Segonzac (:etienne)]

(In reply to Marco Chen [:mchen] from comment #28)
> Hi Etienne,
> 
> I got the fixed from ODM partner for proximity issue.
> So maybe you can give me the version of grabbing wakelock then I can test it
> in advance.
> 
> Thanks.

You'll find it here:
https://github.com/etiennesegonzac/gaia/tree/proximity-screendisabled

We turn off the screen on proximity events while on a call, the interesting code is is |apps/communications/dialer/js/proximity.js|

Comment 30

[Marco Chen [:mchen]]

Hi Etienne,

1. I tested your application and verified that proximity sensor is working well.
and I only saw the effect on screen from light sensor but proximity.

2. The wakelock you grabbed is work because I can see proximity event fired after device got into early suspend.

Maybe you can wait for next kernel releasing then fix this issue.

Comment 31

[Etienne Segonzac (:etienne)]

Will do!

Comment 32

[Etienne Segonzac (:etienne)]

Still no luck, updated the branch on my gaia fork if you want to test.

Comment 33

[Marco Chen [:mchen]]

Attachment: Calibrated data for proximity sensor

Please put attached file into /data/misc/prox/ on your device via ADB. This is a calibration data for proximity sensor. Maybe you can check whether it is on your device or not.

Comment 34

[Etienne Segonzac (:etienne)]

Oh, forgot to mention an important information:
When using the hacky approach of putting a black div on top of everything on the screen instead of turning it off the proximity sensor is working smoothly.

Do I still need to do the calibrated data test?

Comment 35

[Michael Wu [:mwu]]

Etienne, have you been able to reproduce this issue on other devices? I thought you mentioned something about this but I don't remember..

Comment 36

[Etienne Segonzac (:etienne)]

(In reply to Michael Wu [:mwu] from comment #35)
> Etienne, have you been able to reproduce this issue on other devices? I
> thought you mentioned something about this but I don't remember..

Yes my first shot at this was on a nexus S.

Comment 37

[Marco Chen [:mchen]]

Michael Wu, I tested on m4 device. The proximity sensor can work well even device is on early suspend state.

Etienne, I used your branch on comment 29. But even proximity sensor is working I didn't see screen off. Does dialer app (content process?) has permission to access screenEnabled?

Comment 38

[Etienne Segonzac (:etienne)]

(In reply to Marco Chen [:mchen] from comment #37)
> Michael Wu, I tested on m4 device. The proximity sensor can work well even
> device is on early suspend state.
> 
> Etienne, I used your branch on comment 29. But even proximity sensor is
> working I didn't see screen off. Does dialer app (content process?) has
> permission to access screenEnabled?

STR
- open the dialer
- place a call
- while the call screen is displayed, obscure the proximity sensor

The screen will not turn on again.

Comment 39

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Etienne Segonzac (:etienne) from comment #38)
> (In reply to Marco Chen [:mchen] from comment #37)
> > Michael Wu, I tested on m4 device. The proximity sensor can work well even
> > device is on early suspend state.
> > 
> > Etienne, I used your branch on comment 29. But even proximity sensor is
> > working I didn't see screen off. Does dialer app (content process?) has
> > permission to access screenEnabled?
> 
> STR
> - open the dialer
> - place a call
> - while the call screen is displayed, obscure the proximity sensor
> 
> The screen will not turn on again.

Etienne, Dialer app is not assigned with mozPower permission right now. If it is able to turn off the screen, then we are having security bug.

In fact, I don't think Dialer app should get mozPower permission at all. If screen is turned on/off by apps other than System app, idle timer and screenchange events will not be fired in System app, which will be problematic.

Comment 40

[Marco Chen [:mchen]]

I just found the root cause of why proximity event doesn't send to Gaia app during early suspend state. And will file a bug and which will block this one.

During device is on early suspend state, all of the nsPIDOMWindow become "backgound".
And nsDeviceSensor.cpp didn't forward sensor event to the windows which are on backgound.
This is why app can't get proximity event during early suspend state, even proximity sensor really send out the event.

Comment 41

[Etienne Segonzac (:etienne)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #39)
> (In reply to Etienne Segonzac (:etienne) from comment #38)
> > (In reply to Marco Chen [:mchen] from comment #37)
> > > Michael Wu, I tested on m4 device. The proximity sensor can work well even
> > > device is on early suspend state.
> > > 
> > > Etienne, I used your branch on comment 29. But even proximity sensor is
> > > working I didn't see screen off. Does dialer app (content process?) has
> > > permission to access screenEnabled?
> > 
> > STR
> > - open the dialer
> > - place a call
> > - while the call screen is displayed, obscure the proximity sensor
> > 
> > The screen will not turn on again.
> 
> Etienne, Dialer app is not assigned with mozPower permission right now. If
> it is able to turn off the screen, then we are having security bug.

It is in the branch used for testing:
https://github.com/etiennesegonzac/gaia/blob/proximity-screendisabled/apps/communications/manifest.webapp

> 
> In fact, I don't think Dialer app should get mozPower permission at all. If
> screen is turned on/off by apps other than System app, idle timer and
> screenchange events will not be fired in System app, which will be
> problematic.

We should definitely consider those issues.
I'll make sure we talk this trough.

Comment 42

[Etienne Segonzac (:etienne)]

(In reply to Marco Chen [:mchen] from comment #40)
> I just found the root cause of why proximity event doesn't send to Gaia app
> during early suspend state. And will file a bug and which will block this
> one.
> 
> During device is on early suspend state, all of the nsPIDOMWindow become
> "backgound".
> And nsDeviceSensor.cpp didn't forward sensor event to the windows which are
> on backgound.
> This is why app can't get proximity event during early suspend state, even
> proximity sensor really send out the event.

\o/

Comment 43

[Marco Chen [:mchen]]

Attachment: WIPv1

System App - screen_manager.js:
  1. Add support to listen phone state then proximity event.
  2. Follow the proximity event to call screen off or on.

Communication - manifest.webapp:
  1. Add permission - "background-sensors".

I tested it on Otoro already.

Comment 44

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 678227 [details] [diff] [review]
WIPv1

Looks good besides some lint issue; run gjslint before asking for review :)

>+        var inCall = telephony.calls.length;

This line should be |var inCall == !!telephony.calls.length;|

Comment 45

[Marco Chen [:mchen]]

Attachment: Screen_Manager_WIPv2

1. Follow the suggestion.
2. Check by gjslint.

Comment 46

[Marco Chen [:mchen]]

Attachment: dialer_manifest_WIPv1

Add backgound-sensor permission into manifest.webapp, so dialer can listen proximity event even screen is off.

Comment 47

[Etienne Segonzac (:etienne)]

(In reply to Marco Chen [:mchen] from comment #46)
> Created attachment 678641 [details] [diff] [review]
> dialer_manifest_WIPv1
> 
> Add backgound-sensor permission into manifest.webapp, so dialer can listen
> proximity event even screen is off.

I don't understand.
Do we still need to listen to those events from the dialer is the system app is in charge of turning the screen off?

Comment 48

[Marco Chen [:mchen]]

(In reply to Etienne Segonzac (:etienne) from comment #47)
> (In reply to Marco Chen [:mchen] from comment #46)
> I don't understand.
> Do we still need to listen to those events from the dialer is the system app
> is in charge of turning the screen off?

Yes, the dialer app can choose to remove listening proximity and response on screen off.
But it need Etienne's help to remove this then check in the patch here.

Comment 49

[Marco Chen [:mchen]]

By the way, I just noticed that this bug is blocking-basecamp - .
Since this issue will effect the power consumption during phone call, maybe it can be blocking-basecamp +. The workaround now is just black the content area but the backlight is still on.

If anyone agreed this, I just modify the blocking-basecamp to ? again.

Comment 50

[Etienne Segonzac (:etienne)]

(In reply to Marco Chen [:mchen] from comment #49)
> By the way, I just noticed that this bug is blocking-basecamp - .
> Since this issue will effect the power consumption during phone call, maybe
> it can be blocking-basecamp +. The workaround now is just black the content
> area but the backlight is still on.
> 
> If anyone agreed this, I just modify the blocking-basecamp to ? again.

Yep it's fair to renom for basecamp.

Comment 51

[Etienne Segonzac (:etienne)]

Attachment: dialer_proximity_removal_WIP

Comment 52

[Etienne Segonzac (:etienne)]

(In reply to Marco Chen [:mchen] from comment #48)
> (In reply to Etienne Segonzac (:etienne) from comment #47)
> > (In reply to Marco Chen [:mchen] from comment #46)
> > I don't understand.
> > Do we still need to listen to those events from the dialer is the system app
> > is in charge of turning the screen off?
> 
> Yes, the dialer app can choose to remove listening proximity and response on
> screen off.
> But it need Etienne's help to remove this then check in the patch here.

Just attached a patch :)

Comment 53

[Marco Chen [:mchen]]

(In reply to Etienne Segonzac (:etienne) from comment #52)
> (In reply to Marco Chen [:mchen] from comment #48)
> > (In reply to Etienne Segonzac (:etienne) from comment #47)
> > > (In reply to Marco Chen [:mchen] from comment #46)
> Just attached a patch :)

It is great if you can join here to modify part of dialer app,
(Since you already done here. :))
so please help to request for review.

Thanks.

Comment 54

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 678640 [details] [diff] [review]
Screen_Manager_WIPv2

r=me if you can answer/correct those two questions:

>+    var telephony = window.navigator.mozTelephony;
>+    if (telephony) {
>+      telephony.oncallschanged = function onCallsChanged() {

Can this be |telephony.addEventListener('callschanged', this)|?

If so please change to that for future-proof.

>+        var inCall = !!telephony.calls.length;
>+        if (!inCall) {
>+          window.removeEventListener('userproximity', self);
>+          if (self._screenOffByProximity) {
>+            self.turnScreenOn();
>+          }

>+
>+      case 'userproximity':
>+        if (evt.near) {
>+          this.turnScreenOff(true);

During the call, currently there is a cpu wakelock in the dialer.
Do we need to move that here or create another wake lock here, just to make sure turnScreenOff() here will not suspend the phone?

I am sure this works currently -- I just want to be future-proof.

>+        } else {
>+          this.turnScreenOn();
>+        }

Comment 55

[Marco Chen [:mchen]]

Attachment: Screen_Manager_WIPv3

Comment 56

[Marco Chen [:mchen]]

Comment on attachment 679043 [details] [diff] [review]
Screen_Manager_WIPv3

Following the suggestion.
1. Use telephony.addEventListener
2. There are own purposes of asking wakelock by dialer and screen_manager, so I prefer to ask wakelocks individually.

Comment 57

[Marco Chen [:mchen]]

Dear Etienne,

I think these two patches will be better to pull request in together.
What is way you used to do? Or I will integrate your patch into one pull request.

Comment 58

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 679043 [details] [diff] [review]
Screen_Manager_WIPv3

r=me with nit addressed
>+
>+      case 'callschanged':
>+        var telephony = window.navigator.mozTelephony;
>+        if (telephony) {

Remove this check -- telephony always exists if callschanged event handler is called

>+          if (0 == telephony.calls.length) {
>+            window.removeEventListener('userproximity', this);
>+            if (this._screenOffByProximity) {
>+              this.turnScreenOn();
>+            }
>+            if (this._cpuWakeLock) {
>+             this._cpuWakeLock.unlock();
>+             this._cpuWakeLock = null;
>+            }
>+          }
>+          else {
>+            this._cpuWakeLock = navigator.requestWakeLock('cpu');
>+            window.addEventListener('userproximity', this);
>+          }

Let's do

if (!telephony.calls.length) {
  // no calls
  window.removeEventListener('userproximity', this);
  ....

  break;
}

// handle calls
window.addEventListener('userproximity', this);
...

Comment 59

[Etienne Segonzac (:etienne)]

(In reply to Marco Chen [:mchen] from comment #57)
> Dear Etienne,
> 
> I think these two patches will be better to pull request in together.

Yep.

> What is way you used to do? Or I will integrate your patch into one pull
> request.

Feel free to integrate my patch to your commit if it makes things easier.

Comment 60

[Marco Chen [:mchen]]

The patches are ready here and is this bug a blocking-basecamp since  it will effect the power consumption during phone call?

Thanks.

Comment 61

[Marco Chen [:mchen]]

Dear Kevin,
Please refer to comment 60 and help to judge is it a blocking basecamp?
thanks.

Comment 62

[Kai-Chih Hu [:khu]]

For bb+ or bb-, I am fine either way. I will nominate this one as bb+ since it will affect power consumption. We already have review+. If you have any suggestion or comments, please raise it. Thank you very much.

Comment 63

[Kan-Ru Chen [:kanru] (UTC+9)]

Is this bug ready to land?

Comment 64

[Marco Chen [:mchen]]

https://github.com/mozilla-b2g/gaia/pull/6370

Ask but not merged until now.

Comment 65

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://github.com/mozilla-b2g/gaia/pull/6641

Merged.

Comment 66

[Natalya Kot [:nkot]]

fix verified in Build 20130113070202 for Unagi

- proximity sensor is working well,  during phone is near of ear - Screen Off, during phone is far of ear or is end - Screen On