Patchwork [07/13] dmaengine: introduce dma_request_channel and private channels

login
register
mail settings
Submitter Guennadi Liakhovetski
Date Dec. 2, 2008, 3:52 p.m.
Message ID <Pine.LNX.4.64.0812021642120.5663@axis700.grange>
Download mbox | patch
Permalink /patch/11797/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Guennadi Liakhovetski - Dec. 2, 2008, 3:52 p.m.
Hi Dan,

I think, there is a problem with your dma_request_channel() / 
private_candidate() implementation: your current version only tries one 
channel from a dma device list, which matched capabilities. If this 
channel is not accepted by the client, you do not try other channels from 
this device and just go to the next one...

Another problem I encountered with my framebuffer is the initialisation 
order. You initialise dmaengine per subsys_initcall(), whereas the only 
way to guarantee the order:

dmaengine
dma-device driver
framebuffer

when they are all linked into the kernel was to switch dmaengine to 
arch_initcall, put the dma driver under subsys_initcall, and the 
framebuffer under a normal module_init / device_initcall.

Below is a naive patch, adding two more arguments to private_candidate() 
is not very elegant, so, this is not an official submission. Feel free to 
propose a better fix. But if you like, I can make two patches out of this 
- separating the initialisation priority.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

dmaengine: scan all channels if one is rejected, initialise earlier.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - Dec. 2, 2008, 5:16 p.m.
On Tue, Dec 2, 2008 at 8:52 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> Hi Dan,
>
> I think, there is a problem with your dma_request_channel() /
> private_candidate() implementation: your current version only tries one
> channel from a dma device list, which matched capabilities. If this
> channel is not accepted by the client, you do not try other channels from
> this device and just go to the next one...
>

Which dma driver are you using?  The dmaengine code assumes that all
channels on a device are equal.  It sounds like there are differences
between peer-channels on the device in this case.  If the driver
registers a device per channel that should give the flexibility you
want.

> Another problem I encountered with my framebuffer is the initialisation
> order. You initialise dmaengine per subsys_initcall(), whereas the only
> way to guarantee the order:
>
> dmaengine
> dma-device driver
> framebuffer
>

hmm... can the framebuffer be moved to late_initcall?

Regards,
Dan
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Ferre - Dec. 2, 2008, 5:26 p.m.
Guennadi Liakhovetski :
> Another problem I encountered with my framebuffer is the initialisation 
> order. You initialise dmaengine per subsys_initcall(), whereas the only 
> way to guarantee the order:
> 
> dmaengine
> dma-device driver
> framebuffer
> 
> when they are all linked into the kernel was to switch dmaengine to 
> arch_initcall, put the dma driver under subsys_initcall, and the 
> framebuffer under a normal module_init / device_initcall.

I did not dig much in this but I feel also that dmaengine is initialized 
a bit late : it seems to be initialized after atmel-mci is started.
It works but it seems to me that dmaengine has to be initialized very 
early...

...my 2 cents.

Regards,
Guennadi Liakhovetski - Dec. 2, 2008, 5:27 p.m.
On Tue, 2 Dec 2008, Dan Williams wrote:

> On Tue, Dec 2, 2008 at 8:52 AM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > Hi Dan,
> >
> > I think, there is a problem with your dma_request_channel() /
> > private_candidate() implementation: your current version only tries one
> > channel from a dma device list, which matched capabilities. If this
> > channel is not accepted by the client, you do not try other channels from
> > this device and just go to the next one...
> >
> 
> Which dma driver are you using?

This is the idmac dmaengine driver I submitted a few weeks ago, that I am 
porting to your modified dmaengine framework. Initial version:

http://marc.info/?l=linux-arm-kernel&m=122607472721145&w=2

BTW - it does look nicer and more simple now, so, in general, I like the 
change.

> The dmaengine code assumes that all
> channels on a device are equal.  It sounds like there are differences
> between peer-channels on the device in this case.  If the driver
> registers a device per channel that should give the flexibility you
> want.

Ooh... Do you really think registering 32 dma-devices is a better solution 
than allowing non-equal dma-channels? As I explained in the commit 
comment, this is a specialised Image Processing DMA Controller, and each 
its channel has a fixed role. So, each client has to get a specific 
channel.

> > Another problem I encountered with my framebuffer is the initialisation
> > order. You initialise dmaengine per subsys_initcall(), whereas the only
> > way to guarantee the order:
> >
> > dmaengine
> > dma-device driver
> > framebuffer
> 
> hmm... can the framebuffer be moved to late_initcall?

I assumed, that one wants to register the framebuffer as early as 
possible...

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Atsushi Nemoto - Jan. 30, 2009, 5:03 p.m.
On Tue, 2 Dec 2008 10:16:05 -0700, "Dan Williams" <dan.j.williams@intel.com> wrote:
> > I think, there is a problem with your dma_request_channel() /
> > private_candidate() implementation: your current version only tries one
> > channel from a dma device list, which matched capabilities. If this
> > channel is not accepted by the client, you do not try other channels from
> > this device and just go to the next one...
> 
> Which dma driver are you using?  The dmaengine code assumes that all
> channels on a device are equal.  It sounds like there are differences
> between peer-channels on the device in this case.  If the driver
> registers a device per channel that should give the flexibility you
> want.

I'm writing a new dma driver.  My DMAC has multiple channels and only
one channel is capable for generic memcpy and other channels have
fixed role.

Does new framework allow dma driver make only one channel public?
Or should I register two dma_device for DMA_MEMCPY and DMA_SLAVE?
Could you give me some advice?

---
Atsushi Nemoto
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - Jan. 30, 2009, 11:13 p.m.
On Fri, Jan 30, 2009 at 10:03 AM, Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> I'm writing a new dma driver.  My DMAC has multiple channels and only
> one channel is capable for generic memcpy and other channels have
> fixed role.
>
> Does new framework allow dma driver make only one channel public?

Yes, if the driver registers a dma_device for each channel.

> Or should I register two dma_device for DMA_MEMCPY and DMA_SLAVE?
> Could you give me some advice?

Register multiple dma_devices, the public one with a DMA_MEMCPY, and
the fixed role devices with DMA_PRIVATE, DMA_MEMCPY, and DMA_SLAVE
capabilities.

DMA_PRIVATE ensures that a channel is never considered for public consumption.

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski - Jan. 30, 2009, 11:27 p.m.
On Fri, 30 Jan 2009, Dan Williams wrote:

> On Fri, Jan 30, 2009 at 10:03 AM, Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> > I'm writing a new dma driver.  My DMAC has multiple channels and only
> > one channel is capable for generic memcpy and other channels have
> > fixed role.
> >
> > Does new framework allow dma driver make only one channel public?
> 
> Yes, if the driver registers a dma_device for each channel.
> 
> > Or should I register two dma_device for DMA_MEMCPY and DMA_SLAVE?
> > Could you give me some advice?
> 
> Register multiple dma_devices, the public one with a DMA_MEMCPY, and
> the fixed role devices with DMA_PRIVATE, DMA_MEMCPY, and DMA_SLAVE
> capabilities.
> 
> DMA_PRIVATE ensures that a channel is never considered for public consumption.

Maybe just two dma-devices would suffice: one with the public memcpy 
channel, and one with the rest private slave channels?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Atsushi Nemoto - Jan. 31, 2009, 12:18 p.m.
On Sat, 31 Jan 2009 00:27:48 +0100 (CET), Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Register multiple dma_devices, the public one with a DMA_MEMCPY, and
> > the fixed role devices with DMA_PRIVATE, DMA_MEMCPY, and DMA_SLAVE
> > capabilities.
> > 
> > DMA_PRIVATE ensures that a channel is never considered for public consumption.
> 
> Maybe just two dma-devices would suffice: one with the public memcpy 
> channel, and one with the rest private slave channels?

Thank you, I will try this.

---
Atsushi Nemoto
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 94dcc64..677b0c8 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -404,10 +404,12 @@  static void dma_channel_rebalance(void)
 		}
 }
 
-static struct dma_chan *private_candidate(dma_cap_mask_t *mask, struct dma_device *dev)
+static struct dma_chan *private_candidate(dma_cap_mask_t *mask,
+		struct dma_device *dev, dma_filter_fn fn, void *fn_param)
 {
 	struct dma_chan *chan;
 	struct dma_chan *ret = NULL;
+	bool ack;
 
 	/* devices with multiple channels need special handling as we need to
 	 * ensure that all channels are either private or public.
@@ -430,8 +432,18 @@  static struct dma_chan *private_candidate(dma_cap_mask_t *mask, struct dma_devic
 				 __func__, dev_name(&chan->dev));
 			continue;
 		}
-		ret = chan;
-		break;
+
+		if (fn)
+			ack = fn(chan, fn_param);
+		else
+			ack = true;
+
+		if (ack) {
+			ret = chan;
+			break;
+		} else
+			pr_debug("%s: %s filter said false\n",
+				 __func__, dev_name(&chan->dev));
 	}
 
 	return ret;
@@ -447,42 +459,33 @@  struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, v
 {
 	struct dma_device *device, *_d;
 	struct dma_chan *chan = NULL;
-	bool ack;
 	int err;
 
 	/* Find a channel */
 	mutex_lock(&dma_list_mutex);
 	list_for_each_entry_safe(device, _d, &dma_device_list, global_node) {
-		chan = private_candidate(mask, device);
+		chan = private_candidate(mask, device, fn, fn_param);
 		if (!chan)
 			continue;
 
-		if (fn)
-			ack = fn(chan, fn_param);
+		/* Found a suitable channel, try to grab, prep, and
+		 * return it.  We first set DMA_PRIVATE to disable
+		 * balance_ref_count as this channel will not be
+		 * published in the general-purpose allocator
+		 */
+		dma_cap_set(DMA_PRIVATE, device->cap_mask);
+		err = dma_chan_get(chan);
+
+		if (err == -ENODEV) {
+			pr_debug("%s: %s module removed\n", __func__,
+				 dev_name(&chan->dev));
+			list_del_rcu(&device->global_node);
+		} else if (err)
+			pr_err("dmaengine: failed to get %s: (%d)",
+			       dev_name(&chan->dev), err);
 		else
-			ack = true;
-
-		if (ack) {
-			/* Found a suitable channel, try to grab, prep, and
-			 * return it.  We first set DMA_PRIVATE to disable
-			 * balance_ref_count as this channel will not be
-			 * published in the general-purpose allocator
-			 */
-			dma_cap_set(DMA_PRIVATE, device->cap_mask);
-			err = dma_chan_get(chan);
+			break;
 
-			if (err == -ENODEV) {
-				pr_debug("%s: %s module removed\n", __func__,
-					 dev_name(&chan->dev));
-				list_del_rcu(&device->global_node);
-			} else if (err)
-				pr_err("dmaengine: failed to get %s: (%d)",
-				       dev_name(&chan->dev), err);
-			else
-				break;
-		} else
-			pr_debug("%s: %s filter said false\n",
-				 __func__, dev_name(&chan->dev));
 		chan = NULL;
 	}
 	mutex_unlock(&dma_list_mutex);
@@ -912,6 +915,4 @@  static int __init dma_bus_init(void)
 	mutex_init(&dma_list_mutex);
 	return class_register(&dma_devclass);
 }
-subsys_initcall(dma_bus_init);
-
-
+arch_initcall(dma_bus_init);