| Message ID | 20211115141925.60164-1-paul@crapouillou.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: buffer-dma: write() and new DMABUF based API | expand |
On Mon, Nov 15, 2021 at 02:19:10PM +0000, Paul Cercueil wrote: > Hi Jonathan, > > This patchset introduces a new userspace interface based on DMABUF > objects, to complement the existing fileio based API. > > The advantage of this DMABUF based interface vs. the fileio > interface, is that it avoids an extra copy of the data between the > kernel and userspace. This is particularly userful for high-speed > devices which produce several megabytes or even gigabytes of data per > second. > > The first few patches [01/15] to [03/15] are not really related, but > allow to reduce the size of the patches that introduce the new API. > > Patch [04/15] to [06/15] enables write() support to the buffer-dma > implementation of the buffer API, to continue the work done by > Mihail Chindris. > > Patches [07/15] to [12/15] introduce the new DMABUF based API. > > Patches [13/15] and [14/15] add support for cyclic buffers, only through > the new API. A cyclic buffer will be repeated on the output until the > buffer is disabled. > > Patch [15/15] adds documentation about the new API. > > For now, the API allows you to alloc DMABUF objects and mmap() them to > read or write the samples. It does not yet allow to import DMABUFs > parented to other subsystems, but that should eventually be possible > once it's wired. > > This patchset is inspired by the "mmap interface" that was previously > submitted by Alexandru Ardelean and Lars-Peter Clausen, so it would be > great if I could get a review from you guys. Alexandru's commit was > signed with his @analog.com address but he doesn't work at ADI anymore, > so I believe I'll need him to sign with a new email. Why dma-buf? dma-buf looks like something super generic and useful, until you realize that there's a metric ton of gpu/accelerator bagage piled in. So unless buffer sharing with a gpu/video/accel/whatever device is the goal here, and it's just for a convenient way to get at buffer handles, this doesn't sound like a good idea. Also if the idea is to this with gpus/accelerators then I'd really like to see the full thing, since most likely at that point you also want dma_fence. And once we talk dma_fence things get truly horrible from a locking pov :-( Or well, just highly constrained and I get to review what iio is doing with these buffers to make sure it all fits. Cheers, Daniel > > Cheers, > -Paul > > Alexandru Ardelean (1): > iio: buffer-dma: split iio_dma_buffer_fileio_free() function > > Paul Cercueil (14): > iio: buffer-dma: Get rid of incoming/outgoing queues > iio: buffer-dma: Remove unused iio_buffer_block struct > iio: buffer-dma: Use round_down() instead of rounddown() > iio: buffer-dma: Enable buffer write support > iio: buffer-dmaengine: Support specifying buffer direction > iio: buffer-dmaengine: Enable write support > iio: core: Add new DMABUF interface infrastructure > iio: buffer-dma: Use DMABUFs instead of custom solution > iio: buffer-dma: Implement new DMABUF based userspace API > iio: buffer-dma: Boost performance using write-combine cache setting > iio: buffer-dmaengine: Support new DMABUF based userspace API > iio: core: Add support for cyclic buffers > iio: buffer-dmaengine: Add support for cyclic buffers > Documentation: iio: Document high-speed DMABUF based API > > Documentation/driver-api/dma-buf.rst | 2 + > Documentation/iio/dmabuf_api.rst | 94 +++ > Documentation/iio/index.rst | 2 + > drivers/iio/adc/adi-axi-adc.c | 3 +- > drivers/iio/buffer/industrialio-buffer-dma.c | 670 ++++++++++++++---- > .../buffer/industrialio-buffer-dmaengine.c | 42 +- > drivers/iio/industrialio-buffer.c | 49 ++ > include/linux/iio/buffer-dma.h | 43 +- > include/linux/iio/buffer-dmaengine.h | 5 +- > include/linux/iio/buffer_impl.h | 8 + > include/uapi/linux/iio/buffer.h | 30 + > 11 files changed, 783 insertions(+), 165 deletions(-) > create mode 100644 Documentation/iio/dmabuf_api.rst > > -- > 2.33.0 >
Hi Daniel, Le lun., nov. 15 2021 at 15:37:16 +0100, Daniel Vetter <daniel@ffwll.ch> a écrit : > On Mon, Nov 15, 2021 at 02:19:10PM +0000, Paul Cercueil wrote: >> Hi Jonathan, >> >> This patchset introduces a new userspace interface based on DMABUF >> objects, to complement the existing fileio based API. >> >> The advantage of this DMABUF based interface vs. the fileio >> interface, is that it avoids an extra copy of the data between the >> kernel and userspace. This is particularly userful for high-speed >> devices which produce several megabytes or even gigabytes of data >> per >> second. >> >> The first few patches [01/15] to [03/15] are not really related, but >> allow to reduce the size of the patches that introduce the new API. >> >> Patch [04/15] to [06/15] enables write() support to the buffer-dma >> implementation of the buffer API, to continue the work done by >> Mihail Chindris. >> >> Patches [07/15] to [12/15] introduce the new DMABUF based API. >> >> Patches [13/15] and [14/15] add support for cyclic buffers, only >> through >> the new API. A cyclic buffer will be repeated on the output until >> the >> buffer is disabled. >> >> Patch [15/15] adds documentation about the new API. >> >> For now, the API allows you to alloc DMABUF objects and mmap() them >> to >> read or write the samples. It does not yet allow to import DMABUFs >> parented to other subsystems, but that should eventually be possible >> once it's wired. >> >> This patchset is inspired by the "mmap interface" that was >> previously >> submitted by Alexandru Ardelean and Lars-Peter Clausen, so it would >> be >> great if I could get a review from you guys. Alexandru's commit was >> signed with his @analog.com address but he doesn't work at ADI >> anymore, >> so I believe I'll need him to sign with a new email. > > Why dma-buf? dma-buf looks like something super generic and useful, > until > you realize that there's a metric ton of gpu/accelerator bagage piled > in. > So unless buffer sharing with a gpu/video/accel/whatever device is the > goal here, and it's just for a convenient way to get at buffer > handles, > this doesn't sound like a good idea. Good question. The first reason is that a somewhat similar API was intented before[1], but refused upstream as it was kind of re-inventing the wheel. The second reason, is that we want to be able to share buffers too, not with gpu/video but with the network (zctap) and in the future with USB (functionFS) too. [1]: https://lore.kernel.org/linux-iio/20210217073638.21681-1-alexandru.ardelean@analog.com/T/ > Also if the idea is to this with gpus/accelerators then I'd really > like to > see the full thing, since most likely at that point you also want > dma_fence. And once we talk dma_fence things get truly horrible from a > locking pov :-( Or well, just highly constrained and I get to review > what > iio is doing with these buffers to make sure it all fits. There is some dma_fence action in patch #10, which is enough for the userspace apps to use the API. What "horribleness" are we talking about here? It doesn't look that scary to me, but I certainly don't have the complete picture. Cheers, -Paul > Cheers, Daniel > >> >> Cheers, >> -Paul >> >> Alexandru Ardelean (1): >> iio: buffer-dma: split iio_dma_buffer_fileio_free() function >> >> Paul Cercueil (14): >> iio: buffer-dma: Get rid of incoming/outgoing queues >> iio: buffer-dma: Remove unused iio_buffer_block struct >> iio: buffer-dma: Use round_down() instead of rounddown() >> iio: buffer-dma: Enable buffer write support >> iio: buffer-dmaengine: Support specifying buffer direction >> iio: buffer-dmaengine: Enable write support >> iio: core: Add new DMABUF interface infrastructure >> iio: buffer-dma: Use DMABUFs instead of custom solution >> iio: buffer-dma: Implement new DMABUF based userspace API >> iio: buffer-dma: Boost performance using write-combine cache >> setting >> iio: buffer-dmaengine: Support new DMABUF based userspace API >> iio: core: Add support for cyclic buffers >> iio: buffer-dmaengine: Add support for cyclic buffers >> Documentation: iio: Document high-speed DMABUF based API >> >> Documentation/driver-api/dma-buf.rst | 2 + >> Documentation/iio/dmabuf_api.rst | 94 +++ >> Documentation/iio/index.rst | 2 + >> drivers/iio/adc/adi-axi-adc.c | 3 +- >> drivers/iio/buffer/industrialio-buffer-dma.c | 670 >> ++++++++++++++---- >> .../buffer/industrialio-buffer-dmaengine.c | 42 +- >> drivers/iio/industrialio-buffer.c | 49 ++ >> include/linux/iio/buffer-dma.h | 43 +- >> include/linux/iio/buffer-dmaengine.h | 5 +- >> include/linux/iio/buffer_impl.h | 8 + >> include/uapi/linux/iio/buffer.h | 30 + >> 11 files changed, 783 insertions(+), 165 deletions(-) >> create mode 100644 Documentation/iio/dmabuf_api.rst >> >> -- >> 2.33.0 >> > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Mon, Nov 15, 2021 at 02:57:37PM +0000, Paul Cercueil wrote: > Hi Daniel, > > Le lun., nov. 15 2021 at 15:37:16 +0100, Daniel Vetter <daniel@ffwll.ch> a > écrit : > > On Mon, Nov 15, 2021 at 02:19:10PM +0000, Paul Cercueil wrote: > > > Hi Jonathan, > > > > > > This patchset introduces a new userspace interface based on DMABUF > > > objects, to complement the existing fileio based API. > > > > > > The advantage of this DMABUF based interface vs. the fileio > > > interface, is that it avoids an extra copy of the data between the > > > kernel and userspace. This is particularly userful for high-speed > > > devices which produce several megabytes or even gigabytes of data > > > per > > > second. > > > > > > The first few patches [01/15] to [03/15] are not really related, but > > > allow to reduce the size of the patches that introduce the new API. > > > > > > Patch [04/15] to [06/15] enables write() support to the buffer-dma > > > implementation of the buffer API, to continue the work done by > > > Mihail Chindris. > > > > > > Patches [07/15] to [12/15] introduce the new DMABUF based API. > > > > > > Patches [13/15] and [14/15] add support for cyclic buffers, only > > > through > > > the new API. A cyclic buffer will be repeated on the output until > > > the > > > buffer is disabled. > > > > > > Patch [15/15] adds documentation about the new API. > > > > > > For now, the API allows you to alloc DMABUF objects and mmap() them > > > to > > > read or write the samples. It does not yet allow to import DMABUFs > > > parented to other subsystems, but that should eventually be possible > > > once it's wired. > > > > > > This patchset is inspired by the "mmap interface" that was > > > previously > > > submitted by Alexandru Ardelean and Lars-Peter Clausen, so it would > > > be > > > great if I could get a review from you guys. Alexandru's commit was > > > signed with his @analog.com address but he doesn't work at ADI > > > anymore, > > > so I believe I'll need him to sign with a new email. > > > > Why dma-buf? dma-buf looks like something super generic and useful, > > until > > you realize that there's a metric ton of gpu/accelerator bagage piled > > in. > > So unless buffer sharing with a gpu/video/accel/whatever device is the > > goal here, and it's just for a convenient way to get at buffer handles, > > this doesn't sound like a good idea. > > Good question. The first reason is that a somewhat similar API was intented > before[1], but refused upstream as it was kind of re-inventing the wheel. > > The second reason, is that we want to be able to share buffers too, not with > gpu/video but with the network (zctap) and in the future with USB > (functionFS) too. > > [1]: https://lore.kernel.org/linux-iio/20210217073638.21681-1-alexandru.ardelean@analog.com/T/ Hm is that code merged already in upstream already? I know that dma-buf looks really generic, but like I said if there's no need ever to interface with any of the gpu buffer sharing it might be better to use something else (like get_user_pages maybe, would that work?). > > Also if the idea is to this with gpus/accelerators then I'd really like > > to > > see the full thing, since most likely at that point you also want > > dma_fence. And once we talk dma_fence things get truly horrible from a > > locking pov :-( Or well, just highly constrained and I get to review > > what > > iio is doing with these buffers to make sure it all fits. > > There is some dma_fence action in patch #10, which is enough for the > userspace apps to use the API. > > What "horribleness" are we talking about here? It doesn't look that scary to > me, but I certainly don't have the complete picture. You need to annotate all the code involved in signalling that dma_fence using dma_fence_begin/end_signalling, and then enable full lockdep and everything. You can safely assume you'll find bugs, because we even have bugs about this in gpu drivers (where that annotation isn't fully rolled out yet). The tldr is that you can allocate memory in there. And a pile of other restrictions, but not being able to allocate memory (well GFP_ATOMIC is ok, but that can fail) is a very serious restriction. -Daniel > > Cheers, > -Paul > > > Cheers, Daniel > > > > > > > > Cheers, > > > -Paul > > > > > > Alexandru Ardelean (1): > > > iio: buffer-dma: split iio_dma_buffer_fileio_free() function > > > > > > Paul Cercueil (14): > > > iio: buffer-dma: Get rid of incoming/outgoing queues > > > iio: buffer-dma: Remove unused iio_buffer_block struct > > > iio: buffer-dma: Use round_down() instead of rounddown() > > > iio: buffer-dma: Enable buffer write support > > > iio: buffer-dmaengine: Support specifying buffer direction > > > iio: buffer-dmaengine: Enable write support > > > iio: core: Add new DMABUF interface infrastructure > > > iio: buffer-dma: Use DMABUFs instead of custom solution > > > iio: buffer-dma: Implement new DMABUF based userspace API > > > iio: buffer-dma: Boost performance using write-combine cache > > > setting > > > iio: buffer-dmaengine: Support new DMABUF based userspace API > > > iio: core: Add support for cyclic buffers > > > iio: buffer-dmaengine: Add support for cyclic buffers > > > Documentation: iio: Document high-speed DMABUF based API > > > > > > Documentation/driver-api/dma-buf.rst | 2 + > > > Documentation/iio/dmabuf_api.rst | 94 +++ > > > Documentation/iio/index.rst | 2 + > > > drivers/iio/adc/adi-axi-adc.c | 3 +- > > > drivers/iio/buffer/industrialio-buffer-dma.c | 670 > > > ++++++++++++++---- > > > .../buffer/industrialio-buffer-dmaengine.c | 42 +- > > > drivers/iio/industrialio-buffer.c | 49 ++ > > > include/linux/iio/buffer-dma.h | 43 +- > > > include/linux/iio/buffer-dmaengine.h | 5 +- > > > include/linux/iio/buffer_impl.h | 8 + > > > include/uapi/linux/iio/buffer.h | 30 + > > > 11 files changed, 783 insertions(+), 165 deletions(-) > > > create mode 100644 Documentation/iio/dmabuf_api.rst > > > > > > -- > > > 2.33.0 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > >
On Tue, Nov 16, 2021 at 05:02:25PM +0100, Daniel Vetter wrote: > On Mon, Nov 15, 2021 at 02:57:37PM +0000, Paul Cercueil wrote: > > Le lun., nov. 15 2021 at 15:37:16 +0100, Daniel Vetter a écrit : > > > On Mon, Nov 15, 2021 at 02:19:10PM +0000, Paul Cercueil wrote: > > > > Hi Jonathan, > > > > > > > > This patchset introduces a new userspace interface based on DMABUF > > > > objects, to complement the existing fileio based API. > > > > > > > > The advantage of this DMABUF based interface vs. the fileio > > > > interface, is that it avoids an extra copy of the data between the > > > > kernel and userspace. This is particularly userful for high-speed > > > > devices which produce several megabytes or even gigabytes of data per > > > > second. > > > > > > > > The first few patches [01/15] to [03/15] are not really related, but > > > > allow to reduce the size of the patches that introduce the new API. > > > > > > > > Patch [04/15] to [06/15] enables write() support to the buffer-dma > > > > implementation of the buffer API, to continue the work done by > > > > Mihail Chindris. > > > > > > > > Patches [07/15] to [12/15] introduce the new DMABUF based API. > > > > > > > > Patches [13/15] and [14/15] add support for cyclic buffers, only through > > > > the new API. A cyclic buffer will be repeated on the output until the > > > > buffer is disabled. > > > > > > > > Patch [15/15] adds documentation about the new API. > > > > > > > > For now, the API allows you to alloc DMABUF objects and mmap() them > > > > to > > > > read or write the samples. It does not yet allow to import DMABUFs > > > > parented to other subsystems, but that should eventually be possible > > > > once it's wired. > > > > > > > > This patchset is inspired by the "mmap interface" that was previously > > > > submitted by Alexandru Ardelean and Lars-Peter Clausen, so it would be > > > > great if I could get a review from you guys. Alexandru's commit was > > > > signed with his @analog.com address but he doesn't work at ADI anymore, > > > > so I believe I'll need him to sign with a new email. > > > > > > Why dma-buf? dma-buf looks like something super generic and useful, until > > > you realize that there's a metric ton of gpu/accelerator bagage piled in. > > > So unless buffer sharing with a gpu/video/accel/whatever device is the And cameras (maybe they're included in "whatever" ?). > > > goal here, and it's just for a convenient way to get at buffer handles, > > > this doesn't sound like a good idea. > > > > Good question. The first reason is that a somewhat similar API was intented > > before[1], but refused upstream as it was kind of re-inventing the wheel. > > > > The second reason, is that we want to be able to share buffers too, not with > > gpu/video but with the network (zctap) and in the future with USB > > (functionFS) too. > > > > [1]: https://lore.kernel.org/linux-iio/20210217073638.21681-1-alexandru.ardelean@analog.com/T/ > > Hm is that code merged already in upstream already? > > I know that dma-buf looks really generic, but like I said if there's no > need ever to interface with any of the gpu buffer sharing it might be > better to use something else (like get_user_pages maybe, would that work?). Not GUP please. That brings another set of issues, especially when dealing with DMA, we've suffered enough from the USERPTR support in V4L2 to avoid repeating this. Let's make dma-buf better instead. > > > Also if the idea is to this with gpus/accelerators then I'd really like to > > > see the full thing, since most likely at that point you also want > > > dma_fence. And once we talk dma_fence things get truly horrible from a > > > locking pov :-( Or well, just highly constrained and I get to review what > > > iio is doing with these buffers to make sure it all fits. > > > > There is some dma_fence action in patch #10, which is enough for the > > userspace apps to use the API. > > > > What "horribleness" are we talking about here? It doesn't look that scary to > > me, but I certainly don't have the complete picture. > > You need to annotate all the code involved in signalling that dma_fence > using dma_fence_begin/end_signalling, and then enable full lockdep and > everything. > > You can safely assume you'll find bugs, because we even have bugs about > this in gpu drivers (where that annotation isn't fully rolled out yet). > > The tldr is that you can allocate memory in there. And a pile of other > restrictions, but not being able to allocate memory (well GFP_ATOMIC is > ok, but that can fail) is a very serious restriction. > -Daniel > > > > > Alexandru Ardelean (1): > > > > iio: buffer-dma: split iio_dma_buffer_fileio_free() function > > > > > > > > Paul Cercueil (14): > > > > iio: buffer-dma: Get rid of incoming/outgoing queues > > > > iio: buffer-dma: Remove unused iio_buffer_block struct > > > > iio: buffer-dma: Use round_down() instead of rounddown() > > > > iio: buffer-dma: Enable buffer write support > > > > iio: buffer-dmaengine: Support specifying buffer direction > > > > iio: buffer-dmaengine: Enable write support > > > > iio: core: Add new DMABUF interface infrastructure > > > > iio: buffer-dma: Use DMABUFs instead of custom solution > > > > iio: buffer-dma: Implement new DMABUF based userspace API > > > > iio: buffer-dma: Boost performance using write-combine cache setting > > > > iio: buffer-dmaengine: Support new DMABUF based userspace API > > > > iio: core: Add support for cyclic buffers > > > > iio: buffer-dmaengine: Add support for cyclic buffers > > > > Documentation: iio: Document high-speed DMABUF based API > > > > > > > > Documentation/driver-api/dma-buf.rst | 2 + > > > > Documentation/iio/dmabuf_api.rst | 94 +++ > > > > Documentation/iio/index.rst | 2 + > > > > drivers/iio/adc/adi-axi-adc.c | 3 +- > > > > drivers/iio/buffer/industrialio-buffer-dma.c | 670 ++++++++++++++---- > > > > .../buffer/industrialio-buffer-dmaengine.c | 42 +- > > > > drivers/iio/industrialio-buffer.c | 49 ++ > > > > include/linux/iio/buffer-dma.h | 43 +- > > > > include/linux/iio/buffer-dmaengine.h | 5 +- > > > > include/linux/iio/buffer_impl.h | 8 + > > > > include/uapi/linux/iio/buffer.h | 30 + > > > > 11 files changed, 783 insertions(+), 165 deletions(-) > > > > create mode 100644 Documentation/iio/dmabuf_api.rst
Am 16.11.21 um 17:31 schrieb Laurent Pinchart: > On Tue, Nov 16, 2021 at 05:02:25PM +0100, Daniel Vetter wrote: >> On Mon, Nov 15, 2021 at 02:57:37PM +0000, Paul Cercueil wrote: >>> Le lun., nov. 15 2021 at 15:37:16 +0100, Daniel Vetter a écrit : >>>> On Mon, Nov 15, 2021 at 02:19:10PM +0000, Paul Cercueil wrote: >>>>> Hi Jonathan, >>>>> >>>>> This patchset introduces a new userspace interface based on DMABUF >>>>> objects, to complement the existing fileio based API. >>>>> >>>>> The advantage of this DMABUF based interface vs. the fileio >>>>> interface, is that it avoids an extra copy of the data between the >>>>> kernel and userspace. This is particularly userful for high-speed >>>>> devices which produce several megabytes or even gigabytes of data per >>>>> second. >>>>> >>>>> The first few patches [01/15] to [03/15] are not really related, but >>>>> allow to reduce the size of the patches that introduce the new API. >>>>> >>>>> Patch [04/15] to [06/15] enables write() support to the buffer-dma >>>>> implementation of the buffer API, to continue the work done by >>>>> Mihail Chindris. >>>>> >>>>> Patches [07/15] to [12/15] introduce the new DMABUF based API. >>>>> >>>>> Patches [13/15] and [14/15] add support for cyclic buffers, only through >>>>> the new API. A cyclic buffer will be repeated on the output until the >>>>> buffer is disabled. >>>>> >>>>> Patch [15/15] adds documentation about the new API. >>>>> >>>>> For now, the API allows you to alloc DMABUF objects and mmap() them >>>>> to >>>>> read or write the samples. It does not yet allow to import DMABUFs >>>>> parented to other subsystems, but that should eventually be possible >>>>> once it's wired. >>>>> >>>>> This patchset is inspired by the "mmap interface" that was previously >>>>> submitted by Alexandru Ardelean and Lars-Peter Clausen, so it would be >>>>> great if I could get a review from you guys. Alexandru's commit was >>>>> signed with his @analog.com address but he doesn't work at ADI anymore, >>>>> so I believe I'll need him to sign with a new email. >>>> Why dma-buf? dma-buf looks like something super generic and useful, until >>>> you realize that there's a metric ton of gpu/accelerator bagage piled in. >>>> So unless buffer sharing with a gpu/video/accel/whatever device is the > And cameras (maybe they're included in "whatever" ?). > >>>> goal here, and it's just for a convenient way to get at buffer handles, >>>> this doesn't sound like a good idea. >>> Good question. The first reason is that a somewhat similar API was intented >>> before[1], but refused upstream as it was kind of re-inventing the wheel. >>> >>> The second reason, is that we want to be able to share buffers too, not with >>> gpu/video but with the network (zctap) and in the future with USB >>> (functionFS) too. >>> >>> [1]: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-iio%2F20210217073638.21681-1-alexandru.ardelean%40analog.com%2FT%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7C7fffe09e82514577747808d9a91e9dd9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637726772007336951%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=w1vD8IKz5G7ut%2FlsOyuYXKKQRBV1s8dN7qJBUwo8x9g%3D&reserved=0 >> Hm is that code merged already in upstream already? >> >> I know that dma-buf looks really generic, but like I said if there's no >> need ever to interface with any of the gpu buffer sharing it might be >> better to use something else (like get_user_pages maybe, would that work?). > Not GUP please. That brings another set of issues, especially when > dealing with DMA, we've suffered enough from the USERPTR support in V4L2 > to avoid repeating this. Let's make dma-buf better instead. Yeah, when comparing GUP and DMA-buf the later is clearly the lesser evil. DMA-buf indeed has some design issues we need to work on, especially around the async operations and synchronization. But I still think those are solvable. GUP on the other hand has some hard fundamental problems which you can only solved completely if the hardware is capable of fast and reliable recoverable page faults. Regards, Christian.
Hi Daniel, Le mar., nov. 16 2021 at 17:02:25 +0100, Daniel Vetter <daniel@ffwll.ch> a écrit : > On Mon, Nov 15, 2021 at 02:57:37PM +0000, Paul Cercueil wrote: >> Hi Daniel, >> >> Le lun., nov. 15 2021 at 15:37:16 +0100, Daniel Vetter >> <daniel@ffwll.ch> a >> écrit : >> > On Mon, Nov 15, 2021 at 02:19:10PM +0000, Paul Cercueil wrote: >> > > Hi Jonathan, >> > > >> > > This patchset introduces a new userspace interface based on >> DMABUF >> > > objects, to complement the existing fileio based API. >> > > >> > > The advantage of this DMABUF based interface vs. the fileio >> > > interface, is that it avoids an extra copy of the data between >> the >> > > kernel and userspace. This is particularly userful for >> high-speed >> > > devices which produce several megabytes or even gigabytes of >> data >> > > per >> > > second. >> > > >> > > The first few patches [01/15] to [03/15] are not really >> related, but >> > > allow to reduce the size of the patches that introduce the new >> API. >> > > >> > > Patch [04/15] to [06/15] enables write() support to the >> buffer-dma >> > > implementation of the buffer API, to continue the work done by >> > > Mihail Chindris. >> > > >> > > Patches [07/15] to [12/15] introduce the new DMABUF based API. >> > > >> > > Patches [13/15] and [14/15] add support for cyclic buffers, >> only >> > > through >> > > the new API. A cyclic buffer will be repeated on the output >> until >> > > the >> > > buffer is disabled. >> > > >> > > Patch [15/15] adds documentation about the new API. >> > > >> > > For now, the API allows you to alloc DMABUF objects and mmap() >> them >> > > to >> > > read or write the samples. It does not yet allow to import >> DMABUFs >> > > parented to other subsystems, but that should eventually be >> possible >> > > once it's wired. >> > > >> > > This patchset is inspired by the "mmap interface" that was >> > > previously >> > > submitted by Alexandru Ardelean and Lars-Peter Clausen, so it >> would >> > > be >> > > great if I could get a review from you guys. Alexandru's >> commit was >> > > signed with his @analog.com address but he doesn't work at ADI >> > > anymore, >> > > so I believe I'll need him to sign with a new email. >> > >> > Why dma-buf? dma-buf looks like something super generic and >> useful, >> > until >> > you realize that there's a metric ton of gpu/accelerator bagage >> piled >> > in. >> > So unless buffer sharing with a gpu/video/accel/whatever device >> is the >> > goal here, and it's just for a convenient way to get at buffer >> handles, >> > this doesn't sound like a good idea. >> >> Good question. The first reason is that a somewhat similar API was >> intented >> before[1], but refused upstream as it was kind of re-inventing the >> wheel. >> >> The second reason, is that we want to be able to share buffers too, >> not with >> gpu/video but with the network (zctap) and in the future with USB >> (functionFS) too. >> >> [1]: >> https://lore.kernel.org/linux-iio/20210217073638.21681-1-alexandru.ardelean@analog.com/T/ > > Hm is that code merged already in upstream already? No, it was never merged. > I know that dma-buf looks really generic, but like I said if there's > no > need ever to interface with any of the gpu buffer sharing it might be > better to use something else (like get_user_pages maybe, would that > work?). If it was such a bad idea, why didn't you say it in the first place when you replied to my request for feedback? [1] I don't think we have any other solution. We can design a custom API to pass buffers between IIO and user space, but that won't allow us to share these buffers with other subsystems. If dma-buf is not a generic solution, then we need a generic solution. [1]: https://x-lore.kernel.org/io-uring/b0a336c0-ae2f-e77f-3c5f-51fdb3fc51fe@amd.com/T/ >> > Also if the idea is to this with gpus/accelerators then I'd >> really like >> > to >> > see the full thing, since most likely at that point you also want >> > dma_fence. And once we talk dma_fence things get truly horrible >> from a >> > locking pov :-( Or well, just highly constrained and I get to >> review >> > what >> > iio is doing with these buffers to make sure it all fits. >> >> There is some dma_fence action in patch #10, which is enough for the >> userspace apps to use the API. >> >> What "horribleness" are we talking about here? It doesn't look that >> scary to >> me, but I certainly don't have the complete picture. > > You need to annotate all the code involved in signalling that > dma_fence > using dma_fence_begin/end_signalling, and then enable full lockdep and > everything. Doesn't dma_fence_signal() do it for me? Looking at the code, it does call dma_fence_begin/end_signalling. Cheers, -Paul > You can safely assume you'll find bugs, because we even have bugs > about > this in gpu drivers (where that annotation isn't fully rolled out > yet). > > The tldr is that you can allocate memory in there. And a pile of other > restrictions, but not being able to allocate memory (well GFP_ATOMIC > is > ok, but that can fail) is a very serious restriction. > -Daniel > > >> >> Cheers, >> -Paul >> >> > Cheers, Daniel >> > >> > > >> > > Cheers, >> > > -Paul >> > > >> > > Alexandru Ardelean (1): >> > > iio: buffer-dma: split iio_dma_buffer_fileio_free() function >> > > >> > > Paul Cercueil (14): >> > > iio: buffer-dma: Get rid of incoming/outgoing queues >> > > iio: buffer-dma: Remove unused iio_buffer_block struct >> > > iio: buffer-dma: Use round_down() instead of rounddown() >> > > iio: buffer-dma: Enable buffer write support >> > > iio: buffer-dmaengine: Support specifying buffer direction >> > > iio: buffer-dmaengine: Enable write support >> > > iio: core: Add new DMABUF interface infrastructure >> > > iio: buffer-dma: Use DMABUFs instead of custom solution >> > > iio: buffer-dma: Implement new DMABUF based userspace API >> > > iio: buffer-dma: Boost performance using write-combine cache >> > > setting >> > > iio: buffer-dmaengine: Support new DMABUF based userspace API >> > > iio: core: Add support for cyclic buffers >> > > iio: buffer-dmaengine: Add support for cyclic buffers >> > > Documentation: iio: Document high-speed DMABUF based API >> > > >> > > Documentation/driver-api/dma-buf.rst | 2 + >> > > Documentation/iio/dmabuf_api.rst | 94 +++ >> > > Documentation/iio/index.rst | 2 + >> > > drivers/iio/adc/adi-axi-adc.c | 3 +- >> > > drivers/iio/buffer/industrialio-buffer-dma.c | 670 >> > > ++++++++++++++---- >> > > .../buffer/industrialio-buffer-dmaengine.c | 42 +- >> > > drivers/iio/industrialio-buffer.c | 49 ++ >> > > include/linux/iio/buffer-dma.h | 43 +- >> > > include/linux/iio/buffer-dmaengine.h | 5 +- >> > > include/linux/iio/buffer_impl.h | 8 + >> > > include/uapi/linux/iio/buffer.h | 30 + >> > > 11 files changed, 783 insertions(+), 165 deletions(-) >> > > create mode 100644 Documentation/iio/dmabuf_api.rst >> > > >> > > -- >> > > 2.33.0 >> > > >> > >> > -- >> > Daniel Vetter >> > Software Engineer, Intel Corporation >> > http://blog.ffwll.ch >> >> > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
> -----Original Message----- > From: Paul Cercueil <paul@crapouillou.net> > Sent: Mittwoch, 17. November 2021 13:50 > To: Daniel Vetter <daniel@ffwll.ch> > Cc: Jonathan Cameron <jic23@kernel.org>; Hennerich, Michael > <Michael.Hennerich@analog.com>; linux-iio@vger.kernel.org; linux- > kernel@vger.kernel.org; dri-devel@lists.freedesktop.org; Christian König > <christian.koenig@amd.com>; linaro-mm-sig@lists.linaro.org; Alexandru > Ardelean <ardeleanalex@gmail.com>; linux-media@vger.kernel.org > Subject: Re: [PATCH 00/15] iio: buffer-dma: write() and new DMABUF based > API > > [External] > > Hi Daniel, > > Le mar., nov. 16 2021 at 17:02:25 +0100, Daniel Vetter <daniel@ffwll.ch> a > écrit : > > On Mon, Nov 15, 2021 at 02:57:37PM +0000, Paul Cercueil wrote: > >> Hi Daniel, > >> > >> Le lun., nov. 15 2021 at 15:37:16 +0100, Daniel Vetter > >> <daniel@ffwll.ch> a écrit : > >> > On Mon, Nov 15, 2021 at 02:19:10PM +0000, Paul Cercueil wrote: > >> > > Hi Jonathan, > >> > > > >> > > This patchset introduces a new userspace interface based on > >> DMABUF > > objects, to complement the existing fileio based API. > >> > > > >> > > The advantage of this DMABUF based interface vs. the fileio > > >> > interface, is that it avoids an extra copy of the data between the > >> > > kernel and userspace. This is particularly userful for > >> high-speed > > devices which produce several megabytes or even > >> gigabytes of data > > per > > second. > >> > > > >> > > The first few patches [01/15] to [03/15] are not really > >> related, but > > allow to reduce the size of the patches that > >> introduce the new API. > >> > > > >> > > Patch [04/15] to [06/15] enables write() support to the > >> buffer-dma > > implementation of the buffer API, to continue the > >> work done by > > Mihail Chindris. > >> > > > >> > > Patches [07/15] to [12/15] introduce the new DMABUF based API. > >> > > > >> > > Patches [13/15] and [14/15] add support for cyclic buffers, > >> only > > through > > the new API. A cyclic buffer will be repeated > >> on the output until > > the > > buffer is disabled. > >> > > > >> > > Patch [15/15] adds documentation about the new API. > >> > > > >> > > For now, the API allows you to alloc DMABUF objects and mmap() > >> them > > to > > read or write the samples. It does not yet allow > >> to import DMABUFs > > parented to other subsystems, but that should > >> eventually be possible > > once it's wired. > >> > > > >> > > This patchset is inspired by the "mmap interface" that was > > > >> previously > > submitted by Alexandru Ardelean and Lars-Peter > >> Clausen, so it would > > be > > great if I could get a review from > >> you guys. Alexandru's commit was > > signed with his @analog.com > >> address but he doesn't work at ADI > > anymore, > > so I believe > >> I'll need him to sign with a new email. > >> > > >> > Why dma-buf? dma-buf looks like something super generic and > >> useful, > until > you realize that there's a metric ton of > >> gpu/accelerator bagage piled > in. > >> > So unless buffer sharing with a gpu/video/accel/whatever device is > >> the > goal here, and it's just for a convenient way to get at buffer > >> handles, > this doesn't sound like a good idea. > >> > >> Good question. The first reason is that a somewhat similar API was > >> intented before[1], but refused upstream as it was kind of > >> re-inventing the wheel. > >> > >> The second reason, is that we want to be able to share buffers too, > >> not with gpu/video but with the network (zctap) and in the future > >> with USB > >> (functionFS) too. > >> > >> [1]: > >> https://urldefense.com/v3/__https://lore.kernel.org/linux-iio/2021021 > >> 7073638.21681-1- > alexandru.ardelean@analog.com/T/__;!!A3Ni8CS0y2Y!p67g > >> > KdXW2zXUxASbwbCKzXgcwxEo1R3h4AumAE6QHiSPyI3KYaz9TmGpnSIF3wsQ > c3gAgQ$ > > > > Hm is that code merged already in upstream already? > > No, it was never merged. > > > I know that dma-buf looks really generic, but like I said if there's > > no need ever to interface with any of the gpu buffer sharing it might > > be better to use something else (like get_user_pages maybe, would that > > work?). > > If it was such a bad idea, why didn't you say it in the first place when you > replied to my request for feedback? [1] > > I don't think we have any other solution. We can design a custom API to pass > buffers between IIO and user space, but that won't allow us to share these > buffers with other subsystems. If dma-buf is not a generic solution, then we > need a generic solution. I don't think we need another generic solution - dma-buf is the solution: From https://www.kernel.org/doc/html/v5.15/driver-api/dma-buf.html: " The dma-buf subsystem provides the framework for sharing buffers for hardware (DMA) access across multiple device drivers and subsystems, and for synchronizing asynchronous hardware access." That's sounds pretty much like what we need. It lives under linux/drivers/dma-buf if it would be a video only shouldn't this be documented somewhere, and perhaps the code should be somewhere else? Just my 2cents. Greetings, Michael > > [1]: > https://urldefense.com/v3/__https://x-lore.kernel.org/io-uring/b0a336c0- > ae2f-e77f-3c5f- > 51fdb3fc51fe@amd.com/T/__;!!A3Ni8CS0y2Y!p67gKdXW2zXUxASbwbCKzXg > cwxEo1R3h4AumAE6QHiSPyI3KYaz9TmGpnSIF3wu9jAKL1Q$ > > >> > Also if the idea is to this with gpus/accelerators then I'd really > >> like > to > see the full thing, since most likely at that point you > >> also want > dma_fence. And once we talk dma_fence things get truly > >> horrible from a > locking pov :-( Or well, just highly constrained > >> and I get to review > what > iio is doing with these buffers to > >> make sure it all fits. > >> > >> There is some dma_fence action in patch #10, which is enough for the > >> userspace apps to use the API. > >> > >> What "horribleness" are we talking about here? It doesn't look that > >> scary to me, but I certainly don't have the complete picture. > > > > You need to annotate all the code involved in signalling that > > dma_fence using dma_fence_begin/end_signalling, and then enable full > > lockdep and everything. > > Doesn't dma_fence_signal() do it for me? Looking at the code, it does call > dma_fence_begin/end_signalling. > > Cheers, > -Paul > > > You can safely assume you'll find bugs, because we even have bugs > > about this in gpu drivers (where that annotation isn't fully rolled > > out yet). > > > > The tldr is that you can allocate memory in there. And a pile of other > > restrictions, but not being able to allocate memory (well GFP_ATOMIC > > is ok, but that can fail) is a very serious restriction. > > -Daniel > > > > > >> > >> Cheers, > >> -Paul > >> > >> > Cheers, Daniel > >> > > >> > > > >> > > Cheers, > >> > > -Paul > >> > > > >> > > Alexandru Ardelean (1): > >> > > iio: buffer-dma: split iio_dma_buffer_fileio_free() function > >> > > > >> > > Paul Cercueil (14): > >> > > iio: buffer-dma: Get rid of incoming/outgoing queues > >> > > iio: buffer-dma: Remove unused iio_buffer_block struct > >> > > iio: buffer-dma: Use round_down() instead of rounddown() > >> > > iio: buffer-dma: Enable buffer write support > >> > > iio: buffer-dmaengine: Support specifying buffer direction > >> > > iio: buffer-dmaengine: Enable write support > >> > > iio: core: Add new DMABUF interface infrastructure > >> > > iio: buffer-dma: Use DMABUFs instead of custom solution > >> > > iio: buffer-dma: Implement new DMABUF based userspace API > >> > > iio: buffer-dma: Boost performance using write-combine cache > >> > > setting > >> > > iio: buffer-dmaengine: Support new DMABUF based userspace API > >> > > iio: core: Add support for cyclic buffers > >> > > iio: buffer-dmaengine: Add support for cyclic buffers > >> > > Documentation: iio: Document high-speed DMABUF based API > >> > > > >> > > Documentation/driver-api/dma-buf.rst | 2 + > >> > > Documentation/iio/dmabuf_api.rst | 94 +++ > >> > > Documentation/iio/index.rst | 2 + > >> > > drivers/iio/adc/adi-axi-adc.c | 3 +- > >> > > drivers/iio/buffer/industrialio-buffer-dma.c | 670 > >> > > ++++++++++++++---- > >> > > .../buffer/industrialio-buffer-dmaengine.c | 42 +- > >> > > drivers/iio/industrialio-buffer.c | 49 ++ > >> > > include/linux/iio/buffer-dma.h | 43 +- > >> > > include/linux/iio/buffer-dmaengine.h | 5 +- > >> > > include/linux/iio/buffer_impl.h | 8 + > >> > > include/uapi/linux/iio/buffer.h | 30 + > >> > > 11 files changed, 783 insertions(+), 165 deletions(-) > >> > > create mode 100644 Documentation/iio/dmabuf_api.rst > >> > > > >> > > -- > >> > > 2.33.0 > >> > > > >> > > >> > -- > >> > Daniel Vetter > >> > Software Engineer, Intel Corporation > > >> https://urldefense.com/v3/__http://blog.ffwll.ch__;!!A3Ni8CS0y2Y!p67g > >> > KdXW2zXUxASbwbCKzXgcwxEo1R3h4AumAE6QHiSPyI3KYaz9TmGpnSIF3ws_ > BZIjsg$ > >> > >> > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > https://urldefense.com/v3/__http://blog.ffwll.ch__;!!A3Ni8CS0y2Y!p67gK > > > dXW2zXUxASbwbCKzXgcwxEo1R3h4AumAE6QHiSPyI3KYaz9TmGpnSIF3ws_B > ZIjsg$ >
On Mon, 15 Nov 2021 14:19:10 +0000 Paul Cercueil <paul@crapouillou.net> wrote: > Hi Jonathan, > > This patchset introduces a new userspace interface based on DMABUF > objects, to complement the existing fileio based API. > > The advantage of this DMABUF based interface vs. the fileio > interface, is that it avoids an extra copy of the data between the > kernel and userspace. This is particularly userful for high-speed > devices which produce several megabytes or even gigabytes of data per > second. I'm going to defer to others for the discussion of whether dmabuf is the right way to do this. I look forwards to some conclusions. Much like yourselves, I want to see 'a' way of doing this but I'm not knowledgeable enough about the quirks and corner cases of the different options to offer a strong opinion. Either way, it's great to get that discussion going and it would never have happened without having actual code so thanks for doing this! > > The first few patches [01/15] to [03/15] are not really related, but > allow to reduce the size of the patches that introduce the new API. > > Patch [04/15] to [06/15] enables write() support to the buffer-dma > implementation of the buffer API, to continue the work done by > Mihail Chindris. > > Patches [07/15] to [12/15] introduce the new DMABUF based API. > > Patches [13/15] and [14/15] add support for cyclic buffers, only through > the new API. A cyclic buffer will be repeated on the output until the > buffer is disabled. > > Patch [15/15] adds documentation about the new API. > > For now, the API allows you to alloc DMABUF objects and mmap() them to > read or write the samples. It does not yet allow to import DMABUFs > parented to other subsystems, but that should eventually be possible > once it's wired. > > This patchset is inspired by the "mmap interface" that was previously > submitted by Alexandru Ardelean and Lars-Peter Clausen, so it would be > great if I could get a review from you guys. Alexandru's commit was > signed with his @analog.com address but he doesn't work at ADI anymore, > so I believe I'll need him to sign with a new email. Given it reflects a sign off given back then I think it's fine to leave them as analog.com but maybe he can give an Acked-by: on a different email address to reflect that he is happy for these to go forwards. It's far from unusual to have out of date email addresses in sign offs given often code is coming out of trees many years after someone originally wrote it for a vendor tree or similar. Whilst not a lawyer I don't think that's a problem. Jonathan > > Cheers, > -Paul > > Alexandru Ardelean (1): > iio: buffer-dma: split iio_dma_buffer_fileio_free() function > > Paul Cercueil (14): > iio: buffer-dma: Get rid of incoming/outgoing queues > iio: buffer-dma: Remove unused iio_buffer_block struct > iio: buffer-dma: Use round_down() instead of rounddown() > iio: buffer-dma: Enable buffer write support > iio: buffer-dmaengine: Support specifying buffer direction > iio: buffer-dmaengine: Enable write support > iio: core: Add new DMABUF interface infrastructure > iio: buffer-dma: Use DMABUFs instead of custom solution > iio: buffer-dma: Implement new DMABUF based userspace API > iio: buffer-dma: Boost performance using write-combine cache setting > iio: buffer-dmaengine: Support new DMABUF based userspace API > iio: core: Add support for cyclic buffers > iio: buffer-dmaengine: Add support for cyclic buffers > Documentation: iio: Document high-speed DMABUF based API > > Documentation/driver-api/dma-buf.rst | 2 + > Documentation/iio/dmabuf_api.rst | 94 +++ > Documentation/iio/index.rst | 2 + > drivers/iio/adc/adi-axi-adc.c | 3 +- > drivers/iio/buffer/industrialio-buffer-dma.c | 670 ++++++++++++++---- > .../buffer/industrialio-buffer-dmaengine.c | 42 +- > drivers/iio/industrialio-buffer.c | 49 ++ > include/linux/iio/buffer-dma.h | 43 +- > include/linux/iio/buffer-dmaengine.h | 5 +- > include/linux/iio/buffer_impl.h | 8 + > include/uapi/linux/iio/buffer.h | 30 + > 11 files changed, 783 insertions(+), 165 deletions(-) > create mode 100644 Documentation/iio/dmabuf_api.rst >