Hi Robin, Am Montag, den 06.08.2018, 08:04 +0000 schrieb Robin Gong: > Hello Lucas, > Any comment for my reply? So I've looked at this again and sadly I need to NACK this patch. It is a total API abuse of the dma_pool API and even the patch introducing the dma_pool usage in this driver is wrong and should be reverted. The SDMA need contiguous buffer descriptors for each channel, something the dma_pool abstraction isn't able to provide. So either the dma_pool implementation needs to be extended to support this use-case, or you can't use this at all in the sdma driver. Adding hacks, which are abusing the API, to cram a dma_pool into the sdma driver is not a valid way to implement things for upstream. Regards, Lucas > > -----Original Message----- > > From: Robin Gong > > Sent: 2018年7月25日 9:25 > > To: 'Lucas Stach' <l.stach@xxxxxxxxxxxxxx>; vkoul@xxxxxxxxxx; > > dan.j.williams@xxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; linux@xxxxxxxxxxx > > g.uk > > Cc: dmaengine@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; > > kernel@xxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > > linux-kernel@xxxxxxxxxxxxxxx > > Subject: RE: [PATCH v3 3/3] dmaengine: imx-sdma: allocate max 20 > > bds for one > > transfer > > > > > -----Original Message----- > > > From: Lucas Stach [mailto:l.stach@xxxxxxxxxxxxxx] > > > Sent: 2018年7月24日 17:22 > > > To: Robin Gong <yibin.gong@xxxxxxx>; vkoul@xxxxxxxxxx; > > > dan.j.williams@xxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; > > > linux@xxxxxxxxxxxxxxx > > > Cc: dmaengine@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; > > > kernel@xxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > > > linux-kernel@xxxxxxxxxxxxxxx > > > Subject: Re: [PATCH v3 3/3] dmaengine: imx-sdma: allocate max 20 > > > bds > > > for one transfer > > > > > > Am Montag, den 23.07.2018, 13:55 +0000 schrieb Robin Gong: > > > > > -----Original Message----- > > > > > From: Lucas Stach [mailto:l.stach@xxxxxxxxxxxxxx] > > > > > Sent: 2018年7月23日 18:54 > > > > > To: Robin Gong <yibin.gong@xxxxxxx>; vkoul@xxxxxxxxxx; > > > > > dan.j.williams@xxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; > > > > > linux@xxxxxxxxxxx g.uk > > > > > Cc: dmaengine@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxx > > > > > m>; > > > > > kernel@xxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > > > > > linux-kernel@xxxxxxxxxxxxxxx > > > > > Subject: Re: [PATCH v3 3/3] dmaengine: imx-sdma: allocate max > > > > > 20 > > > > > bds for one transfer > > > > > > > > > > Am Dienstag, den 24.07.2018, 01:46 +0800 schrieb Robin Gong: > > > > > > If multi-bds used in one transfer, all bds should be > > > > > > consisten > > > > > > memory.To easily follow it, enlarge the dma pool size into > > > > > > 20 > > > > > > bds, and it will report error if the number of bds is over > > > > > > than > > > > > > 20. For dmatest, the max count for single transfer is > > > > > > NUM_BD * > > > > > > > > > > SDMA_BD_MAX_CNT > > > > > > = 20 * 65535 = ~1.28MB. > > > > > > > > > > Both the commit message and the comment need a lot more care > > > > > to > > > > > actually tell what this commit is trying to achieve. > > > > > Currently I > > > > > don't follow at all. What does "consisten" mean? Do you mean > > > > > BDs > > > > > should be contiguous in memory? > > > > > > > > Yes, BDs should be contiguous one by one in memory. > > > > > > Okay, but this isn't what the code change does. By increasing the > > > size > > > parameter of the dma pool you just allocate 20 times as much > > > memory as > > > needed for each BD. So actually the BDs end up being very non- > > > contiguous in memory as there are now holes of 19 BD sizes > > > between the > > > > start of each BD. > > Please notice only allocate bds memory from dma pool one time even > > in multi > > bds. > > That's different with the common use case that allocate memory from > > dma > > pool everytime for every bd. Why do this is to make sure all bd > > memory is > > contiguous for single transfer whatever single bd or multi-bds, > > since two call > > dma_pool_alloc() can't promise the address is contiguous especially > > for multi > > thread case such as dmatest 'threads_per_chan = 5'. You can change > > to ' > > norandom=true ' and ' test_buf_size = 163840' in dmatest.c to look > > what issue > > coming without this patch. > > > > > > So something isn't right with this change. > > > > I think this patch is the easy way to resolve the bd contiguous > > issue, but the > > cost is to allocate more dma pool memory which may not used. > > > > > > Regards, > > > Lucas > > > > > > > > > > > > > What do you gain by over-allocating each BD by a factor of > > > > > 20? > > > > > > > > I guess dma_pool_alloc will return error in such case, and then > > > > cause dma setup transfer failure. > > > > > > > > > > Regards, > > > > > Lucas > > > > > > > > > > > Signed-off-by: Robin Gong <yibin.gong@xxxxxxx> > > > > > > --- > > > > > > drivers/dma/imx-sdma.c | 17 ++++++++++++++++- > > > > > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx- > > > > > > sdma.c > > > > > > index > > > > > > b4ec2d2..5973489 100644 > > > > > > --- a/drivers/dma/imx-sdma.c > > > > > > +++ b/drivers/dma/imx-sdma.c > > > > > > @@ -298,6 +298,15 @@ struct sdma_context_data { > > > > > > > u32 scratch7; > > > > > > > > > > > > } __attribute__ ((packed)); > > > > > > > > > > > > +/* > > > > > > + * All bds in one transfer should be consitent on SDMA. To > > > > > > easily > > > > > > +follow it,just > > > > > > + * set the dma pool size as the enough bds. For example, > > > > > > in > > > > > > dmatest > > > > > > +case, the > > > > > > + * max 20 bds means the max for single transfer is NUM_BD > > > > > > * > > > > > > +SDMA_BD_MAX_CNT = 20 > > > > > > + * * 65535 = ~1.28MB. 20 bds supposed to be enough > > > > > > basically.If > > > > > > it's > > > > > > +still not > > > > > > + * enough in some specific cases, enlarge it here.Warning > > > > > > message > > > > > > +would also > > > > > > + * appear if the bd numbers is over than 20. > > > > > > + */ > > > > > > +#define NUM_BD 20 > > > > > > > > > > > > struct sdma_engine; > > > > > > > > > > > > @@ -1273,7 +1282,7 @@ static int > > > > > > sdma_alloc_chan_resources(struct dma_chan *chan) > > > > > > > goto disable_clk_ahb; > > > > > > > sdmac->bd_pool = dma_pool_create("bd_pool", > > > > > > > chan- > > > > > > > > device->dev, > > > > > > > > > > > > > > - sizeof(struct > > > > > > > sdma_buffer_descriptor), > > > > > > > + NUM_BD * sizeof(struct > > > > > > > sdma_buffer_descriptor), > > > > > > > 32, 0); > > > > > > > return 0; > > > > > > > > > > > > @@ -1314,6 +1323,12 @@ static struct sdma_desc > > > > > > *sdma_transfer_init(struct sdma_channel *sdmac, > > > > > > { > > > > > > > struct sdma_desc *desc; > > > > > > > + if (bds > NUM_BD) { > > > > > > > + dev_err(sdmac->sdma->dev, "%d bds exceed > > > > > > > the > > > > > > > max %d\n", > > > > > > > + bds, NUM_BD); > > > > > > > + goto err_out; > > > > > > > + } > > > > > > > > > > > > + > > > > > > > desc = kzalloc((sizeof(*desc)), GFP_NOWAIT); > > > > > > > if (!desc) > > > > > > > goto err_out; -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html