lkml.org 
[lkml]   [2008]   [Feb]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRE: [RFC v3 4/7] dmaengine: Add slave DMA interface
Date
From
>From: Haavard Skinnemoen [mailto:hskinnemoen@norway.atmel.com] 
>Sent: Friday, February 15, 2008 1:53 AM
>To: Haavard Skinnemoen
>Cc: Williams, Dan J; linux-kernel@vger.kernel.org; Nelson,
>Shannon; David Brownell; kernel@avr32linux.org; Francis
>Moreau; Paul Mundt; Vladimir A. Barinov; Pierre Ossman
>Subject: Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
>
>On Wed, 13 Feb 2008 20:24:02 +0100
>Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:
>
>> But looking at your latest patch series, I guess we can use the new
>> "next" field instead. It's not like we really need the full
>> capabilities of list_head.
>
>On second thought, if we do this, we would be using the "next" field in
>an undocumented way. It is currently documented as follows:
>
> * @next: at completion submit this descriptor
>
>But that's not how we're going to use it when doing slave transfers:
>We're using it to keep track of all the descriptors that have already
>been submitted.
>
>I think it would actually be better to go the other way and have the
>async_tx API extend the descriptor as well for its private fields. That
>way, we get the pointer we need, but we can document it differently.
>
>So how about we do something along the lines of the patch below? We
>need to update all the users too of course, but apart from making the
>dma_slave_descriptor struct smaller, I don't think it will change the
>actual memory layout at all.
>
>Haavard
>
>diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>index 9bb3407..abaf324 100644
>--- a/include/linux/dmaengine.h
>+++ b/include/linux/dmaengine.h
>@@ -267,8 +267,7 @@ struct dma_client {
>
> typedef void (*dma_async_tx_callback)(void *dma_async_param);
> /**
>- * struct dma_async_tx_descriptor - async transaction descriptor
>- * ---dma generic offload fields---
>+ * struct dma_descriptor - generic dma offload descriptor
> * @cookie: tracking cookie for this transaction, set to -EBUSY if
> * this tx is sitting on a dependency list
> * @ack: the descriptor can not be reused until the client
>acknowledges
>@@ -280,12 +279,8 @@ typedef void
>(*dma_async_tx_callback)(void *dma_async_param);
> * @tx_submit: set the prepared descriptor(s) to be executed
>by the engine
> * @callback: routine to call after this operation is complete
> * @callback_param: general parameter to pass to the callback routine
>- * ---async_tx api specific fields---
>- * @next: at completion submit this descriptor
>- * @parent: pointer to the next level up in the dependency chain
>- * @lock: protect the parent and next pointers
> */
>-struct dma_async_tx_descriptor {
>+struct dma_descriptor {
> dma_cookie_t cookie;
> int ack;
> dma_addr_t phys;
>@@ -294,6 +289,17 @@ struct dma_async_tx_descriptor {
> dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx);
> dma_async_tx_callback callback;
> void *callback_param;
>+};
>+
>+/**
>+ * struct dma_async_tx_descriptor - async transaction descriptor
>+ * @dma: generic dma descriptor
>+ * @next: at completion submit this descriptor
>+ * @parent: pointer to the next level up in the dependency chain
>+ * @lock: protect the parent and next pointers
>+ */
>+struct dma_async_tx_descriptor {
>+ struct dma_descriptor dma;
> struct dma_async_tx_descriptor *next;
> struct dma_async_tx_descriptor *parent;
> spinlock_t lock;
>@@ -301,13 +307,13 @@ struct dma_async_tx_descriptor {
>
> /**
> * struct dma_slave_descriptor - extended DMA descriptor for slave DMA
>- * @async_tx: async transaction descriptor
>- * @client_node: for use by the client, for example when operating on
>- * scatterlists.
>+ * @dma: generic dma descriptor
>+ * @next: for use by the client, for example to keep track of
>+ * submitted descriptors when dealing with scatterlists.
> */
> struct dma_slave_descriptor {
>- struct dma_async_tx_descriptor txd;
>- struct list_head client_node;
>+ struct dma_descriptor dma;
>+ struct dma_slave_descriptor *next;
> };
>
> /**
>

I'll jump in here briefly - I'm okay with the direction this is going,
but I want to be protective of ioatdma performance. As used in struct
ioat_desc_sw, the cookie and ack elements end up very close to the end
of a cache line and I'd like them to not get pushed out across the
boundry. I don't think this proposal changes the layout, I'm just
bringing up my concern.

Selfishly,
sln


\
 
 \ /
  Last update: 2008-02-15 18:21    [from the cache]
©2003-2011 Jasper Spaans