The dmaengine is neither trivial nor properly documented at the moment, which
means a lot of trial and error development, which is not that good for such a
central piece of the system.
Attempt at making such a documentation.
Signed-off-by: Maxime Ripard <[email protected]>
---
Documentation/dmaengine-driver.txt | 293 +++++++++++++++++++++++++++++++++++++
1 file changed, 293 insertions(+)
create mode 100644 Documentation/dmaengine-driver.txt
diff --git a/Documentation/dmaengine-driver.txt b/Documentation/dmaengine-driver.txt
new file mode 100644
index 000000000000..4828b50038c0
--- /dev/null
+++ b/Documentation/dmaengine-driver.txt
@@ -0,0 +1,293 @@
+DMAengine controller documentation
+==================================
+
+Hardware Introduction
++++++++++++++++++++++
+
+Most of the Slave DMA controllers have the same general principles of
+operations.
+
+They have a given number of channels to use for the DMA transfers, and
+a given number of requests lines.
+
+Requests and channels are pretty much orthogonal. Channels can be used
+to serve several to any requests. To simplify, channels are the
+entities that will be doing the copy, and requests what endpoints are
+involved.
+
+The request lines actually correspond to physical lines going from the
+DMA-elligible devices to the controller itself. Whenever the device
+will want to start a transfer, it will assert a DMA request (DRQ) by
+asserting that request line.
+
+A very simple DMA controller would only take into account a single
+parameter: the transfer size. At each clock cycle, it would transfer a
+byte of data from one buffer to another, until the transfer size has
+been reached.
+
+That wouldn't work well in the real world, since slave devices might
+require to have to retrieve various number of bits from memory at a
+time. For example, we probably want to transfer 32 bits at a time when
+doing a simple memory copy operation, but our audio device will
+require to have 16 or 24 bits written to its FIFO. This is why most if
+not all of the DMA controllers can adjust this, using a parameter
+called the width.
+
+Moreover, some DMA controllers, whenever the RAM is involved, can
+group the reads or writes in memory into a buffer, so instead of
+having a lot of small memory accesses, which is not really efficient,
+you'll get several bigger transfers. This is done using a parameter
+called the burst size, that defines how many single reads/writes it's
+allowed to do in a single clock cycle.
+
+Our theorical DMA controller would then only be able to do transfers
+that involve a single contiguous block of data. However, some of the
+transfers we usually have are not, and want to copy data from
+non-contiguous buffers to a contiguous buffer, which is called
+scatter-gather.
+
+DMAEngine, at least for mem2dev transfers, require support for
+scatter-gather. So we're left with two cases here: either we have a
+quite simple DMA controller that doesn't support it, and we'll have to
+implement it in software, or we have a more advanced DMA controller,
+that implements in hardware scatter-gather.
+
+The latter are usually programmed using a collection of chunks to
+transfer, and whenever the transfer is started, the controller will go
+over that collection, doing whatever we programmed there.
+
+This collection is usually either a table or a linked list. You will
+then push either the address of the table and its number of elements,
+or the first item of the list to one channel of the DMA controller,
+and whenever a DRQ will be asserted, it will go through the collection
+to know where to fetch the data from.
+
+Either way, the format of this collection is completely dependent of
+your hardware. Each DMA controller will require a different structure,
+but all of them will require, for every chunk, at least the source and
+destination addresses, wether it should increment these addresses or
+not and the three parameters we saw earlier: the burst size, the bus
+width and the transfer size.
+
+The one last thing is that usually, slave devices won't issue DRQ by
+default, and you have to enable this in your slave device driver first
+whenever you're willing to use DMA.
+
+These were just the general memory-to-memory (also called mem2mem) or
+memory-to-device (mem2dev) transfers. Other kind of transfers might be
+offered by your DMA controller, and are probably already supported by
+dmaengine.
+
+DMAEngine Registration
+++++++++++++++++++++++
+
+struct dma_device Initialization
+--------------------------------
+
+Just like any other kernel framework, the whole DMAEngine registration
+relies on the driver filling a structure and registering against the
+framework. In our case, that structure is dma_device.
+
+The first thing you need to do in your driver is to allocate this
+structure. Any of the usual memory allocator will do, but you'll also
+need to initialize a few fields in there:
+
+ * chancnt: should be the number of channels your driver is exposing
+ to the system.
+ This doesn't have to be the number of physical
+ channels: some DMA controllers also expose virtual
+ channels to the system to overcome the case where you
+ have more consumers than physical channels available.
+
+ * channels: should be initialized as a list using the
+ INIT_LIST_HEAD macro for example
+
+ * dev: should hold the pointer to the struct device associated
+ to your current driver instance.
+
+Supported transaction types
+---------------------------
+The next thing you need is to actually set which transaction type your
+device (and driver) supports.
+
+Our dma_device structure has a field called caps_mask that holds the
+various type of transaction supported, and you need to modify this
+mask using the dma_cap_set function, with various flags depending on
+transaction types you support as an argument.
+
+All those capabilities are defined in the dma_transaction_type enum,
+in include/linux/dmaengine.h
+
+Currently, the types available are:
+ * DMA_MEMCPY
+ - The device is able to do memory to memory copies
+
+ * DMA_XOR
+ - The device is able to perform XOR operations on memory areas
+ - Particularly useful to accelerate XOR intensive tasks, such as
+ RAID5
+
+ * DMA_XOR_VAL
+ - The device is able to perform parity check using the XOR
+ algorithm against a memory buffer.
+
+ * DMA_PQ
+ - The device is able to perform RAID6 P+Q computations, P being a
+ simple XOR, and Q being a Reed-Solomon algorithm.
+
+ * DMA_PQ_VAL
+ - The device is able to perform parity check using RAID6 P+Q
+ algorithm against a memory buffer.
+
+ * DMA_INTERRUPT
+ /* TODO: Is it that the device has one interrupt per channel? */
+
+ * DMA_SG
+ - The device supports memory to memory scatter-gather
+ transfers.
+ - Even though a plain memcpy can look like a particular case of a
+ scatter-gather transfer, with a single chunk to transfer, it's a
+ distinct transaction type in the mem2mem transfers case
+
+ * DMA_PRIVATE
+ - The device can have several client at a time, most likely
+ because it has several parallel channels.
+
+ * DMA_ASYNC_TX
+ - Must not be set by the device, and will be set by the framework
+ if needed
+ - /* TODO: What is it about? */
+
+ * DMA_SLAVE
+ - The device can handle device to memory transfers, including
+ scatter-gather transfers.
+ - While in the mem2mem case we were having two distinct type to
+ deal with a single chunk to copy or a collection of them, here,
+ we just have a single transaction type that is supposed to
+ handle both.
+
+ * DMA_CYCLIC
+ - The device can handle cyclic transfers.
+ - A cyclic transfer is a transfer where the chunk collection will
+ loop over itself, with the last item pointing to the first. It's
+ usually used for audio transfers, where you want to operate on a
+ single big buffer that you will fill with your audio data.
+
+ * DMA_INTERLEAVE
+ - The device support interleaved transfer. Those transfers usually
+ involve an interleaved set of data, with chunks a few bytes
+ wide, where a scatter-gather transfer would be quite
+ inefficient.
+
+These various types will also affect how the source and destination
+addresses change over time, as DMA_SLAVE transfers will usually have
+one of the addresses that will increment, while the other will not,
+DMA_CYCLIC will have one address that will loop, while the other, will
+not change, etc.
+
+Device operations
+-----------------
+
+Our dma_device structure also requires a few function pointers in
+order to implement the actual logic, now that we described what
+operations we were able to perform.
+
+The functions that we have to fill in there, and hence have to
+implement, obviously depend on the transaction type you reported as
+supported.
+
+ * device_alloc_chan_resources
+ * device_free_chan_resources
+ - These functions will be called whenever a driver will call
+ dma_request_channel or dma_release_channel for the first/last
+ time on the channel associated to that driver.
+ - They are in charge of allocating/freeing all the needed
+ resources in order for that channel to be useful for your
+ driver.
+ - These functions can sleep.
+
+ * device_prep_dma_*
+ - These functions are matching the capabilities you registered
+ previously.
+ - These functions all take the buffer or the scatterlist relevant
+ for the transfer being prepared, and should create a hardware
+ descriptor or a list of descriptors from it
+ - These functions can be called from an interrupt context
+ - Any allocation you might do should be using the GFP_NOWAIT
+ flag, in order not to potentially sleep, but without depleting
+ the emergency pool either.
+
+ - It should return a unique instance of the
+ dma_async_tx_descriptor structure, that further represents this
+ particular transfer.
+
+ - This structure can be allocated using the function
+ dma_async_tx_descriptor_init.
+ - You'll also need to set two fields in this structure:
+ + flags:
+ TODO: Can it be modified by the driver itself, or
+ should it be always the flags passed in the arguments
+
+ + tx_submit: A pointer to a function you have to implement,
+ that is supposed to push the current descriptor
+ to a pending queue, waiting for issue_pending to
+ be called.
+
+ * device_issue_pending
+ - Takes the first descriptor in the pending queue, and start the
+ transfer. Whenever that transfer is done, it should move to the
+ next transaction in the list.
+ - It should call the registered callback if any each time a
+ transaction is done.
+ - This function can be called in an interrupt context
+
+ * device_tx_status
+ - Should report the bytes left to go over in the current transfer
+ for the given channel
+ - Should use dma_set_residue to report it
+ - In the case of a cyclic transfer, it should only take into
+ account the current period.
+ - This function can be called in an interrupt context.
+
+ * device_control
+ - Used by client drivers to control and configure the channel it
+ has a handle on.
+ - Called with a command and an argument
+ + The command is one of the values listed by the enum
+ dma_ctrl_cmd. To this date, the valid commands are:
+ + DMA_RESUME
+ + Restarts a transfer on the channel
+ + DMA_PAUSE
+ + Pauses a transfer on the channel
+ + DMA_TERMINATE_ALL
+ + Aborts all the pending and ongoing transfers on the
+ channel
+ + DMA_SLAVE_CONFIG
+ + Reconfigures the channel with passed configuration
+ + FSLDMA_EXTERNAL_START
+ + TODO: Why does that even exist?
+ + The argument is an opaque unsigned long. This actually is a
+ pointer to a struct dma_slave_config that should be used only
+ in the DMA_SLAVE_CONFIG.
+
+Misc notes (stuff that should be documented, but don't really know
+what to say about it)
+------------------------------------------------------------------
+ * dma_run_dependencies
+ - What is it supposed to do/when should it be called?
+ - Some drivers seems to implement it at the end of a transfer, but
+ not all of them do, so it seems we can get away without it
+
+ * device_slave_caps
+ - Isn't that redundant with the cap_mask already?
+ - Only a few drivers seem to implement it
+
+ * dma cookies?
+
+Glossary
+--------
+
+Burst: Usually a few contiguous bytes that will be transfered
+ at once by the DMA controller
+Chunk: A contiguous collection of bursts
+Transfer: A collection of chunks (be it contiguous or not)
--
2.0.2
On Wed, Jul 30, 2014 at 06:03:13PM +0200, Maxime Ripard wrote:
> The dmaengine is neither trivial nor properly documented at the moment, which
> means a lot of trial and error development, which is not that good for such a
> central piece of the system.
>
> Attempt at making such a documentation.
Did you miss Documentation/dmaengine.txt, lots of this is already covered
there. But yes i would be really glad to know what isnt, so that we can fix
that.
> + * device_slave_caps
> + - Isn't that redundant with the cap_mask already?
> + - Only a few drivers seem to implement it
For audio to know what your channel can do rather than hardcoding it
> +
> + * dma cookies?
cookie is dma transaction representation which is monotonically incrementing
number.
--
~Vinod
Hi Vinod,
On Wed, Jul 30, 2014 at 09:36:07PM +0530, Vinod Koul wrote:
> On Wed, Jul 30, 2014 at 06:03:13PM +0200, Maxime Ripard wrote:
> > The dmaengine is neither trivial nor properly documented at the moment, which
> > means a lot of trial and error development, which is not that good for such a
> > central piece of the system.
> >
> > Attempt at making such a documentation.
>
> Did you miss Documentation/dmaengine.txt, lots of this is already covered
> there. But yes i would be really glad to know what isnt, so that we can fix
> that.
I didn't miss it. But I feel like it describes quite nicely the slave
API, but doesn't help at all whenever you're writing a DMAengine driver.
The first lines of the existing document makes it quite clear too.
There's still a bit of duplication, but I don't feel it's such a big
deal.
What I'd like to do with the documentation I just sent is basically
have a clear idea whenever you step into dmaengine what you can/cannot
do, and have a reference document explaining what's expected by the
framework, and hopefully have unified drivers that follow this
pattern.
Because, for the moment, we're pretty much left in the dark with
different drivers doing the same thing in completetely different ways,
with basically no way to tell if it's either the framework that
requires such behaviour, or if the author was just feeling creative.
There's numerous examples for this at the moment:
- The GFP flags, with different drivers using either GFP_ATOMIC,
GFP_NOWAIT or GFP_KERNEL in the same functions
- Having to set device_slave_caps or not?
- Some drivers use dma_run_depedencies, some other don't
- That might just be my experience, but judging from previous
commits, DMA_PRIVATE is completely obscure, and we just set it
because it was making it work, without knowing what it was
supposed to do.
- etc.
And basically, we have no way to tell at the moment which one is
right and which one needs fixing.
The corollary being that it cripples the whole community ability to
maintain the framework and make it evolve.
> > + * device_slave_caps
> > + - Isn't that redundant with the cap_mask already?
> > + - Only a few drivers seem to implement it
> For audio to know what your channel can do rather than hardcoding it
Ah, yes, I see it now. It's not related to the caps mask at all.
Just out of curiosity, wouldn't it be better to move this to the
framework, and have these informations provided through the struct
dma_device? Or would it have some non-trivial side-effects?
> > + * dma cookies?
> cookie is dma transaction representation which is monotonically incrementing
> number.
Ok, and it identifies a unique dma_async_tx_descriptor, right?
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On Thu, Jul 31, 2014 at 09:44:40AM +0200, Maxime Ripard wrote:
> Hi Vinod,
>
> On Wed, Jul 30, 2014 at 09:36:07PM +0530, Vinod Koul wrote:
> > On Wed, Jul 30, 2014 at 06:03:13PM +0200, Maxime Ripard wrote:
> > > The dmaengine is neither trivial nor properly documented at the moment, which
> > > means a lot of trial and error development, which is not that good for such a
> > > central piece of the system.
> > >
> > > Attempt at making such a documentation.
> >
> > Did you miss Documentation/dmaengine.txt, lots of this is already covered
> > there. But yes i would be really glad to know what isnt, so that we can fix
> > that.
>
> I didn't miss it. But I feel like it describes quite nicely the slave
> API, but doesn't help at all whenever you're writing a DMAengine driver.
>
> The first lines of the existing document makes it quite clear too.
>
> There's still a bit of duplication, but I don't feel it's such a big
> deal.
And that made me think that you might have missed it.
I am okay for idea to have this document but it needs to co-exist one. No
point in duplicating as it can create ambiguity in future.
>
> What I'd like to do with the documentation I just sent is basically
> have a clear idea whenever you step into dmaengine what you can/cannot
> do, and have a reference document explaining what's expected by the
> framework, and hopefully have unified drivers that follow this
> pattern.
Sure, can you pls modify this to avoid duplication. I would be happy to
apply that :)
>
> Because, for the moment, we're pretty much left in the dark with
> different drivers doing the same thing in completetely different ways,
> with basically no way to tell if it's either the framework that
> requires such behaviour, or if the author was just feeling creative.
>
> There's numerous examples for this at the moment:
> - The GFP flags, with different drivers using either GFP_ATOMIC,
> GFP_NOWAIT or GFP_KERNEL in the same functions
> - Having to set device_slave_caps or not?
> - Some drivers use dma_run_depedencies, some other don't
> - That might just be my experience, but judging from previous
> commits, DMA_PRIVATE is completely obscure, and we just set it
> because it was making it work, without knowing what it was
> supposed to do.
> - etc.
Thanks for highlighting we should definitely add these in Documentation
>
> And basically, we have no way to tell at the moment which one is
> right and which one needs fixing.
>
> The corollary being that it cripples the whole community ability to
> maintain the framework and make it evolve.
>
> > > + * device_slave_caps
> > > + - Isn't that redundant with the cap_mask already?
> > > + - Only a few drivers seem to implement it
> > For audio to know what your channel can do rather than hardcoding it
>
> Ah, yes, I see it now. It's not related to the caps mask at all.
>
> Just out of curiosity, wouldn't it be better to move this to the
> framework, and have these informations provided through the struct
> dma_device? Or would it have some non-trivial side-effects?
Well the problem is ability to have this queried uniformly from all drivers
across subsystems. If we can do this that would be nice.
>
> > > + * dma cookies?
> > cookie is dma transaction representation which is monotonically incrementing
> > number.
>
> Ok, and it identifies a unique dma_async_tx_descriptor, right?
Yup and this basically represents transactions you have submitted. Thats why
cookie is allocated at tx_submit.
Thanks
--
~Vinod
--
On 07/31/2014 09:44 AM, Maxime Ripard wrote:
[...]
> - Having to set device_slave_caps or not?
Yes. This should in my opinion be mandatory for new drivers. One of the
issues with the DMAengine API is that it is really hard to write generic
drivers that do not already know about the capabilities of the DMA
controller. E.g. if you have a peripheral that is used on SoC A it assumes
that the DMA controller it is connected to has the capabilities of the DMA
controller in SoC A. If the same peripheral is now used on SoC B with a DMA
controller with different capabilities this often ends up with ugly ifdefery
in the peripheral driver. The peripheral driver shouldn't have to know which
specific DMA controller it is connected to (It's a bit as if a GPIO consumer
needed to know which GPIO controller is connected to). We got away with the
current approach since there is not that much diversity in the mixing of
peripherals and DMA controllers (each vendor pretty has their own DMA
controller which it uses for their own peripherals). But with more recent
code consolidation we are on a path to generic DMA support within subsystem
frameworks (there is generic DMA support for audio, there is generic DMA
support for SPI and I also have a (currently) out of tree patch for generic
DMA support for IIO). Also these generic drivers need to be able to discover
the capabilities of the DMA controller to be able to make the right decisions.
- Lars
On Thu, Jul 31, 2014 at 09:44:40AM +0200, Maxime Ripard wrote:
> I didn't miss it. But I feel like it describes quite nicely the slave
> API, but doesn't help at all whenever you're writing a DMAengine driver.
There's actually two documents:
Documentation/crypto/async-tx-api.txt
Documentation/dmaengine.txt
The first is the original DMA engine API for use with offloading things
like memcpy, as well as the crypto API to hardware crypto DMA engines.
The second is documentation for the DMA slave API which modifies some
of the requirements of the first (like, the ability to call the slave
DMA prepare functions from within the callback, which are not permitted
in the async tx API.)
Both documents are relevant for writing a DMA engine driver.
> Because, for the moment, we're pretty much left in the dark with
> different drivers doing the same thing in completetely different ways,
> with basically no way to tell if it's either the framework that
> requires such behaviour, or if the author was just feeling creative.
DMA engine has lacked a lot of infrastructure for common patterns in
drivers; some of that is solved by my efforts with the virt_dma.c
support, and also various cleanups to existing drivers, but it's not
easy to fix this problem after drivers have been merged.
> There's numerous examples for this at the moment:
> - The GFP flags, with different drivers using either GFP_ATOMIC,
> GFP_NOWAIT or GFP_KERNEL in the same functions
The slave prepare APIs should be using GFP_ATOMIC to ensure safety.
> - Having to set device_slave_caps or not?
device_slave_caps is a relatively new invention, so old drivers don't
implement it. Newer drivers should, and there probably should be some
motivation for older drivers to add it.
> - Some drivers use dma_run_depedencies, some other don't
Dependent operations are part of the async-tx API, and aren't really
applicable to the slave DMA API. A driver implementing just the slave
DMA API need not concern itself with dependent operations, but a
driver implementing the async-tx APIs should do.
> - That might just be my experience, but judging from previous
> commits, DMA_PRIVATE is completely obscure, and we just set it
> because it was making it work, without knowing what it was
> supposed to do.
DMA_PRIVATE means that the channel is unavailable for async-tx operations
- in other words, it's slave-only. Setting it before registration results
in the private-use count being incremented, disabling the DMA_PRIVATE
manipulation in the channel request/release APIs (requested channels are
unavailable for async-tx operations, which is why that flag is also set
there.)
To put it another way, if a DMA engine driver only provides slave DMA
support, it must set DMA_PRIVATE to mark the channel unavailable for
async-tx operations. If a DMA engine drivers channels can also be used
for async-tx operations, then it should leave the flag clear.
> And basically, we have no way to tell at the moment which one is
> right and which one needs fixing.
>
> The corollary being that it cripples the whole community ability to
> maintain the framework and make it evolve.
All respect to Vinod (who looks after the slave side of the DMA engine
implementations) and Dan, what it needs is someone who knows the *whole*
DMA engine picture to be in control of the subsystem, and who knows these
details - not must one bit of it. Someone who has the time to put into
reviewing changes to it, and probably more importantly, has the time to
clean up the existing code. Finding someone who fits that is a struggle.
Dan knows the whole picture (or used to when he was working on it at one
of his former employers) but I don't think Dan had the available time to
address various issues with it.
> > cookie is dma transaction representation which is monotonically incrementing
> > number.
>
> Ok, and it identifies a unique dma_async_tx_descriptor, right?
You really should not be concerned with DMA cookies anymore since one
of my cleanups - I added a number of helper functions which hide the
manipulation of DMA cookies, and take away from driver writers the
semantics of how cookies themselves are operated upon. virt-dma goes
a little further and provides descriptor lists and methods to look up
a descriptor given a cookie.
The only thing that a driver writer should concern themselves with is
making the appropriate cookie management calls at the relevant point in
the code - in other words, allocating a cookie at in the submit callback,
completing a cookie when a descriptor is complete, etc.
Drivers should have no reason what so ever to be touching the cookie
directly anymore.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
Hi Lars-Peter,
On Thu, Jul 31, 2014 at 02:44:56PM +0200, Lars-Peter Clausen wrote:
> On 07/31/2014 09:44 AM, Maxime Ripard wrote:
> [...]
> > - Having to set device_slave_caps or not?
>
> Yes. This should in my opinion be mandatory for new drivers.
Ok.
> One of the issues with the DMAengine API is that it is really hard
> to write generic drivers that do not already know about the
> capabilities of the DMA controller. E.g. if you have a peripheral
> that is used on SoC A it assumes that the DMA controller it is
> connected to has the capabilities of the DMA controller in SoC A. If
> the same peripheral is now used on SoC B with a DMA controller with
> different capabilities this often ends up with ugly ifdefery in the
> peripheral driver. The peripheral driver shouldn't have to know
> which specific DMA controller it is connected to (It's a bit as if a
> GPIO consumer needed to know which GPIO controller is connected
> to). We got away with the current approach since there is not that
> much diversity in the mixing of peripherals and DMA controllers
> (each vendor pretty has their own DMA controller which it uses for
> their own peripherals). But with more recent code consolidation we
> are on a path to generic DMA support within subsystem frameworks
> (there is generic DMA support for audio, there is generic DMA
> support for SPI and I also have a (currently) out of tree patch for
> generic DMA support for IIO). Also these generic drivers need to be
> able to discover the capabilities of the DMA controller to be able
> to make the right decisions.
Yeah, I've seen the generic infrastructure in both ASoC and SPI, and
it's great that it's coming to IIO as well.
I wasn't aware that it was relying on device_slave_caps though, and
been mislead by the caps name into thinking that it was related to the
caps_mask, which is obviously not.
From what you're saying, and judging from the drivers that already
implement it, can't it be moved directly to the framework itself ?
The informations put there could be either used elsewhere (like
framework-level filtering of invalid directions/bus width) or could be
derived directly from which callbacks are set (in the pause/terminate
case)?
I guess that would make generic layer much easier to write, since
you'll always have this information.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On Thu, Jul 31, 2014 at 05:26:28PM +0530, Vinod Koul wrote:
> On Thu, Jul 31, 2014 at 09:44:40AM +0200, Maxime Ripard wrote:
> > Hi Vinod,
> >
> > On Wed, Jul 30, 2014 at 09:36:07PM +0530, Vinod Koul wrote:
> > > On Wed, Jul 30, 2014 at 06:03:13PM +0200, Maxime Ripard wrote:
> > > > The dmaengine is neither trivial nor properly documented at the moment, which
> > > > means a lot of trial and error development, which is not that good for such a
> > > > central piece of the system.
> > > >
> > > > Attempt at making such a documentation.
> > >
> > > Did you miss Documentation/dmaengine.txt, lots of this is already covered
> > > there. But yes i would be really glad to know what isnt, so that we can fix
> > > that.
> >
> > I didn't miss it. But I feel like it describes quite nicely the slave
> > API, but doesn't help at all whenever you're writing a DMAengine driver.
> >
> > The first lines of the existing document makes it quite clear too.
> >
> > There's still a bit of duplication, but I don't feel it's such a big
> > deal.
> And that made me think that you might have missed it.
>
> I am okay for idea to have this document but it needs to co-exist one. No
> point in duplicating as it can create ambiguity in future.
The only duplication I'm seeing is about the device_prep* operations,
that get described in dmaengine.txt too.
There's also a minor one about struct dma_slave_config, but both are
rather generic and point to dmaengine.h, so I guess we won't be at
risk of any real ambiguity.
Do you see anything else?
> > What I'd like to do with the documentation I just sent is basically
> > have a clear idea whenever you step into dmaengine what you can/cannot
> > do, and have a reference document explaining what's expected by the
> > framework, and hopefully have unified drivers that follow this
> > pattern.
> Sure, can you pls modify this to avoid duplication. I would be happy to
> apply that :)
See above :)
Also, feel free to add anything that you feel like you keep saying
during the review. If mistakes keep coming, it's probably worth
documenting what you expect.
> > Because, for the moment, we're pretty much left in the dark with
> > different drivers doing the same thing in completetely different ways,
> > with basically no way to tell if it's either the framework that
> > requires such behaviour, or if the author was just feeling creative.
> >
> > There's numerous examples for this at the moment:
> > - The GFP flags, with different drivers using either GFP_ATOMIC,
> > GFP_NOWAIT or GFP_KERNEL in the same functions
> > - Having to set device_slave_caps or not?
> > - Some drivers use dma_run_depedencies, some other don't
> > - That might just be my experience, but judging from previous
> > commits, DMA_PRIVATE is completely obscure, and we just set it
> > because it was making it work, without knowing what it was
> > supposed to do.
> > - etc.
>
> Thanks for highlighting we should definitely add these in Documentation
It's quite clear in the case of the GFP flags now, Lars-Peter and you
cleared up device_slave_caps, but I still could use some help with
DMA_PRIVATE.
> > And basically, we have no way to tell at the moment which one is
> > right and which one needs fixing.
> >
> > The corollary being that it cripples the whole community ability to
> > maintain the framework and make it evolve.
> >
> > > > + * device_slave_caps
> > > > + - Isn't that redundant with the cap_mask already?
> > > > + - Only a few drivers seem to implement it
> > > For audio to know what your channel can do rather than hardcoding it
> >
> > Ah, yes, I see it now. It's not related to the caps mask at all.
> >
> > Just out of curiosity, wouldn't it be better to move this to the
> > framework, and have these informations provided through the struct
> > dma_device? Or would it have some non-trivial side-effects?
> Well the problem is ability to have this queried uniformly from all drivers
> across subsystems. If we can do this that would be nice.
I can work on some premelinary work to do just that, and see if it
works for you then.
> > > > + * dma cookies?
> > > cookie is dma transaction representation which is monotonically incrementing
> > > number.
> >
> > Ok, and it identifies a unique dma_async_tx_descriptor, right?
> Yup and this basically represents transactions you have submitted. Thats why
> cookie is allocated at tx_submit.
Ok, thanks.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On Thu, Jul 31, 2014 at 02:22:23PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jul 31, 2014 at 09:44:40AM +0200, Maxime Ripard wrote:
> > I didn't miss it. But I feel like it describes quite nicely the slave
> > API, but doesn't help at all whenever you're writing a DMAengine driver.
>
> There's actually two documents:
>
> Documentation/crypto/async-tx-api.txt
> Documentation/dmaengine.txt
>
> The first is the original DMA engine API for use with offloading things
> like memcpy, as well as the crypto API to hardware crypto DMA engines.
>
> The second is documentation for the DMA slave API which modifies some
> of the requirements of the first (like, the ability to call the slave
> DMA prepare functions from within the callback, which are not permitted
> in the async tx API.)
>
> Both documents are relevant for writing a DMA engine driver.
They're both relevant, but clearly not enough.
> > Because, for the moment, we're pretty much left in the dark with
> > different drivers doing the same thing in completetely different ways,
> > with basically no way to tell if it's either the framework that
> > requires such behaviour, or if the author was just feeling creative.
>
> DMA engine has lacked a lot of infrastructure for common patterns in
> drivers; some of that is solved by my efforts with the virt_dma.c
> support, and also various cleanups to existing drivers, but it's not
> easy to fix this problem after drivers have been merged.
>
> > There's numerous examples for this at the moment:
> > - The GFP flags, with different drivers using either GFP_ATOMIC,
> > GFP_NOWAIT or GFP_KERNEL in the same functions
>
> The slave prepare APIs should be using GFP_ATOMIC to ensure safety.
You prove my point then. Vinod asks for GFP_NOWAIT in his
reviews. Even though it doesn't change anything relative to the
atomicity of the operations, the policy is still not the same.
> > - Having to set device_slave_caps or not?
>
> device_slave_caps is a relatively new invention, so old drivers don't
> implement it. Newer drivers should, and there probably should be some
> motivation for older drivers to add it.
Yep, just discovered that.
> > - Some drivers use dma_run_depedencies, some other don't
>
> Dependent operations are part of the async-tx API, and aren't really
> applicable to the slave DMA API. A driver implementing just the slave
> DMA API need not concern itself with dependent operations, but a
> driver implementing the async-tx APIs should do.
Ok.
> > - That might just be my experience, but judging from previous
> > commits, DMA_PRIVATE is completely obscure, and we just set it
> > because it was making it work, without knowing what it was
> > supposed to do.
>
> DMA_PRIVATE means that the channel is unavailable for async-tx operations
> - in other words, it's slave-only. Setting it before registration results
> in the private-use count being incremented, disabling the DMA_PRIVATE
> manipulation in the channel request/release APIs (requested channels are
> unavailable for async-tx operations, which is why that flag is also set
> there.)
>
> To put it another way, if a DMA engine driver only provides slave DMA
> support, it must set DMA_PRIVATE to mark the channel unavailable for
> async-tx operations. If a DMA engine drivers channels can also be used
> for async-tx operations, then it should leave the flag clear.
What about channels that can be both used for slave operations and
async-tx (like memcpy) ?
> > And basically, we have no way to tell at the moment which one is
> > right and which one needs fixing.
> >
> > The corollary being that it cripples the whole community ability to
> > maintain the framework and make it evolve.
>
> All respect to Vinod (who looks after the slave side of the DMA engine
> implementations) and Dan, what it needs is someone who knows the *whole*
> DMA engine picture to be in control of the subsystem, and who knows these
> details - not must one bit of it. Someone who has the time to put into
> reviewing changes to it, and probably more importantly, has the time to
> clean up the existing code. Finding someone who fits that is a struggle.
>
> Dan knows the whole picture (or used to when he was working on it at one
> of his former employers) but I don't think Dan had the available time to
> address various issues with it.
Hence why we should document as much as possible then, to spread the
knowledge and avoid it being lost when someone disappears or isn't
available anymore.
(And hopefully reduce the number of "errors" that might be done by
submitters, hence reducing the number of review iterations, lessening
the maintainers load)
> > > cookie is dma transaction representation which is monotonically incrementing
> > > number.
> >
> > Ok, and it identifies a unique dma_async_tx_descriptor, right?
>
> You really should not be concerned with DMA cookies anymore since one
> of my cleanups - I added a number of helper functions which hide the
> manipulation of DMA cookies, and take away from driver writers the
> semantics of how cookies themselves are operated upon. virt-dma goes
> a little further and provides descriptor lists and methods to look up
> a descriptor given a cookie.
>
> The only thing that a driver writer should concern themselves with is
> making the appropriate cookie management calls at the relevant point in
> the code - in other words, allocating a cookie at in the submit callback,
> completing a cookie when a descriptor is complete, etc.
>
> Drivers should have no reason what so ever to be touching the cookie
> directly anymore.
Ok, thanks a lot for this!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 07/31/2014 06:13 PM, Maxime Ripard wrote:
[...]
> From what you're saying, and judging from the drivers that already
> implement it, can't it be moved directly to the framework itself ?
>
What exactly do you mean by moving it directly to the framework? The
slave_caps API is part of the DMAengine framework.
- Lars
On Thu, Jul 31, 2014 at 06:54:11PM +0200, Lars-Peter Clausen wrote:
> On 07/31/2014 06:13 PM, Maxime Ripard wrote:
> [...]
> > From what you're saying, and judging from the drivers that already
> >implement it, can't it be moved directly to the framework itself ?
> >
>
> What exactly do you mean by moving it directly to the framework? The
> slave_caps API is part of the DMAengine framework.
Not its implementation, which is defined by each and every driver,
while the behaviour of device_slave_caps is rather generic.
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com