| Submitter | Dan Williams |
|---|---|
| Date | Dec. 2, 2008, 7:10 p.m. |
| Message ID | <1228245007.8408.20.camel@dwillia2-linux.ch.intel.com> |
| Download | mbox | patch |
| Permalink | /patch/11805/ |
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Comments
On Tue, 2 Dec 2008, Dan Williams wrote: > On Tue, 2008-12-02 at 10:27 -0700, Guennadi Liakhovetski wrote: > > 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. > > I see your point. Rather than doing driver gymnastics we can simply > have dmaengine do the following (basically your patch reformatted a > bit): Good, so, would you commit it? > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c > index e2ccfd0..66d0ae7 100644 > --- a/drivers/dma/dmaengine.c > +++ b/drivers/dma/dmaengine.c > @@ -445,10 +445,10 @@ 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; > > /* devices with multiple channels need special handling as we need to > * ensure that all channels are either private or public. > @@ -471,11 +471,15 @@ static struct dma_chan *private_candidate(dma_cap_mask_t *mask, struct dma_devic > __func__, dma_chan_name(chan)); > continue; > } > - ret = chan; > - break; > + if (fn && !fn(chan, fn_param)) { > + pr_debug("%s: %s filter said false\n", > + __func__, dma_chan_name(chan)); > + continue; > + } > + return chan; > } > > - return ret; > + return NULL; > } > > /** > @@ -488,22 +492,13 @@ 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); > - if (!chan) > - continue; > - > - if (fn) > - ack = fn(chan, fn_param); > - else > - ack = true; > - > - if (ack) { > + chan = private_candidate(mask, device, fn, fn_param); > + if (chan) { > /* 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 > @@ -521,10 +516,8 @@ struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, v > dma_chan_name(chan), err); > else > break; > - } else > - pr_debug("%s: %s filter said false\n", > - __func__, dma_chan_name(chan)); > - chan = NULL; > + chan = NULL; > + } > } > mutex_unlock(&dma_list_mutex); > > > > > > > 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... > > Yes, but I'm hesitant to escalate the initcall level. It sounds like > the channel(s?) for the framebuffer are an integral part of the > framebuffer device so maybe they should not be registered separately? > But that runs into issues if you want the channels to return to the > general pool when the framebuffer driver is unloaded. You mean whether it makes sense at all to manage these framebuffer channels outside of the framebuffer driver in a dma driver? I think yes. Simply because these 2 channels used by the fb share code and _registers_ with the rest 30 channels, which are all also quite specialised. As for excalating the initcall level - the dmaengine init function doesn't do much - it just registers a device class and initialises a mutex - shouldn't be a problem to do it earlier? 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
Patch
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c index e2ccfd0..66d0ae7 100644 --- a/drivers/dma/dmaengine.c +++ b/drivers/dma/dmaengine.c @@ -445,10 +445,10 @@ 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; /* devices with multiple channels need special handling as we need to * ensure that all channels are either private or public. @@ -471,11 +471,15 @@ static struct dma_chan *private_candidate(dma_cap_mask_t *mask, struct dma_devic __func__, dma_chan_name(chan)); continue; } - ret = chan; - break; + if (fn && !fn(chan, fn_param)) { + pr_debug("%s: %s filter said false\n", + __func__, dma_chan_name(chan)); + continue; + } + return chan; } - return ret; + return NULL; } /** @@ -488,22 +492,13 @@ 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); - if (!chan) - continue; - - if (fn) - ack = fn(chan, fn_param); - else - ack = true; - - if (ack) { + chan = private_candidate(mask, device, fn, fn_param); + if (chan) { /* 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 @@ -521,10 +516,8 @@ struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, v dma_chan_name(chan), err); else break; - } else - pr_debug("%s: %s filter said false\n", - __func__, dma_chan_name(chan)); - chan = NULL; + chan = NULL; + } } mutex_unlock(&dma_list_mutex);