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

[Dan Williams <dan.j.williams@intel.com>]

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):

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.

--
Dan