| Message ID | Pine.LNX.4.64.0812021642120.5663@axis700.grange |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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,
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
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
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
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
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
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);
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