| Message ID | 87d15nmude.wl%kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Morimoto-san, Thank you for the patch. (By the way the subject line should have mentioned v2) On Monday, 16 October 2017 10:28:35 EEST Kuninori Morimoto wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > SYS/RT/Audio DMAC have both TCR/TCRB. > Its difference is transfer counter value of read (= TCR) > or write (= TCRB). The relationship is like below. > > TCR TCRB > [SOURCE] -> [DMAC] -> [DESTINATION] > > Thus, for residue calculation, we want to read > TCRB when MEM_TO_MEM/DEV_TO_MEM > TCR when MEM_TO_DEV > Otherwise, Sound Capture has noise after PluseAudio support > (= 07b7acb51d2 ("ASoC: rsnd: update pointer more accurate")) In the MEM_TO_DEV direction, what really matters is how much data has been written to memory or to the device. If the DMA is interrupted between read and write, the data read but not written doesn't matter. It doesn't end up in the destination, so shouldn't be counted. TCRB is thus the register we should use in this cases. In the DEV_TO_MEM direction, the situation is more complex. Both the read and write side are important. What matters from a data consumer point of view is how much data has been written to memory. On the other hand, if the transfer is interrupted between read and write, we'll end up losing data (read from the device but never written to memory), which can also be important to report. In practice, however, I see why DMA could be interrupted between read and write in the MEM_TO_DEV case if the device doesn't acknowledge a write for whatever reason. Interruptions in the DEV_TO_MEM case would surprise me, as the write side shouldn't prevent data from reaching memory (or we'd have much worse problems than DMA). Both TCR and TCRB should thus be equivalent in this case. Similarly the MEM_TO_MEM case should be fine with both TCR and TCRB. There could be problems I'm not aware of, so the explanation above might not be correct, but if it is you could use TCRB unconditionally as you did in v1. In any case, this patch uses TCR for MEM_TO_DEV, while I think the correct register in that case is TCRB. I believe it would be worth capturing the above explanation in the commit message and/or comment. > Signed-off-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com> > [Kuninori: add detail info on log, care direction] > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > v1 -> v2 > > - fixup Subject > - care direction > - From Kuninori Morimoto > > drivers/dma/sh/rcar-dmac.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c > index 2b2c7db..f5d7bb7 100644 > --- a/drivers/dma/sh/rcar-dmac.c > +++ b/drivers/dma/sh/rcar-dmac.c > @@ -1246,6 +1246,7 @@ static unsigned int rcar_dmac_chan_get_residue(struct > rcar_dmac_chan *chan, enum dma_status status; > unsigned int residue = 0; > unsigned int dptr = 0; > + u32 reg; > > if (!desc) > return 0; > @@ -1309,8 +1310,17 @@ static unsigned int rcar_dmac_chan_get_residue(struct > rcar_dmac_chan *chan, residue += chunk->size; > } > > - /* Add the residue for the current chunk. */ > - residue += rcar_dmac_chan_read(chan, RCAR_DMATCR) << desc->xfer_shift; > + /* > + * Add the residue for the current chunk > + * > + * TCR TCRB > + * [SOURCE] -> [DMAC] -> [DESTINATION] > + * > + * MEM_TO_xxx : TCR > + * xxx_TO_MEM : TCRB > + */ > + reg = (desc->direction == DMA_MEM_TO_DEV) ? RCAR_DMATCR : RCAR_DMATCRB; > + residue += rcar_dmac_chan_read(chan, reg) << desc->xfer_shift; > > return residue; > }
Hi Laurent Thank you for your review > (By the way the subject line should have mentioned v2) Yeah. I didn't because I exchanged title... > > TCR TCRB > > [SOURCE] -> [DMAC] -> [DESTINATION] (snip) > In the MEM_TO_DEV direction, what really matters is how much data has been > written to memory or to the device. If the DMA is interrupted between read and > write, the data read but not written doesn't matter. It doesn't end up in the > destination, so shouldn't be counted. TCRB is thus the register we should use > in this cases. > > In the DEV_TO_MEM direction, the situation is more complex. Both the read and > write side are important. What matters from a data consumer point of view is > how much data has been written to memory. On the other hand, if the transfer > is interrupted between read and write, we'll end up losing data (read from the > device but never written to memory), which can also be important to report. > > In practice, however, I see why DMA could be interrupted between read and > write in the MEM_TO_DEV case if the device doesn't acknowledge a write for > whatever reason. Interruptions in the DEV_TO_MEM case would surprise me, as > the write side shouldn't prevent data from reaching memory (or we'd have much > worse problems than DMA). Both TCR and TCRB should thus be equivalent in this > case. > > Similarly the MEM_TO_MEM case should be fine with both TCR and TCRB. > > There could be problems I'm not aware of, so the explanation above might not > be correct, but if it is you could use TCRB unconditionally as you did in v1. > In any case, this patch uses TCR for MEM_TO_DEV, while I think the correct > register in that case is TCRB. > > I believe it would be worth capturing the above explanation in the commit > message and/or comment. Thank you for your explanation. My 1st patch focused to "transfer completed" count (= TCRB) for all case. In any case, "completed" information should be used. But in MEM_TO_DEV case, I thought if is OK if data was read from MEM (= the data will be send to DEV automatically, I didn't care about interruption) But yes, your opinion is correct I think. I think MEM_TO_MEM should use TCRB. I think logic is same as your MEM_TO_DEV explanation ? Anyway, in all case I can use TCRB in v3 patch, and it needs abouve explanation. Best regards --- Kuninori Morimoto -- 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
Hi > Thank you for your explanation. > My 1st patch focused to "transfer completed" count (= TCRB) for all case. > In any case, "completed" information should be used. > But in MEM_TO_DEV case, I thought if is OK if data was read from MEM > (= the data will be send to DEV automatically, I didn't care about interruption) > But yes, your opinion is correct I think. > > I think MEM_TO_MEM should use TCRB. > I think logic is same as your MEM_TO_DEV explanation ? > > Anyway, in all case I can use TCRB in v3 patch, > and it needs abouve explanation. If so, I think v1 is enough... ? "transfer completed count is important for all case" is no doubt... ? Best regards --- Kuninori Morimoto -- 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
Hi Morimoto-san, On Tuesday, 17 October 2017 03:18:49 EEST Kuninori Morimoto wrote: > Hi > > > Thank you for your explanation. > > My 1st patch focused to "transfer completed" count (= TCRB) for all case. > > In any case, "completed" information should be used. > > But in MEM_TO_DEV case, I thought if is OK if data was read from MEM > > (= the data will be send to DEV automatically, I didn't care about > > interruption) But yes, your opinion is correct I think. > > > > I think MEM_TO_MEM should use TCRB. > > I think logic is same as your MEM_TO_DEV explanation ? TCRB is better for MEM_TO_MEM too in my opinion. When reporting residue information we should indicate how much data has been transferred, and that includes both read from source and written to destination. > > Anyway, in all case I can use TCRB in v3 patch, > > and it needs abouve explanation. > > If so, I think v1 is enough... ? > "transfer completed count is important for all case" is no doubt... ? That's correct, but I don't think the explanation was detailed and clear enough. If it was Geert wouldn't have asked for a v2, and you wouldn't have agreed to his request :-)
Hi Vinod, Laurent > > > Anyway, in all case I can use TCRB in v3 patch, > > > and it needs abouve explanation. > > > > If so, I think v1 is enough... ? > > "transfer completed count is important for all case" is no doubt... ? > > That's correct, but I don't think the explanation was detailed and clear > enough. If it was Geert wouldn't have asked for a v2, and you wouldn't have > agreed to his request :-) OK. Let's follow Vinod's decision. Vinod, I'm happy if you are OK on v1. And I'm happy to create v3 patch which includes detail reason which is explained by Laurent if you want. Best regards --- Kuninori Morimoto -- 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
Hi Morimoto-san, On Wednesday, 18 October 2017 03:01:28 EEST Kuninori Morimoto wrote: > Hi Vinod, Laurent > > >>> Anyway, in all case I can use TCRB in v3 patch, > >>> and it needs abouve explanation. > >> > >> If so, I think v1 is enough... ? > >> "transfer completed count is important for all case" is no doubt... ? > > > > That's correct, but I don't think the explanation was detailed and clear > > enough. If it was Geert wouldn't have asked for a v2, and you wouldn't > > have > > agreed to his request :-) > > OK. Let's follow Vinod's decision. > > Vinod, I'm happy if you are OK on v1. > And I'm happy to create v3 patch which includes detail reason > which is explained by Laurent if you want. I'd be happier with v3 :-)
On Wed, Oct 18, 2017 at 3:46 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Wednesday, 18 October 2017 03:01:28 EEST Kuninori Morimoto wrote: >> >>> Anyway, in all case I can use TCRB in v3 patch, >> >>> and it needs abouve explanation. >> >> If so, I think v1 is enough... ? >> >> "transfer completed count is important for all case" is no doubt... ? >> > >> > That's correct, but I don't think the explanation was detailed and clear >> > enough. If it was Geert wouldn't have asked for a v2, and you wouldn't >> > have >> > agreed to his request :-) >> >> OK. Let's follow Vinod's decision. >> >> Vinod, I'm happy if you are OK on v1. >> And I'm happy to create v3 patch which includes detail reason >> which is explained by Laurent if you want. > > I'd be happier with v3 :-) +1 Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- 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
Hi Laurent, Geert > >> > That's correct, but I don't think the explanation was detailed and clear > >> > enough. If it was Geert wouldn't have asked for a v2, and you wouldn't > >> > have > >> > agreed to his request :-) > >> > >> OK. Let's follow Vinod's decision. > >> > >> Vinod, I'm happy if you are OK on v1. > >> And I'm happy to create v3 patch which includes detail reason > >> which is explained by Laurent if you want. > > > > I'd be happier with v3 :-) Oops, I didn't explain this. This DMAC has buffer. thus it will take a while for TCR and TCRB to become equal. I will add this to log in v3 Best regards --- Kuninori Morimoto -- 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 Wed, Oct 18, 2017 at 12:01:28AM +0000, Kuninori Morimoto wrote: > > Hi Vinod, Laurent > > > > > Anyway, in all case I can use TCRB in v3 patch, > > > > and it needs abouve explanation. > > > > > > If so, I think v1 is enough... ? > > > "transfer completed count is important for all case" is no doubt... ? > > > > That's correct, but I don't think the explanation was detailed and clear > > enough. If it was Geert wouldn't have asked for a v2, and you wouldn't have > > agreed to his request :-) > > OK. Let's follow Vinod's decision. > > Vinod, I'm happy if you are OK on v1. > And I'm happy to create v3 patch which includes detail reason > which is explained by Laurent if you want. Yes that is correct assumption. Only completed data matters :)
diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c index 2b2c7db..f5d7bb7 100644 --- a/drivers/dma/sh/rcar-dmac.c +++ b/drivers/dma/sh/rcar-dmac.c @@ -1246,6 +1246,7 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan, enum dma_status status; unsigned int residue = 0; unsigned int dptr = 0; + u32 reg; if (!desc) return 0; @@ -1309,8 +1310,17 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan, residue += chunk->size; } - /* Add the residue for the current chunk. */ - residue += rcar_dmac_chan_read(chan, RCAR_DMATCR) << desc->xfer_shift; + /* + * Add the residue for the current chunk + * + * TCR TCRB + * [SOURCE] -> [DMAC] -> [DESTINATION] + * + * MEM_TO_xxx : TCR + * xxx_TO_MEM : TCRB + */ + reg = (desc->direction == DMA_MEM_TO_DEV) ? RCAR_DMATCR : RCAR_DMATCRB; + residue += rcar_dmac_chan_read(chan, reg) << desc->xfer_shift; return residue; }