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