| Message ID | 1327490378-10904-1-git-send-email-ldewangan@nvidia.com |
|---|---|
| State | Rejected, archived |
| Headers | show |
Laxman Dewangan wrote at Wednesday, January 25, 2012 4:20 AM: > Allow the transfer size to vary in each DMA request, > rather than assuming all requests to be the same size > as the first request made. ... > diff --git a/arch/arm/mach-tegra/dma.c b/arch/arm/mach-tegra/dma.c ... > @@ -434,6 +435,15 @@ static void tegra_dma_update_hw_partial(struct tegra_dma_channel *ch, > writel(apb_ptr, ch->addr + APB_DMA_CHAN_APB_PTR); > writel(ahb_ptr, ch->addr + APB_DMA_CHAN_AHB_PTR); > > + if (ch->mode & TEGRA_DMA_MODE_CONTINOUS) > + ch->req_transfer_count = (req->size >> 3) - 1; > + else > + ch->req_transfer_count = (req->size >> 2) - 1; There are cases in the current continuous mode where HW is working on one buffer and the next buffer is queued in HW. Once we've written the new request to HW, we don't know exactly which buffer HW is operating on. Hence, we don't know which buffer ch->req_transfer_count refers to. If req->size changes, might this change cause the new req->size value to be used when the old buffer's final completion interrupt goes off and hence the old buffer's req->bytes_transferred be wrong? Actually looking at the current continuous mode, I'm not convinced that it correctly handles replacing an in-progress buffer with a new buffer; I certainly see where handle_continuous_dma() checks for a second (SW) queued buffer and tells the HW to use that buffer instead, but I don't see where the (SW) queue management is done; where is the old req removed from the head of ch->list and marked complete? I assume the "out of sync" case is only intended to be an error condition and not part of the buffer switch? Perhaps for the current continuous mode, it'd be best to require the client to dequeue any existing request before queuing another, i.e. to fail tegra_dma_enqueue_req() if there's already something in ch->list in continuous mode? > + csr = readl(ch->addr + APB_DMA_CHAN_CSR); > + csr &= ~CSR_WCOUNT_MASK; > + csr |= ch->req_transfer_count << CSR_WCOUNT_SHIFT; > + writel(csr, ch->addr + APB_DMA_CHAN_CSR); > + > req->status = TEGRA_DMA_REQ_INFLIGHT; > return; > }
On Wednesday 25 January 2012 10:33 PM, Stephen Warren wrote: > Laxman Dewangan wrote at Wednesday, January 25, 2012 4:20 AM: >> Allow the transfer size to vary in each DMA request, >> rather than assuming all requests to be the same size >> as the first request made. > ... >> diff --git a/arch/arm/mach-tegra/dma.c b/arch/arm/mach-tegra/dma.c > ... >> @@ -434,6 +435,15 @@ static void tegra_dma_update_hw_partial(struct tegra_dma_channel *ch, >> writel(apb_ptr, ch->addr + APB_DMA_CHAN_APB_PTR); >> writel(ahb_ptr, ch->addr + APB_DMA_CHAN_AHB_PTR); >> >> + if (ch->mode& TEGRA_DMA_MODE_CONTINOUS) >> + ch->req_transfer_count = (req->size>> 3) - 1; >> + else >> + ch->req_transfer_count = (req->size>> 2) - 1; > There are cases in the current continuous mode where HW is working on > one buffer and the next buffer is queued in HW. Once we've written the > new request to HW, we don't know exactly which buffer HW is operating > on. Hence, we don't know which buffer ch->req_transfer_count refers to. > If req->size changes, might this change cause the new req->size value > to be used when the old buffer's final completion interrupt goes off > and hence the old buffer's req->bytes_transferred be wrong? Then byte_transferred should be calculated based on req->size, not based on ch->req_transfer_count. Still issue may come when we need to read the transfered count from dma through status register whether it has completed the old and started new one or still on old req. This can be resolved by having proper lock between the isr and reading status so that if isr clears the interrupt_done, just raise a flag that int_done arrive and so get-active_count() function can handle it properly before workqueu get scheduled. I am seeing that there is missing synchronization between int_done status and get_active_count(). I am also thinking that we can remove the work queue and handle the queue management in isr only > Actually looking at the current continuous mode, I'm not convinced that > it correctly handles replacing an in-progress buffer with a new buffer; > I certainly see where handle_continuous_dma() checks for a second (SW) > queued buffer and tells the HW to use that buffer instead, but I don't > see where the (SW) queue management is done; where is the old req removed > from the head of ch->list and marked complete? I assume the "out of sync" > case is only intended to be an error condition and not part of the > buffer switch? We are removing the old req from list in handling of full buffer interrupt handling. 660 } else if (req->buffer_status == 661 TEGRA_DMA_REQ_BUF_STATUS_HALF_FULL) { ::::::::::::::::::::::::::::::::::::::: 670 req->buffer_status = TEGRA_DMA_REQ_BUF_STATUS_FULL; 671 req->bytes_transferred = bytes_transferred; 672 req->status = TEGRA_DMA_REQ_SUCCESS; 673 list_del(&req->node); > Perhaps for the current continuous mode, it'd be best to require the > client to dequeue any existing request before queuing another, i.e. to > fail tegra_dma_enqueue_req() if there's already something in ch->list > in continuous mode? > >> + csr = readl(ch->addr + APB_DMA_CHAN_CSR); >> + csr&= ~CSR_WCOUNT_MASK; >> + csr |= ch->req_transfer_count<< CSR_WCOUNT_SHIFT; >> + writel(csr, ch->addr + APB_DMA_CHAN_CSR); >> + >> req->status = TEGRA_DMA_REQ_INFLIGHT; >> return; >> } -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Laxman Dewangan wrote at Wednesday, January 25, 2012 6:46 PM: > On Wednesday 25 January 2012 10:33 PM, Stephen Warren wrote: > > Laxman Dewangan wrote at Wednesday, January 25, 2012 4:20 AM: > >> Allow the transfer size to vary in each DMA request, > >> rather than assuming all requests to be the same size > >> as the first request made. > > ... > >> diff --git a/arch/arm/mach-tegra/dma.c b/arch/arm/mach-tegra/dma.c > > ... > >> @@ -434,6 +435,15 @@ static void tegra_dma_update_hw_partial(struct tegra_dma_channel *ch, > >> writel(apb_ptr, ch->addr + APB_DMA_CHAN_APB_PTR); > >> writel(ahb_ptr, ch->addr + APB_DMA_CHAN_AHB_PTR); > >> > >> + if (ch->mode& TEGRA_DMA_MODE_CONTINOUS) > >> + ch->req_transfer_count = (req->size>> 3) - 1; > >> + else > >> + ch->req_transfer_count = (req->size>> 2) - 1; > > There are cases in the current continuous mode where HW is working on > > one buffer and the next buffer is queued in HW. Once we've written the > > new request to HW, we don't know exactly which buffer HW is operating > > on. Hence, we don't know which buffer ch->req_transfer_count refers to. > > If req->size changes, might this change cause the new req->size value > > to be used when the old buffer's final completion interrupt goes off > > and hence the old buffer's req->bytes_transferred be wrong? > > Then byte_transferred should be calculated based on req->size, not based > on ch->req_transfer_count. Yes, I think that makes sense. > Still issue may come when we need to read the transfered count from dma > through status register whether it has completed the old and started new > one or still on old req. This can be resolved by having proper lock > between the isr and reading status so that if isr clears the > interrupt_done, just raise a flag that int_done arrive and so > get-active_count() function can handle it properly before workqueu get > scheduled. I am seeing that there is missing synchronization between > int_done status and get_active_count(). > I am also thinking that we can remove the work queue and handle the > queue management in isr only So I assume you need to make some/all of those modifications before we can apply this patch? > > Actually looking at the current continuous mode, I'm not convinced that > > it correctly handles replacing an in-progress buffer with a new buffer; > > I certainly see where handle_continuous_dma() checks for a second (SW) > > queued buffer and tells the HW to use that buffer instead, but I don't > > see where the (SW) queue management is done; where is the old req removed > > from the head of ch->list and marked complete? I assume the "out of sync" > > case is only intended to be an error condition and not part of the > > buffer switch? > > We are removing the old req from list in handling of full buffer > interrupt handling. > > 660 } else if (req->buffer_status == > 661 TEGRA_DMA_REQ_BUF_STATUS_HALF_FULL) { > ::::::::::::::::::::::::::::::::::::::: > 670 req->buffer_status = > TEGRA_DMA_REQ_BUF_STATUS_FULL; > 671 req->bytes_transferred = bytes_transferred; > 672 req->status = TEGRA_DMA_REQ_SUCCESS; > 673 list_del(&req->node); Oh right... Maybe I'm misunderstanding continuous mode works then; I thought that the client driver queued a request once, and it continued to execute forever until either a new request was queued to replace it, or the original request for dequeued. It looks like instead, the req is always removed from the queue once we've seen both the ping and the pong interrupts. Am I totally misunderstanding what's happening?
On Thursday 26 January 2012 10:23 PM, Stephen Warren wrote: > Laxman Dewangan wrote at Wednesday, January 25, 2012 6:46 PM: >> On Wednesday 25 January 2012 10:33 PM, Stephen Warren wrote: >>> Laxman Dewangan wrote at Wednesday, January 25, 2012 4:20 AM: >>>> Allow the transfer size to vary in each DMA request, > >> Still issue may come when we need to read the transfered count from dma >> through status register whether it has completed the old and started new >> one or still on old req. This can be resolved by having proper lock >> between the isr and reading status so that if isr clears the >> interrupt_done, just raise a flag that int_done arrive and so >> get-active_count() function can handle it properly before workqueu get >> scheduled. I am seeing that there is missing synchronization between >> int_done status and get_active_count(). >> I am also thinking that we can remove the work queue and handle the >> queue management in isr only >> > So I assume you need to make some/all of those modifications before we can > apply this patch? > Ok, I will push that patches first and then we can go for this patch. >>> Actually looking at the current continuous mode, I'm not convinced that >>> it correctly handles replacing an in-progress buffer with a new buffer; >>> I certainly see where handle_continuous_dma() checks for a second (SW) >>> queued buffer and tells the HW to use that buffer instead, but I don't >>> see where the (SW) queue management is done; where is the old req removed >>> from the head of ch->list and marked complete? I assume the "out of sync" >>> case is only intended to be an error condition and not part of the >>> buffer switch? >> We are removing the old req from list in handling of full buffer >> interrupt handling. >> >> 660 } else if (req->buffer_status == >> 661 TEGRA_DMA_REQ_BUF_STATUS_HALF_FULL) { >> ::::::::::::::::::::::::::::::::::::::: >> 670 req->buffer_status = >> TEGRA_DMA_REQ_BUF_STATUS_FULL; >> 671 req->bytes_transferred = bytes_transferred; >> 672 req->status = TEGRA_DMA_REQ_SUCCESS; >> 673 list_del(&req->node); > Oh right... Maybe I'm misunderstanding continuous mode works then; I > thought that the client driver queued a request once, and it continued > to execute forever until either a new request was queued to replace it, > or the original request for dequeued. It looks like instead, the req > is always removed from the queue once we've seen both the ping and the > pong interrupts. Am I totally misunderstanding what's happening? > I agree that many issues are there in this piece of code. Originally, continuous double support was added only for glitch less audio playback. The audio driver makes some requests and the request in dma-queue get programmed before the current dma complete and so after buffer transfer completion, it will jump to newly programmed value without pausing or transfer gap in dma transfer and so no audio glitch. The last request was always like silence so that dma can be stop before wrapping up actual audio data. Some of the code is missing in this file which was fixed. Currently the audio driver is not using the continuous double buffering mechanism, it is using the oneshot dma and you may have small glitches in audio when you run higher sampling rate audio. Here I agree with your thought: continuous double buffering mechanism should be in loop if it is last request i.e. do not delete last req until client call dequeue or stop_dma. If new request queued then dma will start transfer to new address/size once current is completed and loop on that request forever. Currently it is not implemented like that. I will correct this with proper documentation on header file about this mode. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-tegra/dma.c b/arch/arm/mach-tegra/dma.c index abea4f6..81a7459 100644 --- a/arch/arm/mach-tegra/dma.c +++ b/arch/arm/mach-tegra/dma.c @@ -423,6 +423,7 @@ static void tegra_dma_update_hw_partial(struct tegra_dma_channel *ch, { u32 apb_ptr; u32 ahb_ptr; + u32 csr; if (req->to_memory) { apb_ptr = req->source_addr; @@ -434,6 +435,15 @@ static void tegra_dma_update_hw_partial(struct tegra_dma_channel *ch, writel(apb_ptr, ch->addr + APB_DMA_CHAN_APB_PTR); writel(ahb_ptr, ch->addr + APB_DMA_CHAN_AHB_PTR); + if (ch->mode & TEGRA_DMA_MODE_CONTINOUS) + ch->req_transfer_count = (req->size >> 3) - 1; + else + ch->req_transfer_count = (req->size >> 2) - 1; + csr = readl(ch->addr + APB_DMA_CHAN_CSR); + csr &= ~CSR_WCOUNT_MASK; + csr |= ch->req_transfer_count << CSR_WCOUNT_SHIFT; + writel(csr, ch->addr + APB_DMA_CHAN_CSR); + req->status = TEGRA_DMA_REQ_INFLIGHT; return; }
Allow the transfer size to vary in each DMA request, rather than assuming all requests to be the same size as the first request made. Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> --- arch/arm/mach-tegra/dma.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)