[RFC PATCH 0/2] raw read performance improvement

[RFC PATCH 0/2] raw read performance improvement

[Srinivas Pandruvada]

We see several performance issues reading individual axis, when
raw read is the only option. Since most of the time user space reads x, y
and z, one after other, this results in three sysfs reads, and based on
chipset we have to power up or set up measurement mode and wait for
response.
For example reading x,y and z takes 160ms for ak8975, it is true for
other too. But reading together takes only 80ms.

IIO types already defined a modifier for X_AND_Y_AND_Z, which is used
by one driver to send event code. This modifier has no value assigned
so (null) appears in sysfs. If this is not correct then we may need 
another modifier. 

Since we have now raw_read with capability to read multiple values,
we can use this callback to return values to iio core.

Srinivas Pandruvada (2):
  iio: core: add xyz modifier value
  iio: magnetometer: ak8975: Improve performance of raw reads

 drivers/iio/industrialio-core.c   |  1 +
 drivers/iio/magnetometer/ak8975.c | 58 ++++++++++++++++++++++++++-------------
 2 files changed, 40 insertions(+), 19 deletions(-)

-- 
1.8.3.2

[RFC PATCH 1/2] iio: core: add xyz modifier value

[Srinivas Pandruvada]

iio types defines a modifier called IIO_MOD_X_AND_Y_AND_Z, but since
no modifier name is specified, it shows (null) as channel name.
Here name is assiged a value "xyz".

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/iio/industrialio-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 4b1f375..e8428bb 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -87,6 +87,7 @@ static const char * const iio_modifier_names[] = {
 	[IIO_MOD_QUATERNION] = "quaternion",
 	[IIO_MOD_TEMP_AMBIENT] = "ambient",
 	[IIO_MOD_TEMP_OBJECT] = "object",
+	[IIO_MOD_X_AND_Y_AND_Z] = "xyz",
 };
 
 /* relies on pairs of these shared then separate */
-- 
1.8.3.2

Re: [RFC PATCH 1/2] iio: core: add xyz modifier value

[Daniel Baluta]

On Fri, Jun 27, 2014 at 3:40 AM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> iio types defines a modifier called IIO_MOD_X_AND_Y_AND_Z, but since
> no modifier name is specified, it shows (null) as channel name.
> Here name is assiged a value "xyz".

Minor comments:
* s/assiged/assigned.
* it would be nice to add here the message from cover letter,
otherwise it will be lost on linux-iio archive :).

thanks,
Daniel.

Re: [RFC PATCH 1/2] iio: core: add xyz modifier value

[Jonathan Cameron]

On 27/06/14 01:40, Srinivas Pandruvada wrote:
> iio types defines a modifier called IIO_MOD_X_AND_Y_AND_Z, but since
> no modifier name is specified, it shows (null) as channel name.
> Here name is assiged a value "xyz".
Hmm. The purpose of this particularly modifier was for boolean combinations
of events.  There are devices out there that will only fire an interrupt
if say they detect changes in acceleration in all 3 directions...
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>   drivers/iio/industrialio-core.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 4b1f375..e8428bb 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -87,6 +87,7 @@ static const char * const iio_modifier_names[] = {
>   	[IIO_MOD_QUATERNION] = "quaternion",
>   	[IIO_MOD_TEMP_AMBIENT] = "ambient",
>   	[IIO_MOD_TEMP_OBJECT] = "object",
> +	[IIO_MOD_X_AND_Y_AND_Z] = "xyz",
>   };
>
>   /* relies on pairs of these shared then separate */
>

[RFC PATCH 2/2] iio: magnetometer: ak8975: Improve performance of raw reads

[Srinivas Pandruvada]

Define a channel to read x,y and z together. This raw read takes
50% less time reading all axes together than individual reads.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/iio/magnetometer/ak8975.c | 58 ++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 24fc917..b6041ca 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -1,3 +1,4 @@
+
 /*
  * A sensor driver for the magnetometer AK8975.
  *
@@ -373,12 +374,14 @@ static int wait_conversion_complete_interrupt(struct ak8975_data *data)
 }
 
 /*
- * Emits the raw flux value for the x, y, or z axis.
+ * Emits the raw flux value for the x, y, or z individual or multiple axes
  */
-static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
+static int ak8975_read_axis(struct iio_dev *indio_dev, int axes_size,
+			    int *indexes, int *vals)
 {
 	struct ak8975_data *data = iio_priv(indio_dev);
 	struct i2c_client *client = data->client;
+	int i;
 	int ret;
 
 	mutex_lock(&data->lock);
@@ -421,36 +424,50 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
 
 	/* Read the flux value from the appropriate register
 	   (the register is specified in the iio device attributes). */
-	ret = i2c_smbus_read_word_data(client, ak8975_index_to_reg[index]);
-	if (ret < 0) {
-		dev_err(&client->dev, "Read axis data fails\n");
-		goto exit;
+	for (i = 0; i < axes_size; ++i) {
+		ret = i2c_smbus_read_word_data(client,
+					ak8975_index_to_reg[*indexes++]);
+		if (ret < 0) {
+			dev_err(&client->dev, "Read axis data fails\n");
+			goto exit;
+		}
+		/* Clamp to valid range. */
+		vals[i] = clamp_t(s16, ret, -4096, 4095);
 	}
 
 	mutex_unlock(&data->lock);
 
-	/* Clamp to valid range. */
-	*val = clamp_t(s16, ret, -4096, 4095);
-	return IIO_VAL_INT;
-
+	if (axes_size > 1)
+		return IIO_VAL_INT_MULTIPLE;
+	else
+		return IIO_VAL_INT;
 exit:
 	mutex_unlock(&data->lock);
 	return ret;
 }
 
-static int ak8975_read_raw(struct iio_dev *indio_dev,
-			   struct iio_chan_spec const *chan,
-			   int *val, int *val2,
-			   long mask)
+static int ak8975_read_raw_multi(struct iio_dev *indio_dev,
+				 struct iio_chan_spec const *chan,
+				 int size, int *vals, int *val_len,
+				 long mask)
 {
 	struct ak8975_data *data = iio_priv(indio_dev);
+	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		return ak8975_read_axis(indio_dev, chan->address, val);
+		if (size >= 3 && chan->address == 3) {
+			int indexes[3] = {0, 1, 2};
+
+			ret = ak8975_read_axis(indio_dev, 3, indexes, vals);
+			*val_len = 3;
+		} else
+			ret = ak8975_read_axis(indio_dev, 1,
+					       (int *)&chan->address, vals);
+		return ret;
 	case IIO_CHAN_INFO_SCALE:
-		*val = 0;
-		*val2 = data->raw_to_gauss[chan->address];
+		vals[0] = 0;
+		vals[1] = data->raw_to_gauss[chan->address];
 		return IIO_VAL_INT_PLUS_MICRO;
 	}
 	return -EINVAL;
@@ -467,11 +484,14 @@ static int ak8975_read_raw(struct iio_dev *indio_dev,
 	}
 
 static const struct iio_chan_spec ak8975_channels[] = {
-	AK8975_CHANNEL(X, 0), AK8975_CHANNEL(Y, 1), AK8975_CHANNEL(Z, 2),
+	AK8975_CHANNEL(X, 0),
+	AK8975_CHANNEL(Y, 1),
+	AK8975_CHANNEL(Z, 2),
+	AK8975_CHANNEL(X_AND_Y_AND_Z, 3)
 };
 
 static const struct iio_info ak8975_info = {
-	.read_raw = &ak8975_read_raw,
+	.read_raw_multi = &ak8975_read_raw_multi,
 	.driver_module = THIS_MODULE,
 };
 
-- 
1.8.3.2

Re: [RFC PATCH 0/2] raw read performance improvement

[Jonathan Cameron]

On 27/06/14 01:40, Srinivas Pandruvada wrote:
> We see several performance issues reading individual axis, when
> raw read is the only option. Since most of the time user space reads x, y
> and z, one after other, this results in three sysfs reads, and based on
> chipset we have to power up or set up measurement mode and wait for
> response.
That's probably a case for a change in the driver to schedule the power
down for a little period after the read rather than immediately...
> For example reading x,y and z takes 160ms for ak8975, it is true for
> other too. But reading together takes only 80ms.
>
> IIO types already defined a modifier for X_AND_Y_AND_Z, which is used
> by one driver to send event code. This modifier has no value assigned
> so (null) appears in sysfs. If this is not correct then we may need
> another modifier.
>
> Since we have now raw_read with capability to read multiple values,
> we can use this callback to return values to iio core.
>
I've actually argued fairly strongly against this sort of multiple
channel simply because it means there are multiple ways of presenting the
same data in the ABI. The big difference with the Quaternion case is that
the elements of that have no meaning whatsoever if you don't have them
all.

Previously any indication that people wanted higher performance reads
would mean that I'd just tell them that is what the chardev interface
is for.  Here a simple sysfs trigger and a couple of extra lines in the
driver and you'd have what you want with considerably less overhead.

Hence my initial thought to this patch is that I'm not keen.  However,
lets let it sit for a while and see what other comments people have.
Perhaps if there is enough demand I might rethink my position.

Jonathan
> Srinivas Pandruvada (2):
>    iio: core: add xyz modifier value
>    iio: magnetometer: ak8975: Improve performance of raw reads
>
>   drivers/iio/industrialio-core.c   |  1 +
>   drivers/iio/magnetometer/ak8975.c | 58 ++++++++++++++++++++++++++-------------
>   2 files changed, 40 insertions(+), 19 deletions(-)
>

Re: [RFC PATCH 0/2] raw read performance improvement

[Srinivas Pandruvada]

On 06/28/2014 01:49 AM, Jonathan Cameron wrote:
> On 27/06/14 01:40, Srinivas Pandruvada wrote:
>> We see several performance issues reading individual axis, when
>> raw read is the only option. Since most of the time user space reads 
>> x, y
>> and z, one after other, this results in three sysfs reads, and based on
>> chipset we have to power up or set up measurement mode and wait for
>> response.
> That's probably a case for a change in the driver to schedule the power
> down for a little period after the read rather than immediately...
Agree, if we can do that. But some chips have a single measurement mode,
after measurement is done they go to sleep mode, like AK8975 (they
do have other modes with power penalty).
>> For example reading x,y and z takes 160ms for ak8975, it is true for
>> other too. But reading together takes only 80ms.
>>
>> IIO types already defined a modifier for X_AND_Y_AND_Z, which is used
>> by one driver to send event code. This modifier has no value assigned
>> so (null) appears in sysfs. If this is not correct then we may need
>> another modifier.
>>
>> Since we have now raw_read with capability to read multiple values,
>> we can use this callback to return values to iio core.
>>
> I've actually argued fairly strongly against this sort of multiple
> channel simply because it means there are multiple ways of presenting the
> same data in the ABI. The big difference with the Quaternion case is that
> the elements of that have no meaning whatsoever if you don't have them
> all.
>
> Previously any indication that people wanted higher performance reads
> would mean that I'd just tell them that is what the chardev interface
> is for.  Here a simple sysfs trigger and a couple of extra lines in the
> driver and you'd have what you want with considerably less overhead.
But using these I/F, you loose IIO ABI advantage for user space. This was
the model followed and still followed by, but requires special user 
space changes
for every sensor driver. With IIO model, we let vendors pick their 
components
as long as they have an IIO driver, with no change in user space.
>
> Hence my initial thought to this patch is that I'm not keen. However,
> lets let it sit for a while and see what other comments people have.
> Perhaps if there is enough demand I might rethink my position.
>
Let's see, I feel that it will help for phone/tablet like power 
sensitive devices but demands performance.

Thanks,
Srinivas

> Jonathan
>> Srinivas Pandruvada (2):
>>    iio: core: add xyz modifier value
>>    iio: magnetometer: ak8975: Improve performance of raw reads
>>
>>   drivers/iio/industrialio-core.c   |  1 +
>>   drivers/iio/magnetometer/ak8975.c | 58 
>> ++++++++++++++++++++++++++-------------
>>   2 files changed, 40 insertions(+), 19 deletions(-)
>>
>
>

Re: [RFC PATCH 0/2] raw read performance improvement

[Jonathan Cameron]

On 28/06/14 14:16, Srinivas Pandruvada wrote:
>
> On 06/28/2014 01:49 AM, Jonathan Cameron wrote:
>> On 27/06/14 01:40, Srinivas Pandruvada wrote:
>>> We see several performance issues reading individual axis, when
>>> raw read is the only option. Since most of the time user space reads x, y
>>> and z, one after other, this results in three sysfs reads, and based on
>>> chipset we have to power up or set up measurement mode and wait for
>>> response.
>> That's probably a case for a change in the driver to schedule the power
>> down for a little period after the read rather than immediately...
> Agree, if we can do that. But some chips have a single measurement mode,
> after measurement is done they go to sleep mode, like AK8975 (they
> do have other modes with power penalty).
>>> For example reading x,y and z takes 160ms for ak8975, it is true for
>>> other too. But reading together takes only 80ms.
>>>
>>> IIO types already defined a modifier for X_AND_Y_AND_Z, which is used
>>> by one driver to send event code. This modifier has no value assigned
>>> so (null) appears in sysfs. If this is not correct then we may need
>>> another modifier.
>>>
>>> Since we have now raw_read with capability to read multiple values,
>>> we can use this callback to return values to iio core.
>>>
>> I've actually argued fairly strongly against this sort of multiple
>> channel simply because it means there are multiple ways of presenting the
>> same data in the ABI. The big difference with the Quaternion case is that
>> the elements of that have no meaning whatsoever if you don't have them
>> all.
>>
>> Previously any indication that people wanted higher performance reads
>> would mean that I'd just tell them that is what the chardev interface
>> is for.  Here a simple sysfs trigger and a couple of extra lines in the
>> driver and you'd have what you want with considerably less overhead.
> But using these I/F, you loose IIO ABI advantage for user space. This was
> the model followed and still followed by, but requires special user space changes
> for every sensor driver. With IIO model, we let vendors pick their components
> as long as they have an IIO driver, with no change in user space.
I'm unclear by why this needs special purpose userspace.
Generally it's trivial to establish if a part supplies it's own
trigger (data ready for example). If not then a sysfs trigger
or similar is needed and can be instantiated just fine by
generic userspace.  
>>
>> Hence my initial thought to this patch is that I'm not keen. However,
>> lets let it sit for a while and see what other comments people have.
>> Perhaps if there is enough demand I might rethink my position.
>>
> Let's see, I feel that it will help for phone/tablet like power sensitive devices but demands performance.
Sure - though you are never going to get significant performance through
sysfs interfaces...  They just aren't intended for that purpose.  Last
thing we want to do is convert back and forwards from a string for starters!

J
>
> Thanks,
> Srinivas
>
>> Jonathan
>>> Srinivas Pandruvada (2):
>>>    iio: core: add xyz modifier value
>>>    iio: magnetometer: ak8975: Improve performance of raw reads
>>>
>>>   drivers/iio/industrialio-core.c   |  1 +
>>>   drivers/iio/magnetometer/ak8975.c | 58 ++++++++++++++++++++++++++-------------
>>>   2 files changed, 40 insertions(+), 19 deletions(-)
>>>
>>
>>
>

Re: [RFC PATCH 0/2] raw read performance improvement

[Srinivas Pandruvada]

On 06/28/2014 07:14 AM, Jonathan Cameron wrote:
> On 28/06/14 14:16, Srinivas Pandruvada wrote:
>>
>> On 06/28/2014 01:49 AM, Jonathan Cameron wrote:
>>> On 27/06/14 01:40, Srinivas Pandruvada wrote:
>>>> We see several performance issues reading individual axis, when
>>>> raw read is the only option. Since most of the time user space 
>>>> reads x, y
>>>> and z, one after other, this results in three sysfs reads, and 
>>>> based on
>>>> chipset we have to power up or set up measurement mode and wait for
>>>> response.
>>> That's probably a case for a change in the driver to schedule the power
>>> down for a little period after the read rather than immediately...
>> Agree, if we can do that. But some chips have a single measurement mode,
>> after measurement is done they go to sleep mode, like AK8975 (they
>> do have other modes with power penalty).
>>>> For example reading x,y and z takes 160ms for ak8975, it is true for
>>>> other too. But reading together takes only 80ms.
>>>>
>>>> IIO types already defined a modifier for X_AND_Y_AND_Z, which is used
>>>> by one driver to send event code. This modifier has no value assigned
>>>> so (null) appears in sysfs. If this is not correct then we may need
>>>> another modifier.
>>>>
>>>> Since we have now raw_read with capability to read multiple values,
>>>> we can use this callback to return values to iio core.
>>>>
>>> I've actually argued fairly strongly against this sort of multiple
>>> channel simply because it means there are multiple ways of 
>>> presenting the
>>> same data in the ABI. The big difference with the Quaternion case is 
>>> that
>>> the elements of that have no meaning whatsoever if you don't have them
>>> all.
>>>
>>> Previously any indication that people wanted higher performance reads
>>> would mean that I'd just tell them that is what the chardev interface
>>> is for.  Here a simple sysfs trigger and a couple of extra lines in the
>>> driver and you'd have what you want with considerably less overhead.
>> But using these I/F, you loose IIO ABI advantage for user space. This 
>> was
>> the model followed and still followed by, but requires special user 
>> space changes
>> for every sensor driver. With IIO model, we let vendors pick their 
>> components
>> as long as they have an IIO driver, with no change in user space.
> I'm unclear by why this needs special purpose userspace.
> Generally it's trivial to establish if a part supplies it's own
> trigger (data ready for example). If not then a sysfs trigger
> or similar is needed and can be instantiated just fine by
> generic userspace.
I misunderstood your comment. We always provide the buffer I/F, where we 
have
some data ready interrupts. In this case we are fine. We don't have 
sysfs triggers,
which, I should try to check how much time I can save.
>>>
>>> Hence my initial thought to this patch is that I'm not keen. However,
>>> lets let it sit for a while and see what other comments people have.
>>> Perhaps if there is enough demand I might rethink my position.
>>>
>> Let's see, I feel that it will help for phone/tablet like power 
>> sensitive devices but doemands performance.
> Sure - though you are never going to get significant performance through
> sysfs interfaces...  They just aren't intended for that purpose. Last
> thing we want to do is convert back and forwards from a string for 
> starters!
I am not talking about bandwidth limitation from user/kernel interface, 
which sysfs is not designed for.
We have better options than even char-dev for that purpose.

I understand your point of providing a single way to present data. This 
change breaks this assumption. If someone else needs,
we can discuss again.

Thanks,
Srinivas

>
> J
>>
>> Thanks,
>> Srinivas
>>
>>> Jonathan
>>>> Srinivas Pandruvada (2):
>>>>    iio: core: add xyz modifier value
>>>>    iio: magnetometer: ak8975: Improve performance of raw reads
>>>>
>>>>   drivers/iio/industrialio-core.c   |  1 +
>>>>   drivers/iio/magnetometer/ak8975.c | 58 
>>>> ++++++++++++++++++++++++++-------------
>>>>   2 files changed, 40 insertions(+), 19 deletions(-)
>>>>
>>>
>>>
>>
>
>