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

[Guennadi Liakhovetski <g.liakhovetski@gmx.de>]

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