[RFC] dmaengine/dma_slave: add context parameter to prep_slave_sg callback

[RFC] dmaengine/dma_slave: add context parameter to prep_slave_sg callback

[Alexandre Bounine]

As we agreed during our discussion about adding DMA Engine support for RapidIO
subsystem, RapidIO and similar clients may benefit from adding an extra context
parameter to device_prep_slave_sg() callback.
See https://lkml.org/lkml/2011/10/24/275 for more details.

Adding the context parameter will allow to pass client/target specific
information associated with an individual data transfer request.

In the case of RapidIO support this additional information consists of target
destination ID and its buffer address (which is not mapped into the local CPU
memory space). Because a single RapidIO-capable DMA channel may queue data
transfer requests to different target devices, the per-request configuration
is required.

The proposed change eliminates need for new subsystem-specific API.
Existing DMA_SLAVE clients will ignore the new parameter.

This RFC only demonstrates the API change and does not include corresponding
changes to existing DMA_SLAVE clients. Complete set of patches will be provided
after (if) this API change is accepted.

Signed-off-by: Alexandre Bounine <alexandre.bounine@idt.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
Cc: Russell King <rmk@arm.linux.org.uk> 
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Matt Porter <mporter@kernel.crashing.org>
Cc: Li Yang <leoli@freescale.com>
---
 include/linux/dmaengine.h |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 679b349..79d71bb 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -575,7 +575,7 @@ struct dma_device {
 	struct dma_async_tx_descriptor *(*device_prep_slave_sg)(
 		struct dma_chan *chan, struct scatterlist *sgl,
 		unsigned int sg_len, enum dma_transfer_direction direction,
-		unsigned long flags);
+		unsigned long flags, void *context);
 	struct dma_async_tx_descriptor *(*device_prep_dma_cyclic)(
 		struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
 		size_t period_len, enum dma_transfer_direction direction);
@@ -607,12 +607,13 @@ static inline int dmaengine_slave_config(struct dma_chan *chan,
 
 static inline struct dma_async_tx_descriptor *dmaengine_prep_slave_single(
 	struct dma_chan *chan, void *buf, size_t len,
-	enum dma_transfer_direction dir, unsigned long flags)
+	enum dma_transfer_direction dir, unsigned long flags, void *context)
 {
 	struct scatterlist sg;
 	sg_init_one(&sg, buf, len);
 
-	return chan->device->device_prep_slave_sg(chan, &sg, 1, dir, flags);
+	return chan->device->device_prep_slave_sg(chan, &sg, 1, dir, flags,
+						  context);
 }
 
 static inline int dmaengine_terminate_all(struct dma_chan *chan)
-- 
1.7.8.4

Re: [RFC] dmaengine/dma_slave: add context parameter to prep_slave_sg callback

[Vinod Koul]

On Thu, 2012-01-26 at 16:22 -0500, Alexandre Bounine wrote:
> As we agreed during our discussion about adding DMA Engine support for RapidIO
> subsystem, RapidIO and similar clients may benefit from adding an extra context
> parameter to device_prep_slave_sg() callback.
> See https://lkml.org/lkml/2011/10/24/275 for more details.
> 
> Adding the context parameter will allow to pass client/target specific
> information associated with an individual data transfer request.
> 
> In the case of RapidIO support this additional information consists of target
> destination ID and its buffer address (which is not mapped into the local CPU
> memory space). Because a single RapidIO-capable DMA channel may queue data
> transfer requests to different target devices, the per-request configuration
> is required.
> 
> The proposed change eliminates need for new subsystem-specific API.
> Existing DMA_SLAVE clients will ignore the new parameter.
> 
> This RFC only demonstrates the API change and does not include corresponding
> changes to existing DMA_SLAVE clients. Complete set of patches will be provided
> after (if) this API change is accepted.
This looks good to me. But was thinking if we need to add this new
parameter for other slave calls (circular, interleaved, memcpy...)

> 
> Signed-off-by: Alexandre Bounine <alexandre.bounine@idt.com>
> Cc: Jassi Brar <jaswinder.singh@linaro.org>
> Cc: Russell King <rmk@arm.linux.org.uk> 
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: Matt Porter <mporter@kernel.crashing.org>
> Cc: Li Yang <leoli@freescale.com>
> ---
>  include/linux/dmaengine.h |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 679b349..79d71bb 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -575,7 +575,7 @@ struct dma_device {
>  	struct dma_async_tx_descriptor *(*device_prep_slave_sg)(
>  		struct dma_chan *chan, struct scatterlist *sgl,
>  		unsigned int sg_len, enum dma_transfer_direction direction,
> -		unsigned long flags);
> +		unsigned long flags, void *context);
>  	struct dma_async_tx_descriptor *(*device_prep_dma_cyclic)(
>  		struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
>  		size_t period_len, enum dma_transfer_direction direction);
> @@ -607,12 +607,13 @@ static inline int dmaengine_slave_config(struct dma_chan *chan,
>  
>  static inline struct dma_async_tx_descriptor *dmaengine_prep_slave_single(
>  	struct dma_chan *chan, void *buf, size_t len,
> -	enum dma_transfer_direction dir, unsigned long flags)
> +	enum dma_transfer_direction dir, unsigned long flags, void *context)
>  {
>  	struct scatterlist sg;
>  	sg_init_one(&sg, buf, len);
>  
> -	return chan->device->device_prep_slave_sg(chan, &sg, 1, dir, flags);
> +	return chan->device->device_prep_slave_sg(chan, &sg, 1, dir, flags,
> +						  context);
>  }
>  
>  static inline int dmaengine_terminate_all(struct dma_chan *chan)


-- 
~Vinod

RE: [RFC] dmaengine/dma_slave: add context parameter to prep_slave_sg callback

[Bounine, Alexandre]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2235 bytes --]

On Monday, January 30, 2012 at 4:31 AM, Vinod Koul wrote:
> 
> On Thu, 2012-01-26 at 16:22 -0500, Alexandre Bounine wrote:
> > As we agreed during our discussion about adding DMA Engine support for RapidIO
> > subsystem, RapidIO and similar clients may benefit from adding an extra context
> > parameter to device_prep_slave_sg() callback.
> > See https://lkml.org/lkml/2011/10/24/275 for more details.
> >
> > Adding the context parameter will allow to pass client/target specific
> > information associated with an individual data transfer request.
> >
> > In the case of RapidIO support this additional information consists of target
> > destination ID and its buffer address (which is not mapped into the local CPU
> > memory space). Because a single RapidIO-capable DMA channel may queue data
> > transfer requests to different target devices, the per-request configuration
> > is required.
> >
> > The proposed change eliminates need for new subsystem-specific API.
> > Existing DMA_SLAVE clients will ignore the new parameter.
> >
> > This RFC only demonstrates the API change and does not include corresponding
> > changes to existing DMA_SLAVE clients. Complete set of patches will be provided
> > after (if) this API change is accepted.
>
> This looks good to me. But was thinking if we need to add this new
> parameter for other slave calls (circular, interleaved, memcpy...)
> 

I agree that cyclic and interleaved calls may benefit from adding that parameter as well.
Benefits to the cyclic call are straightforward - same as dma_slave.
Adding a context parameter to the interleaved transfers may be more future proofing option
than an immediate need. Memcopy and other calls that deal with local memory transfers
probably should be left untouched.

What if we limit modifications to:
1) three calls (slave, cyclic and interleaved) OR
2) two (slave and cyclic) at this moment?

I am just more focused on dma_slave just because it fits well to provide RDMA
over RapidIO fabric.

If everybody agrees, I can go ahead and make changes to all three at once.

Alex.


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [RFC] dmaengine/dma_slave: add context parameter to prep_slave_sg callback

[Vinod Koul]

On Mon, 2012-01-30 at 08:55 -0800, Bounine, Alexandre wrote:
> On Monday, January 30, 2012 at 4:31 AM, Vinod Koul wrote:
> > 
> > On Thu, 2012-01-26 at 16:22 -0500, Alexandre Bounine wrote:
> > > As we agreed during our discussion about adding DMA Engine support for RapidIO
> > > subsystem, RapidIO and similar clients may benefit from adding an extra context
> > > parameter to device_prep_slave_sg() callback.
> > > See https://lkml.org/lkml/2011/10/24/275 for more details.
> > >
> > > Adding the context parameter will allow to pass client/target specific
> > > information associated with an individual data transfer request.
> > >
> > > In the case of RapidIO support this additional information consists of target
> > > destination ID and its buffer address (which is not mapped into the local CPU
> > > memory space). Because a single RapidIO-capable DMA channel may queue data
> > > transfer requests to different target devices, the per-request configuration
> > > is required.
> > >
> > > The proposed change eliminates need for new subsystem-specific API.
> > > Existing DMA_SLAVE clients will ignore the new parameter.
> > >
> > > This RFC only demonstrates the API change and does not include corresponding
> > > changes to existing DMA_SLAVE clients. Complete set of patches will be provided
> > > after (if) this API change is accepted.
> >
> > This looks good to me. But was thinking if we need to add this new
> > parameter for other slave calls (circular, interleaved, memcpy...)
> > 
> 
> I agree that cyclic and interleaved calls may benefit from adding that parameter as well.
> Benefits to the cyclic call are straightforward - same as dma_slave.
> Adding a context parameter to the interleaved transfers may be more future proofing option
> than an immediate need. Memcopy and other calls that deal with local memory transfers
> probably should be left untouched.
> 
> What if we limit modifications to:
> 1) three calls (slave, cyclic and interleaved) OR
> 2) two (slave and cyclic) at this moment?
> 
> I am just more focused on dma_slave just because it fits well to provide RDMA
> over RapidIO fabric.
> 
> If everybody agrees, I can go ahead and make changes to all three at once.
For now we need at least slave and cyclic, so pls go ahead and make these changes.
For interleaved, we might need it sooner [1], but I would think it would
need few more changes to the API, so it can be rolled as part of those
changes.

-- 
~Vinod

[1]: https://lkml.org/lkml/2012/1/30/48

Re: [RFC] dmaengine/dma_slave: add context parameter to prep_slave_sg callback

[Guennadi Liakhovetski]

On Mon, 30 Jan 2012, Vinod Koul wrote:

> On Thu, 2012-01-26 at 16:22 -0500, Alexandre Bounine wrote:
> > As we agreed during our discussion about adding DMA Engine support for RapidIO
> > subsystem, RapidIO and similar clients may benefit from adding an extra context
> > parameter to device_prep_slave_sg() callback.
> > See https://lkml.org/lkml/2011/10/24/275 for more details.
> > 
> > Adding the context parameter will allow to pass client/target specific
> > information associated with an individual data transfer request.
> > 
> > In the case of RapidIO support this additional information consists of target
> > destination ID and its buffer address (which is not mapped into the local CPU
> > memory space). Because a single RapidIO-capable DMA channel may queue data
> > transfer requests to different target devices, the per-request configuration
> > is required.
> > 
> > The proposed change eliminates need for new subsystem-specific API.
> > Existing DMA_SLAVE clients will ignore the new parameter.
> > 
> > This RFC only demonstrates the API change and does not include corresponding
> > changes to existing DMA_SLAVE clients. Complete set of patches will be provided
> > after (if) this API change is accepted.
> This looks good to me. But was thinking if we need to add this new
> parameter for other slave calls (circular, interleaved, memcpy...)

Yes, we (shdma.c) also need to pass additional slave configuration 
information to the dmaengine driver and I also was thinking about 
extending the existing API, but my ideas were going more in the direction 
of adding a parameter to __dma_request_channel() along the lines of

-struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param);
+struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param,
+					struct dma_slave_desc *slave);

where struct dma_slave_desc would basically just contain an opaque slave 
ID and would be embedded by dmaengine drivers in their specific 
slave-configuration types. The main difference to the API change being 
proposed here is, that I was going to configure DMA channels only once for 
each slave upon channel allocation, whereas the proposed change does this 
on a per-transfer basis. Therefore my question: do I understand the 
explanation of the RapidIO DMA architecture from the above quoted thread 
right, that DMA channels can be freely used by client drivers, without 
binding them to specific DSP cards? In which case you, probably, don't use 
DMA_PRIVATE? But that the decision - to which DSP card this specific 
transfer is directed - is made by configuring the DMA channels rather than 
embedded in the actual data?

Thanks
Guennadi

> 
> > 
> > Signed-off-by: Alexandre Bounine <alexandre.bounine@idt.com>
> > Cc: Jassi Brar <jaswinder.singh@linaro.org>
> > Cc: Russell King <rmk@arm.linux.org.uk> 
> > Cc: Kumar Gala <galak@kernel.crashing.org>
> > Cc: Matt Porter <mporter@kernel.crashing.org>
> > Cc: Li Yang <leoli@freescale.com>
> > ---
> >  include/linux/dmaengine.h |    7 ++++---
> >  1 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > index 679b349..79d71bb 100644
> > --- a/include/linux/dmaengine.h
> > +++ b/include/linux/dmaengine.h
> > @@ -575,7 +575,7 @@ struct dma_device {
> >  	struct dma_async_tx_descriptor *(*device_prep_slave_sg)(
> >  		struct dma_chan *chan, struct scatterlist *sgl,
> >  		unsigned int sg_len, enum dma_transfer_direction direction,
> > -		unsigned long flags);
> > +		unsigned long flags, void *context);
> >  	struct dma_async_tx_descriptor *(*device_prep_dma_cyclic)(
> >  		struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
> >  		size_t period_len, enum dma_transfer_direction direction);
> > @@ -607,12 +607,13 @@ static inline int dmaengine_slave_config(struct dma_chan *chan,
> >  
> >  static inline struct dma_async_tx_descriptor *dmaengine_prep_slave_single(
> >  	struct dma_chan *chan, void *buf, size_t len,
> > -	enum dma_transfer_direction dir, unsigned long flags)
> > +	enum dma_transfer_direction dir, unsigned long flags, void *context)
> >  {
> >  	struct scatterlist sg;
> >  	sg_init_one(&sg, buf, len);
> >  
> > -	return chan->device->device_prep_slave_sg(chan, &sg, 1, dir, flags);
> > +	return chan->device->device_prep_slave_sg(chan, &sg, 1, dir, flags,
> > +						  context);
> >  }
> >  
> >  static inline int dmaengine_terminate_all(struct dma_chan *chan)
> 
> 
> -- 
> ~Vinod
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

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

Re: [RFC] dmaengine/dma_slave: add context parameter to prep_slave_sg callback

[Vinod Koul]

On Wed, 2012-02-01 at 01:09 +0100, Guennadi Liakhovetski wrote:
> On Mon, 30 Jan 2012, Vinod Koul wrote:
> 
> > On Thu, 2012-01-26 at 16:22 -0500, Alexandre Bounine wrote:
> > > As we agreed during our discussion about adding DMA Engine support for RapidIO
> > > subsystem, RapidIO and similar clients may benefit from adding an extra context
> > > parameter to device_prep_slave_sg() callback.
> > > See https://lkml.org/lkml/2011/10/24/275 for more details.
> > > 
> > > Adding the context parameter will allow to pass client/target specific
> > > information associated with an individual data transfer request.
> > > 
> > > In the case of RapidIO support this additional information consists of target
> > > destination ID and its buffer address (which is not mapped into the local CPU
> > > memory space). Because a single RapidIO-capable DMA channel may queue data
> > > transfer requests to different target devices, the per-request configuration
> > > is required.
> > > 
> > > The proposed change eliminates need for new subsystem-specific API.
> > > Existing DMA_SLAVE clients will ignore the new parameter.
> > > 
> > > This RFC only demonstrates the API change and does not include corresponding
> > > changes to existing DMA_SLAVE clients. Complete set of patches will be provided
> > > after (if) this API change is accepted.
> > This looks good to me. But was thinking if we need to add this new
> > parameter for other slave calls (circular, interleaved, memcpy...)
> 
> Yes, we (shdma.c) also need to pass additional slave configuration 
> information to the dmaengine driver and I also was thinking about 
> extending the existing API, but my ideas were going more in the direction 
> of adding a parameter to __dma_request_channel() along the lines of
So your question is more on the lines of channel mapping/allocation?
The approach here is to pass controller specific parameters which are
required to setup the respective transfer. Since this is dependent on
each transfer, this needs to be passed in respective prepare.

The two things are completely orthogonal and shouldn't be clubbed.
For your issue we need a separate debate on how to solve this... I am
open to ideas...


-- 
~Vinod

Re: [RFC] dmaengine/dma_slave: add context parameter to prep_slave_sg callback

[Guennadi Liakhovetski]

On Wed, 1 Feb 2012, Vinod Koul wrote:

> On Wed, 2012-02-01 at 01:09 +0100, Guennadi Liakhovetski wrote:
> > On Mon, 30 Jan 2012, Vinod Koul wrote:
> > 
> > > On Thu, 2012-01-26 at 16:22 -0500, Alexandre Bounine wrote:
> > > > As we agreed during our discussion about adding DMA Engine support for RapidIO
> > > > subsystem, RapidIO and similar clients may benefit from adding an extra context
> > > > parameter to device_prep_slave_sg() callback.
> > > > See https://lkml.org/lkml/2011/10/24/275 for more details.
> > > > 
> > > > Adding the context parameter will allow to pass client/target specific
> > > > information associated with an individual data transfer request.
> > > > 
> > > > In the case of RapidIO support this additional information consists of target
> > > > destination ID and its buffer address (which is not mapped into the local CPU
> > > > memory space). Because a single RapidIO-capable DMA channel may queue data
> > > > transfer requests to different target devices, the per-request configuration
> > > > is required.
> > > > 
> > > > The proposed change eliminates need for new subsystem-specific API.
> > > > Existing DMA_SLAVE clients will ignore the new parameter.
> > > > 
> > > > This RFC only demonstrates the API change and does not include corresponding
> > > > changes to existing DMA_SLAVE clients. Complete set of patches will be provided
> > > > after (if) this API change is accepted.
> > > This looks good to me. But was thinking if we need to add this new
> > > parameter for other slave calls (circular, interleaved, memcpy...)
> > 
> > Yes, we (shdma.c) also need to pass additional slave configuration 
> > information to the dmaengine driver and I also was thinking about 
> > extending the existing API, but my ideas were going more in the direction 
> > of adding a parameter to __dma_request_channel() along the lines of
> So your question is more on the lines of channel mapping/allocation?
> The approach here is to pass controller specific parameters which are
> required to setup the respective transfer. Since this is dependent on
> each transfer, this needs to be passed in respective prepare.
> 
> The two things are completely orthogonal and shouldn't be clubbed.
> For your issue we need a separate debate on how to solve this... I am
> open to ideas...

Well, I'm not sure whether they are necessarily always orthogonal, they 
don't seem so in my case at least. We definitely can use our approach - 
configure the channel during allocation. I _think_ we could also perform 
the configuration on a per-transfer basis, during the prepare stage, as 
this RFC is suggesting, but that definitely would require reworking the 
driver somewhat and changing the concept. The current concept is a fixed 
DMA channel allocation to slaves for as long as the slave is using DMA. 
This is simpler, avoids some overhead during operation and fits well with 
the dmaengine PRIVATE channel concept. So, given the choice, we would 
prefer to perform the configuration during channel allocation.

Maybe there are cases, where the driver absolutely needs this additional 
information during allocation, in which case my proposal would be the only 
way to go for them.

I'll post an RFC soon - stay tuned:-)

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

Re: [RFC] dmaengine/dma_slave: add context parameter to prep_slave_sg callback

[Vinod Koul]

On Wed, 2012-02-01 at 12:58 +0100, Guennadi Liakhovetski wrote:
> > The two things are completely orthogonal and shouldn't be clubbed.
> > For your issue we need a separate debate on how to solve this... I am
> > open to ideas...
> 
> Well, I'm not sure whether they are necessarily always orthogonal, they 
> don't seem so in my case at least. We definitely can use our approach - 
> configure the channel during allocation. I _think_ we could also perform 
> the configuration on a per-transfer basis, during the prepare stage, as 
> this RFC is suggesting, but that definitely would require reworking the 
> driver somewhat and changing the concept. The current concept is a fixed 
> DMA channel allocation to slaves for as long as the slave is using DMA. 
> This is simpler, avoids some overhead during operation and fits well with 
> the dmaengine PRIVATE channel concept. So, given the choice, we would 
> prefer to perform the configuration during channel allocation.
> 
> Maybe there are cases, where the driver absolutely needs this additional 
> information during allocation, in which case my proposal would be the only 
> way to go for them.
what are you trying to address, sending controller specific information
at allocation or the channel allocation itself. I kind of sense both.
But apprach here is discussed is to pass paramters which are required
for each transfer, not static for a channel, hence the additional
controller specific parameter in respective prepare. 
> 
> I'll post an RFC soon - stay tuned:-) 
Patch is always the best idea :-)

-- 
~Vinod