Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to set buffer watermark correctly #780

Closed
HiFiPhile opened this issue Jan 14, 2022 · 11 comments · Fixed by #781
Closed

Unable to set buffer watermark correctly #780

HiFiPhile opened this issue Jan 14, 2022 · 11 comments · Fixed by #781

Comments

@HiFiPhile
Copy link

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
Copy link
Author

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
Copy link
Contributor

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

@HiFiPhile
Copy link
Author

HiFiPhile commented Jan 14, 2022

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

@pcercuei
Copy link
Contributor

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

@HiFiPhile
Copy link
Author

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
Copy link
Contributor

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 added a commit that referenced this issue Jan 17, 2022
Add support for setting the watermark value, if the driver supports it.
The watermark value represents the number of samples that will be read
in one burst from the hardware. Since we read data at the granularity of
iio_buffer blocks, it makes sense to use the highest watermark value up
to the buffer"s size. If the value is too high, the driver will
automatically adjust it to the maximum watermark value possible.

Fixes #780.

Signed-off-by: Paul Cercueil <[email protected]>
@pcercuei
Copy link
Contributor

pcercuei commented Jan 17, 2022

@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.

pcercuei added a commit that referenced this issue Jan 18, 2022
Add support for setting the watermark value, if the driver supports it.
The watermark value represents the number of samples that will be read
in one burst from the hardware. Since we read data at the granularity of
iio_buffer blocks, it makes sense to use the highest watermark value up
to the buffer"s size. If the value is too high, the driver will
automatically adjust it to the maximum watermark value possible.

Fixes #780.

Signed-off-by: Paul Cercueil <[email protected]>
pcercuei added a commit that referenced this issue Jan 28, 2022
Add support for setting the watermark value, if the driver supports it.
The watermark value represents the number of samples that will be read
in one burst from the hardware. Since we read data at the granularity of
iio_buffer blocks, it makes sense to use the highest watermark value up
to the buffer"s size. If the value is too high, the driver will
automatically adjust it to the maximum watermark value possible.

Fixes #780.

Signed-off-by: Paul Cercueil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants