| Message ID | 20170606092034.1516-3-wsa+renesas@sang-engineering.com |
|---|---|
| State | RFC |
| Headers | show |
Hi Wolfram, On Tue, Jun 6, 2017 at 11:20 AM, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > --- /dev/null > +++ b/Documentation/i2c/DMA-considerations > @@ -0,0 +1,28 @@ > +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 (FIXME: > 8 byte?). So you expect drivers to fall back to PIO automatically if the buffer is not DMA safe. Sounds good to me. > +To support this, drivers wishing to implement DMA can use a helper function > +checking if the size is suitable for DMA or if the buffer is DMA capable: > + > + int i2c_check_msg_for_dma(msg, dma_threshold); > + > +Please check its in kernel documentation for details. > + > +It should be further noted that bounce buffer handling is left to be handled on > +driver level because details like alignment requirements are best known on that > +level. > + > +If you plan to use DMA with I2C (or with any other bus, 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. However, your check for a DMA-capable buffer is invoked only if CONFIG_DMA_API_DEBUG is enabled: #if !defined(CONFIG_DMA_API_DEBUG) if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) { pr_debug("msg buffer to 0x%04x might not be DMA capable\n", msg->addr); return -EFAULT; } #endif So the system will work fine if CONFIG_DMA_API_DEBUG is enabled, and may fail miserably in a production kernel? Furthermore, pr_debug() messages are not printed by default, so the developer who did enable CONFIG_DMA_API_DEBUG may not have noticed at all? If you want to use i2c_check_msg_for_dma() as a generic helper to verify DMA requirements, and decide when to fall back to PIO, I think it should always do the buffer check. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > +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 (FIXME: > 8 byte?). > > So you expect drivers to fall back to PIO automatically if the buffer is > not DMA safe. Sounds good to me. Yes, I strongly recommend that. Otherwise, drivers can always deal with bounce buffers on their own. > However, your check for a DMA-capable buffer is invoked only if > CONFIG_DMA_API_DEBUG is enabled: is *NOT* enabled! > > #if !defined(CONFIG_DMA_API_DEBUG) > if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) { > pr_debug("msg buffer to 0x%04x might not be DMA capable\n", > msg->addr); > return -EFAULT; > } > #endif > The #if block is there because DMA_API_DEBUG does a lot more, but if the check in the helper is enabled, the core will fall back to PIO and you won't get the additional info from DMA_API_DEBUG. I think this needs a comment :) Now OK? Regards, Wolfram
Hi Wolfram, On Tue, Jun 6, 2017 at 12:24 PM, Wolfram Sang <wsa@the-dreams.de> wrote: >> > +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 (FIXME: > 8 byte?). >> >> So you expect drivers to fall back to PIO automatically if the buffer is >> not DMA safe. Sounds good to me. > > Yes, I strongly recommend that. Otherwise, drivers can always deal with > bounce buffers on their own. > >> However, your check for a DMA-capable buffer is invoked only if >> CONFIG_DMA_API_DEBUG is enabled: > > is *NOT* enabled! Oops ;-) >> #if !defined(CONFIG_DMA_API_DEBUG) >> if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) { >> pr_debug("msg buffer to 0x%04x might not be DMA capable\n", >> msg->addr); >> return -EFAULT; >> } >> #endif >> > > The #if block is there because DMA_API_DEBUG does a lot more, but if the > check in the helper is enabled, the core will fall back to PIO and you > won't get the additional info from DMA_API_DEBUG. > > I think this needs a comment :) > > Now OK? So it won't fall back to PIO if CONFIG_DMA_API_DEBUG is enabled? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> So it won't fall back to PIO if CONFIG_DMA_API_DEBUG is enabled?
No, it will complain loudly with more details about the same things (and
some more). But for that to do, it cannot fall back to PIO.
Otherwise, you might get the idea that all is fine since you enabled
CONFIG_DMA_API_DEBUG but miss that most of you transfers fell back to
PIO. That was my thinking...
Hi Wolfram, On Tue, Jun 6, 2017 at 12:54 PM, Wolfram Sang <wsa@the-dreams.de> wrote: >> So it won't fall back to PIO if CONFIG_DMA_API_DEBUG is enabled? > > No, it will complain loudly with more details about the same things (and > some more). But for that to do, it cannot fall back to PIO. > > Otherwise, you might get the idea that all is fine since you enabled > CONFIG_DMA_API_DEBUG but miss that most of you transfers fell back to > PIO. That was my thinking... Hmm.... I have it enabled all the time. So my devices may stop working... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I have it enabled all the time. So my devices may stop working...
If you have enabled it all the time, then there won't be a difference
from now. Or?
That also shows how rarely I2C DMA transfers are triggered from within
the kernel :)
Hi Wolfram, On Tue, Jun 6, 2017 at 1:23 PM, Wolfram Sang <wsa@the-dreams.de> wrote: >> I have it enabled all the time. So my devices may stop working... > > If you have enabled it all the time, then there won't be a difference > from now. Or? > > That also shows how rarely I2C DMA transfers are triggered from within > the kernel :) ... how rarely i2c DMA transfers from possibly unsafe buffers... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/i2c/DMA-considerations b/Documentation/i2c/DMA-considerations new file mode 100644 index 00000000000000..945ac5cfe55c12 --- /dev/null +++ b/Documentation/i2c/DMA-considerations @@ -0,0 +1,28 @@ +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 (FIXME: > 8 byte?). + +To support this, drivers wishing to implement DMA can use a helper function +checking if the size is suitable for DMA or if the buffer is DMA capable: + + int i2c_check_msg_for_dma(msg, dma_threshold); + +Please check its in kernel documentation for details. + +It should be further noted that bounce buffer handling is left to be handled on +driver level because details like alignment requirements are best known on that +level. + +If you plan to use DMA with I2C (or with any other bus, 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.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- Documentation/i2c/DMA-considerations | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 Documentation/i2c/DMA-considerations