Date:   Mon, 06 Aug 2018 14:29:50 +0200
From:   Lucas Stach <l.stach@...gutronix.de>
To:     Robin Gong <yibin.gong@....com>,
        "vkoul@...nel.org" <vkoul@...nel.org>,
        "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>
Cc:     "dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v3 3/3] dmaengine: imx-sdma: allocate max 20 bds for one
 transfer

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@...gutronix.de>; vkoul@...nel.org;
> > dan.j.williams@...el.com; s.hauer@...gutronix.de; linux@...linux.or
> > g.uk
> > Cc: dmaengine@...r.kernel.org; dl-linux-imx <linux-imx@....com>;
> > kernel@...gutronix.de; linux-arm-kernel@...ts.infradead.org;
> > linux-kernel@...r.kernel.org
> > Subject: RE: [PATCH v3 3/3] dmaengine: imx-sdma: allocate max 20
> > bds for one
> > transfer
> > 
> > > -----Original Message-----
> > > From: Lucas Stach [mailto:l.stach@...gutronix.de]
> > > Sent: 2018年7月24日 17:22
> > > To: Robin Gong <yibin.gong@....com>; vkoul@...nel.org;
> > > dan.j.williams@...el.com; s.hauer@...gutronix.de;
> > > linux@...linux.org.uk
> > > Cc: dmaengine@...r.kernel.org; dl-linux-imx <linux-imx@....com>;
> > > kernel@...gutronix.de; linux-arm-kernel@...ts.infradead.org;
> > > linux-kernel@...r.kernel.org
> > > 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@...gutronix.de]
> > > > > Sent: 2018年7月23日 18:54
> > > > > To: Robin Gong <yibin.gong@....com>; vkoul@...nel.org;
> > > > > dan.j.williams@...el.com; s.hauer@...gutronix.de;
> > > > > linux@...linux.or g.uk
> > > > > Cc: dmaengine@...r.kernel.org; dl-linux-imx <linux-imx@....co
> > > > > m>;
> > > > > kernel@...gutronix.de; linux-arm-kernel@...ts.infradead.org;
> > > > > linux-kernel@...r.kernel.org
> > > > > 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@....com>
> > > > > > ---
> > > > > >  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;

Powered by blists - more mailing lists