Message-ID: <20170908085640.42wzzgd2s2roikyd@ninjato>
Date:   Fri, 8 Sep 2017 10:56:40 +0200
From:   Wolfram Sang <wsa@...-dreams.de>
To:     Mauro Carvalho Chehab <mchehab@...pensource.com>
Cc:     Wolfram Sang <wsa+renesas@...g-engineering.com>,
        linux-i2c@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-input@...r.kernel.org, linux-media@...r.kernel.org,
        dri-devel@...ts.freedesktop.org
Subject: Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling

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


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists