* [PATCH] dmaengine/dw_dmac: Add support for device_prep_dma_sg @ 2011-12-13 8:47 Pratyush Anand 2011-12-13 11:54 ` Linus Walleij 2011-12-23 15:58 ` Vinod Koul 0 siblings, 2 replies; 9+ messages in thread From: Pratyush Anand @ 2011-12-13 8:47 UTC (permalink / raw) To: linux-arm-kernel Memory to memory copy with scatter gather option has been implemented in this patch. Driver can manage even if number of nodes in src and dst list is different. Signed-off-by: Pratyush Anand <pratyush.anand@st.com> --- drivers/dma/dw_dmac.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 135 insertions(+), 0 deletions(-) diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c index 96b2750..c1c78c1 100644 --- a/drivers/dma/dw_dmac.c +++ b/drivers/dma/dw_dmac.c @@ -729,6 +729,140 @@ err_desc_get: } static struct dma_async_tx_descriptor * +dwc_prep_dma_sg(struct dma_chan *chan, + struct scatterlist *dsgl, unsigned int dst_nents, + struct scatterlist *ssgl, unsigned int src_nents, + unsigned long flags) +{ + struct dw_dma_chan *dwc = to_dw_dma_chan(chan); + struct dw_desc *desc; + struct dw_desc *first; + struct dw_desc *prev; + size_t xfer_count; + size_t offset; + unsigned int src_width; + unsigned int dst_width; + u32 ctllo; + struct scatterlist *dsg, *ssg; + dma_addr_t src = 0, dst = 0; + unsigned int slen = 0, dlen = 0, total_len = 0; + unsigned int i, nents, len = 0, incs, si = 0, di = 0; + + if (unlikely(!src_nents)) + return NULL; + + if (unlikely(!dst_nents)) + return NULL; + + prev = first = NULL; + + dsg = dsgl; + ssg = ssgl; + nents = max(src_nents, dst_nents); + + if (nents == src_nents) + incs = true; + else + incs = false; + + for (i = 0; i < nents;) { + if (!slen) { + src = sg_dma_address(ssg); + slen = sg_dma_len(ssg); + ssg = sg_next(ssg); + if (!ssg && si < src_nents - 1) + goto err_sg_len; + si++; + } else + src += len; + + if (!dlen) { + dst = sg_dma_address(dsg); + dlen = sg_dma_len(dsg); + dsg = sg_next(dsg); + if (!dsg && di < dst_nents - 1) + goto err_sg_len; + di++; + } else + dst += len; + + if (incs) + i = si; + else + i = di; + len = min(slen, dlen); + slen -= len; + dlen -= len; + + if (!((src | dst | len) & 7)) + src_width = dst_width = 3; + else if (!((src | dst | len) & 3)) + src_width = dst_width = 2; + else if (!((src | dst | len) & 1)) + src_width = dst_width = 1; + else + src_width = dst_width = 0; + + ctllo = DWC_DEFAULT_CTLLO(chan->private) + | DWC_CTLL_DST_WIDTH(dst_width) + | DWC_CTLL_SRC_WIDTH(src_width) + | DWC_CTLL_DST_INC + | DWC_CTLL_SRC_INC + | DWC_CTLL_FC_M2M; + + offset = 0; + while ((len - offset) > 0) { + + xfer_count = min_t(size_t, (len - offset) >> dst_width, + DWC_MAX_COUNT); + + desc = dwc_desc_get(dwc); + if (!desc) + goto err_desc_get; + + desc->lli.sar = src + offset; + desc->lli.dar = dst + offset; + desc->lli.ctllo = ctllo; + desc->lli.ctlhi = xfer_count; + + if (!first) { + first = desc; + } else { + prev->lli.llp = desc->txd.phys; + dma_sync_single_for_device(chan2parent(chan), + prev->txd.phys, + sizeof(prev->lli), + DMA_TO_DEVICE); + list_add_tail(&desc->desc_node, + &first->tx_list); + } + prev = desc; + offset += xfer_count << dst_width; + } + total_len += len; + } + + if (flags & DMA_PREP_INTERRUPT) + /* Trigger interrupt after last block */ + prev->lli.ctllo |= DWC_CTLL_INT_EN; + + prev->lli.llp = 0; + dma_sync_single_for_device(chan2parent(chan), + prev->txd.phys, sizeof(prev->lli), + DMA_TO_DEVICE); + + first->txd.flags = flags; + first->len = total_len; + + return &first->txd; + +err_sg_len: +err_desc_get: + dwc_desc_put(dwc, first); + return NULL; +} + +static struct dma_async_tx_descriptor * dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, unsigned int sg_len, enum dma_transfer_direction direction, unsigned long flags) @@ -1471,6 +1605,7 @@ static int __init dw_probe(struct platform_device *pdev) dw->dma.device_free_chan_resources = dwc_free_chan_resources; dw->dma.device_prep_dma_memcpy = dwc_prep_dma_memcpy; + dw->dma.device_prep_dma_sg = dwc_prep_dma_sg; dw->dma.device_prep_slave_sg = dwc_prep_slave_sg; dw->dma.device_control = dwc_control; -- 1.7.2.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] dmaengine/dw_dmac: Add support for device_prep_dma_sg 2011-12-13 8:47 [PATCH] dmaengine/dw_dmac: Add support for device_prep_dma_sg Pratyush Anand @ 2011-12-13 11:54 ` Linus Walleij 2011-12-23 15:58 ` Vinod Koul 1 sibling, 0 replies; 9+ messages in thread From: Linus Walleij @ 2011-12-13 11:54 UTC (permalink / raw) To: linux-arm-kernel On Tue, Dec 13, 2011 at 9:47 AM, Pratyush Anand <pratyush.anand@st.com> wrote: > Memory to memory copy with scatter gather option has been implemented in > this patch. Driver can manage even if number of nodes in src and dst > list is different. > > Signed-off-by: Pratyush Anand <pratyush.anand@st.com> Looks good to me. Acked-by: Linus Walleij <linus.walleij@linaro.org> Thanks, Linus Walleij ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] dmaengine/dw_dmac: Add support for device_prep_dma_sg 2011-12-13 8:47 [PATCH] dmaengine/dw_dmac: Add support for device_prep_dma_sg Pratyush Anand 2011-12-13 11:54 ` Linus Walleij @ 2011-12-23 15:58 ` Vinod Koul 2012-01-02 11:20 ` Pratyush Anand 1 sibling, 1 reply; 9+ messages in thread From: Vinod Koul @ 2011-12-23 15:58 UTC (permalink / raw) To: linux-arm-kernel On Tue, 2011-12-13 at 14:17 +0530, Pratyush Anand wrote: > Memory to memory copy with scatter gather option has been implemented in > this patch. Driver can manage even if number of nodes in src and dst > list is different. The logic looks okay, but it would be great if we could split this prepare here. Possibly later when we have multiple such cases we can create templates for parsing the non linear sg list and create linear lists from it. > > Signed-off-by: Pratyush Anand <pratyush.anand@st.com> > --- > drivers/dma/dw_dmac.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 135 insertions(+), 0 deletions(-) > > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c > index 96b2750..c1c78c1 100644 > --- a/drivers/dma/dw_dmac.c > +++ b/drivers/dma/dw_dmac.c > @@ -729,6 +729,140 @@ err_desc_get: > } > > static struct dma_async_tx_descriptor * > +dwc_prep_dma_sg(struct dma_chan *chan, > + struct scatterlist *dsgl, unsigned int dst_nents, > + struct scatterlist *ssgl, unsigned int src_nents, > + unsigned long flags) > +{ > + struct dw_dma_chan *dwc = to_dw_dma_chan(chan); > + struct dw_desc *desc; > + struct dw_desc *first; > + struct dw_desc *prev; > + size_t xfer_count; > + size_t offset; > + unsigned int src_width; > + unsigned int dst_width; > + u32 ctllo; > + struct scatterlist *dsg, *ssg; > + dma_addr_t src = 0, dst = 0; > + unsigned int slen = 0, dlen = 0, total_len = 0; > + unsigned int i, nents, len = 0, incs, si = 0, di = 0; > + > + if (unlikely(!src_nents)) > + return NULL; > + > + if (unlikely(!dst_nents)) > + return NULL; > + > + prev = first = NULL; > + > + dsg = dsgl; > + ssg = ssgl; > + nents = max(src_nents, dst_nents); shouldn't you validate if total src sg length and dstn sg lengths are same here? > + > + if (nents == src_nents) > + incs = true; > + else > + incs = false; > + > + for (i = 0; i < nents;) { > + if (!slen) { > + src = sg_dma_address(ssg); > + slen = sg_dma_len(ssg); > + ssg = sg_next(ssg); > + if (!ssg && si < src_nents - 1) > + goto err_sg_len; > + si++; > + } else > + src += len; > + > + if (!dlen) { > + dst = sg_dma_address(dsg); > + dlen = sg_dma_len(dsg); > + dsg = sg_next(dsg); > + if (!dsg && di < dst_nents - 1) > + goto err_sg_len; this could be made better lokking by using helpers > + di++; > + } else > + dst += len; > + > + if (incs) > + i = si; > + else > + i = di; > + len = min(slen, dlen); > + slen -= len; > + dlen -= len; > + > + if (!((src | dst | len) & 7)) > + src_width = dst_width = 3; > + else if (!((src | dst | len) & 3)) > + src_width = dst_width = 2; > + else if (!((src | dst | len) & 1)) > + src_width = dst_width = 1; > + else > + src_width = dst_width = 0; how about putting this is get_me_the_length() > + > + ctllo = DWC_DEFAULT_CTLLO(chan->private) > + | DWC_CTLL_DST_WIDTH(dst_width) > + | DWC_CTLL_SRC_WIDTH(src_width) > + | DWC_CTLL_DST_INC > + | DWC_CTLL_SRC_INC > + | DWC_CTLL_FC_M2M; > + > + offset = 0; > + while ((len - offset) > 0) { > + > + xfer_count = min_t(size_t, (len - offset) >> dst_width, > + DWC_MAX_COUNT); > + > + desc = dwc_desc_get(dwc); > + if (!desc) > + goto err_desc_get; > + > + desc->lli.sar = src + offset; > + desc->lli.dar = dst + offset; > + desc->lli.ctllo = ctllo; > + desc->lli.ctlhi = xfer_count; > + > + if (!first) { > + first = desc; > + } else { > + prev->lli.llp = desc->txd.phys; > + dma_sync_single_for_device(chan2parent(chan), > + prev->txd.phys, > + sizeof(prev->lli), > + DMA_TO_DEVICE); > + list_add_tail(&desc->desc_node, > + &first->tx_list); > + } > + prev = desc; > + offset += xfer_count << dst_width; > + } > + total_len += len; > + } > + > + if (flags & DMA_PREP_INTERRUPT) > + /* Trigger interrupt after last block */ > + prev->lli.ctllo |= DWC_CTLL_INT_EN; > + > + prev->lli.llp = 0; > + dma_sync_single_for_device(chan2parent(chan), > + prev->txd.phys, sizeof(prev->lli), > + DMA_TO_DEVICE); > + > + first->txd.flags = flags; > + first->len = total_len; > + > + return &first->txd; > + > +err_sg_len: > +err_desc_get: > + dwc_desc_put(dwc, first); > + return NULL; > +} > + > +static struct dma_async_tx_descriptor * > dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, > unsigned int sg_len, enum dma_transfer_direction direction, > unsigned long flags) > @@ -1471,6 +1605,7 @@ static int __init dw_probe(struct platform_device *pdev) > dw->dma.device_free_chan_resources = dwc_free_chan_resources; > > dw->dma.device_prep_dma_memcpy = dwc_prep_dma_memcpy; > + dw->dma.device_prep_dma_sg = dwc_prep_dma_sg; > > dw->dma.device_prep_slave_sg = dwc_prep_slave_sg; > dw->dma.device_control = dwc_control; > -- > 1.7.2.2 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- ~Vinod ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] dmaengine/dw_dmac: Add support for device_prep_dma_sg 2011-12-23 15:58 ` Vinod Koul @ 2012-01-02 11:20 ` Pratyush Anand 2012-01-02 11:25 ` Vinod Koul 0 siblings, 1 reply; 9+ messages in thread From: Pratyush Anand @ 2012-01-02 11:20 UTC (permalink / raw) To: linux-arm-kernel On 12/23/2011 9:28 PM, Vinod Koul wrote: > On Tue, 2011-12-13 at 14:17 +0530, Pratyush Anand wrote: >> Memory to memory copy with scatter gather option has been implemented in >> this patch. Driver can manage even if number of nodes in src and dst >> list is different. > The logic looks okay, but it would be great if we could split this > prepare here. > Possibly later when we have multiple such cases we can create templates > for parsing the non linear sg list and create linear lists from it. > Ok. So you suggest to have one patch with a function to align sglist w.r.t length, and to implement this dwc_prep_dma_sg with aligned list only. Caller of dwc_prep_dma_sg function will also call something like align_sg_list(unaligned_dsgl, unaligned ssgl, aligned dsgl,aligned ssgl); dwc_prep_dma_sg(chan, aligned dsgl, dnents, aligned ssgl, snents, flag); Is that what you suggest? will do that. It would be helpful if we can also conclude following discussion. https://lkml.org/lkml/2011/12/20/95 I am waiting for your reply. >> >> Signed-off-by: Pratyush Anand<pratyush.anand@st.com> >> --- >> drivers/dma/dw_dmac.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 135 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c >> index 96b2750..c1c78c1 100644 >> --- a/drivers/dma/dw_dmac.c >> +++ b/drivers/dma/dw_dmac.c >> @@ -729,6 +729,140 @@ err_desc_get: >> } >> >> static struct dma_async_tx_descriptor * >> +dwc_prep_dma_sg(struct dma_chan *chan, >> + struct scatterlist *dsgl, unsigned int dst_nents, >> + struct scatterlist *ssgl, unsigned int src_nents, >> + unsigned long flags) >> +{ >> + struct dw_dma_chan *dwc = to_dw_dma_chan(chan); >> + struct dw_desc *desc; >> + struct dw_desc *first; >> + struct dw_desc *prev; >> + size_t xfer_count; >> + size_t offset; >> + unsigned int src_width; >> + unsigned int dst_width; >> + u32 ctllo; >> + struct scatterlist *dsg, *ssg; >> + dma_addr_t src = 0, dst = 0; >> + unsigned int slen = 0, dlen = 0, total_len = 0; >> + unsigned int i, nents, len = 0, incs, si = 0, di = 0; >> + >> + if (unlikely(!src_nents)) >> + return NULL; >> + >> + if (unlikely(!dst_nents)) >> + return NULL; >> + >> + prev = first = NULL; >> + >> + dsg = dsgl; >> + ssg = ssgl; >> + nents = max(src_nents, dst_nents); > shouldn't you validate if total src sg length and dstn sg lengths are > same here? Yes, will modify. Probably we will not need it having aligned sglists. >> + >> + if (nents == src_nents) >> + incs = true; >> + else >> + incs = false; >> + >> + for (i = 0; i< nents;) { >> + if (!slen) { >> + src = sg_dma_address(ssg); >> + slen = sg_dma_len(ssg); >> + ssg = sg_next(ssg); >> + if (!ssg&& si< src_nents - 1) >> + goto err_sg_len; >> + si++; >> + } else >> + src += len; >> + >> + if (!dlen) { >> + dst = sg_dma_address(dsg); >> + dlen = sg_dma_len(dsg); >> + dsg = sg_next(dsg); >> + if (!dsg&& di< dst_nents - 1) >> + goto err_sg_len; > this could be made better lokking by using helpers ok. having aligned sglists, logic will be lot simpler. > >> + di++; >> + } else >> + dst += len; >> + >> + if (incs) >> + i = si; >> + else >> + i = di; >> + len = min(slen, dlen); >> + slen -= len; >> + dlen -= len; >> + >> + if (!((src | dst | len)& 7)) >> + src_width = dst_width = 3; >> + else if (!((src | dst | len)& 3)) >> + src_width = dst_width = 2; >> + else if (!((src | dst | len)& 1)) >> + src_width = dst_width = 1; >> + else >> + src_width = dst_width = 0; > how about putting this is get_me_the_length() Yes, can do that. Similar logic has been used in other function in this file. So may be I need to modify those functions too in a separate patch. >> + >> + ctllo = DWC_DEFAULT_CTLLO(chan->private) >> + | DWC_CTLL_DST_WIDTH(dst_width) >> + | DWC_CTLL_SRC_WIDTH(src_width) >> + | DWC_CTLL_DST_INC >> + | DWC_CTLL_SRC_INC >> + | DWC_CTLL_FC_M2M; >> + >> + offset = 0; >> + while ((len - offset)> 0) { >> + >> + xfer_count = min_t(size_t, (len - offset)>> dst_width, >> + DWC_MAX_COUNT); >> + >> + desc = dwc_desc_get(dwc); >> + if (!desc) >> + goto err_desc_get; >> + >> + desc->lli.sar = src + offset; >> + desc->lli.dar = dst + offset; >> + desc->lli.ctllo = ctllo; >> + desc->lli.ctlhi = xfer_count; >> + >> + if (!first) { >> + first = desc; >> + } else { >> + prev->lli.llp = desc->txd.phys; >> + dma_sync_single_for_device(chan2parent(chan), >> + prev->txd.phys, >> + sizeof(prev->lli), >> + DMA_TO_DEVICE); >> + list_add_tail(&desc->desc_node, >> + &first->tx_list); >> + } >> + prev = desc; >> + offset += xfer_count<< dst_width; >> + } >> + total_len += len; >> + } >> + >> + if (flags& DMA_PREP_INTERRUPT) >> + /* Trigger interrupt after last block */ >> + prev->lli.ctllo |= DWC_CTLL_INT_EN; >> + >> + prev->lli.llp = 0; >> + dma_sync_single_for_device(chan2parent(chan), >> + prev->txd.phys, sizeof(prev->lli), >> + DMA_TO_DEVICE); >> + >> + first->txd.flags = flags; >> + first->len = total_len; >> + >> + return&first->txd; >> + >> +err_sg_len: >> +err_desc_get: >> + dwc_desc_put(dwc, first); >> + return NULL; >> +} >> + >> +static struct dma_async_tx_descriptor * >> dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, >> unsigned int sg_len, enum dma_transfer_direction direction, >> unsigned long flags) >> @@ -1471,6 +1605,7 @@ static int __init dw_probe(struct platform_device *pdev) >> dw->dma.device_free_chan_resources = dwc_free_chan_resources; >> >> dw->dma.device_prep_dma_memcpy = dwc_prep_dma_memcpy; >> + dw->dma.device_prep_dma_sg = dwc_prep_dma_sg; >> >> dw->dma.device_prep_slave_sg = dwc_prep_slave_sg; >> dw->dma.device_control = dwc_control; >> -- >> 1.7.2.2 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel at lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] dmaengine/dw_dmac: Add support for device_prep_dma_sg 2012-01-02 11:20 ` Pratyush Anand @ 2012-01-02 11:25 ` Vinod Koul 2012-01-03 9:40 ` Russell King - ARM Linux 0 siblings, 1 reply; 9+ messages in thread From: Vinod Koul @ 2012-01-02 11:25 UTC (permalink / raw) To: linux-arm-kernel On Mon, 2012-01-02 at 16:50 +0530, Pratyush Anand wrote: > On 12/23/2011 9:28 PM, Vinod Koul wrote: > > On Tue, 2011-12-13 at 14:17 +0530, Pratyush Anand wrote: > >> Memory to memory copy with scatter gather option has been implemented in > >> this patch. Driver can manage even if number of nodes in src and dst > >> list is different. > > The logic looks okay, but it would be great if we could split this > > prepare here. > > Possibly later when we have multiple such cases we can create templates > > for parsing the non linear sg list and create linear lists from it. > > > > Ok. So you suggest to have one patch with a function to align sglist > w.r.t length, and to implement this dwc_prep_dma_sg with aligned list only. IMO that would be a much better approach > > Caller of dwc_prep_dma_sg function will also call something like > align_sg_list(unaligned_dsgl, unaligned ssgl, aligned dsgl,aligned ssgl); and this one can be common code in dmaengine > dwc_prep_dma_sg(chan, aligned dsgl, dnents, aligned ssgl, snents, flag); > > Is that what you suggest? will do that. > > It would be helpful if we can also conclude following discussion. > https://lkml.org/lkml/2011/12/20/95 > I am waiting for your reply. Missed that, will check it now -- ~Vinod ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] dmaengine/dw_dmac: Add support for device_prep_dma_sg 2012-01-02 11:25 ` Vinod Koul @ 2012-01-03 9:40 ` Russell King - ARM Linux 2012-01-03 13:03 ` Vinod Koul 0 siblings, 1 reply; 9+ messages in thread From: Russell King - ARM Linux @ 2012-01-03 9:40 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 02, 2012 at 04:55:35PM +0530, Vinod Koul wrote: > On Mon, 2012-01-02 at 16:50 +0530, Pratyush Anand wrote: > > On 12/23/2011 9:28 PM, Vinod Koul wrote: > > > On Tue, 2011-12-13 at 14:17 +0530, Pratyush Anand wrote: > > >> Memory to memory copy with scatter gather option has been implemented in > > >> this patch. Driver can manage even if number of nodes in src and dst > > >> list is different. > > > The logic looks okay, but it would be great if we could split this > > > prepare here. > > > Possibly later when we have multiple such cases we can create templates > > > for parsing the non linear sg list and create linear lists from it. > > > > > > > Ok. So you suggest to have one patch with a function to align sglist > > w.r.t length, and to implement this dwc_prep_dma_sg with aligned list only. > IMO that would be a much better approach > > > > Caller of dwc_prep_dma_sg function will also call something like > > align_sg_list(unaligned_dsgl, unaligned ssgl, aligned dsgl,aligned ssgl); > and this one can be common code in dmaengine Wait a moment. This looks like a disaster waiting to happen. The DMA engine code doesn't really handle the DMA API properly as it is - and that has lead to at least one recent oops report (and it still remains unresolved.) The DMA API has the idea of buffer ownership: a buffer is either owned by the CPU, or the DMA device. Only its owner may explicitly access the buffer. Before a buffer can be used for DMA, it must be mapped to the DMA device (using dma_map_sg() or dma_map_single().) Once this call returns, the mapping is setup and the CPU must not explicitly access the buffer until the buffer is unmapped via dma_unmap_sg() or dma_unmap_single(). With the DMA engine API, the caller is responsible for mapping the buffer. This means that if you want to ensure that the buffer is correctly aligned, you must check the alignments _before_ mapping the buffer, and do any appropriate copies at this stage. So, it must be: - align_sg_list - map aligned lists - prep_dma_sg and not - map aligned lists - align_sg_list - prep_dma_sg because then you're violating the DMA API by having the CPU access an already mapped buffer - and that will lead to data corruption. Finally, consider this: you have two scatterlists which ask you to copy between two buffers. The first is offset by one byte from a 32-bit word boundary. The second is offset by two bytes from a 32-bit word boundary. Your DMA engine can only perform naturally aligned 32-bit transfers for both the source and destination. How do you see this situation being dealt with? ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] dmaengine/dw_dmac: Add support for device_prep_dma_sg 2012-01-03 9:40 ` Russell King - ARM Linux @ 2012-01-03 13:03 ` Vinod Koul 2012-01-03 13:43 ` Russell King - ARM Linux 0 siblings, 1 reply; 9+ messages in thread From: Vinod Koul @ 2012-01-03 13:03 UTC (permalink / raw) To: linux-arm-kernel On Tue, 2012-01-03 at 09:40 +0000, Russell King - ARM Linux wrote: > On Mon, Jan 02, 2012 at 04:55:35PM +0530, Vinod Koul wrote: > > On Mon, 2012-01-02 at 16:50 +0530, Pratyush Anand wrote: > > > On 12/23/2011 9:28 PM, Vinod Koul wrote: > > > > On Tue, 2011-12-13 at 14:17 +0530, Pratyush Anand wrote: > > > >> Memory to memory copy with scatter gather option has been implemented in > > > >> this patch. Driver can manage even if number of nodes in src and dst > > > >> list is different. > > > > The logic looks okay, but it would be great if we could split this > > > > prepare here. > > > > Possibly later when we have multiple such cases we can create templates > > > > for parsing the non linear sg list and create linear lists from it. > > > > > > > > > > Ok. So you suggest to have one patch with a function to align sglist > > > w.r.t length, and to implement this dwc_prep_dma_sg with aligned list only. > > IMO that would be a much better approach > > > > > > Caller of dwc_prep_dma_sg function will also call something like > > > align_sg_list(unaligned_dsgl, unaligned ssgl, aligned dsgl,aligned ssgl); > > and this one can be common code in dmaengine > > Wait a moment. This looks like a disaster waiting to happen. The DMA > engine code doesn't really handle the DMA API properly as it is - and > that has lead to at least one recent oops report (and it still remains > unresolved.) > > The DMA API has the idea of buffer ownership: a buffer is either owned by > the CPU, or the DMA device. Only its owner may explicitly access the > buffer. > > Before a buffer can be used for DMA, it must be mapped to the DMA device > (using dma_map_sg() or dma_map_single().) Once this call returns, the > mapping is setup and the CPU must not explicitly access the buffer until > the buffer is unmapped via dma_unmap_sg() or dma_unmap_single(). > > With the DMA engine API, the caller is responsible for mapping the buffer. > > This means that if you want to ensure that the buffer is correctly aligned, > you must check the alignments _before_ mapping the buffer, and do any > appropriate copies at this stage. > > So, it must be: > - align_sg_list > - map aligned lists > - prep_dma_sg Yes, that is something I had in mind as well. For slave-dma as we documented, the peripheral driver needs to take care of mapping hence it should do above as you suggested and not the one below. My suggestion was to have a generic alignment routine for sg list (which may be used by other drivers as well) in some common place. > and not > - map aligned lists > - align_sg_list > - prep_dma_sg > because then you're violating the DMA API by having the CPU access an > already mapped buffer - and that will lead to data corruption. > > Finally, consider this: you have two scatterlists which ask you to copy > between two buffers. The first is offset by one byte from a 32-bit word > boundary. The second is offset by two bytes from a 32-bit word > boundary. Your DMA engine can only perform naturally aligned 32-bit > transfers for both the source and destination. How do you see this > situation being dealt with? in this case, i don't think dmaengine can perform the task. it should check for required alignments in the respective prepare function, and return error if it cant support it. Nevertheless, all prepare functions should be checking for word alignment of source and destination... -- ~Vinod ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] dmaengine/dw_dmac: Add support for device_prep_dma_sg 2012-01-03 13:03 ` Vinod Koul @ 2012-01-03 13:43 ` Russell King - ARM Linux 2012-01-04 13:47 ` Vinod Koul 0 siblings, 1 reply; 9+ messages in thread From: Russell King - ARM Linux @ 2012-01-03 13:43 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 03, 2012 at 06:33:36PM +0530, Vinod Koul wrote: > On Tue, 2012-01-03 at 09:40 +0000, Russell King - ARM Linux wrote: > > Wait a moment. This looks like a disaster waiting to happen. The DMA > > engine code doesn't really handle the DMA API properly as it is - and > > that has lead to at least one recent oops report (and it still remains > > unresolved.) > > > > The DMA API has the idea of buffer ownership: a buffer is either owned by > > the CPU, or the DMA device. Only its owner may explicitly access the > > buffer. > > > > Before a buffer can be used for DMA, it must be mapped to the DMA device > > (using dma_map_sg() or dma_map_single().) Once this call returns, the > > mapping is setup and the CPU must not explicitly access the buffer until > > the buffer is unmapped via dma_unmap_sg() or dma_unmap_single(). > > > > With the DMA engine API, the caller is responsible for mapping the buffer. > > > > This means that if you want to ensure that the buffer is correctly aligned, > > you must check the alignments _before_ mapping the buffer, and do any > > appropriate copies at this stage. > > > > So, it must be: > > - align_sg_list > > - map aligned lists > > - prep_dma_sg > Yes, that is something I had in mind as well. > For slave-dma as we documented, the peripheral driver needs to take care > of mapping hence it should do above as you suggested and not the one > below. > My suggestion was to have a generic alignment routine for sg list (which > may be used by other drivers as well) in some common place. > > and not > > - map aligned lists > > - align_sg_list > > - prep_dma_sg > > because then you're violating the DMA API by having the CPU access an > > already mapped buffer - and that will lead to data corruption. > > > > Finally, consider this: you have two scatterlists which ask you to copy > > between two buffers. The first is offset by one byte from a 32-bit word > > boundary. The second is offset by two bytes from a 32-bit word > > boundary. Your DMA engine can only perform naturally aligned 32-bit > > transfers for both the source and destination. How do you see this > > situation being dealt with? > in this case, i don't think dmaengine can perform the task. > it should check for required alignments in the respective prepare > function, and return error if it cant support it. > Nevertheless, all prepare functions should be checking for word > alignment of source and destination... This is sub-optimal, because then you end up with: - align_sg_list - map lists - prep_dma_sg - unmap lists - perform manual copy which, if your map/unmap is non-trivial, means you take an unnecessary hit. It would be much better if the dmaengine code exposed its alignment properties in such a way that: (a) align_sg_list could be totally generic code (b) it can be found out whether the sg list can be handled by the DMA engine I'd also argue that align_sg_list() probably shouldn't even try to align a sg list - it should really just indicate whether the sg list _could_ be handled by the DMA engine code or not. (The case where the source and destination are identically mis-aligned is probably a rare corner case of the mis-aligned sg list.) ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] dmaengine/dw_dmac: Add support for device_prep_dma_sg 2012-01-03 13:43 ` Russell King - ARM Linux @ 2012-01-04 13:47 ` Vinod Koul 0 siblings, 0 replies; 9+ messages in thread From: Vinod Koul @ 2012-01-04 13:47 UTC (permalink / raw) To: linux-arm-kernel On Tue, 2012-01-03 at 13:43 +0000, Russell King - ARM Linux wrote: > On Tue, Jan 03, 2012 at 06:33:36PM +0530, Vinod Koul wrote: > > On Tue, 2012-01-03 at 09:40 +0000, Russell King - ARM Linux wrote: > > > Wait a moment. This looks like a disaster waiting to happen. The DMA > > > engine code doesn't really handle the DMA API properly as it is - and > > > that has lead to at least one recent oops report (and it still remains > > > unresolved.) > > > > > > The DMA API has the idea of buffer ownership: a buffer is either owned by > > > the CPU, or the DMA device. Only its owner may explicitly access the > > > buffer. > > > > > > Before a buffer can be used for DMA, it must be mapped to the DMA device > > > (using dma_map_sg() or dma_map_single().) Once this call returns, the > > > mapping is setup and the CPU must not explicitly access the buffer until > > > the buffer is unmapped via dma_unmap_sg() or dma_unmap_single(). wouldn't this cause issues with audio users of dmaengine API. They would use DMA API to map the buffer and do a prepare (cyclic). And both cpu and dma and accessing this? > This is sub-optimal, because then you end up with: > > - align_sg_list > - map lists > - prep_dma_sg > - unmap lists > - perform manual copy > > which, if your map/unmap is non-trivial, means you take an unnecessary hit. > > It would be much better if the dmaengine code exposed its alignment > properties in such a way that: > > (a) align_sg_list could be totally generic code > (b) it can be found out whether the sg list can be handled by the DMA > engine > > I'd also argue that align_sg_list() probably shouldn't even try to align > a sg list - it should really just indicate whether the sg list _could_ > be handled by the DMA engine code or not. (The case where the source > and destination are identically mis-aligned is probably a rare corner > case of the mis-aligned sg list.) Yes, we could add something like dmaengine_check_align() which would check if the given word alignment is supported by dmac or not. Based on this peripheral driver can take a right decision on what needs to be done. Of-course, we would need controller to let dmaengine know the word alignment constraints of the dmac. -- ~Vinod ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-01-04 13:47 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-12-13 8:47 [PATCH] dmaengine/dw_dmac: Add support for device_prep_dma_sg Pratyush Anand 2011-12-13 11:54 ` Linus Walleij 2011-12-23 15:58 ` Vinod Koul 2012-01-02 11:20 ` Pratyush Anand 2012-01-02 11:25 ` Vinod Koul 2012-01-03 9:40 ` Russell King - ARM Linux 2012-01-03 13:03 ` Vinod Koul 2012-01-03 13:43 ` Russell King - ARM Linux 2012-01-04 13:47 ` Vinod Koul
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).