On 07/10/2015 06:05 PM, Robert Jarzmik wrote:
Lars-Peter Clausen <lars@xxxxxxxxxx> writes:
On 07/10/2015 01:40 PM, Jun Nie wrote:
Furthermore audio DMA uses cyclic DMA, so it doesn't even hit this code
path, so the patch description doesn't really add up to the changes.
Actually I'd like to know _exactly_ which code is hit there, which dmaengine
user. I have found that fsl_asrc_dma.c uses the flag "DMA_CTRL_ACK", without
using anywhere the async_ack() method (I might be wrong, I don't know that
code).
The question behind is : which driver has a regression, for what purpose is it
setting the DMA_CTRL_ACK bit ? That would help to know if I used a bit whose
purpose what already well defined, or if it's just a driver which sets
DMA_CTRL_ACK without really needing it.
Every client that sets DMA_CTRL_ACK, which is pretty much all of them, has a
regression here. The expected semantics by the client is that the descriptor
will be freed once the transfer is done (this assumption holds regardless of
whether the flag is set or not). Now with the new patch a descriptor that
has the flag set is not freed and we amount massive memory leaks.
The drivers set them because that has always been the recommendation to set
it unless you know you don't need to set it.
If we find that DMA_CTRL_ACK has a specific meaning, or a "dmaengine driver"
specific meaning (ie. not usable by dmaengine clients), then another flag will
probably be necessary.
Sorry, it does. Looked at the wrong line. Reverting b9855f03d560 is probably
the right thing to do as it breaks the existing semantics in a very bad way
causing descriptors to be not freed when they should be.
I guess my change does not meet video requirement for patch
b9855f03d560, but I do not know detail video usage case related to
that patch. You can either revert the patch or confirm change in this
email is OK for the video case.
To handle the video case something else is needed. The current dmaengine
semantics are that once a descriptor is passed to dma_engine_submit() it is
owned by the DMAengine driver and the client must never ever touch it
again.
Commit b9855f03d560 overloaded DMA_CTRL_ACK to mean that the should be
re-usable by the client. But this breaks existing users of DMA_CTRL_ACK since
its a completely new and different meaning to the existing one. For marking a
descriptor as reusable for the client a new flag should probably be
introduced.
Maybe yes.
The problem will be to list the existing users of DMA_CTRL_ACK (dmaengine slave
API side), and see what is their expectation from this bit.
From the discussion we had with Vinod, there was no clear definition so far for
DMA_CTRL_ACK (still for dmaengine slave API). I might have overlooked something
here, so any input will be valuable.
The definition is somewhat fuzzy, but as far as I understand it is for meant
for DMAengine drivers which use descriptor pools and recycle descriptors. By
not setting the DMA_CTRL_ACK flag a client can request that the descriptor
is still in use and the DMAengine driver must not use the descriptor for new
transfers even if is in the pool as long as the flag is not set. This is
somewhat of the reverse of your new definition.
Given the somewhat unclear semantics I'd very much prefer adding a separate
flag for re-usable descriptors rather than trying to re-purpose
DMA_CTRL_ACK. This allows us to define clear semantics and doesn't require
trying to fix up current DMA_CTRL_ACK usage.
And there is more to be considered for re-usable descriptors. Lets say we
define the ownership semantics of the new flag as the following:
* After device_prep_*() returns a descriptor it is owned by the client and
can be modified. E.g. setting a callback.
* Calling dmaengine_submit() will pass the ownership to the DMAengine
driver. The client must no longer modify the the descriptor after this.
* When the complete callback is called ownership is passed back to the
client. And can be modified or submitted again.
There needs to be a API to pass the ownership back to the DMAengine driver
and free the descriptor if the client is done using the descriptor but
currently owns it and doesn't wants to start a new transfer.
Furthermore a DMAengine driver needs to be aware of these new semantics and
need to be updated accordingly. There are many drivers that will just
exhibit undefined behavior if a descriptor is submitted multiple times. So
there should be a feature flag indicating whether a driver supports
re-usable descriptors that can be checked by clients. If the feature flag is
not set the client must no reuse descriptors.
- Lars
--
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
