[PATCH] iio: buffer-dmaengine: Report buffer length requirements

[PATCH] iio: buffer-dmaengine: Report buffer length requirements

[Alexandru Ardelean]

From: Lars-Peter Clausen <lars@metafoo.de>

The dmaengine buffer has some length alignment requirements that can differ
from platform to platform. If the length alignment requirements are not met
unexpected behavior like dropping of samples can occur.

Currently these requirements are not reported and applications need to know
the requirements of the platform by some out-of-band means.

Add a new buffer attribute that reports the length alignment requirements
called `length_align_bytes`. The reported length alignment is in bytes that
means the buffer length alignment in sample sets depends on the number of
enabled channels and the bytes per channel. Applications using this
attribute to determine the buffer size requirements need to consider this.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 .../ABI/testing/sysfs-bus-iio-dma-buffer      | 19 +++++++++++++++++
 .../buffer/industrialio-buffer-dmaengine.c    | 21 +++++++++++++++++++
 2 files changed, 40 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dma-buffer

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dma-buffer b/Documentation/ABI/testing/sysfs-bus-iio-dma-buffer
new file mode 100644
index 000000000000..999de481de82
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-dma-buffer
@@ -0,0 +1,19 @@
+What:		/sys/bus/iio/devices/iio:deviceX/buffer/length_align_bytes
+KernelVersion:	5.4
+Contact:	linux-iio@vger.kernel.org
+Description:
+		DMA buffers tend to have a alignment requirement for the
+		buffers. If this alignment requirement is not met samples might
+		be dropped from the buffer.
+
+		This property reports the alignment requirements in bytes. This means
+		that the buffer size in bytes needs to be a integer multiple of the
+		number reported by this file.
+
+		The alignment requirements in number of sample sets will depend on the
+		enabled channels and the bytes per channel. This means that the
+		alignment requirement in samples sets might change depending on which
+		and how many channels are enabled. Whereas the alignment requirement
+		reported in bytes by this property will remain static and does not
+		depend on which channels are
+		enabled.
diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
index 7d298aaff1f0..b129693af0fd 100644
--- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
+++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/buffer_impl.h>
 #include <linux/iio/buffer-dma.h>
@@ -126,6 +127,24 @@ static const struct iio_dma_buffer_ops iio_dmaengine_default_ops = {
 	.abort = iio_dmaengine_buffer_abort,
 };
 
+static ssize_t iio_dmaengine_buffer_get_length_align(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct dmaengine_buffer *dmaengine_buffer =
+		iio_buffer_to_dmaengine_buffer(indio_dev->buffer);
+
+	return sprintf(buf, "%u\n", dmaengine_buffer->align);
+}
+
+static IIO_DEVICE_ATTR(length_align_bytes, 0444,
+		       iio_dmaengine_buffer_get_length_align, NULL, 0);
+
+static const struct attribute *iio_dmaengine_buffer_attrs[] = {
+	&iio_dev_attr_length_align_bytes.dev_attr.attr,
+	NULL,
+};
+
 /**
  * iio_dmaengine_buffer_alloc() - Allocate new buffer which uses DMAengine
  * @dev: Parent device for the buffer
@@ -179,6 +198,8 @@ struct iio_buffer *iio_dmaengine_buffer_alloc(struct device *dev,
 
 	iio_dma_buffer_init(&dmaengine_buffer->queue, chan->device->dev,
 		&iio_dmaengine_default_ops);
+	iio_buffer_set_attrs(&dmaengine_buffer->queue.buffer,
+		iio_dmaengine_buffer_attrs);
 
 	dmaengine_buffer->queue.buffer.access = &iio_dmaengine_buffer_ops;
 
-- 
2.20.1

Re: [PATCH] iio: buffer-dmaengine: Report buffer length requirements

[Jonathan Cameron]

On Wed, 11 Dec 2019 13:56:15 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Lars-Peter Clausen <lars@metafoo.de>
> 
> The dmaengine buffer has some length alignment requirements that can differ
> from platform to platform. If the length alignment requirements are not met
> unexpected behavior like dropping of samples can occur.
> 
> Currently these requirements are not reported and applications need to know
> the requirements of the platform by some out-of-band means.
> 
> Add a new buffer attribute that reports the length alignment requirements
> called `length_align_bytes`. The reported length alignment is in bytes that
> means the buffer length alignment in sample sets depends on the number of
> enabled channels and the bytes per channel. Applications using this
> attribute to determine the buffer size requirements need to consider this.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Seems like a sensible and well defined change to me, so applied to the togreg
branch of iio.git with the minor tweak mentioned below.


> ---
>  .../ABI/testing/sysfs-bus-iio-dma-buffer      | 19 +++++++++++++++++
>  .../buffer/industrialio-buffer-dmaengine.c    | 21 +++++++++++++++++++
>  2 files changed, 40 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dma-buffer
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dma-buffer b/Documentation/ABI/testing/sysfs-bus-iio-dma-buffer
> new file mode 100644
> index 000000000000..999de481de82
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-dma-buffer
> @@ -0,0 +1,19 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/buffer/length_align_bytes
> +KernelVersion:	5.4
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		DMA buffers tend to have a alignment requirement for the
> +		buffers. If this alignment requirement is not met samples might
> +		be dropped from the buffer.
> +
> +		This property reports the alignment requirements in bytes. This means
> +		that the buffer size in bytes needs to be a integer multiple of the
> +		number reported by this file.
> +
> +		The alignment requirements in number of sample sets will depend on the
> +		enabled channels and the bytes per channel. This means that the
> +		alignment requirement in samples sets might change depending on which
> +		and how many channels are enabled. Whereas the alignment requirement
> +		reported in bytes by this property will remain static and does not
> +		depend on which channels are

Odd line wrapping. I'll fix that.  Whole block is a bit wide, so I'll rewrap it.

Jonathan

> +		enabled.
> diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> index 7d298aaff1f0..b129693af0fd 100644
> --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> @@ -13,6 +13,7 @@
>  #include <linux/module.h>
>  
>  #include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/buffer_impl.h>
>  #include <linux/iio/buffer-dma.h>
> @@ -126,6 +127,24 @@ static const struct iio_dma_buffer_ops iio_dmaengine_default_ops = {
>  	.abort = iio_dmaengine_buffer_abort,
>  };
>  
> +static ssize_t iio_dmaengine_buffer_get_length_align(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct dmaengine_buffer *dmaengine_buffer =
> +		iio_buffer_to_dmaengine_buffer(indio_dev->buffer);
> +
> +	return sprintf(buf, "%u\n", dmaengine_buffer->align);
> +}
> +
> +static IIO_DEVICE_ATTR(length_align_bytes, 0444,
> +		       iio_dmaengine_buffer_get_length_align, NULL, 0);
> +
> +static const struct attribute *iio_dmaengine_buffer_attrs[] = {
> +	&iio_dev_attr_length_align_bytes.dev_attr.attr,
> +	NULL,
> +};
> +
>  /**
>   * iio_dmaengine_buffer_alloc() - Allocate new buffer which uses DMAengine
>   * @dev: Parent device for the buffer
> @@ -179,6 +198,8 @@ struct iio_buffer *iio_dmaengine_buffer_alloc(struct device *dev,
>  
>  	iio_dma_buffer_init(&dmaengine_buffer->queue, chan->device->dev,
>  		&iio_dmaengine_default_ops);
> +	iio_buffer_set_attrs(&dmaengine_buffer->queue.buffer,
> +		iio_dmaengine_buffer_attrs);
>  
>  	dmaengine_buffer->queue.buffer.access = &iio_dmaengine_buffer_ops;
>