[PATCH] iio: imu: inv_mpu6050: Support software triggers

[PATCH] iio: imu: inv_mpu6050: Support software triggers

[Doru Gucea]

In order to support software triggers (e.g.: hr-based trigger) the
operations from trigger enable/disable are moved to buffer enable/
disable.

This also allows us to remove the mutex from inv_mpu6050_read_fifo:
inv_reset_fifo can't access in parallel shared fields because the
buffers must be enabled prior to reading data.

A benefit of removing the mutex is that we avoid a deadlock at
buffer wqdisable time. The scenario was the following:
(1) at buffer disable time the indio_dev->mlock is acquired
(2) free_irq is called and waits for IRQs completion
(3) inv_mpu6050_read_fifo tries to acquire the indio_dev->mlock

Signed-off-by: Doru Gucea <doru.gucea@intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 63 ++++++++++++++++-----------
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     |  1 +
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 20 ---------
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 16 ++++---
 4 files changed, 48 insertions(+), 52 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 2258600..3ce877b 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -542,26 +542,6 @@ static ssize_t inv_attr_show(struct device *dev,
 	}
 }
 
-/**
- * inv_mpu6050_validate_trigger() - validate_trigger callback for invensense
- *                                  MPU6050 device.
- * @indio_dev: The IIO device
- * @trig: The new trigger
- *
- * Returns: 0 if the 'trig' matches the trigger registered by the MPU6050
- * device, -EINVAL otherwise.
- */
-static int inv_mpu6050_validate_trigger(struct iio_dev *indio_dev,
-					struct iio_trigger *trig)
-{
-	struct inv_mpu6050_state *st = iio_priv(indio_dev);
-
-	if (st->trig != trig)
-		return -EINVAL;
-
-	return 0;
-}
-
 #define INV_MPU6050_CHAN(_type, _channel2, _index)                    \
 	{                                                             \
 		.type = _type,                                        \
@@ -634,7 +614,35 @@ static const struct iio_info mpu_info = {
 	.write_raw = &inv_mpu6050_write_raw,
 	.write_raw_get_fmt = &inv_write_raw_get_fmt,
 	.attrs = &inv_attribute_group,
-	.validate_trigger = inv_mpu6050_validate_trigger,
+};
+
+static int mpu6050_buffer_postenable(struct iio_dev *indio_dev)
+{
+	int result;
+
+	result = inv_mpu6050_set_enable(indio_dev, true);
+
+	if (result)
+		return result;
+
+	return iio_triggered_buffer_postenable(indio_dev);
+}
+
+static int mpu6050_buffer_predisable(struct iio_dev *indio_dev)
+{
+	int result;
+
+	result = inv_mpu6050_set_enable(indio_dev, false);
+
+	if (result)
+		return result;
+
+	return iio_triggered_buffer_predisable(indio_dev);
+}
+
+const struct iio_buffer_setup_ops mpu6050_buffer_ops = {
+	.postenable = &mpu6050_buffer_postenable,
+	.predisable = &mpu6050_buffer_predisable,
 };
 
 /**
@@ -727,15 +735,18 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 	result = iio_triggered_buffer_setup(indio_dev,
 					    inv_mpu6050_irq_handler,
 					    inv_mpu6050_read_fifo,
-					    NULL);
+					    &mpu6050_buffer_ops);
 	if (result) {
 		dev_err(dev, "configure buffer fail %d\n", result);
 		return result;
 	}
-	result = inv_mpu6050_probe_trigger(indio_dev);
-	if (result) {
-		dev_err(dev, "trigger probe fail %d\n", result);
-		goto out_unreg_ring;
+
+	if (st->irq > 0) {
+		result = inv_mpu6050_probe_trigger(indio_dev);
+		if (result) {
+			dev_err(dev, "trigger probe fail %d\n", result);
+			goto out_unreg_ring;
+		}
 	}
 
 	INIT_KFIFO(st->timestamps);
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index fcc2f3d..e9055f9 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -250,6 +250,7 @@ enum inv_mpu6050_clock_sel_e {
 	NUM_CLK
 };
 
+int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable);
 irqreturn_t inv_mpu6050_irq_handler(int irq, void *p);
 irqreturn_t inv_mpu6050_read_fifo(int irq, void *p);
 int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev);
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index 1fc5fd9..3ab0033 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -39,13 +39,6 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
 	u8 d;
 	struct inv_mpu6050_state  *st = iio_priv(indio_dev);
 
-	/* disable interrupt */
-	result = regmap_write(st->map, st->reg->int_enable, 0);
-	if (result) {
-		dev_err(regmap_get_device(st->map), "int_enable failed %d\n",
-			result);
-		return result;
-	}
 	/* disable the sensor output to FIFO */
 	result = regmap_write(st->map, st->reg->fifo_en, 0);
 	if (result)
@@ -64,14 +57,6 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
 	/* clear timestamps fifo */
 	inv_clear_kfifo(st);
 
-	/* enable interrupt */
-	if (st->chip_config.accl_fifo_enable ||
-	    st->chip_config.gyro_fifo_enable) {
-		result = regmap_write(st->map, st->reg->int_enable,
-					INV_MPU6050_BIT_DATA_RDY_EN);
-		if (result)
-			return result;
-	}
 	/* enable FIFO reading and I2C master interface*/
 	result = regmap_write(st->map, st->reg->user_ctrl,
 					INV_MPU6050_BIT_FIFO_EN);
@@ -91,8 +76,6 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
 
 reset_fifo_fail:
 	dev_err(regmap_get_device(st->map), "reset fifo failed %d\n", result);
-	result = regmap_write(st->map, st->reg->int_enable,
-					INV_MPU6050_BIT_DATA_RDY_EN);
 
 	return result;
 }
@@ -128,7 +111,6 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 	u16 fifo_count;
 	s64 timestamp;
 
-	mutex_lock(&indio_dev->mlock);
 	if (!(st->chip_config.accl_fifo_enable |
 		st->chip_config.gyro_fifo_enable))
 		goto end_session;
@@ -179,7 +161,6 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 	}
 
 end_session:
-	mutex_unlock(&indio_dev->mlock);
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
@@ -187,7 +168,6 @@ end_session:
 flush_fifo:
 	/* Flush HW and SW FIFOs. */
 	inv_reset_fifo(indio_dev);
-	mutex_unlock(&indio_dev->mlock);
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index 72d6aae..57f6623 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -39,7 +39,7 @@ static void inv_scan_query(struct iio_dev *indio_dev)
  *  @indio_dev:	Device driver instance.
  *  @enable: enable/disable
  */
-static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
+int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
 {
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
 	int result;
@@ -69,10 +69,6 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
 		if (result)
 			return result;
 
-		result = regmap_write(st->map, st->reg->int_enable, 0);
-		if (result)
-			return result;
-
 		result = regmap_write(st->map, st->reg->user_ctrl, 0);
 		if (result)
 			return result;
@@ -103,7 +99,15 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
 static int inv_mpu_data_rdy_trigger_set_state(struct iio_trigger *trig,
 						bool state)
 {
-	return inv_mpu6050_set_enable(iio_trigger_get_drvdata(trig), state);
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct inv_mpu6050_state *st = iio_priv(indio_dev);
+
+	if (state && (st->chip_config.accl_fifo_enable ||
+	    st->chip_config.gyro_fifo_enable))
+		return regmap_write(st->map, st->reg->int_enable,
+				    INV_MPU6050_BIT_DATA_RDY_EN);
+	else
+		return regmap_write(st->map, st->reg->int_enable, 0);
 }
 
 static const struct iio_trigger_ops inv_mpu_trigger_ops = {
-- 
1.9.1

Re: [PATCH] iio: imu: inv_mpu6050: Support software triggers

[Crestez Dan Leonard]

On 05/26/2016 03:23 PM, Doru Gucea wrote:
> In order to support software triggers (e.g.: hr-based trigger) the
> operations from trigger enable/disable are moved to buffer enable/
> disable.
> 
> This also allows us to remove the mutex from inv_mpu6050_read_fifo:
> inv_reset_fifo can't access in parallel shared fields because the
> buffers must be enabled prior to reading data.
>
> A benefit of removing the mutex is that we avoid a deadlock at
> buffer wqdisable time. The scenario was the following:
> (1) at buffer disable time the indio_dev->mlock is acquired
> (2) free_irq is called and waits for IRQs completion
> (3) inv_mpu6050_read_fifo tries to acquire the indio_dev->mlock

I'm not sure this is safe, it seems to me that inv_reset_fifo assumes
mlock is taken and this needs to be assured. I think properly removing
the irq disabling deadlock require either using something other than
mlock or not calling inv_reset_fifo from the interrupt at all. The
latter seems preferable to me. But this issue should be handled
separately from software trigger support.

Another issue is that the fifo is filled at the sampling frequency
configured in device registers and this can be different from the
sampling frequency configured on the software trigger. It seems to me
that the user needs to manually ensure that they are identical.

Even if they are identical the timer ticks might not match perfectly.
When reading from the fifo all available samples are handled.

If device frequency is set too high you will push multiple samples to
userspace, all but one of them with bad timestamps. It's also possible
to find zero samples if device frequency is set higher than timer frequency.

-- 
Regards,
Leonard

Re: [PATCH] iio: imu: inv_mpu6050: Support software triggers

[Gucea Doru]

On Fri, May 27, 2016 at 6:15 PM, Crestez Dan Leonard
<leonard.crestez@intel.com> wrote:
> On 05/26/2016 03:23 PM, Doru Gucea wrote:
>> In order to support software triggers (e.g.: hr-based trigger) the
>> operations from trigger enable/disable are moved to buffer enable/
>> disable.
>>
>> This also allows us to remove the mutex from inv_mpu6050_read_fifo:
>> inv_reset_fifo can't access in parallel shared fields because the
>> buffers must be enabled prior to reading data.
>>
>> A benefit of removing the mutex is that we avoid a deadlock at
>> buffer wqdisable time. The scenario was the following:
>> (1) at buffer disable time the indio_dev->mlock is acquired
>> (2) free_irq is called and waits for IRQs completion
>> (3) inv_mpu6050_read_fifo tries to acquire the indio_dev->mlock
>
> I'm not sure this is safe, it seems to me that inv_reset_fifo assumes
> mlock is taken and this needs to be assured. I think properly removing
> the irq disabling deadlock require either using something other than
> mlock or not calling inv_reset_fifo from the interrupt at all. The
> latter seems preferable to me. But this issue should be handled
> separately from software trigger support.
>

inv_reset_fifo still needs to be called: it might happen that we miss a
DATA_RDY interrupt in which case we'll have a mismatch between
time-stamps and bytes from the FIFO queues. The only way to recover
from such a situation is to flush the FIFO's and restart the interrupt.

Regarding the locking, before my patch, inv_reset_fifo could flush the
FIFO's (see inv_mpu_data_rdy_trigger_set_state) while
inv_mpu6050_read_fifo was reading data and the role of the mlock was
to avoid this situation. This situation is not possible anymore because
the operations from trigger enable/disable were moved to buffer
enable/disable: trigger state can't be changed while buffers are enabled.
Do you see another scenario?

I'll split the software trigger support and deadlock solution in two patches.

> Another issue is that the fifo is filled at the sampling frequency
> configured in device registers and this can be different from the
> sampling frequency configured on the software trigger. It seems to me
> that the user needs to manually ensure that they are identical.
>

A solution would be to hook the iio_hrtimer_info_store_sampling_frequency
method of the hrtimer for setting the sampling frequency together with
the timer frequency. I think this will require changes to the hrtimer for
exposing the above function to drivers. Daniel, what would be the right
approach?

> Even if they are identical the timer ticks might not match perfectly.
> When reading from the fifo all available samples are handled.
>
> If device frequency is set too high you will push multiple samples to
> userspace, all but one of them with bad timestamps. It's also possible
> to find zero samples if device frequency is set higher than timer frequency.
>

In case of timer ticks mismatch there will also be a mismatch between
time-stamps and bytes from the FIFO queues. I wonder if a clean recovery
from such a situation would be to call inv_reset_fifo together with resetting
the hrtimer.

> --
> Regards,
> Leonard
> --
> 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

Regards,
Doru

Re: [PATCH] iio: imu: inv_mpu6050: Support software triggers

[Jonathan Cameron]

On 28/05/16 09:32, Gucea Doru wrote:
> On Fri, May 27, 2016 at 6:15 PM, Crestez Dan Leonard
> <leonard.crestez@intel.com> wrote:
>> On 05/26/2016 03:23 PM, Doru Gucea wrote:
>>> In order to support software triggers (e.g.: hr-based trigger) the
>>> operations from trigger enable/disable are moved to buffer enable/
>>> disable.
>>>
>>> This also allows us to remove the mutex from inv_mpu6050_read_fifo:
>>> inv_reset_fifo can't access in parallel shared fields because the
>>> buffers must be enabled prior to reading data.
>>>
>>> A benefit of removing the mutex is that we avoid a deadlock at
>>> buffer wqdisable time. The scenario was the following:
>>> (1) at buffer disable time the indio_dev->mlock is acquired
>>> (2) free_irq is called and waits for IRQs completion
>>> (3) inv_mpu6050_read_fifo tries to acquire the indio_dev->mlock
>>
>> I'm not sure this is safe, it seems to me that inv_reset_fifo assumes
>> mlock is taken and this needs to be assured. I think properly removing
>> the irq disabling deadlock require either using something other than
>> mlock or not calling inv_reset_fifo from the interrupt at all. The
>> latter seems preferable to me. But this issue should be handled
>> separately from software trigger support.
>>
> 
> inv_reset_fifo still needs to be called: it might happen that we miss a
> DATA_RDY interrupt in which case we'll have a mismatch between
> time-stamps and bytes from the FIFO queues. The only way to recover
> from such a situation is to flush the FIFO's and restart the interrupt.
> 
> Regarding the locking, before my patch, inv_reset_fifo could flush the
> FIFO's (see inv_mpu_data_rdy_trigger_set_state) while
> inv_mpu6050_read_fifo was reading data and the role of the mlock was
> to avoid this situation. This situation is not possible anymore because
> the operations from trigger enable/disable were moved to buffer
> enable/disable: trigger state can't be changed while buffers are enabled.
> Do you see another scenario?
> 
> I'll split the software trigger support and deadlock solution in two patches.
> 
>> Another issue is that the fifo is filled at the sampling frequency
>> configured in device registers and this can be different from the
>> sampling frequency configured on the software trigger. It seems to me
>> that the user needs to manually ensure that they are identical.
>>
> 
> A solution would be to hook the iio_hrtimer_info_store_sampling_frequency
> method of the hrtimer for setting the sampling frequency together with
> the timer frequency. I think this will require changes to the hrtimer for
> exposing the above function to drivers. Daniel, what would be the right
> approach?
Back to basic questions from me ;)

Why do you want to use a software trigger here at all?  Trying to align the two
sampling frequencies is really a non starter - it'll never work however hard
you try.  So I am guessing you want to support sampling when the interrupts
are not wired up?  If so the only way to handle that is to use a thread
to sample the interrupt status register fast enough to get a reasonable idea
of when it changed... basically fake the interrupt wire.

If you want to be able to run this device on say a much slower clock then we
need to look at how to bypass the fifo entirely (read from the relevant sensor
registers instead I think...)






> 
>> Even if they are identical the timer ticks might not match perfectly.
>> When reading from the fifo all available samples are handled.
>>
>> If device frequency is set too high you will push multiple samples to
>> userspace, all but one of them with bad timestamps. It's also possible
>> to find zero samples if device frequency is set higher than timer frequency.
>>
> 
> In case of timer ticks mismatch there will also be a mismatch between
> time-stamps and bytes from the FIFO queues. I wonder if a clean recovery
> from such a situation would be to call inv_reset_fifo together with resetting
> the hrtimer.
This is going to happen frequently  - how frequently will depend on the drift
between the two clocks.  Not a lot of point of having a high frequency sampling
system that resets every few seconds dropping some samples on the floor.

Jonathan
> 
>> --
>> Regards,
>> Leonard
>> --
>> 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
> 
> Regards,
> Doru
> 

Re: [PATCH] iio: imu: inv_mpu6050: Support software triggers

[Gucea Doru]

On Sun, May 29, 2016 at 7:14 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 28/05/16 09:32, Gucea Doru wrote:
>> On Fri, May 27, 2016 at 6:15 PM, Crestez Dan Leonard
>> <leonard.crestez@intel.com> wrote:
>>> On 05/26/2016 03:23 PM, Doru Gucea wrote:
>>>> In order to support software triggers (e.g.: hr-based trigger) the
>>>> operations from trigger enable/disable are moved to buffer enable/
>>>> disable.
>>>>
>>>> This also allows us to remove the mutex from inv_mpu6050_read_fifo:
>>>> inv_reset_fifo can't access in parallel shared fields because the
>>>> buffers must be enabled prior to reading data.
>>>>
>>>> A benefit of removing the mutex is that we avoid a deadlock at
>>>> buffer wqdisable time. The scenario was the following:
>>>> (1) at buffer disable time the indio_dev->mlock is acquired
>>>> (2) free_irq is called and waits for IRQs completion
>>>> (3) inv_mpu6050_read_fifo tries to acquire the indio_dev->mlock
>>>
>>> I'm not sure this is safe, it seems to me that inv_reset_fifo assumes
>>> mlock is taken and this needs to be assured. I think properly removing
>>> the irq disabling deadlock require either using something other than
>>> mlock or not calling inv_reset_fifo from the interrupt at all. The
>>> latter seems preferable to me. But this issue should be handled
>>> separately from software trigger support.
>>>
>>
>> inv_reset_fifo still needs to be called: it might happen that we miss a
>> DATA_RDY interrupt in which case we'll have a mismatch between
>> time-stamps and bytes from the FIFO queues. The only way to recover
>> from such a situation is to flush the FIFO's and restart the interrupt.
>>
>> Regarding the locking, before my patch, inv_reset_fifo could flush the
>> FIFO's (see inv_mpu_data_rdy_trigger_set_state) while
>> inv_mpu6050_read_fifo was reading data and the role of the mlock was
>> to avoid this situation. This situation is not possible anymore because
>> the operations from trigger enable/disable were moved to buffer
>> enable/disable: trigger state can't be changed while buffers are enabled.
>> Do you see another scenario?
>>
>> I'll split the software trigger support and deadlock solution in two patches.
>>
>>> Another issue is that the fifo is filled at the sampling frequency
>>> configured in device registers and this can be different from the
>>> sampling frequency configured on the software trigger. It seems to me
>>> that the user needs to manually ensure that they are identical.
>>>
>>
>> A solution would be to hook the iio_hrtimer_info_store_sampling_frequency
>> method of the hrtimer for setting the sampling frequency together with
>> the timer frequency. I think this will require changes to the hrtimer for
>> exposing the above function to drivers. Daniel, what would be the right
>> approach?
> Back to basic questions from me ;)
>
> Why do you want to use a software trigger here at all?  Trying to align the two
> sampling frequencies is really a non starter - it'll never work however hard
> you try.  So I am guessing you want to support sampling when the interrupts
> are not wired up?  If so the only way to handle that is to use a thread
> to sample the interrupt status register fast enough to get a reasonable idea
> of when it changed... basically fake the interrupt wire.
>

Yes, I was trying to use a software trigger because I have a chip without GPIO
support. I followed the model from BMC150 accel that allows installing
a software
trigger even if there is no interrupt line connected. At this moment,
the probe for
MPU60X0 fails if there is no interrupt line connected.

Another reason for using a sw trigger was for masking the USB latency because
the sensor is connected to an USB to SPI adapter while a hardware
interrupt would
have eaten some bandwidth.

Sampling once the interrupt status register will need one USB TX operations and
one USB RX operation. This is expensive compared to a hardware
interrupt that will
need only a USB RX operation. However, I could send a rework for this patch that
uses a thread for sampling the intr register if you consider that
someone needs it.

> If you want to be able to run this device on say a much slower clock then we
> need to look at how to bypass the fifo entirely (read from the relevant sensor
> registers instead I think...)
>
>
>
>
>
>
>>
>>> Even if they are identical the timer ticks might not match perfectly.
>>> When reading from the fifo all available samples are handled.
>>>
>>> If device frequency is set too high you will push multiple samples to
>>> userspace, all but one of them with bad timestamps. It's also possible
>>> to find zero samples if device frequency is set higher than timer frequency.
>>>
>>
>> In case of timer ticks mismatch there will also be a mismatch between
>> time-stamps and bytes from the FIFO queues. I wonder if a clean recovery
>> from such a situation would be to call inv_reset_fifo together with resetting
>> the hrtimer.
> This is going to happen frequently  - how frequently will depend on the drift
> between the two clocks.  Not a lot of point of having a high frequency sampling
> system that resets every few seconds dropping some samples on the floor.
>
> Jonathan
>>
>>> --
>>> Regards,
>>> Leonard
>>> --
>>> 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
>>
>> Regards,
>> Doru
>>
>

Re: [PATCH] iio: imu: inv_mpu6050: Support software triggers

[Jonathan Cameron]

On 02/06/16 08:55, Gucea Doru wrote:
> On Sun, May 29, 2016 at 7:14 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 28/05/16 09:32, Gucea Doru wrote:
>>> On Fri, May 27, 2016 at 6:15 PM, Crestez Dan Leonard
>>> <leonard.crestez@intel.com> wrote:
>>>> On 05/26/2016 03:23 PM, Doru Gucea wrote:
>>>>> In order to support software triggers (e.g.: hr-based trigger) the
>>>>> operations from trigger enable/disable are moved to buffer enable/
>>>>> disable.
>>>>>
>>>>> This also allows us to remove the mutex from inv_mpu6050_read_fifo:
>>>>> inv_reset_fifo can't access in parallel shared fields because the
>>>>> buffers must be enabled prior to reading data.
>>>>>
>>>>> A benefit of removing the mutex is that we avoid a deadlock at
>>>>> buffer wqdisable time. The scenario was the following:
>>>>> (1) at buffer disable time the indio_dev->mlock is acquired
>>>>> (2) free_irq is called and waits for IRQs completion
>>>>> (3) inv_mpu6050_read_fifo tries to acquire the indio_dev->mlock
>>>>
>>>> I'm not sure this is safe, it seems to me that inv_reset_fifo assumes
>>>> mlock is taken and this needs to be assured. I think properly removing
>>>> the irq disabling deadlock require either using something other than
>>>> mlock or not calling inv_reset_fifo from the interrupt at all. The
>>>> latter seems preferable to me. But this issue should be handled
>>>> separately from software trigger support.
>>>>
>>>
>>> inv_reset_fifo still needs to be called: it might happen that we miss a
>>> DATA_RDY interrupt in which case we'll have a mismatch between
>>> time-stamps and bytes from the FIFO queues. The only way to recover
>>> from such a situation is to flush the FIFO's and restart the interrupt.
>>>
>>> Regarding the locking, before my patch, inv_reset_fifo could flush the
>>> FIFO's (see inv_mpu_data_rdy_trigger_set_state) while
>>> inv_mpu6050_read_fifo was reading data and the role of the mlock was
>>> to avoid this situation. This situation is not possible anymore because
>>> the operations from trigger enable/disable were moved to buffer
>>> enable/disable: trigger state can't be changed while buffers are enabled.
>>> Do you see another scenario?
>>>
>>> I'll split the software trigger support and deadlock solution in two patches.
>>>
>>>> Another issue is that the fifo is filled at the sampling frequency
>>>> configured in device registers and this can be different from the
>>>> sampling frequency configured on the software trigger. It seems to me
>>>> that the user needs to manually ensure that they are identical.
>>>>
>>>
>>> A solution would be to hook the iio_hrtimer_info_store_sampling_frequency
>>> method of the hrtimer for setting the sampling frequency together with
>>> the timer frequency. I think this will require changes to the hrtimer for
>>> exposing the above function to drivers. Daniel, what would be the right
>>> approach?
>> Back to basic questions from me ;)
>>
>> Why do you want to use a software trigger here at all?  Trying to align the two
>> sampling frequencies is really a non starter - it'll never work however hard
>> you try.  So I am guessing you want to support sampling when the interrupts
>> are not wired up?  If so the only way to handle that is to use a thread
>> to sample the interrupt status register fast enough to get a reasonable idea
>> of when it changed... basically fake the interrupt wire.
>>
> 
> Yes, I was trying to use a software trigger because I have a chip without GPIO
> support. I followed the model from BMC150 accel that allows installing
> a software
> trigger even if there is no interrupt line connected. At this moment,
> the probe for
> MPU60X0 fails if there is no interrupt line connected.
> 
> Another reason for using a sw trigger was for masking the USB latency because
> the sensor is connected to an USB to SPI adapter while a hardware
> interrupt would
> have eaten some bandwidth.
> 
> Sampling once the interrupt status register will need one USB TX operations and
> one USB RX operation. This is expensive compared to a hardware
> interrupt that will
> need only a USB RX operation. However, I could send a rework for this patch that
> uses a thread for sampling the intr register if you consider that
> someone needs it.
Sadly there is never a good way to handle this problem - but yes I think
the best we can do is the threaded sampling of that register.  We
did have one driver that allowed a custom overriding to do 'best guess'
timing. So it defaulted to the right thing, but if the user explicitly
overrode it, the driver would start sampling at the supposed sampling
frequency.

Can I remember which one it was... err. Sorry nope..  Not entirely sure
it even merged.

Jonathan
> 
>> If you want to be able to run this device on say a much slower clock then we
>> need to look at how to bypass the fifo entirely (read from the relevant sensor
>> registers instead I think...)
>>
>>
>>
>>
>>
>>
>>>
>>>> Even if they are identical the timer ticks might not match perfectly.
>>>> When reading from the fifo all available samples are handled.
>>>>
>>>> If device frequency is set too high you will push multiple samples to
>>>> userspace, all but one of them with bad timestamps. It's also possible
>>>> to find zero samples if device frequency is set higher than timer frequency.
>>>>
>>>
>>> In case of timer ticks mismatch there will also be a mismatch between
>>> time-stamps and bytes from the FIFO queues. I wonder if a clean recovery
>>> from such a situation would be to call inv_reset_fifo together with resetting
>>> the hrtimer.
>> This is going to happen frequently  - how frequently will depend on the drift
>> between the two clocks.  Not a lot of point of having a high frequency sampling
>> system that resets every few seconds dropping some samples on the floor.
>>
>> Jonathan
>>>
>>>> --
>>>> Regards,
>>>> Leonard
>>>> --
>>>> 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
>>>
>>> Regards,
>>> Doru
>>>
>>