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

login
register
mail settings
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

Dan Williams - Dec. 2, 2008, 7:10 p.m.
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):



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

--
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 - Dec. 2, 2008, 9:28 p.m.
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);