| Message ID | 20180328174029.1045-1-mkelly@xevo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 28 Mar 2018 10:40:29 -0700 Martin Kelly <mkelly@xevo.com> wrote: > When interrupts are generated at a slower rate than the FIFO fills up, > we will have fewer timestamps than samples. Currently, we fill in 0 for > any unmatched timestamps. However, this is very confusing for userspace, > which does not expect discontinuities in timestamps and must somehow > work around the issue. > > Improve the situation by interpolating timestamps when we can't map them > 1:1 to data. We do this by assuming the timestamps we have are the most > recent and interpolating backwards to fill the older data. This makes > sense because inv_mpu6050_read_fifo gets called right after an interrupt > is generated, presumably when a datum was generated, so the most recent > timestamp matches up with the most recent datum. In addition, this > assumption is borne out by observation, which shows monotonically > increasing timestamps when interpolating this way but discontinuities > when interpolating in the other direction. > > Although this method is not perfectly accurate, it is probably the best > we can do if we don't get one interrupt per datum. This patch worries me somewhat - we are basically guessing what the cause of the missed interrupts was. If for example the fifo read itself takes a long time, the interrupt time we have is actually valid for the first sample and we should in interpolating forwards in time. It sounds from discussions like something else is broken on your board. I like the idea of interpolating interrupts, but would like to conduct the same experiments you did when we are running at high speeds and seeing misses - not on the test case you currently have. The problem then will be estimating how long the interrupt took to be handed vs the read out rate of the fifo. Chances are our 'correct' timestamp is somewhat after the first reading but well before the last one. Anyhow, let us know once you have the thing running properly at high speed then we can work out how best to address this. Jonathan > > Signed-off-by: Martin Kelly <mkelly@xevo.com> > --- > drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 2 ++ > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 2 ++ > drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 50 +++++++++++++++++++++++++++--- > 3 files changed, 50 insertions(+), 4 deletions(-) > > v1: > - Set a missing timestamp to the last timestamp we saw. > v2: > - Interpolate missing timestamps. > v3: > - Slight optimization to move timestamp_interp += into the result == 0 case. > > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c > index 7d64be353403..4a95ff8df3b9 100644 > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c > @@ -83,6 +83,7 @@ static const struct inv_mpu6050_chip_config chip_config_6050 = { > .fsr = INV_MPU6050_FSR_2000DPS, > .lpf = INV_MPU6050_FILTER_20HZ, > .fifo_rate = INV_MPU6050_INIT_FIFO_RATE, > + .fifo_period = NSEC_PER_SEC / INV_MPU6050_INIT_FIFO_RATE, > .gyro_fifo_enable = false, > .accl_fifo_enable = false, > .accl_fs = INV_MPU6050_FS_02G, > @@ -630,6 +631,7 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr, > if (result) > goto fifo_rate_fail_power_off; > st->chip_config.fifo_rate = fifo_rate; > + st->chip_config.fifo_period = NSEC_PER_SEC / fifo_rate; > > result = inv_mpu6050_set_lpf(st, fifo_rate); > if (result) > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h > index 065794162d65..3bc7d62822ca 100644 > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h > @@ -86,6 +86,7 @@ enum inv_devices { > * @accl_fifo_enable: enable accel data output > * @gyro_fifo_enable: enable gyro data output > * @fifo_rate: FIFO update rate. > + * @fifo_period: FIFO update period, in nanoseconds. > */ > struct inv_mpu6050_chip_config { > unsigned int fsr:2; > @@ -94,6 +95,7 @@ struct inv_mpu6050_chip_config { > unsigned int accl_fifo_enable:1; > unsigned int gyro_fifo_enable:1; > u16 fifo_rate; > + u32 fifo_period; > }; > > /** > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c > index ff81c6aa009d..40610a316eb0 100644 > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c > @@ -126,7 +126,11 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) > int result; > u8 data[INV_MPU6050_OUTPUT_DATA_SIZE]; > u16 fifo_count; > + u16 fifo_items; > + s32 fifo_diff; > s64 timestamp; > + s64 timestamp_interp; > + s64 offset; > > mutex_lock(&st->lock); > if (!(st->chip_config.accl_fifo_enable | > @@ -156,9 +160,25 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) > if (fifo_count > INV_MPU6050_FIFO_THRESHOLD) > goto flush_fifo; > /* Timestamp mismatch. */ > + fifo_items = fifo_count / bytes_per_datum; > if (kfifo_len(&st->timestamps) > > - fifo_count / bytes_per_datum + INV_MPU6050_TIME_STAMP_TOR) > + fifo_items + INV_MPU6050_TIME_STAMP_TOR) > goto flush_fifo; > + > + /* > + * If we have more data than timestamps, the timestamps we have > + * correspond to the newest items in the FIFO, since some data was > + * generated without a corresponding interrupt and thus timestamp. Since > + * we remove FIFO items oldest to newest, we need to interpolate the > + * older timestamps based on the number of missing timestamps. > + */ > + fifo_diff = fifo_items - kfifo_len(&st->timestamps); > + if (fifo_diff > 0) > + offset = st->chip_config.fifo_period * fifo_diff; > + else > + offset = 0; > + > + timestamp_interp = 0; > while (fifo_count >= bytes_per_datum) { > result = regmap_bulk_read(st->map, st->reg->fifo_r_w, > data, bytes_per_datum); > @@ -166,9 +186,31 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) > goto flush_fifo; > > result = kfifo_out(&st->timestamps, ×tamp, 1); > - /* when there is no timestamp, put timestamp as 0 */ > - if (result == 0) > - timestamp = 0; > + /* When there is no timestamp, interpolate based on period */ > + if (result == 0) { > + /* > + * We have no more timestamps left, so interpolate the > + * rest of them. > + */ > + if (likely(timestamp_interp != 0)) > + timestamp_interp += st->chip_config.fifo_period; > + else > + /* > + * In this unlikely error case, we should output > + * a 0 timestamp instead of 0 + FIFO period. > + */ > + dev_err(regmap_get_device(st->map), > + "Timestamp FIFO is empty!\n"); > + timestamp = timestamp_interp; > + } else { > + /* > + * If we are interpolating, this is an older datum with > + * a newer timestamp, so offset it backwards to account > + * for the time gap. Otherwise, offset will be 0. > + */ > + timestamp -= offset; > + timestamp_interp = timestamp; > + } > > result = iio_push_to_buffers_with_timestamp(indio_dev, data, > timestamp); -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/30/2018 03:36 AM, Jonathan Cameron wrote: > On Wed, 28 Mar 2018 10:40:29 -0700 > Martin Kelly <mkelly@xevo.com> wrote: > >> When interrupts are generated at a slower rate than the FIFO fills up, >> we will have fewer timestamps than samples. Currently, we fill in 0 for >> any unmatched timestamps. However, this is very confusing for userspace, >> which does not expect discontinuities in timestamps and must somehow >> work around the issue. >> >> Improve the situation by interpolating timestamps when we can't map them >> 1:1 to data. We do this by assuming the timestamps we have are the most >> recent and interpolating backwards to fill the older data. This makes >> sense because inv_mpu6050_read_fifo gets called right after an interrupt >> is generated, presumably when a datum was generated, so the most recent >> timestamp matches up with the most recent datum. In addition, this >> assumption is borne out by observation, which shows monotonically >> increasing timestamps when interpolating this way but discontinuities >> when interpolating in the other direction. >> >> Although this method is not perfectly accurate, it is probably the best >> we can do if we don't get one interrupt per datum. > This patch worries me somewhat - we are basically guessing what the cause > of the missed interrupts was. If for example the fifo read itself > takes a long time, the interrupt time we have is actually valid for > the first sample and we should in interpolating forwards in time. > I agree with you that if the delay is somewhere after the IRQ fires but prior to the FIFO read (or the FIFO read itself), then we'd need forward interpolation. That said, if there is more than 1 sample in FIFO when the IRQ fires, we need backwards interpolation (since those samples were certainly created prior to the IRQ firing and the timestamp being generated). I believe the current patch is will accurately handle the backwards interpolation case but be wrong for the forwards interpolation case (though it will at least guarantee samples to be monotonically increasing, as they should be). I thought about this and I think we can fix both issues. I propose we do the following: - When the IRQ fires, immediately timestamp as we already do. Also, immediately check the FIFO length and store it (call this length n). - Right after reading from the FIFO length (calling regmap_bulk_read() at inv_mpu_ring.c:146), take a timestamp again. Call this FIFO length m. - The first n samples (those present when the IRQ fired) should be backwards-interpolated from the timestamp taken when the IRQ fired. - If the two FIFO lengths taken are different (m - n > 0), then some samples were generated between the IRQ firing and the FIFO being read. This could be caused by a slow read, a delay firing off the bottom half of the IRQ, or some other delay in the inv_mpu6050_read_fifo() function. In any case, these m - n samples need to be forward-interpolated between the two timestamps we took, since they were generated sometime between the two timestamps. I believe this approach will fix both issues. Although my board is not seeing the second issue (where m - n > 0), I can simulate it by adding artificial delays. In addition to fixing my issue, this should make the code more resilient to unknown bus and IRQ issues in the future. Jonathan, how does this approach sound to you? > It sounds from discussions like something else is broken on your > board. I like the idea of interpolating interrupts, but would like to > conduct the same experiments you did when we are running at high speeds > and seeing misses - not on the test case you currently have. > > The problem then will be estimating how long the interrupt took to > be handed vs the read out rate of the fifo. Chances are our 'correct' > timestamp is somewhat after the first reading but well before the last > one. > > Anyhow, let us know once you have the thing running properly at > high speed then we can work out how best to address this. > > Jonathan > Once I get the thing working, I will definitely look again at the interpolation and make sure it's still valid. If I see any anomalies, I'll let you know. -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2 Apr 2018 10:07:57 -0700 Martin Kelly <mkelly@xevo.com> wrote: > On 03/30/2018 03:36 AM, Jonathan Cameron wrote: > > On Wed, 28 Mar 2018 10:40:29 -0700 > > Martin Kelly <mkelly@xevo.com> wrote: > > > >> When interrupts are generated at a slower rate than the FIFO fills up, > >> we will have fewer timestamps than samples. Currently, we fill in 0 for > >> any unmatched timestamps. However, this is very confusing for userspace, > >> which does not expect discontinuities in timestamps and must somehow > >> work around the issue. > >> > >> Improve the situation by interpolating timestamps when we can't map them > >> 1:1 to data. We do this by assuming the timestamps we have are the most > >> recent and interpolating backwards to fill the older data. This makes > >> sense because inv_mpu6050_read_fifo gets called right after an interrupt > >> is generated, presumably when a datum was generated, so the most recent > >> timestamp matches up with the most recent datum. In addition, this > >> assumption is borne out by observation, which shows monotonically > >> increasing timestamps when interpolating this way but discontinuities > >> when interpolating in the other direction. > >> > >> Although this method is not perfectly accurate, it is probably the best > >> we can do if we don't get one interrupt per datum. > > This patch worries me somewhat - we are basically guessing what the cause > > of the missed interrupts was. If for example the fifo read itself > > takes a long time, the interrupt time we have is actually valid for > > the first sample and we should in interpolating forwards in time. > > > > I agree with you that if the delay is somewhere after the IRQ fires but > prior to the FIFO read (or the FIFO read itself), then we'd need forward > interpolation. That said, if there is more than 1 sample in FIFO when > the IRQ fires, we need backwards interpolation (since those samples were > certainly created prior to the IRQ firing and the timestamp being > generated). I believe the current patch is will accurately handle the > backwards interpolation case but be wrong for the forwards interpolation > case (though it will at least guarantee samples to be monotonically > increasing, as they should be). > > I thought about this and I think we can fix both issues. I propose we do > the following: > > - When the IRQ fires, immediately timestamp as we already do. Also, > immediately check the FIFO length and store it (call this length n). > > - Right after reading from the FIFO length (calling regmap_bulk_read() > at inv_mpu_ring.c:146), take a timestamp again. Call this FIFO length m. > > - The first n samples (those present when the IRQ fired) should be > backwards-interpolated from the timestamp taken when the IRQ fired. > > - If the two FIFO lengths taken are different (m - n > 0), then some > samples were generated between the IRQ firing and the FIFO being read. > This could be caused by a slow read, a delay firing off the bottom half > of the IRQ, or some other delay in the inv_mpu6050_read_fifo() function. > In any case, these m - n samples need to be forward-interpolated between > the two timestamps we took, since they were generated sometime between > the two timestamps. > > I believe this approach will fix both issues. Although my board is not > seeing the second issue (where m - n > 0), I can simulate it by adding > artificial delays. > > In addition to fixing my issue, this should make the code more resilient > to unknown bus and IRQ issues in the future. > > Jonathan, how does this approach sound to you? Great. > > > It sounds from discussions like something else is broken on your > > board. I like the idea of interpolating interrupts, but would like to > > conduct the same experiments you did when we are running at high speeds > > and seeing misses - not on the test case you currently have. > > > > The problem then will be estimating how long the interrupt took to > > be handed vs the read out rate of the fifo. Chances are our 'correct' > > timestamp is somewhat after the first reading but well before the last > > one. > > > > Anyhow, let us know once you have the thing running properly at > > high speed then we can work out how best to address this. > > > > Jonathan > > > > Once I get the thing working, I will definitely look again at the > interpolation and make sure it's still valid. If I see any anomalies, > I'll let you know. > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, there is just a problem if I'm understanding well. Reading FIFO count register under hard irq handler (when taking the timestamp) is not possible since i2c/spi access is using a mutex. That's why we are using an irq thread for reading chip registers. I've been able to reproduce the issue when doing a long stress-test at high speed (200Hz). Perhaps this is related to the FIFO error handling which looks really over-complex for me. I am currently investigating this issue. But I would be happy to test any new solution if proposed. Best regards, JB From: Jonathan Cameron <Jonathan.Cameron@huawei.com> Sent: Friday, April 6, 2018 5:15:09 PM To: Martin Kelly Cc: Jonathan Cameron; linux-iio@vger.kernel.org; Jean-Baptiste Maneyrol Subject: Re: [PATCH v3] imu: inv_mpu6050: interpolate missing timestamps On Mon, 2 Apr 2018 10:07:57 -0700 Martin Kelly <mkelly@xevo.com> wrote: > On 03/30/2018 03:36 AM, Jonathan Cameron wrote: > > On Wed, 28 Mar 2018 10:40:29 -0700 > > Martin Kelly <mkelly@xevo.com> wrote: > > > >> When interrupts are generated at a slower rate than the FIFO fills up, > >> we will have fewer timestamps than samples. Currently, we fill in 0 for > >> any unmatched timestamps. However, this is very confusing for userspace, > >> which does not expect discontinuities in timestamps and must somehow > >> work around the issue. > >> > >> Improve the situation by interpolating timestamps when we can't map them > >> 1:1 to data. We do this by assuming the timestamps we have are the most > >> recent and interpolating backwards to fill the older data. This makes > >> sense because inv_mpu6050_read_fifo gets called right after an interrupt > >> is generated, presumably when a datum was generated, so the most recent > >> timestamp matches up with the most recent datum. In addition, this > >> assumption is borne out by observation, which shows monotonically > >> increasing timestamps when interpolating this way but discontinuities > >> when interpolating in the other direction. > >> > >> Although this method is not perfectly accurate, it is probably the best > >> we can do if we don't get one interrupt per datum. > > This patch worries me somewhat - we are basically guessing what the cause > > of the missed interrupts was. If for example the fifo read itself > > takes a long time, the interrupt time we have is actually valid for > > the first sample and we should in interpolating forwards in time. > > > > I agree with you that if the delay is somewhere after the IRQ fires but > prior to the FIFO read (or the FIFO read itself), then we'd need forward > interpolation. That said, if there is more than 1 sample in FIFO when > the IRQ fires, we need backwards interpolation (since those samples were > certainly created prior to the IRQ firing and the timestamp being > generated). I believe the current patch is will accurately handle the > backwards interpolation case but be wrong for the forwards interpolation > case (though it will at least guarantee samples to be monotonically > increasing, as they should be). > > I thought about this and I think we can fix both issues. I propose we do > the following: > > - When the IRQ fires, immediately timestamp as we already do. Also, > immediately check the FIFO length and store it (call this length n). > > - Right after reading from the FIFO length (calling regmap_bulk_read() > at inv_mpu_ring.c:146), take a timestamp again. Call this FIFO length m. > > - The first n samples (those present when the IRQ fired) should be > backwards-interpolated from the timestamp taken when the IRQ fired. > > - If the two FIFO lengths taken are different (m - n > 0), then some > samples were generated between the IRQ firing and the FIFO being read. > This could be caused by a slow read, a delay firing off the bottom half > of the IRQ, or some other delay in the inv_mpu6050_read_fifo() function. > In any case, these m - n samples need to be forward-interpolated between > the two timestamps we took, since they were generated sometime between > the two timestamps. > > I believe this approach will fix both issues. Although my board is not > seeing the second issue (where m - n > 0), I can simulate it by adding > artificial delays. > > In addition to fixing my issue, this should make the code more resilient > to unknown bus and IRQ issues in the future. > > Jonathan, how does this approach sound to you? Great. > > > It sounds from discussions like something else is broken on your > > board. I like the idea of interpolating interrupts, but would like to > > conduct the same experiments you did when we are running at high speeds > > and seeing misses - not on the test case you currently have. > > > > The problem then will be estimating how long the interrupt took to > > be handed vs the read out rate of the fifo. Chances are our 'correct' > > timestamp is somewhat after the first reading but well before the last > > one. > > > > Anyhow, let us know once you have the thing running properly at > > high speed then we can work out how best to address this. > > > > Jonathan > > > > Once I get the thing working, I will definitely look again at the > interpolation and make sure it's still valid. If I see any anomalies, > I'll let you know. > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html VGER.KERNEL.ORG - Majordomo info vger.kernel.org Very short Majordomo intro. Send request in email to address <majordomo@vger.kernel.org> . To subscribe a list (``linux-kernel'' is given as an example), use following as the only content of your letter: -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6 April 2018 16:21:05 BST, Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote: >Hello, > > >there is just a problem if I'm understanding well. > > >Reading FIFO count register under hard irq handler (when taking the >timestamp) is not possible since i2c access is using a mutex. That's >why we are using an irq thread for reading FIFO content. Good point. Need more sleep or caffeine! > > >I've been able to reproduce the issue when doing a long stress-test at >high speed (200Hz). Perhaps this is related to the FIFO error handling >which looks really over-complex for me. I am currently investigating >this issue. > > >But I would be happy to test any new solution if proposed. > > >Best regards, > >JB > >________________________________ >From: Jonathan Cameron <Jonathan.Cameron@huawei.com> >Sent: Friday, April 6, 2018 5:15:09 PM >To: Martin Kelly >Cc: Jonathan Cameron; linux-iio@vger.kernel.org; Jean-Baptiste Maneyrol >Subject: Re: [PATCH v3] imu: inv_mpu6050: interpolate missing >timestamps > >On Mon, 2 Apr 2018 10:07:57 -0700 >Martin Kelly <mkelly@xevo.com> wrote: > >> On 03/30/2018 03:36 AM, Jonathan Cameron wrote: >> > On Wed, 28 Mar 2018 10:40:29 -0700 >> > Martin Kelly <mkelly@xevo.com> wrote: >> > >> >> When interrupts are generated at a slower rate than the FIFO fills >up, >> >> we will have fewer timestamps than samples. Currently, we fill in >0 for >> >> any unmatched timestamps. However, this is very confusing for >userspace, >> >> which does not expect discontinuities in timestamps and must >somehow >> >> work around the issue. >> >> >> >> Improve the situation by interpolating timestamps when we can't >map them >> >> 1:1 to data. We do this by assuming the timestamps we have are the >most >> >> recent and interpolating backwards to fill the older data. This >makes >> >> sense because inv_mpu6050_read_fifo gets called right after an >interrupt >> >> is generated, presumably when a datum was generated, so the most >recent >> >> timestamp matches up with the most recent datum. In addition, this >> >> assumption is borne out by observation, which shows monotonically >> >> increasing timestamps when interpolating this way but >discontinuities >> >> when interpolating in the other direction. >> >> >> >> Although this method is not perfectly accurate, it is probably the >best >> >> we can do if we don't get one interrupt per datum. >> > This patch worries me somewhat - we are basically guessing what the >cause >> > of the missed interrupts was. If for example the fifo read itself >> > takes a long time, the interrupt time we have is actually valid for >> > the first sample and we should in interpolating forwards in time. >> > >> >> I agree with you that if the delay is somewhere after the IRQ fires >but >> prior to the FIFO read (or the FIFO read itself), then we'd need >forward >> interpolation. That said, if there is more than 1 sample in FIFO when >> the IRQ fires, we need backwards interpolation (since those samples >were >> certainly created prior to the IRQ firing and the timestamp being >> generated). I believe the current patch is will accurately handle the >> backwards interpolation case but be wrong for the forwards >interpolation >> case (though it will at least guarantee samples to be monotonically >> increasing, as they should be). >> >> I thought about this and I think we can fix both issues. I propose we >do >> the following: >> >> - When the IRQ fires, immediately timestamp as we already do. Also, >> immediately check the FIFO length and store it (call this length n). >> >> - Right after reading from the FIFO length (calling >regmap_bulk_read() >> at inv_mpu_ring.c:146), take a timestamp again. Call this FIFO length >m. >> >> - The first n samples (those present when the IRQ fired) should be >> backwards-interpolated from the timestamp taken when the IRQ fired. >> >> - If the two FIFO lengths taken are different (m - n > 0), then some >> samples were generated between the IRQ firing and the FIFO being >read. >> This could be caused by a slow read, a delay firing off the bottom >half >> of the IRQ, or some other delay in the inv_mpu6050_read_fifo() >function. >> In any case, these m - n samples need to be forward-interpolated >between >> the two timestamps we took, since they were generated sometime >between >> the two timestamps. >> >> I believe this approach will fix both issues. Although my board is >not >> seeing the second issue (where m - n > 0), I can simulate it by >adding >> artificial delays. >> >> In addition to fixing my issue, this should make the code more >resilient >> to unknown bus and IRQ issues in the future. >> >> Jonathan, how does this approach sound to you? >Great. > >> >> > It sounds from discussions like something else is broken on your >> > board. I like the idea of interpolating interrupts, but would like >to >> > conduct the same experiments you did when we are running at high >speeds >> > and seeing misses - not on the test case you currently have. >> > >> > The problem then will be estimating how long the interrupt took to >> > be handed vs the read out rate of the fifo. Chances are our >'correct' >> > timestamp is somewhat after the first reading but well before the >last >> > one. >> > >> > Anyhow, let us know once you have the thing running properly at >> > high speed then we can work out how best to address this. >> > >> > Jonathan >> > >> >> Once I get the thing working, I will definitely look again at the >> interpolation and make sure it's still valid. If I see any anomalies, >> I'll let you know. >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-iio" >in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/06/2018 08:41 AM, Jonathan Cameron wrote: > > > On 6 April 2018 16:21:05 BST, Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote: >> Hello, >> >> >> there is just a problem if I'm understanding well. >> >> >> Reading FIFO count register under hard irq handler (when taking the >> timestamp) is not possible since i2c access is using a mutex. That's >> why we are using an irq thread for reading FIFO content. > > Good point. Need more sleep or caffeine! > > I was about to reply with the same, as I started coding it up :). Too bad, it was such a great plan! I have a little update: When switching to level triggered interrupts, the problem goes away for me, as do the bus errors I get at high frequencies. I'm working on a patch to support other interrupt types than rising edge, which is almost done. I also intend to look again at the bus errors for edge driven interrupts. Since they happen only at high frequency and go away with level driven interrupts (which must be acked and therefore prevent reentrancy), I suspect there's a concurrency bug. That said, I think the question remains: Since we can't get the FIFO count from the hard IRQ handler, and since it could be some time before the bottom half thread is scheduled, I don't see any way to accurately handle forward interpolation. Though we can't do forward interpolation, I think at least having backward interpolation is better than filling in blank timestamps, especially as we haven't seen an actual case of forward interpolation being needed, while we have real use cases that require backward interpolation (and can be easily verified in a logic analyzer). However, that's only my opinion. Jonathan, Jean-Baptiste, and others, what do you think? >> >> >> I've been able to reproduce the issue when doing a long stress-test at >> high speed (200Hz). Perhaps this is related to the FIFO error handling >> which looks really over-complex for me. I am currently investigating >> this issue. >> >> >> But I would be happy to test any new solution if proposed. >> >> >> Best regards, >> >> JB >> Thanks Jean-Baptiste; let me know how that turns out. -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/06/2018 09:33 AM, Martin Kelly wrote: > On 04/06/2018 08:41 AM, Jonathan Cameron wrote: >> >> >> On 6 April 2018 16:21:05 BST, Jean-Baptiste Maneyrol >> <JManeyrol@invensense.com> wrote: >>> Hello, >>> >>> >>> there is just a problem if I'm understanding well. >>> >>> >>> Reading FIFO count register under hard irq handler (when taking the >>> timestamp) is not possible since i2c access is using a mutex. That's >>> why we are using an irq thread for reading FIFO content. >> >> Good point. Need more sleep or caffeine! >> >> > > I was about to reply with the same, as I started coding it up :). Too > bad, it was such a great plan! > > I have a little update: When switching to level triggered interrupts, > the problem goes away for me, as do the bus errors I get at high > frequencies. I'm working on a patch to support other interrupt types > than rising edge, which is almost done. > > I also intend to look again at the bus errors for edge driven > interrupts. Since they happen only at high frequency and go away with > level driven interrupts (which must be acked and therefore prevent > reentrancy), I suspect there's a concurrency bug. > > That said, I think the question remains: Since we can't get the FIFO > count from the hard IRQ handler, and since it could be some time before > the bottom half thread is scheduled, I don't see any way to accurately > handle forward interpolation. > > Though we can't do forward interpolation, I think at least having > backward interpolation is better than filling in blank timestamps, > especially as we haven't seen an actual case of forward interpolation > being needed, while we have real use cases that require backward > interpolation (and can be easily verified in a logic analyzer). > > However, that's only my opinion. Jonathan, Jean-Baptiste, and others, > what do you think? > Hi, What can I do to help get closure on this? Is there any data I could gather that would help make a decision? In cases of a troubled system, I think the interpolation is very useful, and it's awkward to do in userspace, as it involves keeping a history of past records, which can be inconvenient and not always accurate (e.g. if userspace doesn't read fast enough and we miss records). However, I certainly understand the concern about interpolation. Should we consider making the interpolation configurable via sysfs or device-tree? I'd be happy to hear other ideas too about better handling the case of missed interrupts. -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25/04/2018 20:06, Martin Kelly wrote: > On 04/06/2018 09:33 AM, Martin Kelly wrote: >> On 04/06/2018 08:41 AM, Jonathan Cameron wrote: >>> >>> >>> On 6 April 2018 16:21:05 BST, Jean-Baptiste Maneyrol >>> <JManeyrol@invensense.com> wrote: >>>> Hello, >>>> >>>> >>>> there is just a problem if I'm understanding well. >>>> >>>> >>>> Reading FIFO count register under hard irq handler (when taking the >>>> timestamp) is not possible since i2c access is using a mutex. That's >>>> why we are using an irq thread for reading FIFO content. >>> >>> Good point. Need more sleep or caffeine! >>> >>> >> >> I was about to reply with the same, as I started coding it up :). Too >> bad, it was such a great plan! >> >> I have a little update: When switching to level triggered interrupts, >> the problem goes away for me, as do the bus errors I get at high >> frequencies. I'm working on a patch to support other interrupt types >> than rising edge, which is almost done. >> >> I also intend to look again at the bus errors for edge driven >> interrupts. Since they happen only at high frequency and go away with >> level driven interrupts (which must be acked and therefore prevent >> reentrancy), I suspect there's a concurrency bug. >> >> That said, I think the question remains: Since we can't get the FIFO >> count from the hard IRQ handler, and since it could be some time >> before the bottom half thread is scheduled, I don't see any way to >> accurately handle forward interpolation. >> >> Though we can't do forward interpolation, I think at least having >> backward interpolation is better than filling in blank timestamps, >> especially as we haven't seen an actual case of forward interpolation >> being needed, while we have real use cases that require backward >> interpolation (and can be easily verified in a logic analyzer). >> >> However, that's only my opinion. Jonathan, Jean-Baptiste, and others, >> what do you think? >> > > Hi, > > What can I do to help get closure on this? Is there any data I could > gather that would help make a decision? > > In cases of a troubled system, I think the interpolation is very useful, > and it's awkward to do in userspace, as it involves keeping a history of > past records, which can be inconvenient and not always accurate (e.g. if > userspace doesn't read fast enough and we miss records). However, I > certainly understand the concern about interpolation. Should we consider > making the interpolation configurable via sysfs or device-tree? > > I'd be happy to hear other ideas too about better handling the case of > missed interrupts. Hello, I have implemented a new timestamp mechanism that do something similar but in a more precise way. The main ideas are: * check if interrupt timestamp is valid (computes interrupt timestamps interval and check against set period value with a margin) * use validated interrupt timestamps to measure chip internal period from the system (to have more accurate computed timestamp value) * use the interrupt timestamp for data if it is valid * if interrupt timestamp is invalid (meaning interrupt was delayed or missing), computes timestamp using the measured chip period I will send the corresponding patch series as soon as my last clean-up series has been accepted by Jonathan. Regards, JB -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/26/2018 12:35 AM, Jean-Baptiste Maneyrol wrote: > > > On 25/04/2018 20:06, Martin Kelly wrote: >> On 04/06/2018 09:33 AM, Martin Kelly wrote: >>> On 04/06/2018 08:41 AM, Jonathan Cameron wrote: >>>> >>>> >>>> On 6 April 2018 16:21:05 BST, Jean-Baptiste Maneyrol >>>> <JManeyrol@invensense.com> wrote: >>>>> Hello, >>>>> >>>>> >>>>> there is just a problem if I'm understanding well. >>>>> >>>>> >>>>> Reading FIFO count register under hard irq handler (when taking the >>>>> timestamp) is not possible since i2c access is using a mutex. That's >>>>> why we are using an irq thread for reading FIFO content. >>>> >>>> Good point. Need more sleep or caffeine! >>>> >>>> >>> >>> I was about to reply with the same, as I started coding it up :). Too >>> bad, it was such a great plan! >>> >>> I have a little update: When switching to level triggered interrupts, >>> the problem goes away for me, as do the bus errors I get at high >>> frequencies. I'm working on a patch to support other interrupt types >>> than rising edge, which is almost done. >>> >>> I also intend to look again at the bus errors for edge driven >>> interrupts. Since they happen only at high frequency and go away with >>> level driven interrupts (which must be acked and therefore prevent >>> reentrancy), I suspect there's a concurrency bug. >>> >>> That said, I think the question remains: Since we can't get the FIFO >>> count from the hard IRQ handler, and since it could be some time >>> before the bottom half thread is scheduled, I don't see any way to >>> accurately handle forward interpolation. >>> >>> Though we can't do forward interpolation, I think at least having >>> backward interpolation is better than filling in blank timestamps, >>> especially as we haven't seen an actual case of forward interpolation >>> being needed, while we have real use cases that require backward >>> interpolation (and can be easily verified in a logic analyzer). >>> >>> However, that's only my opinion. Jonathan, Jean-Baptiste, and others, >>> what do you think? >>> >> >> Hi, >> >> What can I do to help get closure on this? Is there any data I could >> gather that would help make a decision? >> >> In cases of a troubled system, I think the interpolation is very >> useful, and it's awkward to do in userspace, as it involves keeping a >> history of past records, which can be inconvenient and not always >> accurate (e.g. if userspace doesn't read fast enough and we miss >> records). However, I certainly understand the concern about >> interpolation. Should we consider making the interpolation >> configurable via sysfs or device-tree? >> >> I'd be happy to hear other ideas too about better handling the case of >> missed interrupts. > > Hello, > > I have implemented a new timestamp mechanism that do something similar > but in a more precise way. > > The main ideas are: > * check if interrupt timestamp is valid (computes interrupt timestamps > interval and check against set period value with a margin) > * use validated interrupt timestamps to measure chip internal period > from the system (to have more accurate computed timestamp value) > * use the interrupt timestamp for data if it is valid > * if interrupt timestamp is invalid (meaning interrupt was delayed or > missing), computes timestamp using the measured chip period > > I will send the corresponding patch series as soon as my last clean-up > series has been accepted by Jonathan. > > Regards, > JB Excellent, I look forward to the patches. Jonathan, are you OK with this design approach? -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 26 Apr 2018 09:46:18 -0700 Martin Kelly <mkelly@xevo.com> wrote: > On 04/26/2018 12:35 AM, Jean-Baptiste Maneyrol wrote: > > > > > > On 25/04/2018 20:06, Martin Kelly wrote: > >> On 04/06/2018 09:33 AM, Martin Kelly wrote: > >>> On 04/06/2018 08:41 AM, Jonathan Cameron wrote: > >>>> > >>>> > >>>> On 6 April 2018 16:21:05 BST, Jean-Baptiste Maneyrol > >>>> <JManeyrol@invensense.com> wrote: > >>>>> Hello, > >>>>> > >>>>> > >>>>> there is just a problem if I'm understanding well. > >>>>> > >>>>> > >>>>> Reading FIFO count register under hard irq handler (when taking the > >>>>> timestamp) is not possible since i2c access is using a mutex. That's > >>>>> why we are using an irq thread for reading FIFO content. > >>>> > >>>> Good point. Need more sleep or caffeine! > >>>> > >>>> > >>> > >>> I was about to reply with the same, as I started coding it up :). Too > >>> bad, it was such a great plan! > >>> > >>> I have a little update: When switching to level triggered interrupts, > >>> the problem goes away for me, as do the bus errors I get at high > >>> frequencies. I'm working on a patch to support other interrupt types > >>> than rising edge, which is almost done. > >>> > >>> I also intend to look again at the bus errors for edge driven > >>> interrupts. Since they happen only at high frequency and go away with > >>> level driven interrupts (which must be acked and therefore prevent > >>> reentrancy), I suspect there's a concurrency bug. > >>> > >>> That said, I think the question remains: Since we can't get the FIFO > >>> count from the hard IRQ handler, and since it could be some time > >>> before the bottom half thread is scheduled, I don't see any way to > >>> accurately handle forward interpolation. > >>> > >>> Though we can't do forward interpolation, I think at least having > >>> backward interpolation is better than filling in blank timestamps, > >>> especially as we haven't seen an actual case of forward interpolation > >>> being needed, while we have real use cases that require backward > >>> interpolation (and can be easily verified in a logic analyzer). > >>> > >>> However, that's only my opinion. Jonathan, Jean-Baptiste, and others, > >>> what do you think? > >>> > >> > >> Hi, > >> > >> What can I do to help get closure on this? Is there any data I could > >> gather that would help make a decision? > >> > >> In cases of a troubled system, I think the interpolation is very > >> useful, and it's awkward to do in userspace, as it involves keeping a > >> history of past records, which can be inconvenient and not always > >> accurate (e.g. if userspace doesn't read fast enough and we miss > >> records). However, I certainly understand the concern about > >> interpolation. Should we consider making the interpolation > >> configurable via sysfs or device-tree? > >> > >> I'd be happy to hear other ideas too about better handling the case of > >> missed interrupts. > > > > Hello, > > > > I have implemented a new timestamp mechanism that do something similar > > but in a more precise way. > > > > The main ideas are: > > * check if interrupt timestamp is valid (computes interrupt timestamps > > interval and check against set period value with a margin) > > * use validated interrupt timestamps to measure chip internal period > > from the system (to have more accurate computed timestamp value) > > * use the interrupt timestamp for data if it is valid > > * if interrupt timestamp is invalid (meaning interrupt was delayed or > > missing), computes timestamp using the measured chip period > > > > I will send the corresponding patch series as soon as my last clean-up > > series has been accepted by Jonathan. > > > > Regards, > > JB > > Excellent, I look forward to the patches. Jonathan, are you OK with this > design approach? It does sound a rather complex solution, but should be accurate. We'll see when the patches are out, but it will probably do the job given I think we have concluded we want to hide this from userspace as much as possible. Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c index 7d64be353403..4a95ff8df3b9 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c @@ -83,6 +83,7 @@ static const struct inv_mpu6050_chip_config chip_config_6050 = { .fsr = INV_MPU6050_FSR_2000DPS, .lpf = INV_MPU6050_FILTER_20HZ, .fifo_rate = INV_MPU6050_INIT_FIFO_RATE, + .fifo_period = NSEC_PER_SEC / INV_MPU6050_INIT_FIFO_RATE, .gyro_fifo_enable = false, .accl_fifo_enable = false, .accl_fs = INV_MPU6050_FS_02G, @@ -630,6 +631,7 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr, if (result) goto fifo_rate_fail_power_off; st->chip_config.fifo_rate = fifo_rate; + st->chip_config.fifo_period = NSEC_PER_SEC / fifo_rate; result = inv_mpu6050_set_lpf(st, fifo_rate); if (result) diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h index 065794162d65..3bc7d62822ca 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h @@ -86,6 +86,7 @@ enum inv_devices { * @accl_fifo_enable: enable accel data output * @gyro_fifo_enable: enable gyro data output * @fifo_rate: FIFO update rate. + * @fifo_period: FIFO update period, in nanoseconds. */ struct inv_mpu6050_chip_config { unsigned int fsr:2; @@ -94,6 +95,7 @@ struct inv_mpu6050_chip_config { unsigned int accl_fifo_enable:1; unsigned int gyro_fifo_enable:1; u16 fifo_rate; + u32 fifo_period; }; /** diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c index ff81c6aa009d..40610a316eb0 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c @@ -126,7 +126,11 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) int result; u8 data[INV_MPU6050_OUTPUT_DATA_SIZE]; u16 fifo_count; + u16 fifo_items; + s32 fifo_diff; s64 timestamp; + s64 timestamp_interp; + s64 offset; mutex_lock(&st->lock); if (!(st->chip_config.accl_fifo_enable | @@ -156,9 +160,25 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) if (fifo_count > INV_MPU6050_FIFO_THRESHOLD) goto flush_fifo; /* Timestamp mismatch. */ + fifo_items = fifo_count / bytes_per_datum; if (kfifo_len(&st->timestamps) > - fifo_count / bytes_per_datum + INV_MPU6050_TIME_STAMP_TOR) + fifo_items + INV_MPU6050_TIME_STAMP_TOR) goto flush_fifo; + + /* + * If we have more data than timestamps, the timestamps we have + * correspond to the newest items in the FIFO, since some data was + * generated without a corresponding interrupt and thus timestamp. Since + * we remove FIFO items oldest to newest, we need to interpolate the + * older timestamps based on the number of missing timestamps. + */ + fifo_diff = fifo_items - kfifo_len(&st->timestamps); + if (fifo_diff > 0) + offset = st->chip_config.fifo_period * fifo_diff; + else + offset = 0; + + timestamp_interp = 0; while (fifo_count >= bytes_per_datum) { result = regmap_bulk_read(st->map, st->reg->fifo_r_w, data, bytes_per_datum); @@ -166,9 +186,31 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) goto flush_fifo; result = kfifo_out(&st->timestamps, ×tamp, 1); - /* when there is no timestamp, put timestamp as 0 */ - if (result == 0) - timestamp = 0; + /* When there is no timestamp, interpolate based on period */ + if (result == 0) { + /* + * We have no more timestamps left, so interpolate the + * rest of them. + */ + if (likely(timestamp_interp != 0)) + timestamp_interp += st->chip_config.fifo_period; + else + /* + * In this unlikely error case, we should output + * a 0 timestamp instead of 0 + FIFO period. + */ + dev_err(regmap_get_device(st->map), + "Timestamp FIFO is empty!\n"); + timestamp = timestamp_interp; + } else { + /* + * If we are interpolating, this is an older datum with + * a newer timestamp, so offset it backwards to account + * for the time gap. Otherwise, offset will be 0. + */ + timestamp -= offset; + timestamp_interp = timestamp; + } result = iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
When interrupts are generated at a slower rate than the FIFO fills up, we will have fewer timestamps than samples. Currently, we fill in 0 for any unmatched timestamps. However, this is very confusing for userspace, which does not expect discontinuities in timestamps and must somehow work around the issue. Improve the situation by interpolating timestamps when we can't map them 1:1 to data. We do this by assuming the timestamps we have are the most recent and interpolating backwards to fill the older data. This makes sense because inv_mpu6050_read_fifo gets called right after an interrupt is generated, presumably when a datum was generated, so the most recent timestamp matches up with the most recent datum. In addition, this assumption is borne out by observation, which shows monotonically increasing timestamps when interpolating this way but discontinuities when interpolating in the other direction. Although this method is not perfectly accurate, it is probably the best we can do if we don't get one interrupt per datum. Signed-off-by: Martin Kelly <mkelly@xevo.com> --- drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 2 ++ drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 2 ++ drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 50 +++++++++++++++++++++++++++--- 3 files changed, 50 insertions(+), 4 deletions(-) v1: - Set a missing timestamp to the last timestamp we saw. v2: - Interpolate missing timestamps. v3: - Slight optimization to move timestamp_interp += into the result == 0 case.