Unable to set buffer watermark correctly

[HiFiPhile]

Hi,

I'm reading from a SPI ADC which use software buffer to offload frequent Kernel-user space copy overhead, and I need to set watermark attribute of the buffer to enable this.

However, watermark needed to be set after length and before enable.
Here is the kernel soruce: https://github.com/linux4sam/linux-at91/blob/ea5298f615584931c389b14c81af4c1b5da6fbba/drivers/iio/industrialio-buffer.c#L1182

If I use iio_device_buffer_attr_write(mcp3913, "watermark", "20"); before iio_device_create_buffer(mcp3913, BUFFER_SIZE, false); I got invalid argument since buffer length is still 1.

If I use iio_device_buffer_attr_write(mcp3913, "watermark", "20"); afteriio_device_create_buffer(mcp3913, BUFFER_SIZE, false); I got ressource busy since buffer is already enabled.

The workaround is iio_device_create_buffer -> iio_buffer_destroy -> iio_device_buffer_attr_write -> iio_device_create_buffer but it's ugly.

[HiFiPhile]

Beside if watermark has been set, iio_device_create_buffer() could fail in setting buffer length:
https://github.com/linux4sam/linux-at91/blob/ea5298f615584931c389b14c81af4c1b5da6fbba/drivers/iio/industrialio-buffer.c#L530

[pcercuei]

@HiFiPhile why not just create a buffer of 20 samples?

[HiFiPhile]

Hi @pcercuei,

Because the watermark corresponding to the internal buffer(hardware fifo or software buffer) like this one:
https://github.com/linux4sam/linux-at91/blob/ea5298f615584931c389b14c81af4c1b5da6fbba/drivers/iio/accel/adxl372.c#L911

With watermark set the device will only generate one interrupt for multiples samples, that's reduce much CPU usage.

In my case on SAMA5D2 with a sampling frequency of 3kHz, have a watermark of 20 reduce the CPU load from 15% to 3%.

[pcercuei]

I can update the code to hide the watermark attribute, and (if it exists) always set it to the buffer size internally. So if you create a buffer of 20 samples, the watermark would be 20 samples. Would that work for you?

[HiFiPhile]

I can update the code to hide the watermark attribute,

Thank you that's very kind.

(if it exists) always set it to the buffer size internally.

By default it can be set to buffer length.

I think it's better to have a function (or parameter) to set it, since watermark can have more constraints than buffer length.
For example when hardware has a limited fifo size and a large buffer length is needed.

[pcercuei]

The hardware constraints are already taken care of in each .hwfifo_set_watermark callback on the kernel side, so setting the watermark to the buffer's size should never be a problem - in the case where it's too high, the drivers will simply use the highest possible.

[HiFiPhile]

The hardware constraints are already taken care of in each .hwfifo_set_watermark callback on the kernel side

I think not all drivers does this. ADXL372 above trim watermark to buffer length, while these simply return error if too large.
https://github.com/linux4sam/linux-[at91/blob/d0d603fcdf0c3e50f173a9a01e0228b0242c7306/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c#L1786

https://github.com/linux4sam/linux-at91/blob/ea5298f615584931c389b14c81af4c1b5da6fbba/drivers/iio/adc/at91-sama5d2_adc.c#L1572](https://github.com/linux4sam/linux-at91/blob/d0d603fcdf0c3e50f173a9a01e0228b0242c7306/drivers/iio/adc/at91-sama5d2_adc.c#L1546)

[pcercuei]

That's something that can be fixed in the kernel, though.

[HiFiPhile]

That's something that can be fixed in the kernel, though.

Yes, but not very practical. Since people using older kernel version will probably have their IIO devices stop working. And there is a need to contact all of these driver maintainers.

[pcercuei]

There is no need to contact all maintainers - I can just fix all the current upstream drivers.

Those who use a kernel driver that returns an error if the watermark is too high, never complained about a high CPU usage in the first place ;)

So in libiio I can try to set the watermark to the buffer size, and if it fails, continue without the watermark. That should be good enough for everybody. And then, slowly but surely everybody will move to a kernel that has the upstream fixes.

[pcercuei]

@HiFiPhile could you try PR #781?

Could you also try with a buffer size that is not a multiple of the maximum watermark? (e.g. 26, as your max. watermark value is 20). I am not sure how it will behave in this case.