linux-iio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* iio_compute_scan_bytes does not seem to account for alignment if first channel uses more storagebits than its successors
@ 2019-11-29 14:30 Lars Möllendorf
  2019-11-29 17:23 ` Lars-Peter Clausen
  0 siblings, 1 reply; 11+ messages in thread
From: Lars Möllendorf @ 2019-11-29 14:30 UTC (permalink / raw)
  To: linux-iio

[-- Attachment #1: Type: text/plain, Size: 4160 bytes --]

Hi,

I have written a custom kernel module implementing the IIO device API
backed by an IIO triggered buffer.

My IIO device provides 3 channels + timestamp. The sizes of the channels are

index  | iio_chan_spec.scan_type.storagebits
-------|------------------------------------------------
   0   |  32
   1   |  16
   2   |  16

If I select channel 0 (32bit) and one of channel 1 or 2 (16bit)
indio_dev.scan_bytes and iio_buffer.bytes_per_datum have a value of 6
Byte which does not account for any alignment.


After having a closer look at  `iio_compute_scan_bytes` which is
responsible for calculating both, `indio_dev.scan_bytes` and
`iio_buffer.bytes_per_datum` it seems to me that the order of channels
matter:

```c
	/* How much space will the demuxed element take? */
	for_each_set_bit(i, mask,
			 indio_dev->masklength) {
		length = iio_storage_bytes_for_si(indio_dev, i);
		bytes = ALIGN(bytes, length);
		bytes += length;
	}
```

I understand that in case the length of each scan element is smaller
then the length of the successive scan elements, this algorithm works
because it aligns the current element to its own length. But if, as in
my case, the length of channel 0's scan elements  is greater then the
size of the samples of the consecutive channels no alignment seems to be
taken into account. Do I miss something here?

To make the algorithm work for any case the greatest length of all
enabled scan elements could be used for alignment, e.g.:

```diff
diff --git a/drivers/iio/industrialio-buffer.c
b/drivers/iio/industrialio-buffer.c
index 5d05c38c4ba9..3d2c4e26d818 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -546,16 +546,18 @@ static int iio_compute_scan_bytes(struct iio_dev
*indio_dev,
 				const unsigned long *mask, bool timestamp)
 {
 	unsigned bytes = 0;
-	int length, i;
+	int length, i, align = 0, count = 0;

 	/* How much space will the demuxed element take? */
 	for_each_set_bit(i, mask,
 			 indio_dev->masklength) {
 		length = iio_storage_bytes_for_si(indio_dev, i);
-		bytes = ALIGN(bytes, length);
-		bytes += length;
+		align = max(align, length);
+		count++;
 	}

+	bytes = count * align;
+
 	if (timestamp) {
 		length = iio_storage_bytes_for_timestamp(indio_dev);
 		bytes = ALIGN(bytes, length);

```

And if the storage bytes for timestamp have to be taken into account for
the alignment of the scan elements (what seems not to be the case as it
is currently implemented), then:

```diff
diff --git a/drivers/iio/industrialio-buffer.c
b/drivers/iio/industrialio-buffer.c
index 5d05c38c4ba9..59aee2ea4e19 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -546,21 +546,23 @@ static int iio_compute_scan_bytes(struct iio_dev
*indio_dev,
 				const unsigned long *mask, bool timestamp)
 {
 	unsigned bytes = 0;
-	int length, i;
+	int length, i, align = 0, count = 0;
+
+	if (timestamp) {
+		align = iio_storage_bytes_for_timestamp(indio_dev);
+		count++;
+	}

 	/* How much space will the demuxed element take? */
 	for_each_set_bit(i, mask,
 			 indio_dev->masklength) {
 		length = iio_storage_bytes_for_si(indio_dev, i);
-		bytes = ALIGN(bytes, length);
-		bytes += length;
+		align = max(align, length);
+		count++;
 	}

-	if (timestamp) {
-		length = iio_storage_bytes_for_timestamp(indio_dev);
-		bytes = ALIGN(bytes, length);
-		bytes += length;
-	}
+	bytes = count * align;
+
 	return bytes;
 }
```

But in my case the latter would bloat the buffer from 16 Byte to 4*16 =
64 Byte per scan if all channels are selected and timestamp is active.

For now, I will work around this by using 32 storagebits for all my
channels. This gives my 4 Bytes of overhead per scan if all elements are
selected and additional 2 Byte if timestamp is active.

In "Why do you align the buffer pointer to a multiple of the size of the
current scan element in iio_buffer_foreach_sample()?" on
https://github.com/analogdevicesinc/libiio/issues/324 I have been
pointed to this mailing list.

Regards,
Lars M.

[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 1809 bytes --]

^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-12-23 17:01 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-29 14:30 iio_compute_scan_bytes does not seem to account for alignment if first channel uses more storagebits than its successors Lars Möllendorf
2019-11-29 17:23 ` Lars-Peter Clausen
2019-12-01 12:03   ` Jonathan Cameron
2019-12-01 12:10   ` Jonathan Cameron
2019-12-01 12:29     ` Lars-Peter Clausen
2019-12-02 13:01       ` [PATCH 1/3] iio: buffer: align the size of scan bytes to size of the largest element Lars Möllendorf
2019-12-02 13:37         ` Ardelean, Alexandru
2019-12-02 14:27           ` [PATCH] " Lars Möllendorf
2019-12-06 17:34             ` Jonathan Cameron
2019-12-04  9:24           ` [PATCH 1/3] " Lars Möllendorf
2019-12-04 11:12             ` Ardelean, Alexandru

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).