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

[Jonathan Cameron]

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

> From: Lars-Peter Clausen <lars@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>

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@xxxxxxxxxxxxxxx
> +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;
>