Hi Russell, On Friday 01 August 2014 15:30:20 Russell King - ARM Linux wrote: > On Fri, Aug 01, 2014 at 10:51:26AM +0200, Laurent Pinchart wrote: > > I'll take this opportunity to question why we have a separation between > > tx_submit and issue_pending. What's the rationale for that, especially > > given that dma_issue_pending_all() might kick in at any point and issue > > pending transfers for all devices. A driver could thus see its submitted > > but not issued transactions being issued before it explicitly calls > > dma_async_issue_pending(). > > A prepared but not submitted transaction is not a pending transaction. > > The split is necessary so that a callback can be attached to the > transaction. This partially comes from the async-tx API, and also > gets a lot of use with the slave API. > > The prepare function allocates the descriptor and does the initial > setup, but does not mark the descriptor as a pending transaction. > It returns the descriptor, and the caller is then free to add a > callback function and data pointer to the descriptor before finally > submitting it. No disagreement on that. However, as Geert pointed out, my question was related to the split between dmaengine_submit() and dma_async_issue_pending(), not between the prep_* functions and dmaengine_submit(). > This sequence must occur in a timely manner as some DMA engine > implementations hold a lock between the prepare and submit callbacks (Dan > explicitly permits this as part of the API.) That really triggers a red alarm in the part of my brain that deals with API design, but I suppose it would be too difficult to change that. > > The DMA_PRIVATE capability flag seems to play a role here, but it's far > > from being clear how that mechanism is supposed to work. This should be > > documented as well, and any light you could shed on this dark corner of > > the API would help. > > Why do you think that DMA_PRIVATE has a bearing in the callbacks? It > doesn't. Not on callbacks, but on how pending descriptors are pushed to the hardware. The flag is explicitly checked in dma_issue_pending_all(). > DMA_PRIVATE is all about channel allocation as I explained yesterday, and > whether the channel is available for async_tx usage. > > A channel marked DMA_PRIVATE is not available for async_tx usage at > any moment. A channel without DMA_PRIVATE is available for async_tx > usage until it is allocated for the slave API - at which point the > generic DMA engine code will mark the channel with DMA_PRIVATE, > thereby taking it away from async_tx API usage. When the slave API > frees the channel, DMA_PRIVATE will be cleared, making the channel > available for async_tx usage. > > So, basically, DMA_PRIVATE set -> async_tx usage not allowed. > DMA_PRIVATE clear -> async_tx usage permitted. It really is that > simple. DMA_PRIVATE is a dma_device flag, not a dma_chan flag. As soon as one channel is allocated by __dma_request_channel() the whole device is marked with DMA_PRIVATE, making all channels private. What am I missing ? > > Similarly, the DMA engine API is split in functions with different > > prefixes (mostly dmaengine_*, dma_async_*, dma_*, and various > > unprefixed niceties such as async_tx_ack or txd_lock. If there's a > > rationale for that (beyond just historical reasons) it should also > > be documented, otherwise a cleanup would help all the confused DMA > > engine users (myself included). > > dmaengine_* are generally the interface functions to the DMA engine code, > which have been recently introduced to avoid the multiple levels of > pointer indirection having to be typed in every driver. > > dma_async_* are the DMA engine interface functions for the async_tx API. > > dma_* predate the dmaengine_* naming, and are probably too generic, so > should probably end up being renamed to dmaengine_*. Thank you for the confirmation. I'll see if I can cook up a patch. It will likely be pretty large and broad though, but I guess there's no way around it. > txd_* are all about the DMA engine descriptors. > > async_tx_* are the higher level async_tx API functions. Thank you for the information. How about the dma_async_* functions, should they be renamed to dmaengine_* as well ? Or are some of them part of the async_tx_* API ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
