Message-ID: <CAKnu2MpVwYSYgKBtpuqrrjHFQdjCEU_Q5iYe8qdfO9D1kzN0gw@mail.gmail.com>
Date:	Mon, 25 Jul 2011 12:55:07 +0200
From:	Linus Walleij <linus.ml.walleij@...il.com>
To:	Jassi Brar <jaswinder.singh@...aro.org>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	dan.j.williams@...el.com, sundaram@...com, vinod.koul@...el.com,
	rmk+kernel@....linux.org.uk
Subject: Re: [RFC] DMAEngine: Define generic transfer request api

2011/7/23 Jassi Brar <jaswinder.singh@...aro.org>:

> This is an attempt to define an api that could be used for doing
> fancy data transfers like interleaved to contiguous copy and vice-versa.
> Traditional SG_list based transfers tend to be very inefficient
> in such cases. Such cases call for some very condensed api to convey
> pattern of the transfer. This is an attempt at that condensed api.
>
> The api supports all 4 variants of scatter-gather and contiguous transfer.
> Besides, it could easily represent common operations like
>        device_prep_dma_{cyclic, memset, memcpy}
> and maybe some more that I am not sure of.
>
> Of course, this api too can't help transfers that don't lend to DMA by
> nature, i.e, scattered tiny read/writes with no periodic pattern.
>
> Signed-off-by: Jassi Brar <jaswinder.singh@...aro.org>

I think this also looks interesting, it's the same idea as for using
control commands to set up striding, but by supporting a totally
new function call instead.

Do you think this method can be filled in with a default
implementation that converts the request to a plain sglist request
to memory or slave if interleaved transfers are not available?
That would mean that drivers can use this API on any DMA
engine, no matter whether it supports ICG/striding or not.

If we do that, this is much better than using a special control
command.

+ * @src_inc: If the source address increments after reading from it.
+ * @dst_inc: If the destination address increments after writing to it.

Currently whether source/destination increments or not is
implicit from transaction type, i.e. if it's a mem2mem transfer
it is implicit that both increment, if it's a slave transfer we can
infer from the direction (to/from peripheral) whether source or
destination is supposed to increment.

IMO this means duplication, and removes abstraction from the
API, it looks more like specific bits to be set in the DMA
hardware to control increments.

In this form it makes possible to set up nonsensical operations like
a memory-to-memory transfer to a non-incrementing address
which is not a peripheral FIFO, which is not good IMO. The API
should be semantically designed for setting up sensible operations.

+ * @op: The operation to perform on source data before writing it on
+ *      to destination address.

It is a enum dma_transaction_type after all, so can't it just say
"type of transfer"?

I think you can infer the above increment settings from this type,
so they should be removed.

+ * @frm_irq: If the client expects DMAC driver to do callback after each frame.

I don't understand the usecase for getting this IRQ, and it deviates
from the DMAengine way of setting a flag on the descriptor.
Instead I think you should maybe provide a new flag for the
unsigned long flags you're already passing in here:

+       struct dma_async_tx_descriptor *(*device_prep_dma_xfer)(
+               struct dma_chan *chan, struct xfer_template *xt,
+               unsigned long flags);
                 ^^^^^^^^^^^^^^^^^^^^^^

Thanks,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists