| Message ID | 3b0b5ed8-f224-e589-ce60-e1871e76b25a@invensense.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/12/2018 08:01 AM, Jean-Baptiste Maneyrol wrote: > Hello, > > I've done further investigations on this issue, since I wasn't > understanding why we were loosing interrupts where we shouldn't. And I > think I found the root cause. > > The irq handlers top and bottom are written in a way considering that > every interrupt will be handled by the top part in the hard irq handler, > even if the irq thread is still running in the bottom part. That's why > we are using a FIFO for storing multiple timestamps. > > But by using triggered-buffer setup, we are using the ONESHOT irq mode > that is disabling irq until the thread processing is done. In this way, > we are never reliably catching data interrupts when the thread is > running, and there is no need to have a FIFO for storing timestamps. > > The correct way would be to have the top part in the hard irq handler > without using ONESHOT mode, and use only the thread for reading FIFO > data in the triggered-buffer interrupt. > > I have done a patch which is working correctly on my side. But since I > am not able to trigger the issue in the first time, I cannot guarantee > this is fixing the problem. > > Martin, > is it possible for you to test the following patch and tell me if it is > working better like that? > > For sure, there is a conception issue here, because in the current setup > having a FIFO for storing timestamps is completely useless. > > JB > That's a good point, and sounds like a good change to me. Unfortunately it does not fix the issue I'm seeing, although I'm guessing it would improve it at high frequencies (though that's trickier to measure). My issue is happening even at 10 Hz, in which the thread has ample time to complete before the next interrupt, so I think it's something related to the interrupt controller rather than this driver in particular. -- 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, I am now able to reproduce the issue by generating kernel irq (just plug/unplug mouse or keyboard is sufficient). My last modification doesn't solve anything, since event the hard irq handler is disabled when processing another interrupt. Having a latched interrupt and triggering by level is a workaround, but clearly not perfect. I think we need to work out a new solution for timestamping correctly the data. JB On 12/04/2018 20:16, Martin Kelly wrote: > On 04/12/2018 08:01 AM, Jean-Baptiste Maneyrol wrote: >> Hello, >> >> I've done further investigations on this issue, since I wasn't >> understanding why we were loosing interrupts where we shouldn't. And I >> think I found the root cause. >> >> The irq handlers top and bottom are written in a way considering that >> every interrupt will be handled by the top part in the hard irq >> handler, even if the irq thread is still running in the bottom part. >> That's why we are using a FIFO for storing multiple timestamps. >> >> But by using triggered-buffer setup, we are using the ONESHOT irq mode >> that is disabling irq until the thread processing is done. In this >> way, we are never reliably catching data interrupts when the thread is >> running, and there is no need to have a FIFO for storing timestamps. >> >> The correct way would be to have the top part in the hard irq handler >> without using ONESHOT mode, and use only the thread for reading FIFO >> data in the triggered-buffer interrupt. >> >> I have done a patch which is working correctly on my side. But since I >> am not able to trigger the issue in the first time, I cannot guarantee >> this is fixing the problem. >> >> Martin, >> is it possible for you to test the following patch and tell me if it >> is working better like that? >> >> For sure, there is a conception issue here, because in the current >> setup having a FIFO for storing timestamps is completely useless. >> >> JB >> > > That's a good point, and sounds like a good change to me. Unfortunately > it does not fix the issue I'm seeing, although I'm guessing it would > improve it at high frequencies (though that's trickier to measure). > > My issue is happening even at 10 Hz, in which the thread has ample time > to complete before the next interrupt, so I think it's something related > to the interrupt controller rather than this driver in particular. -- 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/13/2018 02:25 AM, Jean-Baptiste Maneyrol wrote: > Hello, > > I am now able to reproduce the issue by generating kernel irq (just > plug/unplug mouse or keyboard is sufficient). > > My last modification doesn't solve anything, since event the hard irq > handler is disabled when processing another interrupt. Having a latched > interrupt and triggering by level is a workaround, but clearly not perfect. > I'm glad you can reproduce the issue now! What board are you using? My issues have been with the nanopi neo air (based on the Allwinner H3 SoC). > I think we need to work out a new solution for timestamping correctly > the data. > > JB > > I think we should try to solve the missing interrupts issue. Without solving it, the best we can do is interpolate, as I do in the other patch. That said, I think supporting more interrupt types and interpolating are good ideas regardless. Supporting more interrupt types is useful for integrating with more types of hardware, and interpolating is useful for being more robust against systems having issues, which can unexpectedly happen even if we fix the immediate issue we see here. -- 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 Fri, 13 Apr 2018 09:19:41 -0700 Martin Kelly <mkelly@xevo.com> wrote: > On 04/13/2018 02:25 AM, Jean-Baptiste Maneyrol wrote: > > Hello, > > > > I am now able to reproduce the issue by generating kernel irq (just > > plug/unplug mouse or keyboard is sufficient). > > > > My last modification doesn't solve anything, since event the hard irq > > handler is disabled when processing another interrupt. Having a latched > > interrupt and triggering by level is a workaround, but clearly not perfect. > > > > > I'm glad you can reproduce the issue now! What board are you using? My > issues have been with the nanopi neo air (based on the Allwinner H3 SoC). > > > I think we need to work out a new solution for timestamping correctly > > the data. > > > > JB > > > > > I think we should try to solve the missing interrupts issue. Without > solving it, the best we can do is interpolate, as I do in the other patch. > > That said, I think supporting more interrupt types and interpolating are > good ideas regardless. Supporting more interrupt types is useful for > integrating with more types of hardware, and interpolating is useful for > being more robust against systems having issues, which can unexpectedly > happen even if we fix the immediate issue we see here. Agreed on the interrupt types. Interpolating is till rather 'nasty' so the case is a little less clear, but I'm being convinced I think... Anyhow, looking forward to v6 Will want JB reviewed-by or acked-by on this one! 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 7d64be3..ed883ef 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c @@ -941,7 +941,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name, indio_dev->modes = INDIO_BUFFER_TRIGGERED; result = iio_triggered_buffer_setup(indio_dev, - inv_mpu6050_irq_handler, + NULL, inv_mpu6050_read_fifo, NULL); if (result) { diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c index ff81c6a..6935815 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c @@ -102,8 +102,8 @@ int inv_reset_fifo(struct iio_dev *indio_dev) */ irqreturn_t inv_mpu6050_irq_handler(int irq, void *p) { - struct iio_poll_func *pf = p; - struct iio_dev *indio_dev = pf->indio_dev; + struct iio_trigger *trig = p; + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); struct inv_mpu6050_state *st = iio_priv(indio_dev); s64 timestamp; @@ -111,7 +111,7 @@ irqreturn_t inv_mpu6050_irq_handler(int irq, void *p) kfifo_in_spinlocked(&st->timestamps, ×tamp, 1, &st->time_stamp_lock); - return IRQ_WAKE_THREAD; + return iio_trigger_generic_data_rdy_poll(irq, trig); } /** diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c index f963f9f..1cb9ed7 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c @@ -130,7 +130,7 @@ int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev) return -ENOMEM; ret = devm_request_irq(&indio_dev->dev, st->irq, - &iio_trigger_generic_data_rdy_poll, + &inv_mpu6050_irq_handler, IRQF_TRIGGER_RISING, "inv_mpu", st->trig);
Hello, I've done further investigations on this issue, since I wasn't understanding why we were loosing interrupts where we shouldn't. And I think I found the root cause. The irq handlers top and bottom are written in a way considering that every interrupt will be handled by the top part in the hard irq handler, even if the irq thread is still running in the bottom part. That's why we are using a FIFO for storing multiple timestamps. But by using triggered-buffer setup, we are using the ONESHOT irq mode that is disabling irq until the thread processing is done. In this way, we are never reliably catching data interrupts when the thread is running, and there is no need to have a FIFO for storing timestamps. The correct way would be to have the top part in the hard irq handler without using ONESHOT mode, and use only the thread for reading FIFO data in the triggered-buffer interrupt. I have done a patch which is working correctly on my side. But since I am not able to trigger the issue in the first time, I cannot guarantee this is fixing the problem. Martin, is it possible for you to test the following patch and tell me if it is working better like that? For sure, there is a conception issue here, because in the current setup having a FIFO for storing timestamps is completely useless. JB Subject: [PATCH] iio: imu: inv_mpu6050: use top irq handler in the hard irq handler Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> --- drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 2 +- drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 6 +++--- drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-)