dmaengine.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Serge Semin <fancer.lancer@gmail.com>,
	Vineet Gupta <vgupta@synopsys.com>, Vinod Koul <vkoul@kernel.org>,
	Viresh Kumar <vireshk@kernel.org>,
	Dan Williams <dan.j.williams@intel.com>,
	Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>,
	Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
	Paul Burton <paulburton@kernel.org>,
	Ralf Baechle <ralf@linux-mips.org>, Arnd Bergmann <arnd@arndb.de>,
	Rob Herring <robh+dt@kernel.org>, <linux-mips@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <dmaengine@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 3/6] dmaengine: dw: Set DMA device max segment size parameter
Date: Tue, 12 May 2020 00:16:22 +0300	[thread overview]
Message-ID: <20200511211622.yuh3ls2ay76yaxrf@mobilestation> (raw)
In-Reply-To: <20200508112152.GI185537@smile.fi.intel.com>

On Fri, May 08, 2020 at 02:21:52PM +0300, Andy Shevchenko wrote:
> +Cc (Vineet, for information you probably know)
> 
> On Fri, May 08, 2020 at 01:53:01PM +0300, Serge Semin wrote:
> > Maximum block size DW DMAC configuration corresponds to the max segment
> > size DMA parameter in the DMA core subsystem notation. Lets set it with a
> > value specific to the probed DW DMA controller. It shall help the DMA
> > clients to create size-optimized SG-list items for the controller. This in
> > turn will cause less dw_desc allocations, less LLP reinitializations,
> > better DMA device performance.
> 
> Thank you for the patch.
> My comments below.
> 
> ...
> 
> > +		/*
> > +		 * Find maximum block size to be set as the DMA device maximum
> > +		 * segment size. By doing so we'll have size optimized SG-list
> > +		 * items for the channels with biggest block size. This won't
> > +		 * be a problem for the rest of the channels, since they will
> > +		 * still be able to split the requests up by allocating
> > +		 * multiple DW DMA LLP descriptors, which they would have done
> > +		 * anyway.
> > +		 */
> > +		if (dwc->block_size > block_size)
> > +			block_size = dwc->block_size;
> >  	}
> >  
> >  	/* Clear all interrupts on all channels. */
> > @@ -1220,6 +1233,10 @@ int do_dma_probe(struct dw_dma_chip *chip)
> >  			     BIT(DMA_MEM_TO_MEM);
> >  	dw->dma.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
> >  
> > +	/* Block size corresponds to the maximum sg size */
> > +	dw->dma.dev->dma_parms = &dw->dma_parms;
> > +	dma_set_max_seg_size(dw->dma.dev, block_size);
> > +
> >  	err = dma_async_device_register(&dw->dma);
> >  	if (err)
> >  		goto err_dma_register;
> 
> Yeah, I have locally something like this and I didn't dare to upstream because
> there is an issue. We have this information per DMA controller, while we
> actually need this on per DMA channel basis.
> 
> Above will work only for synthesized DMA with all channels having same block
> size. That's why above conditional is not needed anyway.

Hm, I don't really see why the conditional isn't needed and this won't work. As
you can see in the loop above Initially I find a maximum of all channels maximum
block sizes and use it then as a max segment size parameter for the whole device.
If the DW DMA controller has the same max block size of all channels, then it
will be found. If the channels've been synthesized with different block sizes,
then the optimization will work for the one with greatest block size. The SG
list entries of the channels with lesser max block size will be split up
by the DW DMAC driver, which would have been done anyway without
max_segment_size being set. Here we at least provide the optimization for the
channels with greatest max block size.

I do understand that it would be good to have this parameter setup on per generic
DMA channel descriptor basis. But DMA core and device descriptor doesn't provide
such facility, so setting at least some justified value is a good idea.

> 
> OTOH, I never saw the DesignWare DMA to be synthesized differently (I remember
> that Intel Medfield has interesting settings, but I don't remember if DMA
> channels are different inside the same controller).
> 
> Vineet, do you have any information that Synopsys customers synthesized DMA
> controllers with different channel characteristics inside one DMA IP?

AFAICS the DW DMAC channels can be synthesized with different max block size.
The IP core supports such configuration. So we can't assume that such DMAC
release can't be found in a real hardware just because we've never seen one.
No matter what Vineet will have to say in response to your question.

> 
> ...
> 
> >  #include <linux/bitops.h>
> 
> > +#include <linux/device.h>
> 
> Isn't enough to supply
> 
> struct device;
> 
> ?

It's "struct device_dma_parameters" and I'd prefer to include the header file.

> 
> >  #include <linux/interrupt.h>
> >  #include <linux/dmaengine.h>
> 
> Also this change needs a separate patch I suppose.

Ah, just discovered there is no need in adding the dma_parms here because since
commit 7c8978c0837d ("driver core: platform: Initialize dma_parms for platform
devices") the dma_params pointer is already initialized. The same thing is done
for the PCI device too.

-Sergey

> 
> ...
> 
> > -	struct dma_device	dma;
> > -	char			name[20];
> > -	void __iomem		*regs;
> > -	struct dma_pool		*desc_pool;
> > -	struct tasklet_struct	tasklet;
> > +	struct dma_device		dma;
> > +	struct device_dma_parameters	dma_parms;
> > +	char				name[20];
> > +	void __iomem			*regs;
> > +	struct dma_pool			*desc_pool;
> > +	struct tasklet_struct		tasklet;
> >  
> >  	/* channels */
> > -	struct dw_dma_chan	*chan;
> > -	u8			all_chan_mask;
> > -	u8			in_use;
> > +	struct dw_dma_chan		*chan;
> > +	u8				all_chan_mask;
> > +	u8				in_use;
> 
> Please split formatting fixes into a separate patch.
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

  parent reply	other threads:[~2020-05-11 21:16 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-06 13:10 [PATCH 0/5] dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account Sergey.Semin
2020-03-06 13:29 ` Andy Shevchenko
2020-03-06 13:30   ` Andy Shevchenko
2020-03-06 13:43     ` Vinod Koul
     [not found]     ` <20200306135050.40094803087C@mail.baikalelectronics.ru>
2020-03-09 21:45       ` Sergey Semin
     [not found]   ` <20200306133756.0F74C8030793@mail.baikalelectronics.ru>
2020-03-06 13:47     ` Sergey Semin
2020-03-06 14:11       ` Andy Shevchenko
     [not found]       ` <20200306141135.9C4F380307C2@mail.baikalelectronics.ru>
2020-03-09 22:08         ` Sergey Semin
2020-05-08 10:52 ` [PATCH v2 0/6] " Serge Semin
2020-05-08 10:52   ` [PATCH v2 1/6] dt-bindings: dma: dw: Convert DW DMAC to DT binding Serge Semin
2020-05-18 17:50     ` Rob Herring
2020-05-08 10:53   ` [PATCH v2 2/6] dt-bindings: dma: dw: Add max burst transaction length property Serge Semin
2020-05-08 11:12     ` Andy Shevchenko
2020-05-11 20:05       ` Serge Semin
2020-05-11 21:01         ` Andy Shevchenko
2020-05-11 21:35           ` Serge Semin
2020-05-12  9:08             ` Andy Shevchenko
2020-05-12 11:49               ` Serge Semin
2020-05-12 12:38                 ` Andy Shevchenko
2020-05-15  6:09                   ` Vinod Koul
2020-05-15 10:51                     ` Andy Shevchenko
2020-05-15 10:56                       ` Vinod Koul
2020-05-15 11:11                         ` Serge Semin
2020-05-17 17:47                           ` Serge Semin
2020-05-18 17:30                             ` Rob Herring
2020-05-18 19:30                               ` Serge Semin
2020-05-19 17:13                             ` Vinod Koul
2020-05-21  1:33                               ` Serge Semin
2020-05-08 10:53   ` [PATCH v2 3/6] dmaengine: dw: Set DMA device max segment size parameter Serge Semin
2020-05-08 11:21     ` Andy Shevchenko
2020-05-08 18:49       ` Vineet Gupta
2020-05-11 21:16       ` Serge Semin [this message]
2020-05-12 12:35         ` Andy Shevchenko
2020-05-12 17:01           ` Serge Semin
2020-05-15  6:16           ` Vinod Koul
2020-05-15 10:53             ` Andy Shevchenko
2020-05-17 18:22               ` Serge Semin
2020-05-08 10:53   ` [PATCH v2 4/6] dmaengine: dw: Print warning if multi-block is unsupported Serge Semin
2020-05-08 11:26     ` Andy Shevchenko
2020-05-08 11:53       ` Mark Brown
2020-05-08 19:06         ` Andy Shevchenko
2020-05-11  3:13           ` Serge Semin
2020-05-11 14:03             ` Andy Shevchenko
2020-05-11  2:10         ` Serge Semin
2020-05-11 11:58           ` Mark Brown
2020-05-11 13:45             ` Serge Semin
2020-05-11 13:58               ` Andy Shevchenko
2020-05-11 17:48                 ` Mark Brown
2020-05-11 18:25                   ` Serge Semin
2020-05-11 19:32                 ` Serge Semin
2020-05-11 21:07                   ` Andy Shevchenko
2020-05-11 21:08                     ` Andy Shevchenko
2020-05-12 12:42                       ` Serge Semin
2020-05-15  6:30                         ` Vinod Koul
2020-05-17 19:23                           ` Serge Semin
2020-05-19 17:02                             ` Vinod Koul
2020-05-21  1:40                               ` Serge Semin
2020-05-11 17:44               ` Mark Brown
2020-05-11 18:32                 ` Serge Semin
2020-05-11 21:32                   ` Mark Brown
2020-05-08 10:53   ` [PATCH v2 5/6] dmaengine: dw: Introduce max burst length hw config Serge Semin
2020-05-08 11:41     ` Andy Shevchenko
2020-05-12 14:08       ` Serge Semin
2020-05-12 19:12         ` Andy Shevchenko
2020-05-12 19:47           ` Serge Semin
2020-05-15 11:02             ` Andy Shevchenko
2020-05-15  6:39           ` Vinod Koul
2020-05-17 19:38             ` Serge Semin
2020-05-19 17:07               ` Vinod Koul
2020-05-21  1:47                 ` Serge Semin
2020-05-08 10:53   ` [PATCH v2 6/6] dmaengine: dw: Take HC_LLP flag into account for noLLP auto-config Serge Semin
2020-05-08 11:43     ` Andy Shevchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200511211622.yuh3ls2ay76yaxrf@mobilestation \
    --to=sergey.semin@baikalelectronics.ru \
    --cc=Alexey.Malahov@baikalelectronics.ru \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=arnd@arndb.de \
    --cc=dan.j.williams@intel.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dmaengine@vger.kernel.org \
    --cc=fancer.lancer@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=paulburton@kernel.org \
    --cc=ralf@linux-mips.org \
    --cc=robh+dt@kernel.org \
    --cc=tsbogend@alpha.franken.de \
    --cc=vgupta@synopsys.com \
    --cc=vireshk@kernel.org \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).