| Message ID | 1376672707-24527-2-git-send-email-zonque@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/17/2013 01:05 AM, Daniel Mack wrote: > Currently, when an interrupt has occured for a channel, the tasklet > worker code will only look at the very last entry in the running list > and complete its cookie, and then dispose the entire running chain. > Hence, the first transaction's cookie will never complete. > > In fact, the interrupt we should handle will be the one related to the > first descriptor in the chain with the ENDIRQEN bit set, so complete > the second transaction that is in fact still running. > > As a result, the driver can't currently handle multiple transactions on > one chanel, and it's likely that no drivers exist that rely on this > feature. > > Fix this by walking the running_chain and look for the first > descriptor that has the interrupt-enable bit set. Only queue > descriptors up to that point for completion handling, while leaving > the rest intact. Also, only make the channel idle if the list is > completely empty after such a cycle. > > Signed-off-by: Daniel Mack <zonque@gmail.com> > --- > drivers/dma/mmp_pdma.c | 35 +++++++++++++++++++++-------------- > 1 file changed, 21 insertions(+), 14 deletions(-) > Hi, Daniel I think we would not run into the situation that there is a descriptor with ENDIRQEN set in the middle of the running chain. in mmp_pdma.c: mmp_pdma_tx_submit() -> append_pending_queue() -> tail->desc.dcmd &= ~DCMD_ENDIRQEN; So in the pending list (same for running list), only the last descriptor will have ENDIRQEN set. Please correct me if any problems.
Hi Xiang, On 20.08.2013 04:27, Xiang Wang wrote: > I think we would not run into the situation that there is a descriptor > with ENDIRQEN set in the middle of the running chain. > > in mmp_pdma.c: > mmp_pdma_tx_submit() -> append_pending_queue() -> > tail->desc.dcmd &= ~DCMD_ENDIRQEN; > > So in the pending list (same for running list), only the last descriptor > will have ENDIRQEN set. Right, thanks for noting that. But is that what we want? I think that clearing of the ENDIRQEN bit needs to be removed then, because if we really have multiple transactions running, then we want to get an interrupt whenever _any_ of it is completed. Also, the way I solved it now, the ENDIRQEN bit is also looked at when the residue code tries to distinguish the transactions in the list. Hence, would you agree that we should remove that "tail->desc.dcmd &= ~DCMD_ENDIRQEN;" modification in order to make the above logic work? Thanks, Daniel
Daniel Mack <zonque@gmail.com> writes: > Hi Xiang, > > On 20.08.2013 04:27, Xiang Wang wrote: >> I think we would not run into the situation that there is a descriptor >> with ENDIRQEN set in the middle of the running chain. >> >> in mmp_pdma.c: >> mmp_pdma_tx_submit() -> append_pending_queue() -> >> tail->desc.dcmd &= ~DCMD_ENDIRQEN; >> >> So in the pending list (same for running list), only the last descriptor >> will have ENDIRQEN set. > > Right, thanks for noting that. > > But is that what we want? I think that clearing of the ENDIRQEN bit > needs to be removed then, because if we really have multiple > transactions running, then we want to get an interrupt whenever _any_ of > it is completed. At least that's something pxa_camera.c needs, ie. have the dma interrupt handler called as soon as the _first_ of many transactions queued on the same channel is finished. This is necessary to declare the video capture buffer as "done", which is signaled by a terminated DMA transaction. And of course, in video recording, another one is beginning on the channel, while the finished video buffer can be safely unmapped and handed over to userland. > Hence, would you agree that we should remove that "tail->desc.dcmd &= > ~DCMD_ENDIRQEN;" modification in order to make the above logic work? Same question on my side. My understanding is that the current bebaviour violates the dmaengine specification (Documentation/dmaengine.txt, chapter 5, "On completion of each DMA operation, the next in queue is started and a tasklet triggered. The tasklet will then call the client driver completion callback routine for notification, if set.) Cheers. -- Robert
diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c index 579f79a..9929f85 100644 --- a/drivers/dma/mmp_pdma.c +++ b/drivers/dma/mmp_pdma.c @@ -710,25 +710,32 @@ static void dma_do_tasklet(unsigned long data) spin_lock_irqsave(&chan->desc_lock, flags); - /* update the cookie if we have some descriptors to cleanup */ - if (!list_empty(&chan->chain_running)) { - dma_cookie_t cookie; - - desc = to_mmp_pdma_desc(chan->chain_running.prev); - cookie = desc->async_tx.cookie; - dma_cookie_complete(&desc->async_tx); + list_for_each_entry_safe(desc, _desc, &chan->chain_running, node) { + /* + * move the descriptors to a temporary list so we can drop + * the lock during the entire cleanup operation + */ + list_del(&desc->node); + list_add(&desc->node, &chain_cleanup); - dev_dbg(chan->dev, "completed_cookie=%d\n", cookie); + /* + * Look for the first list entry which has the ENDIRQEN flag + * set. That is the descriptor we got an interrupt for, so + * complete that transaction and its cookie. + */ + if (desc->desc.dcmd & DCMD_ENDIRQEN) { + dma_cookie_t cookie = desc->async_tx.cookie; + dma_cookie_complete(&desc->async_tx); + dev_dbg(chan->dev, "completed_cookie=%d\n", cookie); + break; + } } /* - * move the descriptors to a temporary list so we can drop the lock - * during the entire cleanup operation + * The hardware is idle and ready for more when the + * chain_running list is empty. */ - list_splice_tail_init(&chan->chain_running, &chain_cleanup); - - /* the hardware is now idle and ready for more */ - chan->idle = true; + chan->idle = list_empty(&chan->chain_running); /* Start any pending transactions automatically */ start_pending_queue(chan);
Currently, when an interrupt has occured for a channel, the tasklet worker code will only look at the very last entry in the running list and complete its cookie, and then dispose the entire running chain. Hence, the first transaction's cookie will never complete. In fact, the interrupt we should handle will be the one related to the first descriptor in the chain with the ENDIRQEN bit set, so complete the second transaction that is in fact still running. As a result, the driver can't currently handle multiple transactions on one chanel, and it's likely that no drivers exist that rely on this feature. Fix this by walking the running_chain and look for the first descriptor that has the interrupt-enable bit set. Only queue descriptors up to that point for completion handling, while leaving the rest intact. Also, only make the channel idle if the list is completely empty after such a cycle. Signed-off-by: Daniel Mack <zonque@gmail.com> --- drivers/dma/mmp_pdma.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-)