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    [W:0.036 / U:30.340 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site