DMA Offload fixes

dma_sync_wait is declared regardless of whether CONFIG_DMA_ENGINE is enabled, but calling the function without CONFIG_DMA_ENGINE enabled results in the following gcc error.
ERROR: "dma_sync_wait" [drivers/ntb/ntb.ko] undefined!

To get around this, declare dma_sync_wait as an inline function if CONFIG_DMA_ENGINE is undefined.

Signed-off-by Jon Mason <[email protected]>

Acked-by Dave Jiang <[email protected]>

---
 include/linux/dmaengine.h |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

include/linux/dmaengine.h +1 -1

index 96d3e4a..e3652ac 100644
@@ -963,8 +963,8 @@ dma_set_tx_state(struct dma_tx_state *st, dma_cookie_t last, dma_cookie_t used,
        }
 }
 
-enum dma_status dma_sync_wait(struct dma_chan *chan, dma_cookie_t cookie);
 #ifdef CONFIG_DMA_ENGINE
+enum dma_status dma_sync_wait(struct dma_chan *chan, dma_cookie_t cookie);
 enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx);
 void dma_issue_pending_all(void);
 struct dma_chan *__dma_request_channel(const dma_cap_mask_t *mask,

Quoted 31 lines · Jon Mason

dma_sync_wait is declared regardless of whether CONFIG_DMA_ENGINE is enabled, but calling the function without CONFIG_DMA_ENGINE enabled results in the following gcc error.
ERROR: "dma_sync_wait" [drivers/ntb/ntb.ko] undefined!

To get around this, declare dma_sync_wait as an inline function if CONFIG_DMA_ENGINE is undefined.

Signed-off-by Jon Mason <[email protected]>

Acked-by Dave Jiang <[email protected]>

---
 include/linux/dmaengine.h |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

include/linux/dmaengine.h +1 -1

index 96d3e4a..e3652ac 100644
@@ -963,8 +963,8 @@ dma_set_tx_state(struct dma_tx_state *st, dma_cookie_t last, dma_cookie_t used,
        }
 }
 
-enum dma_status dma_sync_wait(struct dma_chan *chan, dma_cookie_t cookie);
 #ifdef CONFIG_DMA_ENGINE
+enum dma_status dma_sync_wait(struct dma_chan *chan, dma_cookie_t cookie);
 enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx);
 void dma_issue_pending_all(void);
 struct dma_chan *__dma_request_channel(const dma_cap_mask_t *mask,

@@ -972,6 +972,10 @@ struct dma_chan *__dma_request_channel(const dma_cap_mask_t *mask,
 struct dma_chan *dma_request_slave_channel(struct device *dev, const char *name);
 void dma_release_channel(struct dma_chan *chan);

include/linux/dmaengine.h struct dma_chan *__dma_request_channel(const dma_cap_mask_t *mask, · line 978

Quoted 5 lines

include/linux/dmaengine.h +4 -0

 #else
+static inline enum dma_status dma_sync_wait(struct dma_chan *chan, dma_cookie_t cookie)
+{
+       return DMA_SUCCESS;
+}

Seems like something we should fix, but why is this not an indication that the code calling this is missing a "depends on DMA_ENGINE".

On second look dma_sync_wait is a use as last resort hack that probably should not escape its internal use in dmaengine.c. The other escape in drivers/media/platform/timblogiw.c already looks problematic by having a live cpu spin immediately following a sleeping wait, obviously something event based was wanted. What's the use case in NTB?

--
Dan

On Tue, Jun 18, 2013 at 06:13:28PM -0700, Dan Williams wrote:

NTB is currently using it to flush any pending DMAs. This is needed to allow the DMA engine and the CPU to perform operations on the same "Memory Window". Without this, it is possible for the operations to complete out of order, which is not a desired outcome for any network traffic over NTB. CPU is preferred over DMA engine for small transfers. Also, it provides an alternative for errors in the DMA engine copy process (e.g., DMA mapping, device_prep_dma_memcpy, and dmaengine_submit).

Quoted 1 line · Jon Mason

On Tue, Jun 18, 2013 at 06:13:28PM -0700, Dan Williams wrote:

[..]

Quoted 8 lines

NTB is currently using it to flush any pending DMAs. This is needed to allow the DMA engine and the CPU to perform operations on the same "Memory Window". Without this, it is possible for the operations to complete out of order, which is not a desired outcome for any network traffic over NTB. CPU is preferred over DMA engine for small transfers. Also, it provides an alternative for errors in the DMA engine copy process (e.g., DMA mapping, device_prep_dma_memcpy, and dmaengine_submit).

Ok there really isn't a better alternative, is NTB always polling or does it also use completion callbacks?

Set the DMA_PRIVATE dma_transaction_type in the capability mask. This enables the ability to get an exclusive ioatdma DMA channel for any devices that requests one via the dma_request_channel function call.

Signed-off-by Jon Mason <[email protected]>

Acked-by Dave Jiang <[email protected]>

---
 drivers/dma/ioat/dma_v3.c |    1 +
 1 file changed, 1 insertion(+)

drivers/dma/ioat/dma_v3.c +0 -0

index ca6ea9b..ac2aeef 100644

Quoted 17 lines · Jon Mason

Set the DMA_PRIVATE dma_transaction_type in the capability mask. This enables the ability to get an exclusive ioatdma DMA channel for any devices that requests one via the dma_request_channel function call.

Signed-off-by Jon Mason <[email protected]>

Acked-by Dave Jiang <[email protected]>

---
  drivers/dma/ioat/dma_v3.c |    1 +
  1 file changed, 1 insertion(+)

drivers/dma/ioat/dma_v3.c +0 -0

index ca6ea9b..ac2aeef 100644
@@ -1883,6 +1883,7 @@ int ioat3_dma_probe(struct ioatdma_device *device, int dca)
                 dma->copy_align = 6;
 

drivers/dma/ioat/dma_v3.c int ioat3_dma_probe(struct ioatdma_device *device, int dca) · line 1887

Quoted 3 lines

drivers/dma/ioat/dma_v3.c +1 -0

         dma_cap_set(DMA_INTERRUPT, dma->cap_mask);
+       dma_cap_set(DMA_PRIVATE, dma->cap_mask);
         dma->device_prep_dma_interrupt = ioat3_prep_interrupt_lock;

DMA_PRIVATE here keeps all channels private, so they couldn't be used elsewhere, for example raid offload. Do you need a private allocation or can you get away with a dynamically assigned channel?

Quoted 17 lines · Jon Mason

Set the DMA_PRIVATE dma_transaction_type in the capability mask. This enables the ability to get an exclusive ioatdma DMA channel for any devices that requests one via the dma_request_channel function call.

Signed-off-by Jon Mason <[email protected]>

Acked-by Dave Jiang <[email protected]>

---
 drivers/dma/ioat/dma_v3.c |    1 +
 1 file changed, 1 insertion(+)

drivers/dma/ioat/dma_v3.c +0 -0

index ca6ea9b..ac2aeef 100644
@@ -1883,6 +1883,7 @@ int ioat3_dma_probe(struct ioatdma_device *device, int dca)
                dma->copy_align = 6;
 

drivers/dma/ioat/dma_v3.c int ioat3_dma_probe(struct ioatdma_device *device, int dca) · line 1887

Quoted 3 lines

drivers/dma/ioat/dma_v3.c +1 -0

        dma_cap_set(DMA_INTERRUPT, dma->cap_mask);
+       dma_cap_set(DMA_PRIVATE, dma->cap_mask);
        dma->device_prep_dma_interrupt = ioat3_prep_interrupt_lock;

DMA_PRIVATE here keeps all channels private, so they couldn't be used elsewhere, for example raid offload. Do you need a private allocation or can you get away with a dynamically assigned channel?

Quoted 3 lines

DMA_PRIVATE here keeps all channels private, so they couldn't be used elsewhere, for example raid offload. Do you need a private allocation or can you get away with a dynamically assigned channel?

I would like to have a dedicated DMA engine. async_tx could cause the copies to complete out of order. Do I need to add infrastructure to allow for private channel usage, and when unused allow the channel to be used by async_tx?

Quoted 8 lines · Jon Mason

Quoted 3 lines

DMA_PRIVATE here keeps all channels private, so they couldn't be used elsewhere, for example raid offload. Do you need a private allocation or can you get away with a dynamically assigned channel?

I would like to have a dedicated DMA engine. async_tx could cause the copies to complete out of order. Do I need to add infrastructure to allow for private channel usage, and when unused allow the channel to be used by async_tx?

Can NTB just call dma_find_channel() once and be done? Also, async_tx will only get things out of order if you allow it to pick a new channel for every operation, but as long as you specify a dependency chain it will keep things in order (same as remembering the result of dma_find_channel).

--
Dan

Quoted 10 lines · Dan Williams

Quoted 8 lines · Jon Mason

Quoted 3 lines

DMA_PRIVATE here keeps all channels private, so they couldn't be used elsewhere, for example raid offload. Do you need a private allocation or can you get away with a dynamically assigned channel?

I would like to have a dedicated DMA engine. async_tx could cause the copies to complete out of order. Do I need to add infrastructure to allow for private channel usage, and when unused allow the channel to be used by async_tx?

Quoted 5 lines

Can NTB just call dma_find_channel() once and be done? Also, async_tx will only get things out of order if you allow it to pick a new channel for every operation, but as long as you specify a dependency chain it will keep things in order (same as remembering the result of dma_find_channel).

Neither of these allow a device exclusive access to a channel. Is this not something that is desired by other users of DMA Engines, or do I have a unique usage model?

Quoted 17 lines · Jon Mason

Quoted 10 lines · Dan Williams

Quoted 8 lines · Jon Mason

Quoted 3 lines

DMA_PRIVATE here keeps all channels private, so they couldn't be used elsewhere, for example raid offload. Do you need a private allocation or can you get away with a dynamically assigned channel?

I would like to have a dedicated DMA engine. async_tx could cause the copies to complete out of order. Do I need to add infrastructure to allow for private channel usage, and when unused allow the channel to be used by async_tx?

Quoted 5 lines

Can NTB just call dma_find_channel() once and be done? Also, async_tx will only get things out of order if you allow it to pick a new channel for every operation, but as long as you specify a dependency chain it will keep things in order (same as remembering the result of dma_find_channel).

Quoted 4 lines

Neither of these allow a device exclusive access to a channel. Is this not something that is desired by other users of DMA Engines, or do I have a unique usage model?

Yes, ioatdma has never been used in a mixed model. Other platforms are either using devicetree bindings for specifying channels to their master devices or are exclusively slave channels. Other slave usage models require use of a dedicated channel, since NTB is alternating between cpu and dma it seems it could get away with using a "public" channel.

Unfortunately, the dma_find_channel model does not led itself to optimal usage of the available channels, as it seems to give out the same channel. Adding some randomizer (or other way to spread the channel selection over multiple channels) would be greatly beneficial.

Alternatively, adding an async_memcpy_mmio function (and perhaps some minimal size to use the DMA engine) would provide a solution that would work.

However, dma_find_channel should be sufficient to get NTB use of DMA engines out for review. I'll clean it up and send it out shortly. Thanks for the insight.