| Message ID | 1436525503-26091-1-git-send-email-jun.nie@linaro.org (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On 07/10/2015 12:51 PM, Jun Nie wrote: > Free descriptor when free chan resource as some device, > such as soc soc-generic-dmaengine-pcm.c in audio side, > may not release channel. This cause too much memory is > cached for descriptor is not released. > [...] > @@ -98,13 +98,10 @@ void vchan_dma_desc_free_list(struct virt_dma_chan *vc, struct list_head *head) > while (!list_empty(head)) { > struct virt_dma_desc *vd = list_first_entry(head, > struct virt_dma_desc, node); > - if (async_tx_test_ack(&vd->tx)) { > - list_move_tail(&vd->node, &vc->desc_allocated); > - } else { > - dev_dbg(vc->chan.device->dev, "txd %p: freeing\n", vd); > - list_del(&vd->node); > - vc->desc_free(vd); > - } > + list_move_tail(&vd->node, &vc->desc_allocated); > + dev_dbg(vc->chan.device->dev, "txd %p: freeing\n", vd); > + list_del(&vd->node); > + vc->desc_free(vd); This is basically a revert of b9855f03d560 ("dmaengine: virt-dma: don't always free descriptor upon completion"). Which was just introduced, so this is probably not what you want. 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. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/10/2015 01:32 PM, Lars-Peter Clausen wrote: > On 07/10/2015 12:51 PM, Jun Nie wrote: >> Free descriptor when free chan resource as some device, >> such as soc soc-generic-dmaengine-pcm.c in audio side, >> may not release channel. This cause too much memory is >> cached for descriptor is not released. >> > [...] >> @@ -98,13 +98,10 @@ void vchan_dma_desc_free_list(struct virt_dma_chan >> *vc, struct list_head *head) >> while (!list_empty(head)) { >> struct virt_dma_desc *vd = list_first_entry(head, >> struct virt_dma_desc, node); >> - if (async_tx_test_ack(&vd->tx)) { >> - list_move_tail(&vd->node, &vc->desc_allocated); >> - } else { >> - dev_dbg(vc->chan.device->dev, "txd %p: freeing\n", vd); >> - list_del(&vd->node); >> - vc->desc_free(vd); >> - } >> + list_move_tail(&vd->node, &vc->desc_allocated); >> + dev_dbg(vc->chan.device->dev, "txd %p: freeing\n", vd); >> + list_del(&vd->node); >> + vc->desc_free(vd); > > This is basically a revert of b9855f03d560 ("dmaengine: virt-dma: don't > always free descriptor upon completion"). Which was just introduced, so this > is probably not what you want. > > 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. 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. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-07-10 19:36 GMT+08:00 Lars-Peter Clausen <lars@metafoo.de>: > On 07/10/2015 01:32 PM, Lars-Peter Clausen wrote: >> >> On 07/10/2015 12:51 PM, Jun Nie wrote: >>> >>> Free descriptor when free chan resource as some device, >>> such as soc soc-generic-dmaengine-pcm.c in audio side, >>> may not release channel. This cause too much memory is >>> cached for descriptor is not released. >>> >> [...] >>> >>> @@ -98,13 +98,10 @@ void vchan_dma_desc_free_list(struct virt_dma_chan >>> *vc, struct list_head *head) >>> while (!list_empty(head)) { >>> struct virt_dma_desc *vd = list_first_entry(head, >>> struct virt_dma_desc, node); >>> - if (async_tx_test_ack(&vd->tx)) { >>> - list_move_tail(&vd->node, &vc->desc_allocated); >>> - } else { >>> - dev_dbg(vc->chan.device->dev, "txd %p: freeing\n", vd); >>> - list_del(&vd->node); >>> - vc->desc_free(vd); >>> - } >>> + list_move_tail(&vd->node, &vc->desc_allocated); >>> + dev_dbg(vc->chan.device->dev, "txd %p: freeing\n", vd); >>> + list_del(&vd->node); >>> + vc->desc_free(vd); >> >> >> This is basically a revert of b9855f03d560 ("dmaengine: virt-dma: don't >> always free descriptor upon completion"). Which was just introduced, so >> this >> is probably not what you want. >> >> 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. > > > 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 unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/10/2015 01:40 PM, Jun Nie wrote: > 2015-07-10 19:36 GMT+08:00 Lars-Peter Clausen <lars@metafoo.de>: >> On 07/10/2015 01:32 PM, Lars-Peter Clausen wrote: >>> >>> On 07/10/2015 12:51 PM, Jun Nie wrote: >>>> >>>> Free descriptor when free chan resource as some device, >>>> such as soc soc-generic-dmaengine-pcm.c in audio side, >>>> may not release channel. This cause too much memory is >>>> cached for descriptor is not released. >>>> >>> [...] >>>> >>>> @@ -98,13 +98,10 @@ void vchan_dma_desc_free_list(struct virt_dma_chan >>>> *vc, struct list_head *head) >>>> while (!list_empty(head)) { >>>> struct virt_dma_desc *vd = list_first_entry(head, >>>> struct virt_dma_desc, node); >>>> - if (async_tx_test_ack(&vd->tx)) { >>>> - list_move_tail(&vd->node, &vc->desc_allocated); >>>> - } else { >>>> - dev_dbg(vc->chan.device->dev, "txd %p: freeing\n", vd); >>>> - list_del(&vd->node); >>>> - vc->desc_free(vd); >>>> - } >>>> + list_move_tail(&vd->node, &vc->desc_allocated); >>>> + dev_dbg(vc->chan.device->dev, "txd %p: freeing\n", vd); >>>> + list_del(&vd->node); >>>> + vc->desc_free(vd); >>> >>> >>> This is basically a revert of b9855f03d560 ("dmaengine: virt-dma: don't >>> always free descriptor upon completion"). Which was just introduced, so >>> this >>> is probably not what you want. >>> >>> 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. >> >> >> 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. - Lars -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Lars-Peter Clausen <lars@metafoo.de> 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. 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. Cheers.
On 07/10/2015 06:05 PM, Robert Jarzmik wrote: > Lars-Peter Clausen <lars@metafoo.de> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 10, 2015 at 06:05:22PM +0200, Robert Jarzmik wrote: > Lars-Peter Clausen <lars@metafoo.de> 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). It is used in dmaengine_pcm_prepare_and_submit() in sound/core/pcm_dmaengine.c > > 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. > > 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. Lars, IIRC the usage in sound/ was to ensure we dont clean up the descriptor once the dma driver completes it so that we keep on using the same descriptor in cyclic fashion, right? > 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. I should have realised sound/ uses it and check this bit. Worst case we need another flag
On 07/10/2015 07:01 PM, Vinod Koul wrote: [...] >>> 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. > Lars, > IIRC the usage in sound/ was to ensure we dont clean up the descriptor once > the dma driver completes it so that we keep on using the same descriptor in > cyclic fashion, right? No, this has nothing to do with cyclic DMA. The flag needs to be set otherwise drivers with descriptor pools will consider the descriptor to be in use, even if we are done using it and hence the it will run out of descriptors eventually. > >> 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. > I should have realised sound/ uses it and check this bit. Worst case we need > another flag It's not just sound. The recommendation has always been set this flag unless you know why you are not setting it. So pretty much every DMAengine client driver sets it. - Lars -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 10, 2015 at 07:06:55PM +0200, Lars-Peter Clausen wrote: > On 07/10/2015 07:01 PM, Vinod Koul wrote: > [...] > >>>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. > >Lars, > >IIRC the usage in sound/ was to ensure we dont clean up the descriptor once > >the dma driver completes it so that we keep on using the same descriptor in > >cyclic fashion, right? > > No, this has nothing to do with cyclic DMA. The flag needs to be set > otherwise drivers with descriptor pools will consider the descriptor > to be in use, even if we are done using it and hence the it will run > out of descriptors eventually. > > > > >>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. > >I should have realised sound/ uses it and check this bit. Worst case we need > >another flag > > It's not just sound. The recommendation has always been set this > flag unless you know why you are not setting it. So pretty much > every DMAengine client driver sets it. Actually there has never been a recommendation for slave cases. It came from async API and slave users should never have used it, untill now. Now to solve the mess, either we can revert this patch and find a new one for actual reuse of descriptors. Alternate would be fix users I am leaning towards former then follow up by fixing the users who dont want this. Since this is going to be a tree wide update it will need one merge cycle to complete. Once done we can bring back the usage required for actual descriptor reuse case. This will lead only one broken driver from Robert. Is the client for your driver merged?
Vinod Koul <vinod.koul@intel.com> writes: >> It's not just sound. The recommendation has always been set this >> flag unless you know why you are not setting it. So pretty much >> every DMAengine client driver sets it. > Actually there has never been a recommendation for slave cases. It came from async > API and slave users should never have used it, untill now. > > Now to solve the mess, either we can revert this patch and find a new one > for actual reuse of descriptors. Alternate would be fix users > > I am leaning towards former then follow up by fixing the users who dont want > this. Since this is going to be a tree wide update it will need one merge > cycle to complete. Once done we can bring back the usage required for actual > descriptor reuse case. This will lead only one broken driver from Robert. Is > the client for your driver merged? No Vinod, it's still under review. And I'll follow any of your directions here, either revert or tree wide update or new flag. I can help to do the sweeping too, if I had a clear idea of what you have in mind. After all, I want that feature, so I can contribute to tidy up. Cheers.
On 07/10/2015 07:23 PM, Vinod Koul wrote: > On Fri, Jul 10, 2015 at 07:06:55PM +0200, Lars-Peter Clausen wrote: >> On 07/10/2015 07:01 PM, Vinod Koul wrote: >> [...] >>>>> 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. >>> Lars, >>> IIRC the usage in sound/ was to ensure we dont clean up the descriptor once >>> the dma driver completes it so that we keep on using the same descriptor in >>> cyclic fashion, right? >> >> No, this has nothing to do with cyclic DMA. The flag needs to be set >> otherwise drivers with descriptor pools will consider the descriptor >> to be in use, even if we are done using it and hence the it will run >> out of descriptors eventually. > >> >>> >>>> 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. >>> I should have realised sound/ uses it and check this bit. Worst case we need >>> another flag >> >> It's not just sound. The recommendation has always been set this >> flag unless you know why you are not setting it. So pretty much >> every DMAengine client driver sets it. > Actually there has never been a recommendation for slave cases. It came from async > API and slave users should never have used it, untill now. That doesn't reflect reality. The fast majority slave API users set the flag and some DMAengine drivers expect it to be set. > > Now to solve the mess, either we can revert this patch and find a new one > for actual reuse of descriptors. Alternate would be fix users > > I am leaning towards former then follow up by fixing the users who dont want > this. Since this is going to be a tree wide update it will need one merge > cycle to complete. Once done we can bring back the usage required for actual > descriptor reuse case. This will lead only one broken driver from Robert. Is > the client for your driver merged? Just blindly removing all the existing users wont work, you'd have to carefully review each case and see if anything potentially breaks by removing it. This will probably take longer than one cycle. Repurposing a flag that is so wildly used with completely different semantics in such a short timespan will cause all kinds of issues. This should really be a new flag with clearly defined semantics. There is nothing gained from reusing DMA_CTRL_ACK it only makes it a lot more difficult. - Lars -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 10, 2015 at 08:10:47PM +0200, Lars-Peter Clausen wrote: > On 07/10/2015 07:23 PM, Vinod Koul wrote: > >On Fri, Jul 10, 2015 at 07:06:55PM +0200, Lars-Peter Clausen wrote: > >>On 07/10/2015 07:01 PM, Vinod Koul wrote: > >>[...] > >>>>>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. > >>>Lars, > >>>IIRC the usage in sound/ was to ensure we dont clean up the descriptor once > >>>the dma driver completes it so that we keep on using the same descriptor in > >>>cyclic fashion, right? > >> > >>No, this has nothing to do with cyclic DMA. The flag needs to be set > >>otherwise drivers with descriptor pools will consider the descriptor > >>to be in use, even if we are done using it and hence the it will run > >>out of descriptors eventually. > > > >> > >>> > >>>>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. > >>>I should have realised sound/ uses it and check this bit. Worst case we need > >>>another flag > >> > >>It's not just sound. The recommendation has always been set this > >>flag unless you know why you are not setting it. So pretty much > >>every DMAengine client driver sets it. > >Actually there has never been a recommendation for slave cases. It came from async > >API and slave users should never have used it, untill now. > > That doesn't reflect reality. The fast majority slave API users set > the flag and some DMAengine drivers expect it to be set. That is simply wrong and we need to fix that > > > > >Now to solve the mess, either we can revert this patch and find a new one > >for actual reuse of descriptors. Alternate would be fix users > > > >I am leaning towards former then follow up by fixing the users who dont want > >this. Since this is going to be a tree wide update it will need one merge > >cycle to complete. Once done we can bring back the usage required for actual > >descriptor reuse case. This will lead only one broken driver from Robert. Is > >the client for your driver merged? > > Just blindly removing all the existing users wont work, you'd have > to carefully review each case and see if anything potentially breaks > by removing it. This will probably take longer than one cycle. > Repurposing a flag that is so wildly used with completely different > semantics in such a short timespan will cause all kinds of issues. NO the idea is *not* to blindly remove BUT fix the users to do the right thing...
Vinod Koul <vinod.koul@intel.com> writes: >> Just blindly removing all the existing users wont work, you'd have >> to carefully review each case and see if anything potentially breaks >> by removing it. This will probably take longer than one cycle. >> Repurposing a flag that is so wildly used with completely different >> semantics in such a short timespan will cause all kinds of issues. > NO the idea is *not* to blindly remove BUT fix the users to do the right > thing... Hi Vinod, With [1], I've made some digging to assess the span of the fix. I have the result in [2]. This gives us all DMA_CTRL_ACK users which do not ack the txs. I suppose the dmaengine drivers ack them given the leftside column and its drivers/dma/* content. Now I need to refine the search a bit. I hope crypto and ata are using the master dmaengine API. As for the others, these are probably the fix target. I you have an idea to limit further the list, that would be great. I must admit I don't have a clear idea what their expectation of DMA_CTRL_ACK is ... Cheers. -- Robert [1] find . -name '*.[ch]' -exec grep -ls async_tx_ack {} \; | sort -u > /tmp/async_tx_ack.txt find . -name '*.[ch]' -exec grep -ls DMA_CTRL_ACK {} \; | sort -u > /tmp/dma_ctrl_ack.txt sdiff -s /tmp/async_tx_ack.txt /tmp/dma_ctrl_ack.txt [2] ./crypto/async_tx/async_tx.c | ./drivers/ata/pata_ep93xx.c > ./drivers/ata/sata_dwc_460ex.c > ./drivers/crypto/atmel-aes.c > ./drivers/crypto/atmel-sha.c > ./drivers/crypto/atmel-tdes.c > ./drivers/crypto/img-hash.c > ./drivers/crypto/omap-aes.c > ./drivers/crypto/omap-des.c > ./drivers/crypto/omap-sham.c > ./drivers/crypto/qce/dma.c > ./drivers/crypto/ux500/cryp/cryp_core.c > ./drivers/crypto/ux500/hash/hash_core.c > ./drivers/dma/at_hdmac.c > ./drivers/dma/dmatest.c ./drivers/dma/fsldma.c | ./drivers/dma/ep93xx_dma.c ./drivers/dma/ioat/dma.c | ./drivers/dma/imx-dma.c ./drivers/dma/ioat/dma_v2.c | ./drivers/dma/imx-sdma.c ./drivers/dma/ioat/dma_v3.c < ./drivers/dma/mmp_pdma.c | ./drivers/dma/mpc512x_dma.c ./drivers/dma/mv_xor.c < ./drivers/dma/pl330.c | ./drivers/dma/pch_dma.c ./drivers/dma/ppc4xx/adma.c | ./drivers/dma/sa11x0-dma.c ./drivers/dma/sh/shdma-base.c | ./drivers/dma/sirf-dma.c ./drivers/dma/xgene-dma.c | ./drivers/dma/tegra20-apb-dma.c ./drivers/dma/xilinx/xilinx_vdma.c | ./drivers/dma/timb_dma.c ./drivers/md/raid5.c | ./drivers/dma/txx9dmac.c ./drivers/media/platform/soc_camera/mx3_camera.c | ./drivers/i2c/busses/i2c-at91.c > ./drivers/i2c/busses/i2c-imx.c > ./drivers/i2c/busses/i2c-mxs.c > ./drivers/i2c/busses/i2c-sh_mobile.c > ./drivers/media/platform/m2m-deinterlace.c > ./drivers/media/platform/omap3isp/isphist.c > ./drivers/media/platform/xilinx/xilinx-dma.c > ./drivers/mmc/host/atmel-mci.c > ./drivers/mmc/host/davinci_mmc.c > ./drivers/mmc/host/jz4740_mmc.c > ./drivers/mmc/host/mmci.c > ./drivers/mmc/host/moxart-mmc.c > ./drivers/mmc/host/mxcmmc.c > ./drivers/mmc/host/mxs-mmc.c > ./drivers/mmc/host/omap.c > ./drivers/mmc/host/omap_hsmmc.c > ./drivers/mmc/host/s3cmci.c > ./drivers/mmc/host/sh_mmcif.c > ./drivers/mmc/host/tmio_mmc_dma.c > ./drivers/mmc/host/usdhi6rol0.c > ./drivers/mtd/nand/atmel_nand.c > ./drivers/mtd/nand/fsmc_nand.c > ./drivers/mtd/nand/gpmi-nand/gpmi-lib.c > ./drivers/mtd/nand/lpc32xx_mlc.c > ./drivers/mtd/nand/lpc32xx_slc.c > ./drivers/mtd/nand/omap2.c > ./drivers/mtd/nand/sh_flctl.c > ./drivers/net/irda/sa1100_ir.c > ./drivers/rapidio/devices/tsi721_dma.c > ./drivers/soc/tegra/fuse/fuse-tegra20.c > ./drivers/spi/spi-atmel.c > ./drivers/spi/spi-davinci.c > ./drivers/spi/spi-dw-mid.c > ./drivers/spi/spi-ep93xx.c > ./drivers/spi/spi-imx.c > ./drivers/spi/spi-mxs.c > ./drivers/spi/spi-omap2-mcspi.c > ./drivers/spi/spi-pl022.c > ./drivers/spi/spi-pxa2xx-dma.c > ./drivers/spi/spi-rspi.c > ./drivers/spi/spi-sh-msiof.c > ./drivers/spi/spi-sirf.c > ./drivers/spi/spi-tegra114.c > ./drivers/spi/spi-tegra20-slink.c > ./drivers/tty/serial/8250/8250_dma.c > ./drivers/tty/serial/8250/8250_omap.c > ./drivers/tty/serial/amba-pl011.c ./drivers/tty/serial/fsl_lpuart.c | ./drivers/tty/serial/mxs-auart.c ./drivers/tty/serial/samsung.c < ./drivers/tty/serial/serial-tegra.c < ./drivers/video/fbdev/mx3fb.c | ./drivers/usb/musb/musb_cppi41.c > ./drivers/usb/musb/ux500_dma.c > ./drivers/usb/renesas_usbhs/fifo.c > ./sound/core/pcm_dmaengine.c > ./sound/soc/fsl/fsl_asrc_dma.c > ./sound/soc/intel/common/sst-firmware.c > ./sound/soc/sh/fsi.c > ./sound/soc/sh/rcar/dma.c > ./sound/soc/sh/siu_pcm.c > ./sound/soc/txx9/txx9aclc.c -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/13/2015 07:14 AM, Vinod Koul wrote: > On Fri, Jul 10, 2015 at 08:10:47PM +0200, Lars-Peter Clausen wrote: >> On 07/10/2015 07:23 PM, Vinod Koul wrote: >>> On Fri, Jul 10, 2015 at 07:06:55PM +0200, Lars-Peter Clausen wrote: >>>> On 07/10/2015 07:01 PM, Vinod Koul wrote: >>>> [...] >>>>>>> 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. >>>>> Lars, >>>>> IIRC the usage in sound/ was to ensure we dont clean up the descriptor once >>>>> the dma driver completes it so that we keep on using the same descriptor in >>>>> cyclic fashion, right? >>>> >>>> No, this has nothing to do with cyclic DMA. The flag needs to be set >>>> otherwise drivers with descriptor pools will consider the descriptor >>>> to be in use, even if we are done using it and hence the it will run >>>> out of descriptors eventually. >>> >>>> >>>>> >>>>>> 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. >>>>> I should have realised sound/ uses it and check this bit. Worst case we need >>>>> another flag >>>> >>>> It's not just sound. The recommendation has always been set this >>>> flag unless you know why you are not setting it. So pretty much >>>> every DMAengine client driver sets it. >>> Actually there has never been a recommendation for slave cases. It came from async >>> API and slave users should never have used it, untill now. >> >> That doesn't reflect reality. The fast majority slave API users set >> the flag and some DMAengine drivers expect it to be set. > That is simply wrong and we need to fix that Could you explain why you think that it is wrong? I think we might have different understandings of what exactly the DMA_CTRL_ACK flag means. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 13, 2015 at 01:00:54PM +0200, Lars-Peter Clausen wrote: > >>>Actually there has never been a recommendation for slave cases. It came from async > >>>API and slave users should never have used it, untill now. > >> > >>That doesn't reflect reality. The fast majority slave API users set > >>the flag and some DMAengine drivers expect it to be set. > >That is simply wrong and we need to fix that > > Could you explain why you think that it is wrong? > > I think we might have different understandings of what exactly the > DMA_CTRL_ACK flag means. This is documented now * DMA_CTRL_ACK - If set, the transfer can be reused after being completed. - There is a guarantee the transfer won't be freed until it is acked by async_tx_ack(). - As a consequence, if a device driver wants to skip the dma_map_sg() and dma_unmap_sg() in between 2 transfers, because the DMA'd data wasn't used, it can resubmit the transfer right after its completion. Any user who doesnt want this interpretation should be fixed, any driver not doing this should be fixed :)
On 07/16/2015 03:08 PM, Vinod Koul wrote: > On Mon, Jul 13, 2015 at 01:00:54PM +0200, Lars-Peter Clausen wrote: >>>>> Actually there has never been a recommendation for slave cases. It came from async >>>>> API and slave users should never have used it, untill now. >>>> >>>> That doesn't reflect reality. The fast majority slave API users set >>>> the flag and some DMAengine drivers expect it to be set. >>> That is simply wrong and we need to fix that >> >> Could you explain why you think that it is wrong? >> >> I think we might have different understandings of what exactly the >> DMA_CTRL_ACK flag means. > This is documented now > > * DMA_CTRL_ACK > - If set, the transfer can be reused after being completed. > - There is a guarantee the transfer won't be freed until it is acked > by async_tx_ack(). > - As a consequence, if a device driver wants to skip the dma_map_sg() > and > dma_unmap_sg() in between 2 transfers, because the DMA'd data wasn't used, > it can resubmit the transfer right after its completion. > > Any user who doesnt want this interpretation should be fixed, any driver not > doing this should be fixed :) That would be every user and every driver. This documentation was added as part of Robert's series, but does not agree, with either other documentation of the flag, e.g. in include/linux/dmaengine.h, nor with any of the users. The documentation needs to be fixed, not the other way around. Re-usable descriptors should use a new flag, as this is something different and there are more things to consider than just adding a flag. - Lars -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Lars-Peter Clausen <lars@metafoo.de> writes: > On 07/16/2015 03:08 PM, Vinod Koul wrote: >> On Mon, Jul 13, 2015 at 01:00:54PM +0200, Lars-Peter Clausen wrote: >>>>>> Actually there has never been a recommendation for slave cases. It came from async >>>>>> API and slave users should never have used it, untill now. >>>>> >>>>> That doesn't reflect reality. The fast majority slave API users set >>>>> the flag and some DMAengine drivers expect it to be set. >>>> That is simply wrong and we need to fix that >>> >>> Could you explain why you think that it is wrong? >>> >>> I think we might have different understandings of what exactly the >>> DMA_CTRL_ACK flag means. >> This is documented now >> >> * DMA_CTRL_ACK >> - If set, the transfer can be reused after being completed. >> - There is a guarantee the transfer won't be freed until it is acked >> by async_tx_ack(). >> - As a consequence, if a device driver wants to skip the dma_map_sg() >> and >> dma_unmap_sg() in between 2 transfers, because the DMA'd data wasn't used, >> it can resubmit the transfer right after its completion. >> >> Any user who doesnt want this interpretation should be fixed, any driver not >> doing this should be fixed :) > > That would be every user and every driver. This documentation was added as part > of Robert's series, but does not agree, with either other documentation of the > flag, e.g. in include/linux/dmaengine.h, nor with any of the users. The > documentation needs to be fixed, not the other way around. Might it be possible that I implemented code and documentation with the inverted logic ? Or said differently, if I had written : - If *clear*, the transfer can be reused after being completed. And inverted the test in virt-dma.c, would it be better, Vinod ? I'm asking because in the code I read : - include/linux/dmaengine.h: if clear, the descriptor cannot be reused until the client acknowledges receipt. - drivers/dma/virt-dma.c If set, the transfer can be reused after being completed. If I invert my DMA_CTRL_ACK documentation and code logic in virt-dma.c, would that fit the old meaning, and would that still be acceptable by Vinod ? Cheers. -- Robert -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/16/2015 09:05 PM, Robert Jarzmik wrote: > Lars-Peter Clausen <lars@metafoo.de> writes: > >> On 07/16/2015 03:08 PM, Vinod Koul wrote: >>> On Mon, Jul 13, 2015 at 01:00:54PM +0200, Lars-Peter Clausen wrote: >>>>>>> Actually there has never been a recommendation for slave cases. It came from async >>>>>>> API and slave users should never have used it, untill now. >>>>>> >>>>>> That doesn't reflect reality. The fast majority slave API users set >>>>>> the flag and some DMAengine drivers expect it to be set. >>>>> That is simply wrong and we need to fix that >>>> >>>> Could you explain why you think that it is wrong? >>>> >>>> I think we might have different understandings of what exactly the >>>> DMA_CTRL_ACK flag means. >>> This is documented now >>> >>> * DMA_CTRL_ACK >>> - If set, the transfer can be reused after being completed. >>> - There is a guarantee the transfer won't be freed until it is acked >>> by async_tx_ack(). >>> - As a consequence, if a device driver wants to skip the dma_map_sg() >>> and >>> dma_unmap_sg() in between 2 transfers, because the DMA'd data wasn't used, >>> it can resubmit the transfer right after its completion. >>> >>> Any user who doesnt want this interpretation should be fixed, any driver not >>> doing this should be fixed :) >> >> That would be every user and every driver. This documentation was added as part >> of Robert's series, but does not agree, with either other documentation of the >> flag, e.g. in include/linux/dmaengine.h, nor with any of the users. The >> documentation needs to be fixed, not the other way around. > > Might it be possible that I implemented code and documentation with the inverted > logic ? Or said differently, if I had written : > - If *clear*, the transfer can be reused after being completed. > And inverted the test in virt-dma.c, would it be better, Vinod ? > > I'm asking because in the code I read : > - include/linux/dmaengine.h: > if clear, the descriptor cannot be reused until the client > acknowledges receipt. > - drivers/dma/virt-dma.c > If set, the transfer can be reused after being completed. > > If I invert my DMA_CTRL_ACK documentation and code logic in virt-dma.c, would > that fit the old meaning, and would that still be acceptable by Vinod ? It would certainly bring the two closer and fix most of the fallout we are seeing at the moment. But I still believe that support for re-usable descriptors deserve their own flag. Drivers which support DMA_CTRL_ACK wont automatically be able to support re-usable descriptors. The assumption currently is that each descriptor is submitted exactly once. The dmaengine documentation even states that submit() needs to be called right after device_prep_*() since some driver might take a lock in the first and release it in the second. This is something that will obviously not work with re-usable descriptors. - Lars -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 17, 2015 at 12:13:48PM +0200, Lars-Peter Clausen wrote: > On 07/16/2015 09:05 PM, Robert Jarzmik wrote: > >Lars-Peter Clausen <lars@metafoo.de> writes: > > > >>On 07/16/2015 03:08 PM, Vinod Koul wrote: > >>>On Mon, Jul 13, 2015 at 01:00:54PM +0200, Lars-Peter Clausen wrote: > >>>>>>>Actually there has never been a recommendation for slave cases. It came from async > >>>>>>>API and slave users should never have used it, untill now. > >>>>>> > >>>>>>That doesn't reflect reality. The fast majority slave API users set > >>>>>>the flag and some DMAengine drivers expect it to be set. > >>>>>That is simply wrong and we need to fix that > >>>> > >>>>Could you explain why you think that it is wrong? > >>>> > >>>>I think we might have different understandings of what exactly the > >>>>DMA_CTRL_ACK flag means. > >>>This is documented now > >>> > >>> * DMA_CTRL_ACK > >>> - If set, the transfer can be reused after being completed. > >>> - There is a guarantee the transfer won't be freed until it is acked > >>> by async_tx_ack(). > >>> - As a consequence, if a device driver wants to skip the dma_map_sg() > >>> and > >>> dma_unmap_sg() in between 2 transfers, because the DMA'd data wasn't used, > >>> it can resubmit the transfer right after its completion. > >>> > >>>Any user who doesnt want this interpretation should be fixed, any driver not > >>>doing this should be fixed :) > >> > >>That would be every user and every driver. This documentation was added as part > >>of Robert's series, but does not agree, with either other documentation of the > >>flag, e.g. in include/linux/dmaengine.h, nor with any of the users. The > >>documentation needs to be fixed, not the other way around. I agree with this part... > >Might it be possible that I implemented code and documentation with the inverted > >logic ? Or said differently, if I had written : > > - If *clear*, the transfer can be reused after being completed. > >And inverted the test in virt-dma.c, would it be better, Vinod ? Okay, I am thinking not to overload stuff and go with Lar's suggestion of having anew flag and lets have this explicitly > >I'm asking because in the code I read : > > - include/linux/dmaengine.h: > > if clear, the descriptor cannot be reused until the client > > acknowledges receipt. > > - drivers/dma/virt-dma.c > > If set, the transfer can be reused after being completed. > > > >If I invert my DMA_CTRL_ACK documentation and code logic in virt-dma.c, would > >that fit the old meaning, and would that still be acceptable by Vinod ? > > It would certainly bring the two closer and fix most of the fallout > we are seeing at the moment. > > But I still believe that support for re-usable descriptors deserve > their own flag. Drivers which support DMA_CTRL_ACK wont > automatically be able to support re-usable descriptors. The > assumption currently is that each descriptor is submitted exactly > once. The dmaengine documentation even states that submit() needs to > be called right after device_prep_*() since some driver might take a > lock in the first and release it in the second. This is something > that will obviously not work with re-usable descriptors. That would be better idea also not confuse folks. So I would revert the original patch, Documentation update, add new flag DMA_DESC_REUSE for this and let Robert's driver use this So is this plan fine with all, if so I push these changes tomorrow
Vinod Koul <vinod.koul@intel.com> writes: > That would be better idea also not confuse folks. > > So I would revert the original patch, Documentation update, add new flag > DMA_DESC_REUSE for this and let Robert's driver use this > > So is this plan fine with all, if so I push these changes tomorrow Sure, works for me. I small detail : - can I submit a quick patch to renable virt-dma to have reusable txs, using DMA_DESC_REUSE instead of DMA_CTR_ACK so that you can pick it up in the same cycle ? - can I add a function to dmaengine to mark a tx as "finished", ie. not reusable any more, and freeable ? If so, I'd like a name for it please. What about void async_tx_finish_reusable(struct dma_async_tx_descriptor *tx) ? Cheers.
On 07/19/2015 07:44 PM, Robert Jarzmik wrote: > Vinod Koul <vinod.koul@intel.com> writes: > >> That would be better idea also not confuse folks. >> >> So I would revert the original patch, Documentation update, add new flag >> DMA_DESC_REUSE for this and let Robert's driver use this >> >> So is this plan fine with all, if so I push these changes tomorrow > Sure, works for me. > I small detail : > > - can I submit a quick patch to renable virt-dma to have reusable txs, using > DMA_DESC_REUSE instead of DMA_CTR_ACK so that you can pick it up in the same > cycle ? > > - can I add a function to dmaengine to mark a tx as "finished", ie. not > reusable any more, and freeable ? > If so, I'd like a name for it please. > What about void async_tx_finish_reusable(struct dma_async_tx_descriptor *tx) > ? The naming convention for dmaengine API functions is a bit cluttered at the moment. For new functions we should agree on a single prefix. I'd suggest to call the function dmaengine_desc_release(). The other thing is making sure that the new API is complete enough so that it is possible for different types of dmaengine drivers to implement it. Currently the dmaengine API has the built-in assumption that dmaengine_prep_*() function and dmaengine_submit() are always called in pairs. And some drivers rely on this assumption. It might be a good idea to add a dmaengine_prep_reuse() function that takes a already allocated descriptor and must be called before dmaengine_submit() can be called again. This solves two issues, the first is that prep_*() and submit() will still be called in pairs, the second one is that drivers have the chance to re-initialize some state for the descriptor, e.g. set the progress for the descriptor to 0, or similar. Drivers that don't need to do anything special to reuse a descriptor don't need to implement the prep_reuse() callback and the call becomes a no-op. The other thing I'd like to see is a feature flag in the dma_slave_caps struct to indicate weather a dmaengine driver supports re-usable descriptors or not. This is important for generic code so it can figure out in advance whether a driver supports re-usable descriptors or if it has to fallback allocating individual descriptors for each transfer. - Lars -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 20, 2015 at 12:55:35PM +0200, Lars-Peter Clausen wrote: > On 07/19/2015 07:44 PM, Robert Jarzmik wrote: > >Vinod Koul <vinod.koul@intel.com> writes: > > > >>That would be better idea also not confuse folks. > >> > >>So I would revert the original patch, Documentation update, add new flag > >>DMA_DESC_REUSE for this and let Robert's driver use this > >> > >>So is this plan fine with all, if so I push these changes tomorrow > >Sure, works for me. > >I small detail : > > > > - can I submit a quick patch to renable virt-dma to have reusable txs, using > > DMA_DESC_REUSE instead of DMA_CTR_ACK so that you can pick it up in the same > > cycle ? > > > > - can I add a function to dmaengine to mark a tx as "finished", ie. not > > reusable any more, and freeable ? > > If so, I'd like a name for it please. > > What about void async_tx_finish_reusable(struct dma_async_tx_descriptor *tx) > > ? > > The naming convention for dmaengine API functions is a bit cluttered > at the moment. For new functions we should agree on a single prefix. > I'd suggest to call the function dmaengine_desc_release(). Yup no new APIs with async_xxx > > The other thing is making sure that the new API is complete enough > so that it is possible for different types of dmaengine drivers to > implement it. > > Currently the dmaengine API has the built-in assumption that > dmaengine_prep_*() function and dmaengine_submit() are always called > in pairs. And some drivers rely on this assumption. It might be a > good idea to add a dmaengine_prep_reuse() function that takes a > already allocated descriptor and must be called before > dmaengine_submit() can be called again. This solves two issues, the > first is that prep_*() and submit() will still be called in pairs, > the second one is that drivers have the chance to re-initialize some > state for the descriptor, e.g. set the progress for the descriptor > to 0, or similar. Drivers that don't need to do anything special to > reuse a descriptor don't need to implement the prep_reuse() callback > and the call becomes a no-op. > > The other thing I'd like to see is a feature flag in the > dma_slave_caps struct to indicate weather a dmaengine driver > supports re-usable descriptors or not. This is important for generic > code so it can figure out in advance whether a driver supports > re-usable descriptors or if it has to fallback allocating individual > descriptors for each transfer. Yes both of these suggestions make sense. We absolutely need caps to understand device support.
diff --git a/drivers/dma/virt-dma.c b/drivers/dma/virt-dma.c index 7d2c17d..71bc1e4 100644 --- a/drivers/dma/virt-dma.c +++ b/drivers/dma/virt-dma.c @@ -98,13 +98,10 @@ void vchan_dma_desc_free_list(struct virt_dma_chan *vc, struct list_head *head) while (!list_empty(head)) { struct virt_dma_desc *vd = list_first_entry(head, struct virt_dma_desc, node); - if (async_tx_test_ack(&vd->tx)) { - list_move_tail(&vd->node, &vc->desc_allocated); - } else { - dev_dbg(vc->chan.device->dev, "txd %p: freeing\n", vd); - list_del(&vd->node); - vc->desc_free(vd); - } + list_move_tail(&vd->node, &vc->desc_allocated); + dev_dbg(vc->chan.device->dev, "txd %p: freeing\n", vd); + list_del(&vd->node); + vc->desc_free(vd); } } EXPORT_SYMBOL_GPL(vchan_dma_desc_free_list); diff --git a/drivers/dma/virt-dma.h b/drivers/dma/virt-dma.h index 189e75d..e8b54f7 100644 --- a/drivers/dma/virt-dma.h +++ b/drivers/dma/virt-dma.h @@ -149,14 +149,11 @@ static inline void vchan_get_all_descriptors(struct virt_dma_chan *vc, static inline void vchan_free_chan_resources(struct virt_dma_chan *vc) { - struct virt_dma_desc *vd; unsigned long flags; LIST_HEAD(head); spin_lock_irqsave(&vc->lock, flags); vchan_get_all_descriptors(vc, &head); - list_for_each_entry(vd, &head, node) - async_tx_clear_ack(&vd->tx); spin_unlock_irqrestore(&vc->lock, flags); vchan_dma_desc_free_list(vc, &head);
Free descriptor when free chan resource as some device, such as soc soc-generic-dmaengine-pcm.c in audio side, may not release channel. This cause too much memory is cached for descriptor is not released. Signed-off-by: Jun Nie <jun.nie@linaro.org> --- drivers/dma/virt-dma.c | 11 ++++------- drivers/dma/virt-dma.h | 3 --- 2 files changed, 4 insertions(+), 10 deletions(-)