[RFC PATCH 0/3] allow better control for flushing the hardware fifo

[RFC PATCH 0/3] allow better control for flushing the hardware fifo

[Octavian Purdila]

Hi Jonathan,

The first patch is a small enhancement that makes it easier to add
hwfifo support in drivers.

The other two are adding new ABIs to allow on demand trigger of a
hardware fifo flush operation and to signal when the flush operation
has been completed. The user for this interface is Android's sensor
HAL.

Thanks,
Tavi

Octavian Purdila (3):
  iio: add hwfifo attributes helpers
  iio: allow better control for flushing the hardware fifo
  iio: accel: bmc150: add support for hwfifo_flush and flush events

 Documentation/ABI/testing/sysfs-bus-iio | 11 +++++
 drivers/iio/accel/bmc150-accel.c        | 71 +++++++++++++++++++++++----------
 include/linux/iio/sysfs.h               | 15 +++++++
 include/uapi/linux/iio/types.h          |  1 +
 4 files changed, 78 insertions(+), 20 deletions(-)

-- 
1.9.1

[RFC PATCH 1/3] iio: add hwfifo attributes helpers

[Octavian Purdila]

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/iio/accel/bmc150-accel.c | 11 ++++-------
 include/linux/iio/sysfs.h        | 12 ++++++++++++
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index 73e8773..b4ca361 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -870,13 +870,10 @@ static ssize_t bmc150_accel_get_fifo_state(struct device *dev,
 	return sprintf(buf, "%d\n", state);
 }
 
-static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
-static IIO_CONST_ATTR(hwfifo_watermark_max,
-		      __stringify(BMC150_ACCEL_FIFO_LENGTH));
-static IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO,
-		       bmc150_accel_get_fifo_state, NULL, 0);
-static IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO,
-		       bmc150_accel_get_fifo_watermark, NULL, 0);
+static IIO_CONST_ATTR_HWFIFO_WATERMARK_MIN(1);
+static IIO_CONST_ATTR_HWFIFO_WATERMARK_MAX(BMC150_ACCEL_FIFO_LENGTH);
+static IIO_DEV_ATTR_HWFIFO_ENABLED(bmc150_accel_get_fifo_state);
+static IIO_DEV_ATTR_HWFIFO_WATERMARK(bmc150_accel_get_fifo_watermark);
 
 static const struct attribute *bmc150_accel_fifo_attributes[] = {
 	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
index 8a1d186..8822bab 100644
--- a/include/linux/iio/sysfs.h
+++ b/include/linux/iio/sysfs.h
@@ -124,4 +124,16 @@ struct iio_const_attr {
 #define IIO_CONST_ATTR_TEMP_SCALE(_string)		\
 	IIO_CONST_ATTR(in_temp_scale, _string)
 
+#define IIO_CONST_ATTR_HWFIFO_WATERMARK_MIN(_min)	\
+	IIO_CONST_ATTR(hwfifo_watermark_min, __stringify(_min))
+
+#define IIO_CONST_ATTR_HWFIFO_WATERMARK_MAX(_max)	\
+	IIO_CONST_ATTR(hwfifo_watermark_max, __stringify(_max))
+
+#define IIO_DEV_ATTR_HWFIFO_ENABLED(_hwfifo_get_state)	\
+	IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO, _hwfifo_get_state, NULL, 0)
+
+#define IIO_DEV_ATTR_HWFIFO_WATERMARK(_hwfifo_get_wm)	\
+	IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO, _hwfifo_get_wm, NULL, 0)
+
 #endif /* _INDUSTRIAL_IO_SYSFS_H_ */
-- 
1.9.1

[RFC PATCH 2/3] iio: allow better control for flushing the hardware fifo

[Octavian Purdila]

Some applications need to be able to flush [1] the hardware fifo of
the device and to receive events of when that happened [2] so that it
can ignore stale data.

This patch adds a new event (IIO_EV_TYPE_HWFIFO_FLUSHED) that should
be sent to userspace when a flush has been completed. The application
will be able to identify which are the samples to ignore based on the
timestamp of the event.

To allow applications to accurately generate a hardware fifo flush on
demand, this patch also adds a new sysfs entry that triggers a
hardware fifo flush when written to.

[1] https://source.android.com/devices/sensors/hal-interface.html#flush_sensor
[2] https://source.android.com/devices/sensors/hal-interface.html#metadata_flush_complete_events

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 11 +++++++++++
 include/linux/iio/sysfs.h               |  3 +++
 include/uapi/linux/iio/types.h          |  1 +
 3 files changed, 15 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 866b4ec..bb4d8de 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1375,3 +1375,14 @@ Description:
 		The emissivity ratio of the surface in the field of view of the
 		contactless temperature sensor.  Emissivity varies from 0 to 1,
 		with 1 being the emissivity of a black body.
+
+What:		/sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_flush
+KernelVersion:	4.2
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Write only entry that accepts a single strictly positive integer
+		specifying the number of samples to flush from the hardware fifo
+		to the device buffer. When the flush is completed an
+		IIO_EV_TYPE_HWFIFO_FLUSHED event is generated. The event has the
+		timestamp equal with the timestamp of last sample that was
+		flushed from the hardware fifo.
diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
index 8822bab..8fcc25b 100644
--- a/include/linux/iio/sysfs.h
+++ b/include/linux/iio/sysfs.h
@@ -136,4 +136,7 @@ struct iio_const_attr {
 #define IIO_DEV_ATTR_HWFIFO_WATERMARK(_hwfifo_get_wm)	\
 	IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO, _hwfifo_get_wm, NULL, 0)
 
+#define IIO_DEV_ATTR_HWFIFO_FLUSH(_hwfifo_flush)	\
+	IIO_DEVICE_ATTR(hwfifo_flush, S_IWUSR, NULL, _hwfifo_flush, 0)
+
 #endif /* _INDUSTRIAL_IO_SYSFS_H_ */
diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
index 5c46019..4600157 100644
--- a/include/uapi/linux/iio/types.h
+++ b/include/uapi/linux/iio/types.h
@@ -79,6 +79,7 @@ enum iio_event_type {
 	IIO_EV_TYPE_THRESH_ADAPTIVE,
 	IIO_EV_TYPE_MAG_ADAPTIVE,
 	IIO_EV_TYPE_CHANGE,
+	IIO_EV_TYPE_HWFIFO_FLUSHED,
 };
 
 enum iio_event_direction {
-- 
1.9.1

[RFC PATCH 3/3] iio: accel: bmc150: add support for hwfifo_flush and flush events

[Octavian Purdila]

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/iio/accel/bmc150-accel.c | 68 ++++++++++++++++++++++++++++++----------
 1 file changed, 51 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index b4ca361..0c5fdf6 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -870,19 +870,6 @@ static ssize_t bmc150_accel_get_fifo_state(struct device *dev,
 	return sprintf(buf, "%d\n", state);
 }
 
-static IIO_CONST_ATTR_HWFIFO_WATERMARK_MIN(1);
-static IIO_CONST_ATTR_HWFIFO_WATERMARK_MAX(BMC150_ACCEL_FIFO_LENGTH);
-static IIO_DEV_ATTR_HWFIFO_ENABLED(bmc150_accel_get_fifo_state);
-static IIO_DEV_ATTR_HWFIFO_WATERMARK(bmc150_accel_get_fifo_watermark);
-
-static const struct attribute *bmc150_accel_fifo_attributes[] = {
-	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
-	&iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
-	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
-	NULL,
-};
-
 static int bmc150_accel_set_watermark(struct iio_dev *indio_dev, unsigned val)
 {
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
@@ -952,7 +939,8 @@ static int bmc150_accel_fifo_transfer(const struct i2c_client *client,
 }
 
 static int __bmc150_accel_fifo_flush(struct iio_dev *indio_dev,
-				     unsigned samples, bool irq)
+				     unsigned samples, bool irq,
+				     bool event)
 {
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 	int ret, i;
@@ -1030,6 +1018,12 @@ static int __bmc150_accel_fifo_flush(struct iio_dev *indio_dev,
 		tstamp += sample_period;
 	}
 
+	if (event)
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(IIO_ACCEL, 0,
+						    IIO_EV_TYPE_HWFIFO_FLUSHED,
+						    IIO_EV_DIR_NONE),
+			       tstamp);
 	return count;
 }
 
@@ -1039,12 +1033,50 @@ static int bmc150_accel_fifo_flush(struct iio_dev *indio_dev, unsigned samples)
 	int ret;
 
 	mutex_lock(&data->mutex);
-	ret = __bmc150_accel_fifo_flush(indio_dev, samples, false);
+	ret = __bmc150_accel_fifo_flush(indio_dev, samples, false, false);
 	mutex_unlock(&data->mutex);
 
 	return ret;
 }
 
+static ssize_t bmc150_accel_sysfs_fifo_flush(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf,
+					     size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	unsigned int samples;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &samples);
+	if (ret)
+		return ret;
+	if (!samples)
+		return -EINVAL;
+
+	mutex_lock(&data->mutex);
+	ret = __bmc150_accel_fifo_flush(indio_dev, samples, false, true);
+	mutex_unlock(&data->mutex);
+
+	return ret ? ret : len;
+}
+
+static IIO_CONST_ATTR_HWFIFO_WATERMARK_MIN(1);
+static IIO_CONST_ATTR_HWFIFO_WATERMARK_MAX(BMC150_ACCEL_FIFO_LENGTH);
+static IIO_DEV_ATTR_HWFIFO_ENABLED(bmc150_accel_get_fifo_state);
+static IIO_DEV_ATTR_HWFIFO_WATERMARK(bmc150_accel_get_fifo_watermark);
+static IIO_DEV_ATTR_HWFIFO_FLUSH(bmc150_accel_sysfs_fifo_flush);
+
+static const struct attribute *bmc150_accel_fifo_attributes[] = {
+	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
+	&iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
+	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
+	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	&iio_dev_attr_hwfifo_flush.dev_attr.attr,
+	NULL,
+};
+
 static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
 		"15.620000 31.260000 62.50000 125 250 500 1000 2000");
 
@@ -1346,7 +1378,8 @@ static irqreturn_t bmc150_accel_irq_thread_handler(int irq, void *private)
 
 	if (data->fifo_mode) {
 		ret = __bmc150_accel_fifo_flush(indio_dev,
-						BMC150_ACCEL_FIFO_LENGTH, true);
+						BMC150_ACCEL_FIFO_LENGTH, true,
+						false);
 		if (ret > 0)
 			ack = true;
 	}
@@ -1578,7 +1611,8 @@ static int bmc150_accel_buffer_predisable(struct iio_dev *indio_dev)
 		goto out;
 
 	bmc150_accel_set_interrupt(data, BMC150_ACCEL_INT_WATERMARK, false);
-	__bmc150_accel_fifo_flush(indio_dev, BMC150_ACCEL_FIFO_LENGTH, false);
+	__bmc150_accel_fifo_flush(indio_dev, BMC150_ACCEL_FIFO_LENGTH, false,
+				  false);
 	data->fifo_mode = 0;
 	bmc150_accel_fifo_set_mode(data);
 
-- 
1.9.1

Re: [RFC PATCH 2/3] iio: allow better control for flushing the hardware fifo

[Lars-Peter Clausen]

On 04/29/2015 01:18 PM, Octavian Purdila wrote:
> Some applications need to be able to flush [1] the hardware fifo of
> the device and to receive events of when that happened [2] so that it
> can ignore stale data.
>
> This patch adds a new event (IIO_EV_TYPE_HWFIFO_FLUSHED) that should
> be sent to userspace when a flush has been completed. The application
> will be able to identify which are the samples to ignore based on the
> timestamp of the event.
>
> To allow applications to accurately generate a hardware fifo flush on
> demand, this patch also adds a new sysfs entry that triggers a
> hardware fifo flush when written to.
>
> [1] https://source.android.com/devices/sensors/hal-interface.html#flush_sensor
> [2] https://source.android.com/devices/sensors/hal-interface.html#metadata_flush_complete_events

Since there is no asynchronous queue for commands to be executed in IIO 
adding a asynchronous completion event doesn't make too much sense. This is 
something that needs to be handled at the HAL level.

The HAL needs to have a queue of commands that need to be executed where new 
events can be added asynchronously, then has a loop which goes through the 
commands in the queue and executes them, and once executed generated the 
appropriate completion event.

I really wish that document would specify what is actually meant by flush. 
Copy the FIFO content to a software buffer or discard the FIFO content.

>
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>   Documentation/ABI/testing/sysfs-bus-iio | 11 +++++++++++
>   include/linux/iio/sysfs.h               |  3 +++
>   include/uapi/linux/iio/types.h          |  1 +
>   3 files changed, 15 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 866b4ec..bb4d8de 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1375,3 +1375,14 @@ Description:
>   		The emissivity ratio of the surface in the field of view of the
>   		contactless temperature sensor.  Emissivity varies from 0 to 1,
>   		with 1 being the emissivity of a black body.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_flush
> +KernelVersion:	4.2
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Write only entry that accepts a single strictly positive integer
> +		specifying the number of samples to flush from the hardware fifo
> +		to the device buffer. When the flush is completed an
> +		IIO_EV_TYPE_HWFIFO_FLUSHED event is generated. The event has the
> +		timestamp equal with the timestamp of last sample that was
> +		flushed from the hardware fifo.

I'd prefer this to be handled through the normal read() API rather than 
having a side channel for it. Big question is how though. We could specify 
that reading in O_NONBLOCK mode will always read data if it is available and 
not only if it is above the watermark threshold.

Re: [RFC PATCH 2/3] iio: allow better control for flushing the hardware fifo

[Octavian Purdila]

On Sat, May 2, 2015 at 8:42 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 04/29/2015 01:18 PM, Octavian Purdila wrote:
>>
>> Some applications need to be able to flush [1] the hardware fifo of
>> the device and to receive events of when that happened [2] so that it
>> can ignore stale data.
>>
>> This patch adds a new event (IIO_EV_TYPE_HWFIFO_FLUSHED) that should
>> be sent to userspace when a flush has been completed. The application
>> will be able to identify which are the samples to ignore based on the
>> timestamp of the event.
>>
>> To allow applications to accurately generate a hardware fifo flush on
>> demand, this patch also adds a new sysfs entry that triggers a
>> hardware fifo flush when written to.
>>
>> [1]
>> https://source.android.com/devices/sensors/hal-interface.html#flush_sensor
>> [2]
>> https://source.android.com/devices/sensors/hal-interface.html#metadata_flush_complete_events
>
>
> Since there is no asynchronous queue for commands to be executed in IIO
> adding a asynchronous completion event doesn't make too much sense. This is
> something that needs to be handled at the HAL level.
>
> The HAL needs to have a queue of commands that need to be executed where new
> events can be added asynchronously, then has a loop which goes through the
> commands in the queue and executes them, and once executed generated the
> appropriate completion event.
>

Hi Lars,

Thanks for the review.

We can't do this at the HAL level because the needed information is
only available at the HAL level. At the HAL level each received sample
from the driver is converted to an event. When doing a flush the HAL
must add a special event (flush complete) after the last sample in the
hardware fifo. But the HAL does not know how many samples are in the
hardware fifo, how many are in the device buffer, etc.

>
> I really wish that document would specify what is actually meant by flush.
> Copy the FIFO content to a software buffer or discard the FIFO content.
>

It does say: "... and flushes the FIFO; those events are delivered as
usual (i.e.: as if the maximum reporting latency had expired) ..."

>
>>
>> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
>> ---
>>   Documentation/ABI/testing/sysfs-bus-iio | 11 +++++++++++
>>   include/linux/iio/sysfs.h               |  3 +++
>>   include/uapi/linux/iio/types.h          |  1 +
>>   3 files changed, 15 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio
>> b/Documentation/ABI/testing/sysfs-bus-iio
>> index 866b4ec..bb4d8de 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>> @@ -1375,3 +1375,14 @@ Description:
>>                 The emissivity ratio of the surface in the field of view
>> of the
>>                 contactless temperature sensor.  Emissivity varies from 0
>> to 1,
>>                 with 1 being the emissivity of a black body.
>> +
>> +What:          /sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_flush
>> +KernelVersion: 4.2
>> +Contact:       linux-iio@vger.kernel.org
>> +Description:
>> +               Write only entry that accepts a single strictly positive
>> integer
>> +               specifying the number of samples to flush from the
>> hardware fifo
>> +               to the device buffer. When the flush is completed an
>> +               IIO_EV_TYPE_HWFIFO_FLUSHED event is generated. The event
>> has the
>> +               timestamp equal with the timestamp of last sample that was
>> +               flushed from the hardware fifo.
>
>
> I'd prefer this to be handled through the normal read() API rather than
> having a side channel for it. Big question is how though. We could specify
> that reading in O_NONBLOCK mode will always read data if it is available and
> not only if it is above the watermark threshold.

Do you mean to try and flush when the available data in the device
buffer is less then the requested size? That should work and hopefully
the ABI change does not matter since the hwfifo stuff has not been
released yet.

I prefer the explicit flush though. I think it is better to have the
ABIs clearly visible instead of being buried in the details.

Re: [RFC PATCH 2/3] iio: allow better control for flushing the hardware fifo

[Lars-Peter Clausen]

On 05/03/2015 08:11 AM, Octavian Purdila wrote:
> On Sat, May 2, 2015 at 8:42 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 04/29/2015 01:18 PM, Octavian Purdila wrote:
>>>
>>> Some applications need to be able to flush [1] the hardware fifo of
>>> the device and to receive events of when that happened [2] so that it
>>> can ignore stale data.
>>>
>>> This patch adds a new event (IIO_EV_TYPE_HWFIFO_FLUSHED) that should
>>> be sent to userspace when a flush has been completed. The application
>>> will be able to identify which are the samples to ignore based on the
>>> timestamp of the event.
>>>
>>> To allow applications to accurately generate a hardware fifo flush on
>>> demand, this patch also adds a new sysfs entry that triggers a
>>> hardware fifo flush when written to.
>>>
>>> [1]
>>> https://source.android.com/devices/sensors/hal-interface.html#flush_sensor
>>> [2]
>>> https://source.android.com/devices/sensors/hal-interface.html#metadata_flush_complete_events
>>
>>
>> Since there is no asynchronous queue for commands to be executed in IIO
>> adding a asynchronous completion event doesn't make too much sense. This is
>> something that needs to be handled at the HAL level.
>>
>> The HAL needs to have a queue of commands that need to be executed where new
>> events can be added asynchronously, then has a loop which goes through the
>> commands in the queue and executes them, and once executed generated the
>> appropriate completion event.
>>
>
> Hi Lars,
>
> Thanks for the review.
>
> We can't do this at the HAL level because the needed information is
> only available at the HAL level. At the HAL level each received sample
> from the driver is converted to an event. When doing a flush the HAL
> must add a special event (flush complete) after the last sample in the
> hardware fifo. But the HAL does not know how many samples are in the
> hardware fifo, how many are in the device buffer, etc.

Ok, so that's what you need the timestamp for I presume? So the signature of 
the of the sync function is basically.

timestamp sync(device)

where timestamp is greater or equal to the timestamp of all samples before 
the sync and smaller or equal to all samples after the sync.

What your implementation does is implement a synchronous API to flush the 
FIFO but delivers the result of the operation asynchronously via a rather 
arbitrary delivery mechanism. That is in my opinion not a good API/ABI and 
might even have some race condition issues.

If you do a flush, then read as much data as available you know that this 
data is from before the flush and any data read at a later point is after 
the flush.

>
>>
>> I really wish that document would specify what is actually meant by flush.
>> Copy the FIFO content to a software buffer or discard the FIFO content.
>>
>
> It does say: "... and flushes the FIFO; those events are delivered as
> usual (i.e.: as if the maximum reporting latency had expired) ..."
>

Missed that, thanks.

>>
>>>
>>> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
>>> ---
>>>    Documentation/ABI/testing/sysfs-bus-iio | 11 +++++++++++
>>>    include/linux/iio/sysfs.h               |  3 +++
>>>    include/uapi/linux/iio/types.h          |  1 +
>>>    3 files changed, 15 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio
>>> b/Documentation/ABI/testing/sysfs-bus-iio
>>> index 866b4ec..bb4d8de 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>> @@ -1375,3 +1375,14 @@ Description:
>>>                  The emissivity ratio of the surface in the field of view
>>> of the
>>>                  contactless temperature sensor.  Emissivity varies from 0
>>> to 1,
>>>                  with 1 being the emissivity of a black body.
>>> +
>>> +What:          /sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_flush
>>> +KernelVersion: 4.2
>>> +Contact:       linux-iio@vger.kernel.org
>>> +Description:
>>> +               Write only entry that accepts a single strictly positive
>>> integer
>>> +               specifying the number of samples to flush from the
>>> hardware fifo
>>> +               to the device buffer. When the flush is completed an
>>> +               IIO_EV_TYPE_HWFIFO_FLUSHED event is generated. The event
>>> has the
>>> +               timestamp equal with the timestamp of last sample that was
>>> +               flushed from the hardware fifo.
>>
>>
>> I'd prefer this to be handled through the normal read() API rather than
>> having a side channel for it. Big question is how though. We could specify
>> that reading in O_NONBLOCK mode will always read data if it is available and
>> not only if it is above the watermark threshold.
>
> Do you mean to try and flush when the available data in the device
> buffer is less then the requested size? That should work and hopefully
> the ABI change does not matter since the hwfifo stuff has not been
> released yet.

Basically only let poll() and blocking read() block when not enough data is 
available. But for non-blocking read return as much data as possible if data 
is available.

>
> I prefer the explicit flush though. I think it is better to have the
> ABIs clearly visible instead of being buried in the details.
>

Yea, it's a bit tricky. What should be used for this sysfs, IOCTL, read()...

- Lars

Re: [RFC PATCH 2/3] iio: allow better control for flushing the hardware fifo

[Octavian Purdila]

On Mon, May 4, 2015 at 5:38 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 05/03/2015 08:11 AM, Octavian Purdila wrote:
>>
>> On Sat, May 2, 2015 at 8:42 PM, Lars-Peter Clausen <lars@metafoo.de>
>> wrote:
>>>
>>> On 04/29/2015 01:18 PM, Octavian Purdila wrote:
>>>>
>>>>
>>>> Some applications need to be able to flush [1] the hardware fifo of
>>>> the device and to receive events of when that happened [2] so that it
>>>> can ignore stale data.
>>>>
>>>> This patch adds a new event (IIO_EV_TYPE_HWFIFO_FLUSHED) that should
>>>> be sent to userspace when a flush has been completed. The application
>>>> will be able to identify which are the samples to ignore based on the
>>>> timestamp of the event.
>>>>
>>>> To allow applications to accurately generate a hardware fifo flush on
>>>> demand, this patch also adds a new sysfs entry that triggers a
>>>> hardware fifo flush when written to.
>>>>
>>>> [1]
>>>>
>>>> https://source.android.com/devices/sensors/hal-interface.html#flush_sensor
>>>> [2]
>>>>
>>>> https://source.android.com/devices/sensors/hal-interface.html#metadata_flush_complete_events
>>>
>>>
>>>
>>> Since there is no asynchronous queue for commands to be executed in IIO
>>> adding a asynchronous completion event doesn't make too much sense. This
>>> is
>>> something that needs to be handled at the HAL level.
>>>
>>> The HAL needs to have a queue of commands that need to be executed where
>>> new
>>> events can be added asynchronously, then has a loop which goes through
>>> the
>>> commands in the queue and executes them, and once executed generated the
>>> appropriate completion event.
>>>
>>
>> Hi Lars,
>>
>> Thanks for the review.
>>
>> We can't do this at the HAL level because the needed information is
>> only available at the HAL level. At the HAL level each received sample
>> from the driver is converted to an event. When doing a flush the HAL
>> must add a special event (flush complete) after the last sample in the
>> hardware fifo. But the HAL does not know how many samples are in the
>> hardware fifo, how many are in the device buffer, etc.
>
>
> Ok, so that's what you need the timestamp for I presume? So the signature of
> the of the sync function is basically.
>
> timestamp sync(device)
>
> where timestamp is greater or equal to the timestamp of all samples before
> the sync and smaller or equal to all samples after the sync.
>

Yes that is the idea, although the implementation is that we need to
insert an Android flush_complete event right before the event with the
timestamp strictly greater then the timestamp of the IIO fifo flushed
event.


> What your implementation does is implement a synchronous API to flush the
> FIFO but delivers the result of the operation asynchronously via a rather
> arbitrary delivery mechanism. That is in my opinion not a good API/ABI and
> might even have some race condition issues.
>
> If you do a flush, then read as much data as available you know that this
> data is from before the flush and any data read at a later point is after
> the flush.
>

We tried something similar (read in a loop until -EGAIN was received)
but run into issue where we needed to cycle a lot to get the -EAGAIN
at high sample frequencies and our guess was that read() is taking
more then the time to receive a new sample.

But if we modify the non-blocking case to flush the fifo even when the
available data is non-zero and we have requested more than what is
available I think we can avoid that issue. We'll try that and I'll get
back with more datapoints.

>>
>>>
>>> I really wish that document would specify what is actually meant by
>>> flush.
>>> Copy the FIFO content to a software buffer or discard the FIFO content.
>>>
>>
>> It does say: "... and flushes the FIFO; those events are delivered as
>> usual (i.e.: as if the maximum reporting latency had expired) ..."
>>
>
> Missed that, thanks.
>
>
>>>
>>>>
>>>> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
>>>> ---
>>>>    Documentation/ABI/testing/sysfs-bus-iio | 11 +++++++++++
>>>>    include/linux/iio/sysfs.h               |  3 +++
>>>>    include/uapi/linux/iio/types.h          |  1 +
>>>>    3 files changed, 15 insertions(+)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio
>>>> b/Documentation/ABI/testing/sysfs-bus-iio
>>>> index 866b4ec..bb4d8de 100644
>>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>>> @@ -1375,3 +1375,14 @@ Description:
>>>>                  The emissivity ratio of the surface in the field of
>>>> view
>>>> of the
>>>>                  contactless temperature sensor.  Emissivity varies from
>>>> 0
>>>> to 1,
>>>>                  with 1 being the emissivity of a black body.
>>>> +
>>>> +What:          /sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_flush
>>>> +KernelVersion: 4.2
>>>> +Contact:       linux-iio@vger.kernel.org
>>>> +Description:
>>>> +               Write only entry that accepts a single strictly positive
>>>> integer
>>>> +               specifying the number of samples to flush from the
>>>> hardware fifo
>>>> +               to the device buffer. When the flush is completed an
>>>> +               IIO_EV_TYPE_HWFIFO_FLUSHED event is generated. The event
>>>> has the
>>>> +               timestamp equal with the timestamp of last sample that
>>>> was
>>>> +               flushed from the hardware fifo.
>>>
>>>
>>>
>>> I'd prefer this to be handled through the normal read() API rather than
>>> having a side channel for it. Big question is how though. We could
>>> specify
>>> that reading in O_NONBLOCK mode will always read data if it is available
>>> and
>>> not only if it is above the watermark threshold.
>>
>>
>> Do you mean to try and flush when the available data in the device
>> buffer is less then the requested size? That should work and hopefully
>> the ABI change does not matter since the hwfifo stuff has not been
>> released yet.
>
>
> Basically only let poll() and blocking read() block when not enough data is
> available. But for non-blocking read return as much data as possible if data
> is available.
>
>>
>> I prefer the explicit flush though. I think it is better to have the
>> ABIs clearly visible instead of being buried in the details.
>>
>
> Yea, it's a bit tricky. What should be used for this sysfs, IOCTL, read()...
>
> - Lars