RE: [RFC v3 4/7] dmaengine: Add slave DMA interface
>From: Haavard Skinnemoen [mailto:[EMAIL PROTECTED] >Sent: Monday, February 18, 2008 5:30 AM >To: Nelson, Shannon >Cc: Haavard Skinnemoen; Williams, Dan J; >linux-kernel@vger.kernel.org; David Brownell; >[EMAIL PROTECTED]; Francis Moreau; Paul Mundt; Vladimir A. >Barinov; Pierre Ossman >Subject: Re: [RFC v3 4/7] dmaengine: Add slave DMA interface > >On Fri, 15 Feb 2008 09:12:35 -0800 >"Nelson, Shannon" <[EMAIL PROTECTED]> wrote: > >> 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. > >Sure, performance is very important, and it's good to see that you're >critical about the changes I'm proposing. That said, the memory layout >doesn't change at all with this patch -- the fields that didn't go into >the generic dma descriptor were at the end of the struct to begin with. > >I can add a comment saying that cookie and ack must always come first. >Any other fields that we need to be careful about? > >Haavard > Those are the only two that I'm worried about at the moment. I'm just hoping that a quirk in some compiler's struct packing doesn't push them over that edge. Thanks, sln -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [RFC v3 4/7] dmaengine: Add slave DMA interface
From: Haavard Skinnemoen [mailto:[EMAIL PROTECTED] Sent: Monday, February 18, 2008 5:30 AM To: Nelson, Shannon Cc: Haavard Skinnemoen; Williams, Dan J; linux-kernel@vger.kernel.org; David Brownell; [EMAIL PROTECTED]; Francis Moreau; Paul Mundt; Vladimir A. Barinov; Pierre Ossman Subject: Re: [RFC v3 4/7] dmaengine: Add slave DMA interface On Fri, 15 Feb 2008 09:12:35 -0800 Nelson, Shannon [EMAIL PROTECTED] wrote: 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. Sure, performance is very important, and it's good to see that you're critical about the changes I'm proposing. That said, the memory layout doesn't change at all with this patch -- the fields that didn't go into the generic dma descriptor were at the end of the struct to begin with. I can add a comment saying that cookie and ack must always come first. Any other fields that we need to be careful about? Haavard Those are the only two that I'm worried about at the moment. I'm just hoping that a quirk in some compiler's struct packing doesn't push them over that edge. Thanks, sln -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Feb 18, 2008 6:22 AM, Haavard Skinnemoen <[EMAIL PROTECTED]> wrote: > On Sat, 16 Feb 2008 13:06:54 -0700 > "Dan Williams" <[EMAIL PROTECTED]> wrote: > > > I like the direction of the patch, i.e. splitting out separate > > functionality into separate structs. However, I do not want to break > > the model of clients sourcing the operations and drivers sinking them > > which dma_slave_descriptor appears to do. How about adding a > > scatterlist pointer and an 'unmap_type' to the common descriptor? > > Where unmap_type selects between, page, single, sg, or no-unmap. > > Drivers already know the length and direction. > > But there are currently no operations available for submitting > scatterlists as a single descriptor -- the client iterates over the > scatterlist and submits one descriptor for each entry. So there's no > way you can associated a scatterlist with a single descriptor. You can > perhaps attach it to the last one, but that may get you into trouble if > the transfer is terminated early for some reason. > Drivers know how to treat a group of descriptors as one operation. I see this as no different than the case where an operation exceeds the hardware's per descriptor max transfer length. The driver internally creates extra descriptors but the client only needs to worry about one handle. [..] > Btw, this discussion is a bit off-topic for the patch in question, but > it's an issue that needs to be resolved. > Agreed, more code less words :-). I'll try to brew this into a patch. -- Dan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Fri, 15 Feb 2008 09:12:35 -0800 "Nelson, Shannon" <[EMAIL PROTECTED]> wrote: > 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. Sure, performance is very important, and it's good to see that you're critical about the changes I'm proposing. That said, the memory layout doesn't change at all with this patch -- the fields that didn't go into the generic dma descriptor were at the end of the struct to begin with. I can add a comment saying that cookie and ack must always come first. Any other fields that we need to be careful about? Haavard -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Sat, 16 Feb 2008 13:06:54 -0700
"Dan Williams" <[EMAIL PROTECTED]> wrote:
> I like the direction of the patch, i.e. splitting out separate
> functionality into separate structs. However, I do not want to break
> the model of clients sourcing the operations and drivers sinking them
> which dma_slave_descriptor appears to do. How about adding a
> scatterlist pointer and an 'unmap_type' to the common descriptor?
> Where unmap_type selects between, page, single, sg, or no-unmap.
> Drivers already know the length and direction.
But there are currently no operations available for submitting
scatterlists as a single descriptor -- the client iterates over the
scatterlist and submits one descriptor for each entry. So there's no
way you can associated a scatterlist with a single descriptor. You can
perhaps attach it to the last one, but that may get you into trouble if
the transfer is terminated early for some reason.
I don't think the pure source/sink model is very realistic -- clients
can't just submit DMA transfers and then forget about them. They must
at the very least check the status and take appropriate action if there
was an error. They must also notify the driver that the descriptor can
be reused (although I guess if a client doesn't care about the result
it can do this immediately after submission, but I really think clients
_should_ care about the result.)
And since they need to do some sort of cleanup anyway, they might as
well unmap the buffers (or call some dma_memcpy_finish() type of
function that does it for them.)
The clients certainly know the length and direction too, but they don't
necessarily know the physical address since the mapping is done by a
"middle layer". I guess that's the main problem with the model I'm
proposing.
How about we add a kind of "address cookie" struct like this (feel free
to suggest better names):
struct dma_buf_addr {
void *cpu_addr;
dma_addr_t dma_addr;
};
The client initializes the cpu_addr part and passes the struct to
dma_async_memcpy_buf_to_buf() or whatever, the middle layer sets the
dma_addr after mapping the buffer (or page), and the client passes the
same struct to dma_finish_memcpy_buf_to_buf(). Which will then be able
to unmap both buffers appropriately.
This will also eliminate the hack in crypto/async_tx/async_xor.c and
make HIGHMEM64G work again.
Scatterlists currently don't have any middle-layer support, so we can
ignore them for now and let the client take the full responsibility of
mapping and unmapping the buffers. If we were to add some middle-layer
functions for dealing with scatterlists, I don't think they would need
any special treatment -- the dma address is kept in the struct
scatterlist array passed by the client, so it would work pretty much
the same way without any special treatment.
Alternatively, we could convert the whole API to use scatterlists. But
that's probably overdoing it.
Btw, this discussion is a bit off-topic for the patch in question, but
it's an issue that needs to be resolved.
Haavard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Sat, 16 Feb 2008 13:06:54 -0700
Dan Williams [EMAIL PROTECTED] wrote:
I like the direction of the patch, i.e. splitting out separate
functionality into separate structs. However, I do not want to break
the model of clients sourcing the operations and drivers sinking them
which dma_slave_descriptor appears to do. How about adding a
scatterlist pointer and an 'unmap_type' to the common descriptor?
Where unmap_type selects between, page, single, sg, or no-unmap.
Drivers already know the length and direction.
But there are currently no operations available for submitting
scatterlists as a single descriptor -- the client iterates over the
scatterlist and submits one descriptor for each entry. So there's no
way you can associated a scatterlist with a single descriptor. You can
perhaps attach it to the last one, but that may get you into trouble if
the transfer is terminated early for some reason.
I don't think the pure source/sink model is very realistic -- clients
can't just submit DMA transfers and then forget about them. They must
at the very least check the status and take appropriate action if there
was an error. They must also notify the driver that the descriptor can
be reused (although I guess if a client doesn't care about the result
it can do this immediately after submission, but I really think clients
_should_ care about the result.)
And since they need to do some sort of cleanup anyway, they might as
well unmap the buffers (or call some dma_memcpy_finish() type of
function that does it for them.)
The clients certainly know the length and direction too, but they don't
necessarily know the physical address since the mapping is done by a
middle layer. I guess that's the main problem with the model I'm
proposing.
How about we add a kind of address cookie struct like this (feel free
to suggest better names):
struct dma_buf_addr {
void *cpu_addr;
dma_addr_t dma_addr;
};
The client initializes the cpu_addr part and passes the struct to
dma_async_memcpy_buf_to_buf() or whatever, the middle layer sets the
dma_addr after mapping the buffer (or page), and the client passes the
same struct to dma_finish_memcpy_buf_to_buf(). Which will then be able
to unmap both buffers appropriately.
This will also eliminate the hack in crypto/async_tx/async_xor.c and
make HIGHMEM64G work again.
Scatterlists currently don't have any middle-layer support, so we can
ignore them for now and let the client take the full responsibility of
mapping and unmapping the buffers. If we were to add some middle-layer
functions for dealing with scatterlists, I don't think they would need
any special treatment -- the dma address is kept in the struct
scatterlist array passed by the client, so it would work pretty much
the same way without any special treatment.
Alternatively, we could convert the whole API to use scatterlists. But
that's probably overdoing it.
Btw, this discussion is a bit off-topic for the patch in question, but
it's an issue that needs to be resolved.
Haavard
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Fri, 15 Feb 2008 09:12:35 -0800 Nelson, Shannon [EMAIL PROTECTED] wrote: 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. Sure, performance is very important, and it's good to see that you're critical about the changes I'm proposing. That said, the memory layout doesn't change at all with this patch -- the fields that didn't go into the generic dma descriptor were at the end of the struct to begin with. I can add a comment saying that cookie and ack must always come first. Any other fields that we need to be careful about? Haavard -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Feb 18, 2008 6:22 AM, Haavard Skinnemoen [EMAIL PROTECTED] wrote: On Sat, 16 Feb 2008 13:06:54 -0700 Dan Williams [EMAIL PROTECTED] wrote: I like the direction of the patch, i.e. splitting out separate functionality into separate structs. However, I do not want to break the model of clients sourcing the operations and drivers sinking them which dma_slave_descriptor appears to do. How about adding a scatterlist pointer and an 'unmap_type' to the common descriptor? Where unmap_type selects between, page, single, sg, or no-unmap. Drivers already know the length and direction. But there are currently no operations available for submitting scatterlists as a single descriptor -- the client iterates over the scatterlist and submits one descriptor for each entry. So there's no way you can associated a scatterlist with a single descriptor. You can perhaps attach it to the last one, but that may get you into trouble if the transfer is terminated early for some reason. Drivers know how to treat a group of descriptors as one operation. I see this as no different than the case where an operation exceeds the hardware's per descriptor max transfer length. The driver internally creates extra descriptors but the client only needs to worry about one handle. [..] Btw, this discussion is a bit off-topic for the patch in question, but it's an issue that needs to be resolved. Agreed, more code less words :-). I'll try to brew this into a patch. -- Dan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Feb 15, 2008 2:53 AM, Haavard Skinnemoen <[EMAIL PROTECTED]> wrote: > On Wed, 13 Feb 2008 20:24:02 +0100 > Haavard Skinnemoen <[EMAIL PROTECTED]> 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. > I like the direction of the patch, i.e. splitting out separate functionality into separate structs. However, I do not want to break the model of clients sourcing the operations and drivers sinking them which dma_slave_descriptor appears to do. How about adding a scatterlist pointer and an 'unmap_type' to the common descriptor? Where unmap_type selects between, page, single, sg, or no-unmap. Drivers already know the length and direction. Regards, Dan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Feb 15, 2008 2:53 AM, Haavard Skinnemoen [EMAIL PROTECTED] wrote: On Wed, 13 Feb 2008 20:24:02 +0100 Haavard Skinnemoen [EMAIL PROTECTED] 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. I like the direction of the patch, i.e. splitting out separate functionality into separate structs. However, I do not want to break the model of clients sourcing the operations and drivers sinking them which dma_slave_descriptor appears to do. How about adding a scatterlist pointer and an 'unmap_type' to the common descriptor? Where unmap_type selects between, page, single, sg, or no-unmap. Drivers already know the length and direction. Regards, Dan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [RFC v3 4/7] dmaengine: Add slave DMA interface
>From: Haavard Skinnemoen [mailto:[EMAIL PROTECTED]
>Sent: Friday, February 15, 2008 1:53 AM
>To: Haavard Skinnemoen
>Cc: Williams, Dan J; linux-kernel@vger.kernel.org; Nelson,
>Shannon; David Brownell; [EMAIL PROTECTED]; 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 <[EMAIL PROTECTED]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Wed, 13 Feb 2008 20:24:02 +0100
Haavard Skinnemoen <[EMAIL PROTECTED]> 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;
};
/**
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Wed, 13 Feb 2008 20:24:02 +0100
Haavard Skinnemoen [EMAIL PROTECTED] 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;
};
/**
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [RFC v3 4/7] dmaengine: Add slave DMA interface
From: Haavard Skinnemoen [mailto:[EMAIL PROTECTED]
Sent: Friday, February 15, 2008 1:53 AM
To: Haavard Skinnemoen
Cc: Williams, Dan J; linux-kernel@vger.kernel.org; Nelson,
Shannon; David Brownell; [EMAIL PROTECTED]; 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 [EMAIL PROTECTED] 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Wed, 13 Feb 2008 12:07:26 -0700
"Dan Williams" <[EMAIL PROTECTED]> wrote:
> > +struct dma_slave_descriptor {
> > + struct dma_async_tx_descriptor txd;
> > + struct list_head client_node;
> > +};
>
> Can you explain a bit why client_node is needed? I do not think we
> need dma_slave_descriptor if dma_unmap data / control is added to
> dma_async_tx_descriptor. Hmm?
Well, it's perhaps not needed for slave transfers as such. But the MMC
driver (and I suspect quite a few other users of the slave interface)
deals with scatterlists, so it needs a way to keep track of all the
descriptors it submits. Hence the list node.
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.
Haavard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Feb 12, 2008 9:43 AM, Haavard Skinnemoen <[EMAIL PROTECTED]> wrote:
[..]
> /**
> + * 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.
> + */
> +struct dma_slave_descriptor {
> + struct dma_async_tx_descriptor txd;
> + struct list_head client_node;
> +};
Can you explain a bit why client_node is needed? I do not think we
need dma_slave_descriptor if dma_unmap data / control is added to
dma_async_tx_descriptor. Hmm?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Wed, 13 Feb 2008 00:21:41 -0700
"Dan Williams" <[EMAIL PROTECTED]> wrote:
> On Feb 12, 2008 9:43 AM, Haavard Skinnemoen <[EMAIL PROTECTED]> wrote:
> [..]
> > +enum dma_slave_direction {
> > + DMA_SLAVE_TO_MEMORY,
> > + DMA_SLAVE_FROM_MEMORY,
> > +};
>
> Just reuse enum dma_data_direction from the dma-mapping api.
Hmm...ok. That will add two "directions" that are a bit useless in this
context (DMA_BIDIRECTIONAL and DMA_NONE.) But I guess it's still worth
it since we can pass the direction on unchanged when syncing the
buffers.
Haavard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Wed, 13 Feb 2008 00:21:41 -0700
Dan Williams [EMAIL PROTECTED] wrote:
On Feb 12, 2008 9:43 AM, Haavard Skinnemoen [EMAIL PROTECTED] wrote:
[..]
+enum dma_slave_direction {
+ DMA_SLAVE_TO_MEMORY,
+ DMA_SLAVE_FROM_MEMORY,
+};
Just reuse enum dma_data_direction from the dma-mapping api.
Hmm...ok. That will add two directions that are a bit useless in this
context (DMA_BIDIRECTIONAL and DMA_NONE.) But I guess it's still worth
it since we can pass the direction on unchanged when syncing the
buffers.
Haavard
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Feb 12, 2008 9:43 AM, Haavard Skinnemoen [EMAIL PROTECTED] wrote:
[..]
/**
+ * 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.
+ */
+struct dma_slave_descriptor {
+ struct dma_async_tx_descriptor txd;
+ struct list_head client_node;
+};
Can you explain a bit why client_node is needed? I do not think we
need dma_slave_descriptor if dma_unmap data / control is added to
dma_async_tx_descriptor. Hmm?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Wed, 13 Feb 2008 12:07:26 -0700
Dan Williams [EMAIL PROTECTED] wrote:
+struct dma_slave_descriptor {
+ struct dma_async_tx_descriptor txd;
+ struct list_head client_node;
+};
Can you explain a bit why client_node is needed? I do not think we
need dma_slave_descriptor if dma_unmap data / control is added to
dma_async_tx_descriptor. Hmm?
Well, it's perhaps not needed for slave transfers as such. But the MMC
driver (and I suspect quite a few other users of the slave interface)
deals with scatterlists, so it needs a way to keep track of all the
descriptors it submits. Hence the list node.
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.
Haavard
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Feb 12, 2008 9:43 AM, Haavard Skinnemoen <[EMAIL PROTECTED]> wrote:
[..]
> +enum dma_slave_direction {
> + DMA_SLAVE_TO_MEMORY,
> + DMA_SLAVE_FROM_MEMORY,
> +};
Just reuse enum dma_data_direction from the dma-mapping api.
--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
On Feb 12, 2008 9:43 AM, Haavard Skinnemoen [EMAIL PROTECTED] wrote:
[..]
+enum dma_slave_direction {
+ DMA_SLAVE_TO_MEMORY,
+ DMA_SLAVE_FROM_MEMORY,
+};
Just reuse enum dma_data_direction from the dma-mapping api.
--
Dan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
