Received-SPF: pass (google.com: domain of linux-kernel+bounces-23301-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <bbd6e9d6f239efee8886e08f3c3493fc968e53ce.camel@crapouillou.net>
Subject: Re: [PATCH v5 0/8] iio: new DMABUF based API, v5
From: Paul Cercueil <paul@crapouillou.net>
To: Andrew Davis <afd@ti.com>, Jonathan Cameron <jic23@kernel.org>, 
	Lars-Peter Clausen
	 <lars@metafoo.de>, Sumit Semwal <sumit.semwal@linaro.org>, Christian
	=?ISO-8859-1?Q?K=F6nig?=
	 <christian.koenig@amd.com>, Vinod Koul <vkoul@kernel.org>, Jonathan Corbet
	 <corbet@lwn.net>
Cc: Michael Hennerich <Michael.Hennerich@analog.com>, 
 linux-doc@vger.kernel.org, linux-iio@vger.kernel.org, 
 linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, 
 linaro-mm-sig@lists.linaro.org, Nuno =?ISO-8859-1?Q?S=E1?=
 <noname.nuno@gmail.com>, dmaengine@vger.kernel.org,
 linux-media@vger.kernel.org
Date: Thu, 11 Jan 2024 10:20:06 +0100
In-Reply-To: <6ec8c7c4-588a-48b5-b0c5-56ca5216a757@ti.com>
References: <20231219175009.65482-1-paul@crapouillou.net>
	 <6ec8c7c4-588a-48b5-b0c5-56ca5216a757@ti.com>
Autocrypt: addr=paul@crapouillou.net; prefer-encrypt=mutual;
 keydata=mQENBF0KhcEBCADkfmrzdTOp/gFOMQX0QwKE2WgeCJiHPWkpEuPH81/HB2dpjPZNW03ZMLQfECbbaEkdbN4YnPfXgcc1uBe5mwOAPV1MBlaZcEt4M67iYQwSNrP7maPS3IaQJ18ES8JJ5Uf5UzFZaUawgH+oipYGW+v31cX6L3k+dGsPRM0Pyo0sQt52fsopNPZ9iag0iY7dGNuKenaEqkYNjwEgTtNz8dt6s3hMpHIKZFL3OhAGi88wF/21isv0zkF4J0wlf9gYUTEEY3Eulx80PTVqGIcHZzfavlWIdzhe+rxHTDGVwseR2Y1WjgFGQ2F+vXetAB8NEeygXee+i9nY5qt9c07m8mzjABEBAAG0JFBhdWwgQ2VyY3VlaWwgPHBhdWxAY3JhcG91aWxsb3UubmV0PokBTgQTAQoAOBYhBNdHYd8OeCBwpMuVxnPua9InSr1BBQJdCoXBAhsDBQsJCAcCBhUKCQgLAgQWAgMBAh4BAheAAAoJEHPua9InSr1BgvIH/0kLyrI3V0f33a6D3BJwc1grbygPVYGuC5l5eMnAI+rDmLR19E2yvibRpgUc87NmPEQPpbbtAZt8On/2WZoE5OIPdlId/AHNpdgAtGXo0ZX4LGeVPjxjdkbrKVHxbcdcnY+zzaFglpbVSvp76pxqgVg8PgxkAAeeJV+ET4t0823Gz2HzCL/6JZhvKAEtHVulOWoBh368SYdolp1TSfORWmHzvQiCCCA+j0cMkYVGzIQzEQhX7Urf9N/nhU5/SGLFEi9DcBfXoGzhyQyLXflhJtKm3XGB1K/pPulbKaPcKAl6rIDWPuFpHkSbmZ9r4KFlBwgAhlGy6nqP7O3u7q23hRW5AQ0EXQqFwQEIAMo+MgvYHsyjX3Ja4Oolg1Txzm8woj30ch2nACFCqaO0R/1kLj2VVeLrDyQUOlXx9PD6IQI4M8wy8m0sR4wV2p/g/paw7k65cjzYYLh+FdLNyO7IW
	YXndJO+wDPi3aK/YKUYepqlP+QsmaHNYNdXEQDRKqNfJg8t0f5rfzp9ryxd1tCnbV+tG8VHQWiZXNqN7062DygSNXFUfQ0vZ3J2D4oAcIAEXTymRQ2+hr3Hf7I61KMHWeSkCvCG2decTYsHlw5Erix/jYWqVOtX0roOOLqWkqpQQJWtU+biWrAksmFmCp5fXIg1Nlg39v21xCXBGxJkxyTYuhdWyu1yDQ+LSIUAEQEAAYkBNgQYAQoAIBYhBNdHYd8OeCBwpMuVxnPua9InSr1BBQJdCoXBAhsMAAoJEHPua9InSr1B4wsH/Az767YCT0FSsMNt1jkkdLCBi7nY0GTW+PLP1a4zvVqFMo/vD6uz1ZflVTUAEvcTi3VHYZrlgjcxmcGu239oruqUS8Qy/xgZBp9KF0NTWQSl1iBfVbIU5VV1vHS6r77W5x0qXgfvAUWOH4gmN3MnF01SH2zMcLiaUGF+mcwl15rHbjnT3Nu2399aSE6cep86igfCAyFUOXjYEGlJy+c6UyT+DUylpjQg0nl8MlZ/7Whg2fAU9+FALIbQYQzGlT4c71SibR9T741jnegHhlmV4WXXUD6roFt54t0MSAFSVxzG8mLcSjR2cLUJ3NIPXixYUSEn3tQhfZj07xIIjWxAYZo=
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0

Hi Andrew,

Le lundi 08 janvier 2024 =C3=A0 15:12 -0600, Andrew Davis a =C3=A9crit=C2=
=A0:
> On 12/19/23 11:50 AM, Paul Cercueil wrote:
> > [V4 was: "iio: Add buffer write() support"][1]
> >=20
> > Hi Jonathan,
> >=20
> > This is a respin of the V3 of my patchset that introduced a new
> > interface based on DMABUF objects [2].
> >=20
> > The V4 was a split of the patchset, to attempt to upstream buffer
> > write() support first. But since there is no current user upstream,
> > it
> > was not merged. This V5 is about doing the opposite, and contains
> > the
> > new DMABUF interface, without adding the buffer write() support. It
> > can
> > already be used with the upstream adi-axi-adc driver.
> >=20
> > In user-space, Libiio uses it to transfer back and forth blocks of
> > samples between the hardware and the applications, without having
> > to
> > copy the data.
> >=20
> > On a ZCU102 with a FMComms3 daughter board, running Libiio from the
> > pcercuei/dev-new-dmabuf-api branch [3], compiled with
> > WITH_LOCAL_DMABUF_API=3DOFF (so that it uses fileio):
> > =C2=A0=C2=A0 sudo utils/iio_rwdev -b 4096 -B cf-ad9361-lpc
> > =C2=A0=C2=A0 Throughput: 116 MiB/s
> >=20
> > Same hardware, with the DMABUF API (WITH_LOCAL_DMABUF_API=3DON):
> > =C2=A0=C2=A0 sudo utils/iio_rwdev -b 4096 -B cf-ad9361-lpc
> > =C2=A0=C2=A0 Throughput: 475 MiB/s
> >=20
> > This benchmark only measures the speed at which the data can be
> > fetched
> > to iio_rwdev's internal buffers, and does not actually try to read
> > the
> > data (e.g. to pipe it to stdout). It shows that fetching the data
> > is
> > more than 4x faster using the new interface.
> >=20
> > When actually reading the data, the performance difference isn't
> > that
> > impressive (maybe because in case of DMABUF the data is not in
> > cache):
> >=20
> > WITH_LOCAL_DMABUF_API=3DOFF (so that it uses fileio):
> > =C2=A0=C2=A0 sudo utils/iio_rwdev -b 4096 cf-ad9361-lpc | dd of=3D/dev/=
zero
> > status=3Dprogress
> > =C2=A0=C2=A0 2446422528 bytes (2.4 GB, 2.3 GiB) copied, 22 s, 111 MB/s
> >=20
> > WITH_LOCAL_DMABUF_API=3DON:
> > =C2=A0=C2=A0 sudo utils/iio_rwdev -b 4096 cf-ad9361-lpc | dd of=3D/dev/=
zero
> > status=3Dprogress
> > =C2=A0=C2=A0 2334388736 bytes (2.3 GB, 2.2 GiB) copied, 21 s, 114 MB/s
> >=20
> > One interesting thing to note is that fileio is (currently)
> > actually
> > faster than the DMABUF interface if you increase a lot the buffer
> > size.
> > My explanation is that the cache invalidation routine takes more
> > and
> > more time the bigger the DMABUF gets. This is because the DMABUF is
> > backed by small-size pages, so a (e.g.) 64 MiB DMABUF is backed by
> > up
> > to 16 thousands pages, that have to be invalidated one by one. This
> > can
> > be addressed by using huge pages, but the udmabuf driver does not
> > (yet)
> > support creating DMABUFs backed by huge pages.
> >=20
>=20
> Have you tried DMABUFs created using the DMABUF System heap exporter?
> (drivers/dma-buf/heaps/system_heap.c) It should be able to handle
> larger allocation better here, and if you don't have any active
> mmaps or vmaps then it can skip CPU-side coherency maintenance
> (useful for device to device transfers).

I didn't know about it!

But udmabuf also allows you to skip CPU-side coherency maintenance,
since DMABUFs have two ioctls to start/finish CPU access anyway.

> Allocating DMABUFs out of user pages has a bunch of other issues you
> might run into also. I'd argue udmabuf is now completely superseded
> by DMABUF system heaps. Try it out :)

I'm curious, what other issues?

The good thing about udmabuf is that the memory is backed by pages, so
we can use MSG_ZEROCOPY on sockets to transfer the mmapped data over
the network (having a DMABUF interface to the network stack would be
better, but I'm not opening that can of worms).

> Andrew

Cheers,
-Paul

> > Anyway, the real benefits happen when the DMABUFs are either shared
> > between IIO devices, or between the IIO subsystem and another
> > filesystem. In that case, the DMABUFs are simply passed around
> > drivers,
> > without the data being copied at any moment.
> >=20
> > We use that feature to transfer samples from our transceivers to
> > USB,
> > using a DMABUF interface to FunctionFS [4].
> >=20
> > This drastically increases the throughput, to about 274 MiB/s over
> > a
> > USB3 link, vs. 127 MiB/s using IIO's fileio interface + write() to
> > the
> > FunctionFS endpoints, for a lower CPU usage (0.85 vs. 0.65 load
> > avg.).
> >=20
> > Based on linux-next/next-20231219.
> >=20
> > Cheers,
> > -Paul
> >=20
> > [1]
> > https://lore.kernel.org/all/20230807112113.47157-1-paul@crapouillou.net=
/
> > [2]
> > https://lore.kernel.org/all/20230403154800.215924-1-paul@crapouillou.ne=
t/
> > [3]
> > https://github.com/analogdevicesinc/libiio/tree/pcercuei/dev-new-dmabuf=
-api
> > [4]
> > https://lore.kernel.org/all/20230322092118.9213-1-paul@crapouillou.net/
> >=20
> > ---
> > Changelog:
> > - [3/8]: Replace V3's dmaengine_prep_slave_dma_array() with a new
> > =C2=A0=C2=A0 dmaengine_prep_slave_dma_vec(), which uses a new 'dma_vec'
> > struct.
> > =C2=A0=C2=A0 Note that at some point we will need to support cyclic tra=
nsfers
> > =C2=A0=C2=A0 using dmaengine_prep_slave_dma_vec(). Maybe with a new "fl=
ags"
> > =C2=A0=C2=A0 parameter to the function?
> >=20
> > - [4/8]: Implement .device_prep_slave_dma_vec() instead of V3's
> > =C2=A0=C2=A0 .device_prep_slave_dma_array().
> >=20
> > =C2=A0=C2=A0 @Vinod: this patch will cause a small conflict with my oth=
er
> > =C2=A0=C2=A0 patchset adding scatter-gather support to the axi-dmac dri=
ver.
> > =C2=A0=C2=A0 This patch adds a call to axi_dmac_alloc_desc(num_sgs), bu=
t the
> > =C2=A0=C2=A0 prototype of this function changed in my other patchset - =
it
> > would
> > =C2=A0=C2=A0 have to be passed the "chan" variable. I don't know how yo=
u
> > prefer it
> > =C2=A0=C2=A0 to be resolved. Worst case scenario (and if @Jonathan is o=
kay
> > with
> > =C2=A0=C2=A0 that) this one patch can be re-sent later, but it would ma=
ke
> > this
> > =C2=A0=C2=A0 patchset less "atomic".
> >=20
> > - [5/8]:
> > =C2=A0=C2=A0 - Use dev_err() instead of pr_err()
> > =C2=A0=C2=A0 - Inline to_iio_dma_fence()
> > =C2=A0=C2=A0 - Add comment to explain why we unref twice when detaching
> > dmabuf
> > =C2=A0=C2=A0 - Remove TODO comment. It is actually safe to free the fil=
e's
> > =C2=A0=C2=A0=C2=A0=C2=A0 private data even when transfers are still pen=
ding because it
> > =C2=A0=C2=A0=C2=A0=C2=A0 won't be accessed.
> > =C2=A0=C2=A0 - Fix documentation of new fields in struct
> > iio_buffer_access_funcs
> > =C2=A0=C2=A0 - iio_dma_resv_lock() does not need to be exported, make i=
t
> > static
> >=20
> > - [7/8]:
> > =C2=A0=C2=A0 - Use the new dmaengine_prep_slave_dma_vec().
> > =C2=A0=C2=A0 - Restrict to input buffers, since output buffers are not =
yet
> > =C2=A0=C2=A0=C2=A0=C2=A0 supported by IIO buffers.
> >=20
> > - [8/8]:
> > =C2=A0=C2=A0 Use description lists for the documentation of the three n=
ew
> > IOCTLs
> > =C2=A0=C2=A0 instead of abusing subsections.
> >=20
> > ---
> > Alexandru Ardelean (1):
> > =C2=A0=C2=A0 iio: buffer-dma: split iio_dma_buffer_fileio_free() functi=
on
> >=20
> > Paul Cercueil (7):
> > =C2=A0=C2=A0 iio: buffer-dma: Get rid of outgoing queue
> > =C2=A0=C2=A0 dmaengine: Add API function dmaengine_prep_slave_dma_vec()
> > =C2=A0=C2=A0 dmaengine: dma-axi-dmac: Implement device_prep_slave_dma_v=
ec
> > =C2=A0=C2=A0 iio: core: Add new DMABUF interface infrastructure
> > =C2=A0=C2=A0 iio: buffer-dma: Enable support for DMABUFs
> > =C2=A0=C2=A0 iio: buffer-dmaengine: Support new DMABUF based userspace =
API
> > =C2=A0=C2=A0 Documentation: iio: Document high-speed DMABUF based API
> >=20
> > =C2=A0 Documentation/iio/dmabuf_api.rst=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 54 +++
> > =C2=A0 Documentation/iio/index.rst=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=
=C2=A0=C2=A0 2 +
> > =C2=A0 drivers/dma/dma-axi-dmac.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0 |=C2=A0 40 ++
> > =C2=A0 drivers/iio/buffer/industrialio-buffer-dma.c=C2=A0 | 242 +++++++=
+---
> > =C2=A0 .../buffer/industrialio-buffer-dmaengine.c=C2=A0=C2=A0=C2=A0 |=
=C2=A0 52 ++-
> > =C2=A0 drivers/iio/industrialio-buffer.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | 402
> > ++++++++++++++++++
> > =C2=A0 include/linux/dmaengine.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 |=C2=A0 25 ++
> > =C2=A0 include/linux/iio/buffer-dma.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 33 +-
> > =C2=A0 include/linux/iio/buffer_impl.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 26 ++
> > =C2=A0 include/uapi/linux/iio/buffer.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 22 +
> > =C2=A0 10 files changed, 836 insertions(+), 62 deletions(-)
> > =C2=A0 create mode 100644 Documentation/iio/dmabuf_api.rst
> >=20