* [RFC PATCH v4 0/6] i2c: document DMA handling and add helpers for it @ 2017-08-17 14:14 Wolfram Sang 2017-08-17 14:14 ` [RFC PATCH v4 1/6] i2c: add a message flag for DMA safe buffers Wolfram Sang ` (6 more replies) 0 siblings, 7 replies; 16+ messages in thread From: Wolfram Sang @ 2017-08-17 14:14 UTC (permalink / raw) To: linux-i2c Cc: linux-renesas-soc, linux-kernel, linux-iio, linux-input, linux-media, dri-devel, Wolfram Sang So, after revisiting old mail threads, taking part in a similar discussion on the USB list, and implementing a not-convincing solution before, here is what I cooked up to document and ease DMA handling for I2C within Linux. Please have a look at the documentation introduced in patch 3 for details. While the previous versions tried to magically apply bounce buffers when needed, it became clear that detecting DMA safe buffers is too fragile. This approach is now opt-in, a DMA_SAFE flag needs to be set on an i2c_msg. The outcome so far is very convincing IMO. The core additions are simple and easy to understand (makes me even think of inlining them again?). The driver changes for the Renesas IP cores became easier to understand, too. While only a tad for the i2c-sh_mobile driver, the situation became a LOT better for the i2c-rcar driver. No more DMA disabling for the whole transfer in case of unsafe buffers, we are back to per-msg handling. And the code fix is now an easy to understand one line change. Yay! Of course, we must now whitelist DMA safe buffers. An example for I2C_RDWR case is in this series. It makes the i2ctransfer utility have DMA_SAFE buffers, which is nice for testing as i2cdump will (currently) not use DMA_SAFE buffers. My plan is to add two new calls: i2c_master_{send|receive}_dma_safe which can be used if DMA_SAFE buffers are provided. So, drivers can simply switch to them. Also, the buffers used within i2c_smbus_xfer_emulated() need to be converted to be DMA_SAFE which will cover a huge bunch of use cases. The rest is then updating drivers which can be done when needed. As these conversions are not done yet, this patch series has RFC status. But I already would like to get opinions on this approach, so I'll cc mailing lists of the heavier I2C users. Please let me know what you think. All patches have been tested with a Renesas Salvator-X board (r8a7796/M3-W). The branch can be found here: git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/i2c-core-dma-rfc-v4 And big kudos to Renesas Electronics for funding this work, thank you very much! Regards, Wolfram Changes since v3: * completely redesigned Wolfram Sang (6): i2c: add a message flag for DMA safe buffers i2c: add helpers to ease DMA handling i2c: add docs to clarify DMA handling i2c: sh_mobile: use helper to decide if DMA is useful i2c: rcar: skip DMA if buffer is not safe i2c: dev: mark RDWR buffers as DMA_SAFE Documentation/i2c/DMA-considerations | 50 ++++++++++++++++++++++++++++++++++++ drivers/i2c/busses/i2c-rcar.c | 2 +- drivers/i2c/busses/i2c-sh_mobile.c | 8 ++++-- drivers/i2c/i2c-core-base.c | 45 ++++++++++++++++++++++++++++++++ drivers/i2c/i2c-dev.c | 2 ++ include/linux/i2c.h | 3 +++ include/uapi/linux/i2c.h | 3 +++ 7 files changed, 110 insertions(+), 3 deletions(-) create mode 100644 Documentation/i2c/DMA-considerations -- 2.11.0 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH v4 1/6] i2c: add a message flag for DMA safe buffers 2017-08-17 14:14 [RFC PATCH v4 0/6] i2c: document DMA handling and add helpers for it Wolfram Sang @ 2017-08-17 14:14 ` Wolfram Sang 2017-08-17 14:14 ` [RFC PATCH v4 2/6] i2c: add helpers to ease DMA handling Wolfram Sang ` (5 subsequent siblings) 6 siblings, 0 replies; 16+ messages in thread From: Wolfram Sang @ 2017-08-17 14:14 UTC (permalink / raw) To: linux-i2c Cc: linux-renesas-soc, linux-kernel, linux-iio, linux-input, linux-media, dri-devel, Wolfram Sang I2C has no requirement that the buffer of a message needs to be DMA safe. In case it is, it can now be flagged, so drivers wishing to do DMA can use the buffer directly. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- include/uapi/linux/i2c.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h index 009e27bb9abe19..1c683cb319e4b7 100644 --- a/include/uapi/linux/i2c.h +++ b/include/uapi/linux/i2c.h @@ -71,6 +71,9 @@ struct i2c_msg { #define I2C_M_RD 0x0001 /* read data, from slave to master */ /* I2C_M_RD is guaranteed to be 0x0001! */ #define I2C_M_TEN 0x0010 /* this is a ten bit chip address */ +#define I2C_M_DMA_SAFE 0x0200 /* the buffer of this message is DMA safe */ + /* makes only sense in kernelspace */ + /* userspace buffers are copied anyway */ #define I2C_M_RECV_LEN 0x0400 /* length will be first received byte */ #define I2C_M_NO_RD_ACK 0x0800 /* if I2C_FUNC_PROTOCOL_MANGLING */ #define I2C_M_IGNORE_NAK 0x1000 /* if I2C_FUNC_PROTOCOL_MANGLING */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH v4 2/6] i2c: add helpers to ease DMA handling 2017-08-17 14:14 [RFC PATCH v4 0/6] i2c: document DMA handling and add helpers for it Wolfram Sang 2017-08-17 14:14 ` [RFC PATCH v4 1/6] i2c: add a message flag for DMA safe buffers Wolfram Sang @ 2017-08-17 14:14 ` Wolfram Sang 2017-08-17 14:14 ` [RFC PATCH v4 3/6] i2c: add docs to clarify " Wolfram Sang ` (4 subsequent siblings) 6 siblings, 0 replies; 16+ messages in thread From: Wolfram Sang @ 2017-08-17 14:14 UTC (permalink / raw) To: linux-i2c Cc: linux-renesas-soc, linux-kernel, linux-iio, linux-input, linux-media, dri-devel, Wolfram Sang One helper checks if DMA is suitable and optionally creates a bounce buffer, if not. The other function returns the bounce buffer and makes sure the data is properly copied back to the message. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/i2c-core-base.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ include/linux/i2c.h | 3 +++ 2 files changed, 48 insertions(+) diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 12822a4b8f8f09..a104ebc2d05af8 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -2241,6 +2241,51 @@ void i2c_put_adapter(struct i2c_adapter *adap) } EXPORT_SYMBOL(i2c_put_adapter); +/** + * i2c_get_dma_safe_msg_buf() - get a DMA safe buffer for the given i2c_msg + * @msg: the message to be checked + * @threshold: the amount of byte from which using DMA makes sense + * + * Return: NULL if a DMA safe buffer was not obtained. Use msg->buf with PIO. + * + * Or a valid pointer to be used with DMA. Note that it can either be + * msg->buf or a bounce buffer. After use, release it by calling + * i2c_release_dma_safe_msg_buf(). + * + * This function must only be called from process context! + */ +u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold) +{ + if (msg->len < threshold) + return NULL; + + if (msg->flags & I2C_M_DMA_SAFE) + return msg->buf; + + if (msg->flags & I2C_M_RD) + return kzalloc(msg->len, GFP_KERNEL); + else + return kmemdup(msg->buf, msg->len, GFP_KERNEL); +} +EXPORT_SYMBOL_GPL(i2c_get_dma_safe_msg_buf); + +/** + * i2c_release_dma_safe_msg_buf - release DMA safe buffer and sync with i2c_msg + * @msg: the message to be synced with + * @buf: the buffer obtained from i2c_get_dma_safe_msg_buf(). May be NULL. + */ +void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf) +{ + if (!buf || buf == msg->buf) + return; + + if (msg->flags & I2C_M_RD) + memcpy(msg->buf, buf, msg->len); + + kfree(buf); +} +EXPORT_SYMBOL_GPL(i2c_release_dma_safe_msg_buf); + MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>"); MODULE_DESCRIPTION("I2C-Bus main module"); MODULE_LICENSE("GPL"); diff --git a/include/linux/i2c.h b/include/linux/i2c.h index d501d3956f13f0..1e99342f180f45 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -767,6 +767,9 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg) return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0); } +u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold); +void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf); + int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr); /** * module_i2c_driver() - Helper macro for registering a modular I2C driver -- 2.11.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling 2017-08-17 14:14 [RFC PATCH v4 0/6] i2c: document DMA handling and add helpers for it Wolfram Sang 2017-08-17 14:14 ` [RFC PATCH v4 1/6] i2c: add a message flag for DMA safe buffers Wolfram Sang 2017-08-17 14:14 ` [RFC PATCH v4 2/6] i2c: add helpers to ease DMA handling Wolfram Sang @ 2017-08-17 14:14 ` Wolfram Sang 2017-08-27 11:37 ` Mauro Carvalho Chehab 2017-08-17 14:14 ` [RFC PATCH v4 4/6] i2c: sh_mobile: use helper to decide if DMA is useful Wolfram Sang ` (3 subsequent siblings) 6 siblings, 1 reply; 16+ messages in thread From: Wolfram Sang @ 2017-08-17 14:14 UTC (permalink / raw) To: linux-i2c Cc: linux-renesas-soc, linux-kernel, linux-iio, linux-input, linux-media, dri-devel, Wolfram Sang Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- Documentation/i2c/DMA-considerations | 50 ++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 Documentation/i2c/DMA-considerations diff --git a/Documentation/i2c/DMA-considerations b/Documentation/i2c/DMA-considerations new file mode 100644 index 00000000000000..a4b4a107102452 --- /dev/null +++ b/Documentation/i2c/DMA-considerations @@ -0,0 +1,50 @@ +Linux I2C and DMA +----------------- + +Given that I2C is a low-speed bus where largely small messages are transferred, +it is not considered a prime user of DMA access. At this time of writing, only +10% of I2C bus master drivers have DMA support implemented. And the vast +majority of transactions are so small that setting up DMA for it will likely +add more overhead than a plain PIO transfer. + +Therefore, it is *not* mandatory that the buffer of an I2C message is DMA safe. +It does not seem reasonable to apply additional burdens when the feature is so +rarely used. However, it is recommended to use a DMA-safe buffer if your +message size is likely applicable for DMA. Most drivers have this threshold +around 8 bytes (as of today, this is mostly an educated guess, however). For +any message of 16 byte or larger, it is probably a really good idea. + +If you use such a buffer in a i2c_msg, set the I2C_M_DMA_SAFE flag with it. +Then, the I2C core and drivers know they can safely operate DMA on it. Note +that setting this flag makes only sense in kernel space. User space data is +copied into kernel space anyhow. The I2C core makes sure the destination +buffers in kernel space are always DMA capable. + +FIXME: Need to implement i2c_master_{send|receive}_dma and proper buffers for i2c_smbus_xfer_emulated. + +Drivers wishing to implement DMA can use helper functions from the I2C core. +One gives you a DMA-safe buffer for a given i2c_msg as long as a certain +threshold is met. + + dma_buf = i2c_get_dma_safe_msg_buf(msg, threshold_in_byte); + +If a buffer is returned, it either msg->buf for the I2C_M_DMA_SAFE case or a +bounce buffer. But you don't need to care about that detail. If NULL is +returned, the threshold was not met or a bounce buffer could not be allocated. +Fall back to PIO in that case. + +In any case, a buffer obtained from above needs to be released. It ensures data +is copied back to the message and a potentially used bounce buffer is freed. + + i2c_release_dma_safe_msg_buf(msg, dma_buf); + +The bounce buffer handling from the core is generic and simple. It will always +allocate a new bounce buffer. If you want a more sophisticated handling (e.g. +reusing pre-allocated buffers), you are free to implement your own. + +Please also check the in-kernel documentation for details. The i2c-sh_mobile +driver can be used as a reference example how to use the above helpers. + +Final note: If you plan to use DMA with I2C (or with anything else, actually) +make sure you have CONFIG_DMA_API_DEBUG enabled during development. It can help +you find various issues which can be complex to debug otherwise. -- 2.11.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling 2017-08-17 14:14 ` [RFC PATCH v4 3/6] i2c: add docs to clarify " Wolfram Sang @ 2017-08-27 11:37 ` Mauro Carvalho Chehab 2017-09-08 8:56 ` Wolfram Sang 2017-09-20 17:18 ` Wolfram Sang 0 siblings, 2 replies; 16+ messages in thread From: Mauro Carvalho Chehab @ 2017-08-27 11:37 UTC (permalink / raw) To: Wolfram Sang Cc: linux-i2c, linux-renesas-soc, linux-kernel, linux-iio, linux-input, linux-media, dri-devel Em Thu, 17 Aug 2017 16:14:46 +0200 Wolfram Sang <wsa+renesas@sang-engineering.com> escreveu: > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > Documentation/i2c/DMA-considerations | 50 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > create mode 100644 Documentation/i2c/DMA-considerations > > diff --git a/Documentation/i2c/DMA-considerations b/Documentation/i2c/DMA-considerations > new file mode 100644 > index 00000000000000..a4b4a107102452 > --- /dev/null > +++ b/Documentation/i2c/DMA-considerations > @@ -0,0 +1,50 @@ > +Linux I2C and DMA > +----------------- I would use, instead: ================= Linux I2C and DMA ================= As this is the way we're starting document titles, after converted to ReST. So, better to have it already using the right format, as one day someone will convert i2c documentation to ReST. So, it would be really cool if this document could be just renamed without needing to patch it during such conversion :-) There are also a couple of things here that Sphinx would complain. So, it could be worth to rename it to *.rst, while you're writing it, and see what: make htmldocs will complain and how it will look in html. > + > +Given that I2C is a low-speed bus where largely small messages are transferred, > +it is not considered a prime user of DMA access. At this time of writing, only > +10% of I2C bus master drivers have DMA support implemented. Are you sure about that? I'd say that, on media, at least half of the drivers use DMA for I2C bus access, as the I2C bus is on a remote board that talks with CPU via USB, using DMA, and all communication with USB should be DMA-safe. I guess what you really wanted to say on most of this section is about SoC (and other CPUs) where the I2C bus master is is at the mainboard, and not on some peripheral. > And the vast > +majority of transactions are so small that setting up DMA for it will likely > +add more overhead than a plain PIO transfer. > + > +Therefore, it is *not* mandatory that the buffer of an I2C message is DMA safe. Again, that may not be true on media boards. The code that implements the I2C transfers there, on most boards, have to be DMA safe, as it won't otherwise send/receive commands from the chips that are after the USB bridge. > +It does not seem reasonable to apply additional burdens when the feature is so > +rarely used. However, it is recommended to use a DMA-safe buffer if your > +message size is likely applicable for DMA. Most drivers have this threshold > +around 8 bytes (as of today, this is mostly an educated guess, however). For > +any message of 16 byte or larger, it is probably a really good idea. > + > +If you use such a buffer in a i2c_msg, set the I2C_M_DMA_SAFE flag with it. > +Then, the I2C core and drivers know they can safely operate DMA on it. Note > +that setting this flag makes only sense in kernel space. User space data is > +copied into kernel space anyhow. The I2C core makes sure the destination > +buffers in kernel space are always DMA capable. > + > +FIXME: Need to implement i2c_master_{send|receive}_dma and proper buffers for i2c_smbus_xfer_emulated. > + > +Drivers wishing to implement DMA can use helper functions from the I2C core. > +One gives you a DMA-safe buffer for a given i2c_msg as long as a certain > +threshold is met. > + > + dma_buf = i2c_get_dma_safe_msg_buf(msg, threshold_in_byte); I'm concerned about the new bits added by this call. Right now, USB drivers just use kalloc() for transfer buffers used to send and receive URB control messages for both setting the main device and for I2C messages. Before this changeset, buffers allocated this way are DMA save and have been working for years. When you add some flags that would make the I2C subsystem aware that a buffer is now DMA safe, I guess you could be breaking those drivers, as a DMA safe buffer will now be considered as DMA-unsafe. So, you'll likely need to patch all media USB drivers to fix it, or come up with some other solution. > + > +If a buffer is returned, it either msg->buf for the I2C_M_DMA_SAFE case or a > +bounce buffer. But you don't need to care about that detail. If NULL is > +returned, the threshold was not met or a bounce buffer could not be allocated. > +Fall back to PIO in that case. > + > +In any case, a buffer obtained from above needs to be released. It ensures data > +is copied back to the message and a potentially used bounce buffer is freed. > + > + i2c_release_dma_safe_msg_buf(msg, dma_buf); > + > +The bounce buffer handling from the core is generic and simple. It will always > +allocate a new bounce buffer. If you want a more sophisticated handling (e.g. > +reusing pre-allocated buffers), you are free to implement your own. > + > +Please also check the in-kernel documentation for details. The i2c-sh_mobile > +driver can be used as a reference example how to use the above helpers. > + > +Final note: If you plan to use DMA with I2C (or with anything else, actually) > +make sure you have CONFIG_DMA_API_DEBUG enabled during development. It can help > +you find various issues which can be complex to debug otherwise. Thanks, Mauro ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling 2017-08-27 11:37 ` Mauro Carvalho Chehab @ 2017-09-08 8:56 ` Wolfram Sang 2017-09-08 11:08 ` Mauro Carvalho Chehab 2017-09-20 17:18 ` Wolfram Sang 1 sibling, 1 reply; 16+ messages in thread From: Wolfram Sang @ 2017-09-08 8:56 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, linux-kernel, linux-iio, linux-input, linux-media, dri-devel [-- Attachment #1: Type: text/plain, Size: 3305 bytes --] Hi Mauro, thanks for your comments. Much appreciated! > There are also a couple of things here that Sphinx would complain. > So, it could be worth to rename it to *.rst, while you're writing > it, and see what: > make htmldocs > will complain and how it will look in html. OK, I'll check that. > > +Given that I2C is a low-speed bus where largely small messages are transferred, > > +it is not considered a prime user of DMA access. At this time of writing, only > > +10% of I2C bus master drivers have DMA support implemented. > > Are you sure about that? I'd say that, on media, at least half of the > drivers use DMA for I2C bus access, as the I2C bus is on a remote > board that talks with CPU via USB, using DMA, and all communication > with USB should be DMA-safe. Well, the DMA-safe requirement comes then from the USB subsystem, doesn't it? Or do you really use DMA on the remote board to transfer data via I2C to an I2C client? > I guess what you really wanted to say on most of this section is > about SoC (and other CPUs) where the I2C bus master is is at the > mainboard, and not on some peripheral. I might be biased to that, yes. So, it is good talking about it. > > And the vast > > +majority of transactions are so small that setting up DMA for it will likely > > +add more overhead than a plain PIO transfer. > > + > > +Therefore, it is *not* mandatory that the buffer of an I2C message is DMA safe. > > Again, that may not be true on media boards. The code that implements the > I2C transfers there, on most boards, have to be DMA safe, as it won't > otherwise send/receive commands from the chips that are after the USB > bridge. That still sounds to me like the DMA-safe requirement comes from USB (which is fine, of course.). In any case, a sentence like "Other subsystem you might use for bridging I2C might impose other DMA requirements" sounds like to nice to have. > > +Drivers wishing to implement DMA can use helper functions from the I2C core. > > +One gives you a DMA-safe buffer for a given i2c_msg as long as a certain > > +threshold is met. > > + > > + dma_buf = i2c_get_dma_safe_msg_buf(msg, threshold_in_byte); > > I'm concerned about the new bits added by this call. Right now, > USB drivers just use kalloc() for transfer buffers used to send and > receive URB control messages for both setting the main device and > for I2C messages. Before this changeset, buffers allocated this > way are DMA save and have been working for years. Can you give me a pointer to a driver doing this? I glimpsed around in drivers/media/usb and found that most drivers are having their i2c_msg buffers on the stack. Which is clearly not DMA-safe. > When you add some flags that would make the I2C subsystem aware > that a buffer is now DMA safe, I guess you could be breaking > those drivers, as a DMA safe buffer will now be considered as > DMA-unsafe. Well, this flag is only relevant for i2c master drivers wishing to do DMA. So, grepping in the above directory grep dma $(grep -rl i2c_add_adapter *) only gives one driver which is irrelevant because the i2c master it registers is not doing any DMA? Am I missing something? We are clearly not aligned yet... Regards, Wolfram [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling 2017-09-08 8:56 ` Wolfram Sang @ 2017-09-08 11:08 ` Mauro Carvalho Chehab 2017-09-09 15:27 ` Wolfram Sang 0 siblings, 1 reply; 16+ messages in thread From: Mauro Carvalho Chehab @ 2017-09-08 11:08 UTC (permalink / raw) To: Wolfram Sang Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, linux-kernel, linux-iio, linux-input, linux-media, dri-devel Em Fri, 8 Sep 2017 10:56:40 +0200 Wolfram Sang <wsa@the-dreams.de> escreveu: > Hi Mauro, > > thanks for your comments. Much appreciated! > > > There are also a couple of things here that Sphinx would complain. > > So, it could be worth to rename it to *.rst, while you're writing > > it, and see what: > > make htmldocs > > will complain and how it will look in html. > > OK, I'll check that. Ok. > > > +Given that I2C is a low-speed bus where largely small messages are transferred, > > > +it is not considered a prime user of DMA access. At this time of writing, only > > > +10% of I2C bus master drivers have DMA support implemented. > > > > Are you sure about that? I'd say that, on media, at least half of the > > drivers use DMA for I2C bus access, as the I2C bus is on a remote > > board that talks with CPU via USB, using DMA, and all communication > > with USB should be DMA-safe. > > Well, the DMA-safe requirement comes then from the USB subsystem, > doesn't it? Yes, but the statistics that 10% of I2C bus master drivers are DMA-safe is not true, if you take those into account ;-) Perhaps you could write it as (or something similar): At this time of writing, only +10% of I2C bus master drivers for non-remote boards have DMA support implemented. > Or do you really use DMA on the remote board to transfer > data via I2C to an I2C client? > > > I guess what you really wanted to say on most of this section is > > about SoC (and other CPUs) where the I2C bus master is is at the > > mainboard, and not on some peripheral. > > I might be biased to that, yes. So, it is good talking about it. > > > > And the vast > > > +majority of transactions are so small that setting up DMA for it will likely > > > +add more overhead than a plain PIO transfer. > > > + > > > +Therefore, it is *not* mandatory that the buffer of an I2C message is DMA safe. > > > > Again, that may not be true on media boards. The code that implements the > > I2C transfers there, on most boards, have to be DMA safe, as it won't > > otherwise send/receive commands from the chips that are after the USB > > bridge. > > That still sounds to me like the DMA-safe requirement comes from USB > (which is fine, of course.). In any case, a sentence like "Other > subsystem you might use for bridging I2C might impose other DMA > requirements" sounds like to nice to have. Agreed. > > > > +Drivers wishing to implement DMA can use helper functions from the I2C core. > > > +One gives you a DMA-safe buffer for a given i2c_msg as long as a certain > > > +threshold is met. > > > + > > > + dma_buf = i2c_get_dma_safe_msg_buf(msg, threshold_in_byte); > > > > I'm concerned about the new bits added by this call. Right now, > > USB drivers just use kalloc() for transfer buffers used to send and > > receive URB control messages for both setting the main device and > > for I2C messages. Before this changeset, buffers allocated this > > way are DMA save and have been working for years. > > Can you give me a pointer to a driver doing this? I glimpsed around in > drivers/media/usb and found that most drivers are having their i2c_msg > buffers on the stack. Which is clearly not DMA-safe. The way it is implemented depends on the driver, and usually envolves double-buffering, e. g., on some place of the driver, a buffer that may not be DMA-save is copied into a DMA safe buffer. On most cases, like on this driver: drivers/media/usb/dvb-usb-v2/az6007.c The i2c_xfer logic (or the read/write functions) is the one responsible for double-buffering. In this specific example, the DVB usbv2 core allocates the device's "state" struct using kmalloc (it uses .size_of_priv field to know the size of the "state" buffer). On struct az6007_device_state, there is a "data" buffer with 4096 bytes. At the i2c transfer function, it retrieves and use it: struct az6007_device_state *st = d_to_priv(d) ... ret = __az6007_read(d->udev, req, value, index, st->data, length); ... ret = __az6007_write(d->udev, req, value, index, st->data, length); In the past, on lots of drivers, the i2c_xfer logic just used to assume that the I2C client driver allocated a DMA-safe buffer, as it just used to pass whatever buffer it receives directly to USB core. We did an effort to change it, as it can be messy, but I'm not sure if this is solved everywhere. The __az6007_read() and __az6007_write() indirectly do DMA (for most USB host drivers), when they call usb_control_msg(). > > > When you add some flags that would make the I2C subsystem aware > > that a buffer is now DMA safe, I guess you could be breaking > > those drivers, as a DMA safe buffer will now be considered as > > DMA-unsafe. > > Well, this flag is only relevant for i2c master drivers wishing to do > DMA. So, grepping in the above directory > > grep dma $(grep -rl i2c_add_adapter *) The usage of I2C at the media subsystem predates the I2C subsystem. at V4L2 drivers, a great effort was done to port it to fully use the I2C subsystem when it was added upstream, but there were some problems a the initial implementation of the i2c subsystem that prevented its usage for the DVB drivers. By the time such restrictions got removed, it was too late, as there were already a large amount of drivers relying on i2c "low level" functions. So, almost all DVB drivers don't use i2c_add_adapter(). Instead, the I2C clients just call directly the I2C transfer functions: drivers/media/dvb-frontends/au8522_common.c: ret = i2c_transfer(state->i2c, msg, 2); > only gives one driver which is irrelevant because the i2c master it > registers is not doing any DMA? Even on the drivers that use i2c_add_adapter(), the usage of DMA can't be get by the above grep, as the DMA usage is actually at drivers/usb. For example, the em28xx driver uses i2c_add_adapter(): $ git grep i2c_add_adapter drivers/media/usb/em28xx/ drivers/media/usb/em28xx/em28xx-i2c.c: retval = i2c_add_adapter(&dev->i2c_adap[bus]); drivers/media/usb/em28xx/em28xx-i2c.c: "%s: i2c_add_adapter failed! retval [%d]\n", And the actual data transfer happens via usb_control_msg(): $ git grep usb_control_msg drivers/media/usb/em28xx/ drivers/media/usb/em28xx/em28xx-core.c: ret = usb_control_msg(udev, pipe, req, drivers/media/usb/em28xx/em28xx-core.c: ret = usb_control_msg(udev, pipe, req, If you modify your grep function, you'll see that usb_control_msg() is DMA-dependent. Try: $ grep dma $(grep -rl usb_control_msg) drivers/usb/core/ > Am I missing something? We are clearly not aligned yet... > > Regards, > > Wolfram > Thanks, Mauro ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling 2017-09-08 11:08 ` Mauro Carvalho Chehab @ 2017-09-09 15:27 ` Wolfram Sang 2017-09-09 19:34 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 16+ messages in thread From: Wolfram Sang @ 2017-09-09 15:27 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, linux-kernel, linux-iio, linux-input, linux-media, dri-devel [-- Attachment #1: Type: text/plain, Size: 3717 bytes --] > Yes, but the statistics that 10% of I2C bus master drivers > are DMA-safe is not true, if you take those into account ;-) > > Perhaps you could write it as (or something similar): > > At this time of writing, only +10% of I2C bus master > drivers for non-remote boards have DMA support implemented. No, this is confusing IMO. Being remote has nothing to with DMA. What has to do with DMA is that these are using USB as communication channel. And USB clearly needs DMA-safe buffers. The encapsulation needs DMA-safe buffers. So, I think the new sentence "other subsystems might impose" mentions that. Let me recap what this series is for: a) It makes clear that I2C subsystem does not require DMA-safety because for the reasons given in the textfile. If I2C is encapsulated over USB, then DMA-safety is of course required, but this comes from USB. So, those USB-I2C master drivers need to do something to make sure DMA-safety is guaranteed because i2c_msg buffers don't need to be DMA safe because of a). They could use the newly introduced features, but they don't have to. b) a flag for DMA safe i2c_msgs is added So, for messages we know to be DMA safe, we can flag that. Drivers could check that and use a bounce buffer if the flag is not set. However, this is all optional. Your drivers can still choose to not check the flag and everything will stay as before. Check patch 5 for a use case. c) helper functions for bounce buffers are added These are *helper* functions for drivers wishing to do DMA. A super simple bounce buffer support. Check patch 4 for a use case. Again, this is optional. Drivers can implement their own bounce buffer support. Or, as in your case, if you know that your buffers are good, then don't use any of this and things will keep going. This all is to allow bus master drivers to opt-in for DMA safety if they want to do DMA directly. Because currently, with a lot of i2c_msgs on stack, this is more or less working accidently. And, yes, I know I have to add this new flag to a few central places in client drivers. Otherwise, master drivers checking for DMA safety will initially have a performance regression. This is scheduled for V5 and mentioned in this series. > In the past, on lots of drivers, the i2c_xfer logic just used to assume > that the I2C client driver allocated a DMA-safe buffer, as it just used to > pass whatever buffer it receives directly to USB core. We did an effort to > change it, as it can be messy, but I'm not sure if this is solved everywhere. Good, I can imagine this being some effort. But again, this is because USB needs the DMA-safety, not I2C. AFAICS, most i2c_msgs are register accesses and thus, small messages. > The usage of I2C at the media subsystem predates the I2C subsystem. > at V4L2 drivers, a great effort was done to port it to fully use the > I2C subsystem when it was added upstream, but there were some problems > a the initial implementation of the i2c subsystem that prevented its > usage for the DVB drivers. By the time such restrictions got removed, > it was too late, as there were already a large amount of drivers relying > on i2c "low level" functions. Didn't know that, but this is good to know! Thanks. > Even on the drivers that use i2c_add_adapter(), the usage of DMA can't > be get by the above grep, as the DMA usage is actually at drivers/usb. Ok. But as said before, what works now will continue to work. This series is about clarifying that i2c_msg buffers need not to be DMA safe and offering some helpers for those bus master drivers wanting to opt-in to be sure. Clearer now? Regards, Wolfram [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling 2017-09-09 15:27 ` Wolfram Sang @ 2017-09-09 19:34 ` Mauro Carvalho Chehab 0 siblings, 0 replies; 16+ messages in thread From: Mauro Carvalho Chehab @ 2017-09-09 19:34 UTC (permalink / raw) To: Wolfram Sang Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, linux-kernel, linux-iio, linux-input, linux-media, dri-devel Em Sat, 9 Sep 2017 17:27:06 +0200 Wolfram Sang <wsa@the-dreams.de> escreveu: > > Yes, but the statistics that 10% of I2C bus master drivers > > are DMA-safe is not true, if you take those into account ;-) > > > > Perhaps you could write it as (or something similar): > > > > At this time of writing, only +10% of I2C bus master > > drivers for non-remote boards have DMA support implemented. > > No, this is confusing IMO. Being remote has nothing to with DMA. What > has to do with DMA is that these are using USB as communication channel. > And USB clearly needs DMA-safe buffers. The encapsulation needs DMA-safe > buffers. So, I think the new sentence "other subsystems might impose" > mentions that. > > Let me recap what this series is for: > > a) It makes clear that I2C subsystem does not require DMA-safety because > for the reasons given in the textfile. > > If I2C is encapsulated over USB, then DMA-safety is of course required, > but this comes from USB. So, those USB-I2C master drivers need to do > something to make sure DMA-safety is guaranteed because i2c_msg buffers > don't need to be DMA safe because of a). They could use the newly > introduced features, but they don't have to. > > b) a flag for DMA safe i2c_msgs is added > > So, for messages we know to be DMA safe, we can flag that. Drivers > could check that and use a bounce buffer if the flag is not set. > However, this is all optional. Your drivers can still choose to not > check the flag and everything will stay as before. Check patch 5 for a > use case. > > c) helper functions for bounce buffers are added > > These are *helper* functions for drivers wishing to do DMA. A super > simple bounce buffer support. Check patch 4 for a use case. Again, this > is optional. Drivers can implement their own bounce buffer support. Or, > as in your case, if you know that your buffers are good, then don't use > any of this and things will keep going. > > > This all is to allow bus master drivers to opt-in for DMA safety if they > want to do DMA directly. Because currently, with a lot of i2c_msgs on > stack, this is more or less working accidently. > > And, yes, I know I have to add this new flag to a few central places in > client drivers. Otherwise, master drivers checking for DMA safety will > initially have a performance regression. This is scheduled for V5 and > mentioned in this series. > > > In the past, on lots of drivers, the i2c_xfer logic just used to assume > > that the I2C client driver allocated a DMA-safe buffer, as it just used to > > pass whatever buffer it receives directly to USB core. We did an effort to > > change it, as it can be messy, but I'm not sure if this is solved everywhere. > > Good, I can imagine this being some effort. But again, this is because > USB needs the DMA-safety, not I2C. AFAICS, most i2c_msgs are register > accesses and thus, small messages. > > > The usage of I2C at the media subsystem predates the I2C subsystem. > > at V4L2 drivers, a great effort was done to port it to fully use the > > I2C subsystem when it was added upstream, but there were some problems > > a the initial implementation of the i2c subsystem that prevented its > > usage for the DVB drivers. By the time such restrictions got removed, > > it was too late, as there were already a large amount of drivers relying > > on i2c "low level" functions. > > Didn't know that, but this is good to know! Thanks. > > > Even on the drivers that use i2c_add_adapter(), the usage of DMA can't > > be get by the above grep, as the DMA usage is actually at drivers/usb. > > Ok. But as said before, what works now will continue to work. OK, good! > This series is about clarifying that i2c_msg buffers need not to be DMA safe > and offering some helpers for those bus master drivers wanting to opt-in > to be sure. > > Clearer now? Yeah, it is clearer for me. Thanks! Anyway, it doesn't hurt if you could improve the documentation patch to mention the above somewhow, as I want to avoid patches trying to make existing media USB drivers to use the new flag without changing the already existing DMA-safe logic there (causing double-buffering), or, even worse, making the I2C bus DMA-unsafe because someone misread this document. Regards, Mauro ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling 2017-08-27 11:37 ` Mauro Carvalho Chehab 2017-09-08 8:56 ` Wolfram Sang @ 2017-09-20 17:18 ` Wolfram Sang 2017-09-20 18:22 ` Mauro Carvalho Chehab 1 sibling, 1 reply; 16+ messages in thread From: Wolfram Sang @ 2017-09-20 17:18 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, linux-kernel, linux-iio, linux-input, linux-media, dri-devel [-- Attachment #1: Type: text/plain, Size: 887 bytes --] Hi Mauro, > > +Linux I2C and DMA > > +----------------- > > I would use, instead: > > ================= > Linux I2C and DMA > ================= > > As this is the way we're starting document titles, after converted to > ReST. So, better to have it already using the right format, as one day I did this. > There are also a couple of things here that Sphinx would complain. The only complaint I got was WARNING: document isn't included in any toctree which makes sense because I renamed it only temporarily to *.rst > So, it could be worth to rename it to *.rst, while you're writing > it, and see what: > make htmldocs > will complain and how it will look in html. So, no complaints from Sphinx and the HTML output looks good IMO. Was there anything specific you had in mind when saying that Sphinx would complain? Regards, Wolfram [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling 2017-09-20 17:18 ` Wolfram Sang @ 2017-09-20 18:22 ` Mauro Carvalho Chehab 2017-09-20 18:45 ` Wolfram Sang 0 siblings, 1 reply; 16+ messages in thread From: Mauro Carvalho Chehab @ 2017-09-20 18:22 UTC (permalink / raw) To: Wolfram Sang Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, linux-kernel, linux-iio, linux-input, linux-media, dri-devel [-- Attachment #1: Type: text/plain, Size: 2006 bytes --] Em Wed, 20 Sep 2017 19:18:40 +0200 Wolfram Sang <wsa@the-dreams.de> escreveu: > Hi Mauro, > > > > +Linux I2C and DMA > > > +----------------- > > > > I would use, instead: > > > > ================= > > Linux I2C and DMA > > ================= > > > > As this is the way we're starting document titles, after converted to > > ReST. So, better to have it already using the right format, as one day > > I did this. > > > There are also a couple of things here that Sphinx would complain. > > The only complaint I got was > > WARNING: document isn't included in any toctree > > which makes sense because I renamed it only temporarily to *.rst Yeah, that is expected. > > So, it could be worth to rename it to *.rst, while you're writing > > it, and see what: > > make htmldocs > > will complain and how it will look in html. > > So, no complaints from Sphinx and the HTML output looks good IMO. Was > there anything specific you had in mind when saying that Sphinx would > complain? Perhaps my comments weren't clear enough. Sorry! I didn't actually tried to parse it with Sphinx. Just wanted to hint you about that, as testing the docs with Sphinx could be useful when writing documentation. Usually, things like function declarations produce warnings if they contain pointers, e. g. something like: foo(void *bar); as asterisks mean italics. It would complain about the lack of an end asterisk. In order to avoid that, and to place them into a box using monotonic fonts, I usually add "::" at the preceding line, e. g.: :: foo(void *bar); or: some description:: foo(void *bar) on all functions (even the ones that don't use asterisks, as the html output looks nicer. I double-checked this patch: it doesn't contain anything that would cause warnings or parse errors. Still, I would prefer to use **not** instead of *not*, and would add the "::", but that's my personal taste. Thanks, Mauro [-- Attachment #2: Assinatura digital OpenPGP --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling 2017-09-20 18:22 ` Mauro Carvalho Chehab @ 2017-09-20 18:45 ` Wolfram Sang 0 siblings, 0 replies; 16+ messages in thread From: Wolfram Sang @ 2017-09-20 18:45 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, linux-kernel, linux-iio, linux-input, linux-media, dri-devel [-- Attachment #1: Type: text/plain, Size: 236 bytes --] > In order to avoid that, and to place them into a box using monotonic fonts, > I usually add "::" at the preceding line, e. g.: Just in time: I added the '::' and will resubmit the new version in a minute. Thanks for the pointers! [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH v4 4/6] i2c: sh_mobile: use helper to decide if DMA is useful 2017-08-17 14:14 [RFC PATCH v4 0/6] i2c: document DMA handling and add helpers for it Wolfram Sang ` (2 preceding siblings ...) 2017-08-17 14:14 ` [RFC PATCH v4 3/6] i2c: add docs to clarify " Wolfram Sang @ 2017-08-17 14:14 ` Wolfram Sang 2017-08-17 14:14 ` [RFC PATCH v4 5/6] i2c: rcar: skip DMA if buffer is not safe Wolfram Sang ` (2 subsequent siblings) 6 siblings, 0 replies; 16+ messages in thread From: Wolfram Sang @ 2017-08-17 14:14 UTC (permalink / raw) To: linux-i2c Cc: linux-renesas-soc, linux-kernel, linux-iio, linux-input, linux-media, dri-devel, Wolfram Sang This ensures that we fall back to PIO if the message length is too small for DMA being useful. Otherwise, we use DMA. A bounce buffer might be applied by the helper if the original message buffer is not DMA safe. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/busses/i2c-sh_mobile.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c index 2e097d97d258bc..5efdb7becd83d6 100644 --- a/drivers/i2c/busses/i2c-sh_mobile.c +++ b/drivers/i2c/busses/i2c-sh_mobile.c @@ -145,6 +145,7 @@ struct sh_mobile_i2c_data { struct dma_chan *dma_rx; struct scatterlist sg; enum dma_data_direction dma_direction; + u8 *dma_buf; }; struct sh_mobile_dt_config { @@ -548,6 +549,8 @@ static void sh_mobile_i2c_dma_callback(void *data) pd->pos = pd->msg->len; pd->stop_after_dma = true; + i2c_release_dma_safe_msg_buf(pd->msg, pd->dma_buf); + iic_set_clr(pd, ICIC, 0, ICIC_TDMAE | ICIC_RDMAE); } @@ -608,7 +611,7 @@ static void sh_mobile_i2c_xfer_dma(struct sh_mobile_i2c_data *pd) if (IS_ERR(chan)) return; - dma_addr = dma_map_single(chan->device->dev, pd->msg->buf, pd->msg->len, dir); + dma_addr = dma_map_single(chan->device->dev, pd->dma_buf, pd->msg->len, dir); if (dma_mapping_error(chan->device->dev, dma_addr)) { dev_dbg(pd->dev, "dma map failed, using PIO\n"); return; @@ -665,7 +668,8 @@ static int start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg, pd->pos = -1; pd->sr = 0; - if (pd->msg->len > 8) + pd->dma_buf = i2c_get_dma_safe_msg_buf(pd->msg, 8); + if (pd->dma_buf) sh_mobile_i2c_xfer_dma(pd); /* Enable all interrupts to begin with */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH v4 5/6] i2c: rcar: skip DMA if buffer is not safe 2017-08-17 14:14 [RFC PATCH v4 0/6] i2c: document DMA handling and add helpers for it Wolfram Sang ` (3 preceding siblings ...) 2017-08-17 14:14 ` [RFC PATCH v4 4/6] i2c: sh_mobile: use helper to decide if DMA is useful Wolfram Sang @ 2017-08-17 14:14 ` Wolfram Sang 2017-08-17 14:14 ` [RFC PATCH v4 6/6] i2c: dev: mark RDWR buffers as DMA_SAFE Wolfram Sang 2017-08-20 10:14 ` [RFC PATCH v4 0/6] i2c: document DMA handling and add helpers for it Jonathan Cameron 6 siblings, 0 replies; 16+ messages in thread From: Wolfram Sang @ 2017-08-17 14:14 UTC (permalink / raw) To: linux-i2c Cc: linux-renesas-soc, linux-kernel, linux-iio, linux-input, linux-media, dri-devel, Wolfram Sang This HW is prone to races, so it needs to setup new messages in irq context. That means we can't alloc bounce buffers if a message buffer is not DMA safe. So, in that case, simply fall back to PIO. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/busses/i2c-rcar.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index 93c1a54981df08..5654a7142bffec 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -359,7 +359,7 @@ static void rcar_i2c_dma(struct rcar_i2c_priv *priv) int len; /* Do not use DMA if it's not available or for messages < 8 bytes */ - if (IS_ERR(chan) || msg->len < 8) + if (IS_ERR(chan) || msg->len < 8 || !(msg->flags & I2C_M_DMA_SAFE)) return; if (read) { -- 2.11.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH v4 6/6] i2c: dev: mark RDWR buffers as DMA_SAFE 2017-08-17 14:14 [RFC PATCH v4 0/6] i2c: document DMA handling and add helpers for it Wolfram Sang ` (4 preceding siblings ...) 2017-08-17 14:14 ` [RFC PATCH v4 5/6] i2c: rcar: skip DMA if buffer is not safe Wolfram Sang @ 2017-08-17 14:14 ` Wolfram Sang 2017-08-20 10:14 ` [RFC PATCH v4 0/6] i2c: document DMA handling and add helpers for it Jonathan Cameron 6 siblings, 0 replies; 16+ messages in thread From: Wolfram Sang @ 2017-08-17 14:14 UTC (permalink / raw) To: linux-i2c Cc: linux-renesas-soc, linux-kernel, linux-iio, linux-input, linux-media, dri-devel, Wolfram Sang Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/i2c-dev.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c index 6f638bbc922db4..bbc7aadb4c899d 100644 --- a/drivers/i2c/i2c-dev.c +++ b/drivers/i2c/i2c-dev.c @@ -280,6 +280,8 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client, res = PTR_ERR(rdwr_pa[i].buf); break; } + /* memdup_user allocates with GFP_KERNEL, so DMA is ok */ + rdwr_pa[i].flags |= I2C_M_DMA_SAFE; /* * If the message length is received from the slave (similar -- 2.11.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v4 0/6] i2c: document DMA handling and add helpers for it 2017-08-17 14:14 [RFC PATCH v4 0/6] i2c: document DMA handling and add helpers for it Wolfram Sang ` (5 preceding siblings ...) 2017-08-17 14:14 ` [RFC PATCH v4 6/6] i2c: dev: mark RDWR buffers as DMA_SAFE Wolfram Sang @ 2017-08-20 10:14 ` Jonathan Cameron 6 siblings, 0 replies; 16+ messages in thread From: Jonathan Cameron @ 2017-08-20 10:14 UTC (permalink / raw) To: Wolfram Sang Cc: linux-i2c, linux-renesas-soc, linux-kernel, linux-iio, linux-input, linux-media, dri-devel On Thu, 17 Aug 2017 16:14:43 +0200 Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > So, after revisiting old mail threads, taking part in a similar discussion on > the USB list, and implementing a not-convincing solution before, here is what I > cooked up to document and ease DMA handling for I2C within Linux. Please have a > look at the documentation introduced in patch 3 for details. > > While the previous versions tried to magically apply bounce buffers when > needed, it became clear that detecting DMA safe buffers is too fragile. This > approach is now opt-in, a DMA_SAFE flag needs to be set on an i2c_msg. The > outcome so far is very convincing IMO. The core additions are simple and easy > to understand (makes me even think of inlining them again?). The driver changes > for the Renesas IP cores became easier to understand, too. While only a tad for > the i2c-sh_mobile driver, the situation became a LOT better for the i2c-rcar > driver. No more DMA disabling for the whole transfer in case of unsafe buffers, > we are back to per-msg handling. And the code fix is now an easy to understand > one line change. Yay! > > Of course, we must now whitelist DMA safe buffers. An example for I2C_RDWR case > is in this series. It makes the i2ctransfer utility have DMA_SAFE buffers, > which is nice for testing as i2cdump will (currently) not use DMA_SAFE buffers. > My plan is to add two new calls: i2c_master_{send|receive}_dma_safe which can > be used if DMA_SAFE buffers are provided. So, drivers can simply switch to > them. Also, the buffers used within i2c_smbus_xfer_emulated() need to be > converted to be DMA_SAFE which will cover a huge bunch of use cases. The rest > is then updating drivers which can be done when needed. > > As these conversions are not done yet, this patch series has RFC status. But I > already would like to get opinions on this approach, so I'll cc mailing lists > of the heavier I2C users. Please let me know what you think. > > All patches have been tested with a Renesas Salvator-X board (r8a7796/M3-W). > > The branch can be found here: > > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/i2c-core-dma-rfc-v4 > > And big kudos to Renesas Electronics for funding this work, thank you very much! All looks good to me. I should really set up some testing for this to see if it gains us much on the various platforms I use, but that will 'a while' so don't wait on me! This is particularly true as none of them have dma support in the master drivers yet. Thanks for you work on this and cool than Renesas are funding it! Jonathan > > Regards, > > Wolfram > > Changes since v3: > * completely redesigned > > Wolfram Sang (6): > i2c: add a message flag for DMA safe buffers > i2c: add helpers to ease DMA handling > i2c: add docs to clarify DMA handling > i2c: sh_mobile: use helper to decide if DMA is useful > i2c: rcar: skip DMA if buffer is not safe > i2c: dev: mark RDWR buffers as DMA_SAFE > > Documentation/i2c/DMA-considerations | 50 ++++++++++++++++++++++++++++++++++++ > drivers/i2c/busses/i2c-rcar.c | 2 +- > drivers/i2c/busses/i2c-sh_mobile.c | 8 ++++-- > drivers/i2c/i2c-core-base.c | 45 ++++++++++++++++++++++++++++++++ > drivers/i2c/i2c-dev.c | 2 ++ > include/linux/i2c.h | 3 +++ > include/uapi/linux/i2c.h | 3 +++ > 7 files changed, 110 insertions(+), 3 deletions(-) > create mode 100644 Documentation/i2c/DMA-considerations > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-09-20 18:45 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-17 14:14 [RFC PATCH v4 0/6] i2c: document DMA handling and add helpers for it Wolfram Sang 2017-08-17 14:14 ` [RFC PATCH v4 1/6] i2c: add a message flag for DMA safe buffers Wolfram Sang 2017-08-17 14:14 ` [RFC PATCH v4 2/6] i2c: add helpers to ease DMA handling Wolfram Sang 2017-08-17 14:14 ` [RFC PATCH v4 3/6] i2c: add docs to clarify " Wolfram Sang 2017-08-27 11:37 ` Mauro Carvalho Chehab 2017-09-08 8:56 ` Wolfram Sang 2017-09-08 11:08 ` Mauro Carvalho Chehab 2017-09-09 15:27 ` Wolfram Sang 2017-09-09 19:34 ` Mauro Carvalho Chehab 2017-09-20 17:18 ` Wolfram Sang 2017-09-20 18:22 ` Mauro Carvalho Chehab 2017-09-20 18:45 ` Wolfram Sang 2017-08-17 14:14 ` [RFC PATCH v4 4/6] i2c: sh_mobile: use helper to decide if DMA is useful Wolfram Sang 2017-08-17 14:14 ` [RFC PATCH v4 5/6] i2c: rcar: skip DMA if buffer is not safe Wolfram Sang 2017-08-17 14:14 ` [RFC PATCH v4 6/6] i2c: dev: mark RDWR buffers as DMA_SAFE Wolfram Sang 2017-08-20 10:14 ` [RFC PATCH v4 0/6] i2c: document DMA handling and add helpers for it Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).