Xilinx / dma_ip_drivers

Xilinx QDMA IP Drivers
https://xilinx.github.io/dma_ip_drivers/
581 stars 421 forks source link

XDMA: Add lseek() to all interfaces and add sanity checks for addresses #299

Open Prandr opened 2 weeks ago

Prandr commented 2 weeks ago

XDMA driver doesn't implement lseek for AXI Lite interface, which is not only completely illogical, because it is supposed to set the AXI addresses, but potentially problematic, because since version 6.0 Linux kernel no longer checks for null f_ops.

This pull request adds an implementation of lseek to all interface types. Additionally, SEEK_END properly sets address from the end of the address space. This was done by adding suitable entry to thexdma_dev structure.

This allowed to consolidate and add sanity checks for addresses that:

Please have a look, because I am not a Linux kernel driver expert, and also test for memory-mapped DMA and AXI Bridge (so called DMA Bypass)

hinxx commented 2 weeks ago

I'm not sure having seek is useful in the case of user BAR. As pointed out in the https://github.com/Xilinx/dma_ip_drivers/issues/96#issuecomment-859010927 one should use mmap instead of read/write/seek. For example, in our designs the user BAR is 16MB of CSR (32-bit registers) space and we are accessing them via mmap. This is the fastest and most straightforward way to work with the registers, AFAIK, because read/write/seek introduce more syscalls and potential sleeping points due to scheduling.

which is not only completely illogical, because it is supposed to set the AXI addresses

I'm not sure what you mean with this. I never had any problems working with the user BAR so far.

FWIW, Xilinx (today AMD) has not been maintaining this repo for a long time now hence any contributions are not being merged.

[AMD managed to get XDMA DMA engine in upstream Linux kernel GIT couple of months ago and that seems to be the path forward for them. Sadly, DMA engine is not usable as an end product; one needs to create a device specific driver around the DMA engine. That is why this driver is still useful - it allows working with XDMA from userspace. OTOH, this is also its limitation ie. one can not use it to expose a v4l device from kernel or similar. We still find it useful for our application, though.]

Prandr commented 2 weeks ago

I'm not sure having seek is useful in the case of user BAR. As pointed out in the #96 (comment) one should use mmap instead of read/write/seek. For example, in our designs the user BAR is 16MB of CSR (32-bit registers) space and we are accessing them via mmap. This is the fastest and most straightforward way to work with the registers, AFAIK, because read/write/seek introduce more syscalls and potential sleeping points due to scheduling.

This is not true. As I explain in the reworked answer in the linked thread, already upstream Xilinx DMA implements read/write functions, that enable access with POSIX preadand pwrite. However, if one wants to use simple writeand read, lseekis necessary to set the AXI-Lite address. This is particularly useful when accessing from other languages, like Java, which call standard read/write.

In any case there are no side effects of this pull request. All other access methods remain available.

FWIW, Xilinx (today AMD) has not been maintaining this repo for a long time now hence any contributions are not being merged.

I know that. This pull request is primarily to give other community members a chance to look at and check implementation. And perhaps someone even finds it useful, unlike you

[AMD managed to get XDMA DMA engine in upstream Linux kernel GIT couple of months ago and that seems to be the path forward for them. Sadly, DMA engine is not usable as an end product; one needs to create a device specific driver around the DMA engine. That is why this driver is still useful - it allows working with XDMA from userspace. OTOH, this is also its limitation ie. one can not use it to expose a v4l device from kernel or similar. We still find it useful for our application, though.]

This is good to know. Do you mean https://github.com/torvalds/linux/blob/master/drivers/dma/xilinx/xdma.c Looks like a cut down version that does not define any file operations, not even ioctl.