lkml.org 
[lkml]   [2012]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()
On Wed, 7 Mar 2012, Russell King - ARM Linux wrote:

> On Wed, Mar 07, 2012 at 01:30:23PM +0100, Guennadi Liakhovetski wrote:
> > 1. The current scheme is:
> >
> > (a) client issues
> > dma_request_channel()
> > with an optional filter function as parameter
> > (b) the core picks up a suitable from its PoV DMA controller device and a
> > channel on it and calls the filter function with that channel as an
> > argument
> > (c) the filter function can verify, whether that channel is suitable or
> > not (*)
> > (d) the client driver then can call
> > dmaengine_slave_config()
> > to provide any additional channel configuration information to the DMA
> > controller driver (**)
> > (e) if the filter has rejected this channel, the core jumps to the next
> > DMA controller instance (***)
>
> No - if the filter function rejects the first free channel, the next free
> channel on the same controller will be tried. When all channels have
> been tried, the next DMA controller is checked.

Right, sorry, I confused it with an error, returned by DMA driver's
.device_alloc_chan_resources()

> > 2. (goal: eliminate filter function look-ups) proposed by Linus W
> >
> > (a) client issues
> > dma_request_slave_channel(dev, "MMC-RX")
> > (b) the dmaengine core scans a platform-provided list of channel mappings
> > and picks up _the_ correct channel (****)
>
> That doesn't work if you have multiple DMA controllers supporting the
> same client.

Right, that's why I was against it, but it would work with virtual
channels (and virtual devices)?

> > 3. Jassi's idea with capabilities has been rejected by Russell
> >
> > 4. (goal: simplify the allocation and configuration procedure) proposed by
> > myself
> >
> > (a) as in (1) client issues
> > dma_request_channel()
> > with an additional slave configuration parameter
> > (b) the core picks up a suitable from its PoV DMA controller device and a
> > channel on it, (optionally) calls the filter
>
> How can it work out what's a suitable DMA controller device?

It doesn't, it will have to probe all DMA devices, until
.device_alloc_chan_resources() succeeds in (c) below.

> Even knowing
> where the DMA register is, the burst size and width doesn't really narrow
> down the selection of the DMA controller.
>
> > (c) the core calls DMA controller driver's
> > .device_alloc_chan_resources()
> > method, which verifies, whether the channel can be configured for the
> > requesting slave, if not, an error is returned and the next DMA
> > controller instance is checked by the core
>
> And this effectively prevents a channel being reconfigured to target a
> different burst size or different transfer width without freeing and
> re-requesting it.

Cannot dmaengine_slave_config() be used for that?

> > Naturally, my preference goes for (4) because (a) I think, it is the DMA
> > controller driver, that has to decide, whether the channel is suitable for
> > a specific slave,
>
> We already effectively do that with many of the DMA engine drivers. The
> DMA engine drivers export their filter function which should be used when
> requesting a channel (if you care about the channel you end up with.)

This is one of the things I'd like to avoid - having to extend the
standard API with hardware-specific methods... It's already bad enough,
that client drivers often have to use DMA-controller specific types to
configure transfers. Ideally I'd prefer to have 0 DMA device knowledge in
client drivers. If needed, they should just pass DMA device data from
platform code to the DMA controller driver as opaque handles.

> > (b) changes to the core are minimal, simple and
> > trivially backwards-compatible, (c) the core is not cluttered with
> > hw-specific channel mappings, (d) the additional call to
> > dmaengine_slave_config() can be eliminated.
>
> The call to dmaengine_slave_config() actually simplifies the DMA engine
> support for some drivers though, so eliminating it doesn't help.

Right, sorry, I didn't mean, that I'd like to get rid of it completely. I
just meant, that being forced to use it for every slave channel allocation
isn't very nice.

> What
> would be useful is to have a helper function along these lines:
>
> struct dma_chan *dma_request_channel_config(mask, fn, data, config)
> {
> struct dma_chan *c = dma_request_channel(mask, fn, data);
>
> if (c) {
> if (dmaengine_slave_config(c, config)) {
> dma_release_channel(c);
> c = NULL;
> }
> }
> return c;
> }

Hm, yeah... That seems like an over-complication to me: to "just" allocae
a channel you cann dma_request_channel(), which scans your devices and
channels on them, calls your filter, calls the DMA controller driver's
allocation method, only to eventually call dmaengine_slave_config() and
see it fail, after which you release the channel and start anew...

Ah, there's the problem actually: you cannot try to find another channel,
if dmaengine_slave_config() fails - the scan will restart from the
beginning and you end up with the same failure again. So, we cannot rely
on dmaengine_slave_config() to be the first instance, where the DMA
controller driver actually gets the channel configuration and has a chance
to verify its suitability.

> which would simplify some of the DMA engine users. There'll still be
> some though which would want to call dmaengine_slave_config() to change
> the channels configuration when the mode of the device switches.
>
> However, I don't see anything in struct dma_slave_config which could be
> used to select an appropriate channel.

That's also my problem with it, and the reason, why I suggested, that it
has to be embedded in a hardware-specific channel configuration type.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

\
 
 \ /
  Last update: 2012-03-07 14:53    [from the cache]
©2003-2011 Jasper Spaans