New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 896347 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Last visit > 30 days ago
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Feature



Sign in to add a comment

kernel: 4.14 update cros_ec_sensor_ring

Project Member Reported by gwendal@chromium.org, Oct 17

Issue description

4.14 sensor_ring is behind its 4.4 counterpart:

git log --oneline --left-right --graph --cherry-pick cros/chromeos-4.4...HEAD drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c

Shows the latest changes for ARCore are missing:

< 78ea86c66f254 CHROMIUM: cros_ec: enable tight_timestamps automatically in sensor_ring
< 49864518ef212 cros_ec: fix the timestamp comparator used to obtain medians
< ff122fe0872dd (cros/stabilize-11101.B-chromeos-4.4) cros_ec: flip the comparison used to clear the filter history
< 9d940d7b699e5 CHROMIUM: iio: cros_ec: Reset filter when there've been no recent interrupts
< 92a6a71a1e5ab CHROMIUM: cros_ec_sensor_ring: Deal with interleaved samples
< 7f2196441f4a8 CHROMIUM: iio: cros_ec: Add back the legacy timestamp processing code
< 5c206eb768bd7 FIXUP: CHROMIUM: iio: cros_ec: Add predictive filter on fifo_timestamp
< 4f2d6d214d832 CHROMIUM: iio: cros_ec: Add predictive filter on fifo_timestamp
< a51a04df4c512 FIXUP: CHROMIUM: iio: cros_ec: Spread timestamps better
< 3af8b8cf09074 CHROMIUM: iio: cros_ec: Spread timestamps better
< 597d29e5f00d5 CHROMIUM: cros-ec: record event timestamp in the hard irq


Having 4.14 up to date will make 4.19 rebase easier.
 
Cc: amstan@chromium.org
Status: Assigned (was: Untriaged)
4.19 will have the same problem very soon (probably more important).
How soon is "very soon"?
Status: Started (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 31

Labels: merge-merged-chromeos-4.14
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/448f9550647ca51d00a999a96003cd28a553df4a

commit 448f9550647ca51d00a999a96003cd28a553df4a
Author: Alexandru M Stan <amstan@chromium.org>
Date: Wed Oct 31 19:42:21 2018

CHROMIUM: iio: cros_ec: Spread timestamps better

Sometimes the EC receives only one interrupt (hence timestamp) for
a batch of samples. Only the first sample will have the correct
timestamp. So we must interpolate the other samples.
We use the previous batch timestamp and our current batch timestamp
as a way to calculate period, then spread the samples evenly.

  s0 int, 0ms
  s1 int, 10ms
  s2 int, 20ms
  30ms point goes by, no interrupt, previous one is still asserted
  downloading s2 and s3
  s3 sample, 20ms (incorrect timestamp)
  s4 int, 40ms

The batches are [(s0), (s1), (s2, s3), (s4)]. Since the 3rd batch
has 2 samples in them, we adjust the timestamp of s3.
s2 - s1 = 10ms, so s3 must be s2 + 10ms => 20ms. If s1 would have
been part of a bigger batch things would have gotten a little
more complicated.

BUG=b:73551961, b:67743747, chromium:896347
TEST="help" or "gpioget" on the EC does not cause a blip on the
period graph

Change-Id: Ie0dc866bb909144dc20e8e59463abd855add2e75
Signed-off-by: Alexandru M Stan <amstan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1064477
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>

FIXUP: CHROMIUM: iio: cros_ec: Spread timestamps better

Commit 3af8b8cf09074 ("CHROMIUM: iio: cros_ec: Spread timestamps better") causes
test builds to fail.

drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c:
	In function 'cros_ec_ring_fifo_toggle':
drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c:107:2: error:
	'for' loop initial declarations are only allowed in C99 or C11 mode

Furthermore, there are build warnings.

drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c:
	In function cros_ec_ring_handler:
drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c:352:28: warning:
	format %d expects argument of type int, but argument 6 has type s64 {aka long long int}

BUG=b:73551961, b:67743747, chromium:896347
TEST="Build with allmodconfig kernel configurations"

Change-Id: Ib9d64c1c21dafecbda405822339e171a6e992cb9
Signed-off-by: Guenter Roeck <groeck@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1085382
Reviewed-by: Alexandru M Stan <amstan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1308055
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Enrico Granata <egranata@chromium.org>

[modify] https://crrev.com/448f9550647ca51d00a999a96003cd28a553df4a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c

Project Member

Comment 7 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/9af26e57e8a528204d727072911f5788a25651b3

commit 9af26e57e8a528204d727072911f5788a25651b3
Author: Alexandru M Stan <amstan@chromium.org>
Date: Tue Nov 20 03:13:46 2018

CHROMIUM: iio: cros_ec: Add predictive filter on fifo_timestamp

fifo_timestamp is recorded in the AP interrupt (which sometimes has
higher latency), if we were to just do a' = c - b + a, we would couple
all the jitter from the interrupt into the timestamps of sensor samples
from different bursts of data.

Here's an attempt at decomposing (c[n], b[n]), (c[n-1], b[n-1]) into
the slope (labeled m), median filtering the last 30 ms. calculate a good
offset too (also using a median filter), then figure out a'.

BUG=b:67743747, b:73552314, chromium:896347
TEST=Graph the period of a sensor, note how jitter stays under
200us peak-to-peak, especially when system is doing other stuff
TEST=Graph c-b*1000, see the line be pretty jaggedy, graph
cros_ec_ring_ts_filter(b)-b*1000, see it be super smooth

Change-Id: I8a9ea489fe793655feba4ed89359155dfd7a359b
Suggested-by: Gwendal Grignou <gwendal@chromium.org>
Suggested-by: David Schneider <dnschneid@chromium.org>
Signed-off-by: Alexandru M Stan <amstan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/899776
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>

FIXUP: CHROMIUM: iio: cros_ec: Add predictive filter on fifo_timestamp

Avoid

ERROR: "__aeabi_ldivmod" [drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.ko] undefined!

by using 64-bit divide operations where needed.

Also fix the following sparse warnings.

drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c:161:67: sparse:
	Using plain integer as NULL pointer
drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c:220:6: sparse:
	symbol 'cros_ec_ring_ts_filter_update' was not declared. Should it be static?
drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c:301:5: sparse:
	symbol 'cros_ec_ring_ts_filter' was not declared. Should it be static?
drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c:808:25: sparse:
   	symbol 'cros_ec_ring_pm_ops' was not declared. Should it be static?

BUG=b:67743747, b:73552314, chromium:896347
TEST=Build on 32-bit target

Change-Id: I43140614eafeaa2004546bbf3fdf084869703a37
Signed-off-by: Guenter Roeck <groeck@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1094983
Reviewed-by: Alexandru M Stan <amstan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1308057
Commit-Ready: Enrico Granata <egranata@chromium.org>
Tested-by: Enrico Granata <egranata@chromium.org>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>

[modify] https://crrev.com/9af26e57e8a528204d727072911f5788a25651b3/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c

Project Member

Comment 8 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/874a5da607c32610ae03d627340401f7e45ee4d1

commit 874a5da607c32610ae03d627340401f7e45ee4d1
Author: Alexandru M Stan <amstan@chromium.org>
Date: Tue Nov 20 03:13:48 2018

CHROMIUM: iio: cros_ec: Add back the legacy timestamp processing code

Turns out the new code is not happy if used with the old firmware.
"New code" consists of:
* CHROMIUM: iio: cros_ec: Spread timestamps better
* CHROMIUM: iio: cros_ec: Add predictive filter on fifo_timestamp

We'll have to keep the old code since there's a ton of corner cases
for the old firmware behavior that's required to pass CTS. The new code
can be enabled based on the tight_timestamps variable.

Ideally we want the new/old kernel behavior based on a feature flag
that the EC exposes (TODO), but for now we allow it to be switched via
the "tight_timestamps" userspace attribute in /sys/bus/iio/devices.

Summary of changes:
* Moved new spreading code from inline to cros_ec_ring_spread_add()
* Copied old spreading code from before commit 3af8b8cf0907
  ("CHROMIUM: iio: cros_ec: Spread timestamps better") to
  cros_ec_ring_spread_add_legacy()
* Added state->tight_timestamps variable, default false
* Added IIO_DEVICE_ATTR for tight_timestamps
* Based on if tight_timestamps==1
    * Conditionally used timestamp filtering
    * Conditionally used cros_ec_ring_spread_add{,legacy}()
* last_batch_len[] is now an int, no need for it to be 64 bits.
* div64 in places
* Small fixups

CQ-DEPEND=CL:1128257
TEST=With old scarlet firmware, CTS still passes
TEST=With new scarlet firmware (and udev hack, CL:1128257), timestamps are
up to the 100us jitter spec
BUG=b/111079027, b/109786990, chromium:896347

Change-Id: I6e8b4d53f7172fa47ba8e9725387ec4b0ddbc374
Signed-off-by: Alexandru M Stan <amstan@chromium.org>
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1123651
Reviewed-on: https://chromium-review.googlesource.com/1308058
Commit-Ready: Enrico Granata <egranata@chromium.org>
Tested-by: Enrico Granata <egranata@chromium.org>

[modify] https://crrev.com/874a5da607c32610ae03d627340401f7e45ee4d1/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c

Project Member

Comment 9 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/fad4e8e8eeb62fef62813de4220ca72003ccdfef

commit fad4e8e8eeb62fef62813de4220ca72003ccdfef
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Tue Nov 20 03:13:49 2018

CHROMIUM: cros_ec_sensor_ring: Deal with interleaved samples

In the new timestamp spreading algoirthm deal with
samples of different types that come from the same sensor:

TS
ACCEL1
GYRO1
ACCEL2
GYRO2
..

Before, ACCEL1 and ACCEL2 would have the same TS timestamp.
After, ACCEL1 and ACCEL2 are spread.

BUG=b:73551961, chromium:896347
TEST=Compile, CTS tests that passed still do.
Checked with tight_timestamps set to 1.
TEST=Start 2 sensor types, graph 1, cause a batch with "help" on the EC,
stuff is correct now

Change-Id: I9de978a43d444fb7e6237bfb2e37e7eca5e89e2a
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Signed-off-by: Alexandru M Stan <amstan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1128501
Reviewed-on: https://chromium-review.googlesource.com/1308059
Commit-Ready: Enrico Granata <egranata@chromium.org>
Tested-by: Enrico Granata <egranata@chromium.org>

[modify] https://crrev.com/fad4e8e8eeb62fef62813de4220ca72003ccdfef/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c

Project Member

Comment 10 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/2c07c775c8db176f09c82b4b60405ad9b6235b6c

commit 2c07c775c8db176f09c82b4b60405ad9b6235b6c
Author: Alexandru M Stan <amstan@chromium.org>
Date: Tue Nov 20 03:13:51 2018

CHROMIUM: iio: cros_ec: Reset filter when there've been no recent interrupts

When something (ex: CTS) turns off sensors for a while, changes
the state of the CPU (eg: load => heat => clock drift) then starts
the sensors again, the filter is not going to be able to smoothly
transition from the old drift to the new one with the sudden change
in drift.

We should just start from scratch to make sure there's no major
discontinuity in the data. Hopefully there are no big irq jitters
around that time.

For now every time there's a bigger gap than 500ms, we clear the filter
and only start using it again after it has at least 8 irq entries in the
history.

TEST=Change TS_HISTORY_BORED_US to something much lower, now a simple
batch will cause it to clear history, note how there's no big
discontinuity (beyond the irq jitter that the filter is supposed to
solve) in the period graph.
TEST=CTS should fail less, especially around the beginning of a test.
BUG=b:67743747, chromium:896347

Change-Id: Ie638451091287d4a7486021d9dff83f394d983c0
Signed-off-by: Alexandru M Stan <amstan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1136000
Tested-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1308060
Commit-Ready: Enrico Granata <egranata@chromium.org>
Tested-by: Enrico Granata <egranata@chromium.org>

[modify] https://crrev.com/2c07c775c8db176f09c82b4b60405ad9b6235b6c/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c

Project Member

Comment 11 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/5a9f7be497c0026a911119b71b1731d3626bad5b

commit 5a9f7be497c0026a911119b71b1731d3626bad5b
Author: Enrico Granata <egranata@chromium.org>
Date: Tue Nov 20 03:13:52 2018

CHROMIUM: cros_ec: flip the comparison used to clear the filter history

The filter history should be cleared when *more* than 500ms have elapsed.
The code as written did the opposite of the intent. Reverse the direction
of the comparison in order to be true to the intended meaning.

BUG=b:112111610, chromium:896347
TEST=checked incorrect behavior on Nocturne

Change-Id: Iab10b98ba9a08d8c93c333408c0cb32a48c67bfe
Signed-off-by: Enrico Granata <egranata@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1237603
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1308061

[modify] https://crrev.com/5a9f7be497c0026a911119b71b1731d3626bad5b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c

Project Member

Comment 12 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/f9fe66f76f88c26c5df47efd75aab19952fa6682

commit f9fe66f76f88c26c5df47efd75aab19952fa6682
Author: Enrico Granata <egranata@chromium.org>
Date: Tue Nov 20 03:13:53 2018

CHROMIUM: cros_ec: fix the timestamp comparator used to obtain medians

Implementing the comparator by the difference of the timestamps has
the potential to return an incorrect result due to signed integer truncation rules.
Fix this by spelling out what we truly want the comparator to do.

BUG=b:112111610, chromium:896347
TEST=observe no bad sorts in filter output on Nocturne

Change-Id: Ifdceee6c579f51260771ffbf3f46c1fb4a883a3f
Signed-off-by: Enrico Granata <egranata@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1229185
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1308062

[modify] https://crrev.com/f9fe66f76f88c26c5df47efd75aab19952fa6682/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c

Project Member

Comment 13 by bugdroid1@chromium.org, Jan 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ec03d15d862b5a44fa426eb072b671999d351f86

commit ec03d15d862b5a44fa426eb072b671999d351f86
Author: Enrico Granata <egranata@chromium.org>
Date: Thu Jan 10 21:58:40 2019

CHROMIUM: cros_ec: enable tight_timestamps automatically in sensor_ring

This commit teaches cros_ec_sensors_ring how to read the EC feature flags
to detect support for tight timestamps and - if support is detected -
automatically enables tight_timestamps.

BUG=b:112111610, chromium:896347
TEST=tight_timestamps automatically enabled on Nocturne

Conflict:
Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)

	deleted by us:   drivers/platform/chrome/cros_ec_dev.c
Resolved: cros_ec_dev.c sits at drivers/mfd in kernel 4.14

Conflict:
        include/linux/mfd/cros_ec.h
Resolved: moved cros_ec_remove comment below

Reviewed-on: https://chromium-review.googlesource.com/1255904
Reviewed-by: Alexandru M Stan <amstan@chromium.org>

Change-Id: I275640a58e38fd10b71bc56567d2b22b03e93dba
Signed-off-by: Enrico Granata <egranata@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1308064
Commit-Ready: Jett Rink <jettrink@chromium.org>
Reviewed-by: Aaron Durbin <adurbin@chromium.org>

[modify] https://crrev.com/ec03d15d862b5a44fa426eb072b671999d351f86/include/linux/mfd/cros_ec.h
[modify] https://crrev.com/ec03d15d862b5a44fa426eb072b671999d351f86/drivers/mfd/cros_ec_dev.c
[modify] https://crrev.com/ec03d15d862b5a44fa426eb072b671999d351f86/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_ring.c

Sign in to add a comment