SDM845 battery life with sensors
This is mostly speculative right now, but I think there might be some impact on power consumption with sensors support in SDM845. I noticed when recording a video of my op6 that the proximity sensor is always activated with the IR LED blinking. I guess this is the case for other sensors too. We should restrict some sensors like proximity to only be used on-demand (or if supported some lower frequency polling handled by the SLPI).
Cc: @dylanvanassche
Activity
added device-oneplus-enchilada type::bug labels
libsscprovides already a way to disable each sensor, this function is called when theiio-sensor-proxyclient stops claiming the sensor. Basically, we have to check if DE e.g. Phosh, Plasma, etc. or any otheriio-sensor-proxyclient stops claiming the sensor. If that's the case, the sensor should be disabled.Configuring the frequency etc. is not supported yet by
libsscbecauseiio-sensor-proxydoes not support anything like that. However, we should maybe verify if the sensor is configured by default with the lowest sample frequency. During development, I noticed that they are configured by default with the lowest frequency, but verifying it and configuring it during enablement of the sensor would not hurt.By Dylan Van Assche on 2023-12-24T06:52:02
Edited by Ghost User-
libssc provides already a way to disable each sensor, this function is called when the iio-sensor-proxy client stops claiming the sensor. Basically, we have to check if DE e.g. Phosh, Plasma, etc. or any other iio-sensor-proxy client stops claiming the sensor. If that's the case, the sensor should be disabled.
I am new to
iio-sensor-proxycode and C, let aloneGlib, so some of the driver stuff is a bit over my head. However, while runningiio-sensor-proxyin debug mode with my new input proximity driver, I noticed that it always reports the proximity states, whereas the accelerometer and ambient light sensor only report data when the sensor is claimed (by, e.g.monitor-sensor).I am yet to test it, but i think it is because i followed the ssc driver, and did not implement the
<driver>_set_pollingmethod. So currently, it seems your ssc drivers (as well as mine) permanently and unconditionally open the sensor after the<driver>_openis called, https://gitlab.com/dylanvanassche/iio-sensor-proxy/-/blob/ssc/src/drv-ssc-proximity.c#L70. This fits in with the observation by @calebccff that their proximity sensor is always activated.iio-sensor-proxyseems to stop reading from the sensor by setting<driver>_set_pollingtoFALSEin theclient_releasemethod, https://gitlab.com/dylanvanassche/iio-sensor-proxy/-/blob/ssc/src/iio-sensor-proxy.c#L412No idea if implementing
<driver>_set_pollingwill really solve the problem mentioned here, but should hopefully make some difference. E.g. proximity sensors are generally only claimed by the dialer, and I would guess they only claim it when there's an ongoing call.By Sicelo on 2023-12-24T14:58:07
Edited by Administrator -
Yes, adding
<driver>_set_polling, even though it doesn't actually poll, makes the proximity sensor send events only when claimed. The rest of the time, there are no events emitted. I'll update my MR with this.diff --git a/src/drv-input-proximity.c b/src/drv-input-proximity.c index 2b79830..1f888bb 100644 --- a/src/drv-input-proximity.c +++ b/src/drv-input-proximity.c @@ -22,6 +22,7 @@ typedef struct DrvData { GIOChannel *channel; struct input_event event; gsize offset; + guint watch_id; } DrvData; static gint @@ -186,7 +187,7 @@ watch_input_proximity (gpointer user_data) } /* watch this */ - g_io_add_watch (drv_data->channel, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL, proximity_changed, sensor_device); + drv_data->watch_id = g_io_add_watch (drv_data->channel, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL, proximity_changed, sensor_device); drv_data->last_switch_state = test_bit (SW_FRONT_PROXIMITY, bitmask); return TRUE; @@ -211,6 +212,23 @@ input_proximity_discover (GUdevDevice *device) return TRUE; } +static void +input_proximity_set_polling (SensorDevice *sensor_device, gboolean state) +{ + DrvData *drv_data = (DrvData *) sensor_device->priv; + + if (drv_data->watch_id > 0 && state) + return; + if (drv_data->watch_id == 0 && !state) + return; + + g_clear_handle_id (&drv_data->watch_id, g_source_remove); + if (state) { + /* start watching for proximity events */ + watch_input_proximity (sensor_device); + } +} + static SensorDevice * input_proximity_open (GUdevDevice *device) { @@ -225,9 +243,6 @@ input_proximity_open (GUdevDevice *device) drv_data->dev = g_object_ref (device); drv_data->native_path = g_strdup (g_udev_device_get_sysfs_path (device)); - /* start watching for proximity events */ - watch_input_proximity (sensor_device); - return sensor_device; } @@ -247,4 +262,5 @@ SensorDriver input_proximity = { .discover = input_proximity_discover, .open = input_proximity_open, .close = input_proximity_close, + .set_polling = input_proximity_set_polling, };By Sicelo on 2023-12-24T07:33:00
Edited by Administrator -
Nice catch! I looked at the source code and I can confirm your observations :)
Definitely agreed thatset_pollingshould be renamed, probably a left-over from when only polling based sensors are supported. Made an issue, let's discuss things there: https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/issues/385By Dylan Van Assche on 2024-05-08T00:25:52
Edited by Ghost User -
Sorry / not-sorry to be the person, but.. any progress?
Currently I'm suffering by side effect of constant polling and that's the rotation of screen is too fast, so when I accidentally move the phone in a rotated manner, it immediately without delay rotates. I think the reasonable delay in polling would save this trouble and also the battery.
Second option would be adding delay before UI rotation, but it seems to be unnecessary and don't resolve the battery issue.
By David Heidelberg on 2024-09-30T18:27:33
Edited by Ghost User -
I don't understand everything well, but I think what I'd like is to have a greater threshold before which the rotation can be triggered, currently it's 45° but it's too little, you can e.g. lay in bed with your phone in portrait mode because of that. I'd expect the phone to activate the rotation after passing 60° or maybe even 75°, plus a very short delay (250ms?) to confirm the rotation. That way to rotate you would just have to put the phone in the desired rotation, wait just a tiny bit, and then be able to move freely once it's triggered the rotation.
That's all unrelated to the polling issue but well, both likely should be fixed.
By Adrien Plazas on 2024-07-01T07:08:50
-
I'd expect the phone to activate the rotation after passing 60° or maybe even 75°
That's a threshold we need to fix in iio-sensor-proxy. The C file responsible is https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/blob/master/src/orientation.c
There's already a threshold set to 35, maybe we need to experiment a bit here?
plus a very short delay (250ms?)
That should be already the case if we add this
set_pollingto the driver. Name of the function seems to be a bit wrong though, there's an issue about that in iio-sensor-proxy. I'm not sure yet what the new name should be (or what the exact function is of this method in the driver).By Dylan Van Assche on 2024-07-01T07:35:09
Edited by Administrator -
That's a threshold we need to fix in iio-sensor-proxy. The C file responsible is https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/blob/master/src/orientation.c There's already a threshold set to 35, maybe we need to experiment a bit here?
Maybe make it configurable somehow? It is reasonable to expect sensitivities to vary across devices
That should be already the case if we add this set_polling to the driver. Name of the function seems to be a bit wrong though, there's an issue about that in iio-sensor-proxy. I'm not sure yet what the new name should be (or what the exact function is of this method in the driver).
regarding method names and functions, here's my (very limited) understanding:
method purpose possible new method name discoversearches for supported sensors NO CHANGE opencreate a handle for a sensor (and possibly any needed setup to get things into a ready state) registercloseremove handle for a sensor unregister/deregisterset_pollingread live data from sensor when called with trueas 2nd argument (acquiring a "Claim" does this), and stop reading when that argument isfalse(done when "Claim" is released)readBy Sicelo on 2024-07-01T09:16:01
Edited by Administrator
added device-category::community label
-
A thought I've had thinking on this issue again: do we need to keep a handle open with the DSP all the time? Could libssc simply open a connection each time the value is polled?
if there's minimal latency here then this ought to help?
Unless the way things work atm means GNOME/phosh are always polling which is definitely not great...
By Caleb Connolly on 2024-07-01T07:57:18
-
Unless the way things work atm means GNOME/phosh are always polling which is definitely not great...
Phosh polls the sensors if you activate functions related to them. E.g. Accelerometer is always polled if you enable UI auto-rotation, and not polled if that's disabled. On my L5, ALS is always polled - no idea if there's a setting to disable adaptive brightness ...
No idea about GNOME as I've not run it, but I should expect similar behavior.
tldr; i think the UIs are doing correct thing
Could libssc simply open a connection each time the value is polled?
opening such connection is what
.set_pollingwould need to do. for the ssc driver in iio-sensor-proxy, that logic is currently in.open, hence the constant pollingBy Sicelo on 2024-07-01T11:32:58
Edited by Ghost User -
I think I get how it fits in iio-sensor-proxy:
- Regular IIO sensors: read them every X times with polling
- Buffered IIO sensors: read the buffer from the kernel every X times with polling
- libssc sensors: incoming stream of sensors, similar to buffer
- NOW: read the stream on every value change, speed depends on how fast the DSP sends data
- FUTURE: store the stream data of the last X values, read the stored data every X times with polling. So emitting new sensor data by iio-sensor-proxy must happen in
set_polling's timer function, not when the data comes in. Ifset_polling== TRUE or the queue is empty, we should block until a new value is received. This would help GNOME Mobile as well, see https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/issues/378 as when claiming the sensor, sometimes the data is not recent when storing it in a buffer.
This future approach will make sure the polling times are guaranteed: we define how fast the data is processed by iio-sensor-proxy instead of the DSP deciding how fast the data propagates.
By Dylan Van Assche on 2024-07-01T11:32:58
marked #3250 (closed) as a duplicate of this issue
marked this issue as related to #3250 (closed)
Was just made aware of this in #3250 (comment 452310) and wanted to drop that this also affects e.g. sdm670 / the Pixel 3a.
Given how long this has been a know issue and assuming it will still take a while until somebody has time/motivation to address it properly, I wonder if we should disable the proximity sensor implementation for the time being (assuming that's enough to close this issue).
IIUC that would come down to dropping or somehow guarding (behind a env var?) the implementation in https://gitlab.postmarketos.org/postmarketOS/pmaports/-/blob/master/temp/iio-sensor-proxy/0002-proximity-support-SSC-proximity-sensor.patch
The feature is arguably not important enough to wear down batteries at the rate it currently does.
WDYT? (cc @pabloyoyoista @craftyguy in case you're not subscribed)
Edited by Robert Mader
I think that makes sense, but it's probably on @DylanVanAssche to judge if that makes sense? I'm totally no expert in that code
Does disabling the proximity sensor affect screen wakeup though?
PlaMo has double tap to wake, that can't be turned off. If disabling the sensor makes the phone wakeup more often in pockets then it might make battery life worse.
I'm not actually sure that the proximity sensor it used by PlaMo for wakeup detection though, I think I have seen the phone wakeup in my pocket with the proximity sensor sensor working
It depends if the DE is claiming the sensor continuously or not.
I haven't verified this myself yet, however I was told DE devs that this should already be the case (i.e. only claiming the sensors when needed) and that it's most likely a driver/iio-sensor-proxy issue (also mentioned above). AFAIK the SSC driver is the only one known to show this behavior - the excessive CPU usage - atm, somewhat supporting that claim :/
Does disabling the proximity sensor affect screen wakeup though?
Yeah, according to Jonas Dreßler Gnome-Mobile uses the proximity only when the screen is or gets turned on.
Besides proximity, there are many other sensors active.
Hm, that would make things more difficult indeed. I'll try to verify which exact sensors keep iio-sensor-proxy busy, in case it's not all of them.
- dynamic screen brightness based on ambient light, then ALS is claimed
- UI autorotation, then accelerometer is claimed
DEs should be smart enough to release the sensors when the screen is turned off though (and I'm being told they do). The main issue I see here is that
iio-sensor-proxyis even active when it really shouldn't (only with SSC though).Edited by Robert MaderIt might be that we are missing something in the SSC driver of iio-sensor-proxy, but I have no idea what to be honest
@DylanVanAssche I'm a little sad to read this, after having spent time studying the code and explaining it here, here and on Matrix, as well as providing a summary (with a nice table) on what seems to be the function of each of the methods.
One more attempt however - disclaimer: I do not have an SSC-device, and thus have no way to test. However, the main problem that I see is that the
iio-sensor-proxydrivers are still missing the_set_pollingmethod. Do not focus on the obviously-wrong method name, but on what it actually does.Quick recap: when
iio-sensor-proxyloads a sensor driver, it first executes the_discovermethod, then the_openmethod. After that, it waits (does not read anything from the sensor) until a Claim on a sensor is obtained, after which it runs_set_polling, which causes sensor data to flow intoiio-sensor-proxy.In their current state, the SSC drivers in
iio-sensor-proxy, are already reading from sensors (SSC) while waiting in the_openmethod, and that's one of the reasons why sensors appear to be hyperactive. IOW, the actions of Claiming or releasing Claim are basically a No-Op for these drivers -iio-sensor-proxysimply does nothing, since there's no_set_pollingto run.So, as far as I understand this, the following needs to be done, at least as a start. I'll use the proximity driver as an example:
- Remove https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/blob/2d716dc3f23d2c768675e21eb461fafc98122186/src/drv-ssc-proximity.c#L80-L91 from the
_openmethod - add
ssc_proximity_set_polling, and paste the code removed from_open. That code should be protected byif (state) { ... } - move https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/blob/2d716dc3f23d2c768675e21eb461fafc98122186/src/drv-ssc-proximity.c#L102-L108 out of
_close, and paste it in_set_polling, under anif ( !state ) { ... }- Reason for this change and the previous one: Claiming calls_set_pollingwithstate == TRUEand releasing claim calls the same method, withstate == FALSE - make any other needed adjustments so the code makes sense, e.g. keeping a flag in
DrvDatathat tracks if sensor is currently activated for reading. This is useful in_set_pollingto avoid wasting time trying to activate a sensor when it's already open, i.e. when a 2nd client tries to claim a sensor that is already claimed, this should No-Op in_set_polling. It also ensures that the sensor is not shutdown until there are no clients anymore. This flag generally has_idsuffix in existingiio-sensor-proxydrivers. For quick and dirty testing, it can probably be skipped - build and see if there is any difference
It is possible that some modifications are also needed in the underlying
libsscitself, but doing the above is an important first step for theiio-sensor-proxyside of the problem- Remove https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/blob/2d716dc3f23d2c768675e21eb461fafc98122186/src/drv-ssc-proximity.c#L80-L91 from the
Yeah, I'm very sorry about this, I'm so occupied lately with my daily job and life, I haven't touched anything in months. The
set_pollingis being added upstream. I just have no idea about the 'fix'. I agree we need this method of course! I even missed the relation between your work above (which is SUPER nice) and this comment
Let me put it differently: I will not be able to fix it myself due to the lack of time, or even have time to locate what we need exactly.
It appears that by now you understand the code better than I do currently... Maybe I shouldn't be writing code that is 'popular'
Edited by Dylan Van AsscheCool.
@DylanVanAssche Out of curiosity, why SSC? What does the first S stand for? I guess the last SC means Sensor Core
@rmader want to give the suggestion above a go? Shouldn't need you to write any extensive code (if any), but will at least show us if we're going in the correct direction (I absolutely think we are!)
Edited by sicelo-
@sicelo SSC = Snapdragon Sensor Core which is the name that is used here and there for it.
Nice, thanks @DylanVanAssche and @sicelo, sounds like we have a clear path to move this forward - and hopefully upstream the patches eventually.
I'll try to give it a go in a couple of days if nobody beats me to it - sounds like a rather low-hanging fruit.
Looks like there's already a start for that: https://gitlab.freedesktop.org/verdre/iio-sensor-proxy/-/commit/74a5860474b6e79cd6e3be13d1ff8dd9cd810b8b
mentioned in commit alistair23/pmaports@157a3ddf
mentioned in merge request !5853 (merged)
mentioned in commit alistair23/pmaports@de3d8d07
mentioned in commit alistair23/pmaports@367ca486
mentioned in commit alistair23/pmaports@49e70bd5
closed with merge request !5853 (merged)
mentioned in commit d68e24db
