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

[RFC] Multiple changes #32

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[RFC] Multiple changes #32

wants to merge 4 commits into from

Conversation

corna
Copy link

@corna corna commented Dec 1, 2017

  • Add axidma_get_residue()

This commit adds a new IOCTL which can be used to retrieve the residue (= how many bytes weren't transmitted/received), which can be used to calculate how many bytes were really received.
Unfortunately the Xilinx kernel is bugged at the moment and always returns 0, no matter how many bytes have been actually transmitted. As soon as I understand which is the procedure to propose kernel patches to Xilinx I'll try to get my patch merged.

  • Add MODULE_DEVICE_TABLE to automatically load the module when needed

If this module is built in-tree (Linux doesn't automatically load out-of-tree modules), a matching entry in the DT is enough to automatically load this module.

  • Add support for multiple AXI DMA devices

To accomplish this a couple of backward-incompatible changes were needed. The library interface is the same, but the IOCTLs have changed a little.

The channel_id value in the callback is now passed in the sig_info.si_errno field: this is not very clean, but it is easy to be implemented (and the si_errno field wasn't used anyways). Feel free to criticize this solution.

Tell me if you're interested in some (or all) of these commits.

corna added 3 commits November 7, 2017 13:15
The residue returned by the Xilinx VDMA implementation of
dmaengine_tx_status() is bugged and always returns 0; a kernel patch is
needed to fix the functionality (and give some meaning to this commit).
Different AXI DMA devices can be identified by a new DT property "index"
and will appear as /dev/axidma<index>. Index 0 will still appear as
/dev/axidma, to keep backwards compatibility.
@bperez77
Copy link
Owner

bperez77 commented Dec 2, 2017

Hi @corna, thanks for sharing your updates! I'm a bit busy today, but I'll take a look within the next few days.

@bperez77
Copy link
Owner

Apologies, I lost track of this and didn't get around reviewing it until now (I was reminded by a recent issue). The first two commits look good, I'm definitely interested in adding those as-is (I'll resolve that merge conflict that has appeared).

The question I have is about the third set of changes, with the multiple AXI DMA character devices. The changes with signal handling look fine, but I'm unlear why multiple character devices are needed. Following up from the discussion in #38, and from your description of the commit, it seems that there in some issue with getting multiple DMA devices to run. This has been supported since the beginning of this driver, but it seems something might've changed in the kernel that broke this.

Do you have some more details about the issue you had when trying to run multiple devices? I'd prefer not to use multiple character devices for the sake of simplicity and because it would break backwards-compatibility.

@corna
Copy link
Author

corna commented Jan 26, 2018

The character device is opened in exclusive mode, and I wanted to have different processes on the DMAs. I also think that a different character device for each DMA is the most logical choice, especially if in the future we want to add multichannel DMAs.

I won't have access to my FPGA for a month or so, so I'm unable to send you the logs or do some tests, I'm sorry.

@bperez77
Copy link
Owner

bperez77 commented Jan 30, 2018

I see. Yeah the driver also needs support for multi-threaded and multi-process applications. I haven't gotten around to adding the proper synchronization and locks.

It looks like the problem in #38 was because of a different issue, so don't worry about that.

What are the advantages you see to the character device per DMA apporach? From the end-user perspective, it shouldn't matter as long as they go through the library's interface, but I want to see your perspective on it.

@corna
Copy link
Author

corna commented Jan 31, 2018

Mainly to have one character device per process. In this way each process can work independently on its DMA, as there aren't shared resources which would require synchronization.

At the moment I'm using my patches for an AXI-TCP bidirectional bridge; two instances are running simultaneously on two DMAs, without issues.

@bperez77
Copy link
Owner

Got it, yeah unfortunately that's a big downside with the driver is the lack of handling inter-process and inter-thread synchronization. When I first wrote the driver, I was on a tight deadline for a class project, and I decided to skip doing that.

Having a file for each AXI DMA engine is a good intermediate workaround, but long term I prefer keeping everything in one character device and uses proper synchronization to allow multiple processes to access the DMA engines.

If you don't mind I'd like to take the residue and MODULE_DEVICE_TABLE commits, but not the multi-process support commit. It's definitely very useful, but I want to maintain backward compatibility in terms of the clean interface of having a single character device. There will definitely people who will need multi-process support, so I'll add a reference to your fork in the README.

@bperez77
Copy link
Owner

The updates to the README are in b7d2ea0.

@corna
Copy link
Author

corna commented Feb 28, 2018

No problem, I already knew it was a questionable big change.

I'm currently in contact with the Xilinx kernel devs, which have pointed out that the residue is supposed to be reported only when the DMA transaction is in progress

Please refer kernel-doc- include/linux/dmaengine.h
 * @residue: the remaining number of bytes left to transmit
 * on the selected transfer for states DMA_IN_PROGRESS and
 * DMA_PAUSED if this is implemented in the driver, else 0 */ struct
 dma_tx_state { dma_cookie_t last; dma_cookie_t used;
 u32 residue;
 };

We're figuring out a way to report this kind of information, so I suggest you to avoid merging the residue commit for now.

Thanks for the mention in the README btw.

@bperez77
Copy link
Owner

bperez77 commented Mar 4, 2018

Got it, yeah I guess I'll wait to merge the changes in until that issue is resolved. And no problem.

@bperez77 bperez77 requested review from bperez77 and removed request for bperez77 March 4, 2018 20:53
@Livius90
Copy link

Livius90 commented Jul 24, 2021

Is there are any plans to introduce the Multi-Process Support? I realy want to use AXI MCDMA with 3 tx channels in parallel threads. Can you improve this driver with multi-channel mode? AXI MCDMA is available since 2019 in Xiilinx DMA driver in Linux, it is time to get it in operation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants