Linaro-mm-sig April 2025

linaro-mm-sig@lists.linaro.org

24 participants
100 discussions

Re: [PATCH 0/3] uio/dma-buf: Give UIO users access to DMA addresses.

by Christian König

Am 14.04.25 um 10:17 schrieb Thomas Petazzoni: > Hello Christian, > > On Fri, 11 Apr 2025 14:41:56 +0200 > Christian König <christian.koenig(a)amd.com> wrote: > >> But anyway please note that when you want to create new UAPI you need >> to provide an open source user of it. E.g. link to a repository or >> something similar in the covert letter should do it. > Could you clarify what is the "reference" user-space user of UIO that > exists today? I honestly don't know. I have never looked into UIO before this patch set arrived. What I mean is that the general rule to justify adding UAPI to the Linux kernel is that you need to have an open source user of that UAPI. In other words for UIO this means that you *can't* do things like exposing some kernel functionality like DMA-buf through UIO and then write a binary only driver on top of it. Regards, Christian. > > Thomas

1 week, 5 days

Re: [PATCH 0/3] uio/dma-buf: Give UIO users access to DMA addresses.

by Christoph Hellwig

On Thu, Apr 10, 2025 at 04:53:17PM +0200, Bastien Curutchet wrote: > Hi all, > > Many UIO users performing DMA from their UIO device need to access the > DMA addresses of the allocated buffers. There are out-of-tree drivers > that allow to do it but nothing in the mainline. In which case it does not matter at all for mainline. FYI the proper and safe way to do DMA from userspace is to use vfio/iommufd.

1 week, 5 days

Re: [PATCH 3/3] uio: Add UIO_DMABUF_HEAP

by Nicolas Dufresne

Hi Bastien, Le jeudi 10 avril 2025 à 16:53 +0200, Bastien Curutchet a écrit : > Some UIO users need to access DMA addresses from userspace to be able to > configure DMA done by the UIO device. Currently there is no way of doing > this. > > Add a UIO_DMABUF_HEAP Kconfig option. When selected, a dma-heap > allocator is created for every new UIO device. This allocator only > implements 4 basic operations: allocate, release, mmap and get_dma_addr. > The buffer allocation is done through dma_alloc_coherent(). This is quite redundant with the CMA heap. I believe a better design is to make UIO devices dmabuf importers. This will make your UIO dmabuf implementation a lot more useful. As for the physical addresses, everywhere you currently pass a physical address, you should be able to add ioctl to pass a DMABuf FD, or a handle to an UIO specific object (similar to buffer objects in DRM) and hide these. regards, Nicolas > > Signed-off-by: Bastien Curutchet <bastien.curutchet(a)bootlin.com> > --- > drivers/uio/Kconfig | 9 ++++ > drivers/uio/Makefile | 1 + > drivers/uio/uio.c | 4 ++ > drivers/uio/uio_heap.c | 120 +++++++++++++++++++++++++++++++++++++++++++++ > include/linux/uio_driver.h | 2 + > 5 files changed, 136 insertions(+) > > diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig > index b060dcd7c6350191726c0830a1ae7b9a388ca4bb..2f3b1e57fceb01354b65cc4d39f342f645a238db 100644 > --- a/drivers/uio/Kconfig > +++ b/drivers/uio/Kconfig > @@ -164,4 +164,13 @@ config UIO_DFL > opae-sdk/tools/libopaeuio/ > > If you compile this as a module, it will be called uio_dfl. > + > +config UIO_DMABUF_HEAP > + bool "DMA-BUF UIO Heap" > + select DMABUF_HEAPS > + help > + Choose this option to enable DMA-BUF UIO heap. It will create a new > + heap allocator under /dev/dma_heap/ for every UIO device. This > + allocator allows userspace applications to allocate DMA buffers and > + access their DMA addresses thanks to the DMA_BUF_IOCTL_GET_DMA_HANDLE > endif > diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile > index 1c5f3b5a95cf5b681a843b745a046d7ce123255d..f6696daa36567a4e5d18b1b89ba688057e758400 100644 > --- a/drivers/uio/Makefile > +++ b/drivers/uio/Makefile > @@ -11,3 +11,4 @@ obj-$(CONFIG_UIO_MF624) += uio_mf624.o > obj-$(CONFIG_UIO_FSL_ELBC_GPCM) += uio_fsl_elbc_gpcm.o > obj-$(CONFIG_UIO_HV_GENERIC) += uio_hv_generic.o > obj-$(CONFIG_UIO_DFL) += uio_dfl.o > +obj-$(CONFIG_UIO_DMABUF_HEAP) += uio_heap.o > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c > index d93ed4e86a174b5bad193a61aa522cd833fe7bb5..f31936a897805a284165cccfee3d66e96acd4b39 100644 > --- a/drivers/uio/uio.c > +++ b/drivers/uio/uio.c > @@ -1046,7 +1046,11 @@ int __uio_register_device(struct module *owner, > } > } > > +#if defined(CONFIG_UIO_DMABUF_HEAP) > + return add_uio_heap(idev); > +#else > return 0; > +#endif > > err_request_irq: > uio_dev_del_attributes(idev); > diff --git a/drivers/uio/uio_heap.c b/drivers/uio/uio_heap.c > new file mode 100644 > index 0000000000000000000000000000000000000000..2e836b503458e280babba0e0adc4f6d8344efc82 > --- /dev/null > +++ b/drivers/uio/uio_heap.c > @@ -0,0 +1,120 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <linux/dma-buf.h> > +#include <linux/dma-heap.h> > +#include <linux/uio_driver.h> > + > +struct uio_heap { > + struct dma_heap *heap; > + struct device *dev; > +}; > + > +struct uio_heap_buffer { > + struct uio_heap *heap; > + dma_addr_t dma_addr; > + unsigned long len; > + void *vaddr; > +}; > + > +static int uio_heap_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma) > +{ > + struct uio_heap_buffer *buffer = dmabuf->priv; > + > + return dma_mmap_coherent(buffer->heap->dev, vma, buffer->vaddr, > + buffer->dma_addr, buffer->len); > +} > + > +static void uio_heap_dma_buf_release(struct dma_buf *dmabuf) > +{ > + struct uio_heap_buffer *buffer = dmabuf->priv; > + > + dma_free_coherent(buffer->heap->dev, buffer->len, buffer->vaddr, > + buffer->dma_addr); > + kfree(buffer); > +} > + > +static int uio_heap_get_dma_addr(struct dma_buf *dmabuf, u64 *addr) > +{ > + struct uio_heap_buffer *buffer = dmabuf->priv; > + > + *addr = buffer->dma_addr; > + return 0; > +} > + > +static const struct dma_buf_ops uio_heap_buf_ops = { > + .mmap = uio_heap_mmap, > + .release = uio_heap_dma_buf_release, > + .get_dma_addr = uio_heap_get_dma_addr, > +}; > + > +static struct dma_buf *uio_heap_allocate(struct dma_heap *heap, > + unsigned long len, > + u32 fd_flags, > + u64 heap_flags) > +{ > + struct uio_heap *uio_heap = dma_heap_get_drvdata(heap); > + DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > + struct uio_heap_buffer *buffer; > + struct dma_buf *dmabuf; > + > + buffer = kzalloc(sizeof(*buffer), GFP_KERNEL); > + if (!buffer) > + return ERR_PTR(-ENOMEM); > + > + dma_set_coherent_mask(uio_heap->dev, DMA_BIT_MASK(32)); > + > + buffer->heap = uio_heap; > + buffer->len = len; > + buffer->vaddr = dma_alloc_coherent(uio_heap->dev, buffer->len, > + &buffer->dma_addr, GFP_KERNEL); > + if (IS_ERR(buffer->vaddr)) > + goto free_buf; > + > + exp_info.exp_name = dma_heap_get_name(heap); > + exp_info.ops = &uio_heap_buf_ops; > + exp_info.size = buffer->len; > + exp_info.flags = fd_flags; > + exp_info.priv = buffer; > + dmabuf = dma_buf_export(&exp_info); > + if (IS_ERR(dmabuf)) > + goto free_dma; > + > + return dmabuf; > + > +free_dma: > + dma_free_coherent(uio_heap->dev, buffer->len, buffer->vaddr, buffer->dma_addr); > +free_buf: > + kfree(buffer); > + > + return ERR_PTR(-ENOMEM); > +} > + > +static const struct dma_heap_ops uio_heap_ops = { > + .allocate = uio_heap_allocate, > +}; > + > +int add_uio_heap(struct uio_device *uio) > +{ > + struct dma_heap_export_info exp_info; > + struct uio_heap *uio_heap; > + > + uio_heap = kzalloc(sizeof(*uio_heap), GFP_KERNEL); > + if (!uio_heap) > + return -ENOMEM; > + > + uio_heap->dev = &uio->dev; > + > + /* Use device name as heap name */ > + exp_info.name = uio_heap->dev->kobj.name; > + exp_info.ops = &uio_heap_ops; > + exp_info.priv = uio_heap; > + > + uio_heap->heap = dma_heap_add(&exp_info); > + if (IS_ERR(uio_heap->heap)) { > + int ret = PTR_ERR(uio_heap->heap); > + > + kfree(uio_heap); > + return ret; > + } > + > + return 0; > +} > diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h > index 18238dc8bfd356a20996ba6cd84f1ff508bbb81c..f8b774d2fa1c7de4b6af881f1e53dfa9f25b3dbf 100644 > --- a/include/linux/uio_driver.h > +++ b/include/linux/uio_driver.h > @@ -143,6 +143,8 @@ extern int __must_check > struct device *parent, > struct uio_info *info); > > +extern int add_uio_heap(struct uio_device *uio); > + > /* use a define to avoid include chaining to get THIS_MODULE */ > > /**

2 weeks

Re: [PATCH 2/3] dma-buf: Add DMA_BUF_IOCTL_GET_DMA_ADDR

by Nicolas Dufresne

Hi Bastien, Le jeudi 10 avril 2025 à 16:53 +0200, Bastien Curutchet a écrit : > There is no way to transmit the DMA address of a buffer to userspace. > Some UIO users need this to handle DMA from userspace. To me this API is against all safe practice we've been pushing forward and has no place in DMA_BUF API. If this is fine for the UIO subsystem to pass around physicial addresses, then make this part of the UIO device ioctl. regards, Nicolas > > Add a new dma_buf_ops operation that returns the DMA address. > Add a new ioctl to transmit this DMA address to userspace. > > Signed-off-by: Bastien Curutchet <bastien.curutchet(a)bootlin.com> > --- > drivers/dma-buf/dma-buf.c | 21 +++++++++++++++++++++ > include/linux/dma-buf.h | 1 + > include/uapi/linux/dma-buf.h | 1 + > 3 files changed, 23 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index > 398418bd9731ad7a3a1f12eaea6a155fa77a22fe..cbbb518981e54e50f479c3d1fcf > 6da6971f639c1 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -454,6 +454,24 @@ static long dma_buf_import_sync_file(struct > dma_buf *dmabuf, > } > #endif > > +static int dma_buf_get_dma_addr(struct dma_buf *dmabuf, u64 __user > *arg) > +{ > + u64 addr; > + int ret; > + > + if (!dmabuf->ops->get_dma_addr) > + return -EINVAL; > + > + ret = dmabuf->ops->get_dma_addr(dmabuf, &addr); > + if (ret) > + return ret; > + > + if (copy_to_user(arg, &addr, sizeof(u64))) > + return -EFAULT; > + > + return 0; > +} > + > static long dma_buf_ioctl(struct file *file, > unsigned int cmd, unsigned long arg) > { > @@ -504,6 +522,9 @@ static long dma_buf_ioctl(struct file *file, > return dma_buf_import_sync_file(dmabuf, (const void > __user *)arg); > #endif > > + case DMA_BUF_IOCTL_GET_DMA_ADDR: > + return dma_buf_get_dma_addr(dmabuf, (u64 __user > *)arg); > + > default: > return -ENOTTY; > } > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index > 36216d28d8bdc01a9c9c47e27c392413f7f6c5fb..ed4bf15d3ce82e7a86323fff459 > 699a9bc8baa3b 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -285,6 +285,7 @@ struct dma_buf_ops { > > int (*vmap)(struct dma_buf *dmabuf, struct iosys_map *map); > void (*vunmap)(struct dma_buf *dmabuf, struct iosys_map > *map); > + int (*get_dma_addr)(struct dma_buf *dmabuf, u64 *addr); > }; > > /** > diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma- > buf.h > index > 5a6fda66d9adf01438619e7e67fa69f0fec2d88d..f3aba46942042de6a2e3a4cca3e > b3f87175e29c9 100644 > --- a/include/uapi/linux/dma-buf.h > +++ b/include/uapi/linux/dma-buf.h > @@ -178,5 +178,6 @@ struct dma_buf_import_sync_file { > #define DMA_BUF_SET_NAME_B _IOW(DMA_BUF_BASE, 1, __u64) > #define DMA_BUF_IOCTL_EXPORT_SYNC_FILE _IOWR(DMA_BUF_BASE, 2, > struct dma_buf_export_sync_file) > #define DMA_BUF_IOCTL_IMPORT_SYNC_FILE _IOW(DMA_BUF_BASE, 3, struct > dma_buf_import_sync_file) > +#define DMA_BUF_IOCTL_GET_DMA_ADDR _IOR(DMA_BUF_BASE, 4, __u64 > *) > > #endif

2 weeks

[PATCH v2] dma-buf/sw_sync: Decrement refcount on error in sw_sync_ioctl_get_deadline()

by Dan Carpenter

Call dma_fence_put(fence) before returning an error if dma_fence_to_sync_pt() fails. Use an unwind ladder at the end of the function to do the cleanup. Fixes: 70e67aaec2f4 ("dma-buf/sw_sync: Add fence deadline support") Signed-off-by: Dan Carpenter <dan.carpenter(a)linaro.org> --- v2: style changes. drivers/dma-buf/sw_sync.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index f5905d67dedb..22a808995f10 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -438,15 +438,17 @@ static int sw_sync_ioctl_get_deadline(struct sync_timeline *obj, unsigned long a return -EINVAL; pt = dma_fence_to_sync_pt(fence); - if (!pt) - return -EINVAL; + if (!pt) { + ret = -EINVAL; + goto put_fence; + } spin_lock_irqsave(fence->lock, flags); - if (test_bit(SW_SYNC_HAS_DEADLINE_BIT, &fence->flags)) { - data.deadline_ns = ktime_to_ns(pt->deadline); - } else { + if (!test_bit(SW_SYNC_HAS_DEADLINE_BIT, &fence->flags)) { ret = -ENOENT; + goto unlock; } + data.deadline_ns = ktime_to_ns(pt->deadline); spin_unlock_irqrestore(fence->lock, flags); dma_fence_put(fence); @@ -458,6 +460,13 @@ static int sw_sync_ioctl_get_deadline(struct sync_timeline *obj, unsigned long a return -EFAULT; return 0; + +unlock: + spin_unlock_irqrestore(fence->lock, flags); +put_fence: + dma_fence_put(fence); + + return ret; } static long sw_sync_ioctl(struct file *file, unsigned int cmd, -- 2.47.2

2 weeks

Re: [PATCH 1/3] drm/nouveau: Prevent signaled fences in pending list

by Christian König

Am 11.04.25 um 14:44 schrieb Philipp Stanner: > On Fri, 2025-04-11 at 13:05 +0200, Christian König wrote: >> Am 11.04.25 um 11:29 schrieb Philipp Stanner: >> >>> [SNIP] >>> >>> It could be, however, that at the same moment >>> nouveau_fence_signal() is >>> removing that entry, holding the appropriate lock. >>> >>> So we have a race. Again. >>> >> >> Ah, yes of course. If signaled is called with or without the lock is >> actually undetermined. >> >> >>> >>> You see, fixing things in Nouveau is difficult :) >>> It gets more difficult if you want to clean it up "properly", so it >>> conforms to rules such as those from dma_fence. >>> >>> I have now provided two fixes that both work, but you are not >>> satisfied >>> with from the dma_fence-maintainer's perspective. I understand >>> that, >>> but please also understand that it's actually not my primary task >>> to >>> work on Nouveau. I just have to fix this bug to move on with my >>> scheduler work. >>> >> >> Well I'm happy with whatever solution as long as it works, but as >> far as I can see the approach with the callback simply doesn't. >> >> You just can't drop the fence reference for the list from the >> callback. >> >> >>> >>> So if you have another idea, feel free to share it. But I'd like to >>> know how we can go on here. >>> >> >> Well the fence code actually works, doesn't it? The problem is >> rather that setting the error throws a warning because it doesn't >> expect signaled fences on the pending list. >> >> Maybe we should fix that instead. > The fence code works as the author intended, but I would be happy if it > were more explicitly documented. > > Regarding the WARN_ON: It occurs in dma_fence_set_error() because there > is an attempt to set an error code on a signaled fence. I don't think > that should be "fixed", it works as intended: You must not set an error > code of a fence that was already signaled. > > The reason seems to be that once a fence is signaled, a third party > might evaluate the error code. Yeah, more or less correct. The idea is you can't declare an operation as having an error after the operation has already completed. Because everyone will just wait for the completion and nobody checks the status again after that. > > But I think this wasn't wat you meant with "fix". The idea was to avoid calling dma_fence_set_error() on already signaled fences. Something like this: @@ -90,7 +90,7 @@ nouveau_fence_context_kill(struct nouveau_fence_chan *fctx, int error) while (!list_empty(&fctx->pending)) { fence = list_entry(fctx->pending.next, typeof(*fence), head); - if (error) + if (error & !dma_fence_is_signaled_locked(&fence->base)) dma_fence_set_error(&fence->base, error); if (nouveau_fence_signal(fence)) That would also improve the handling quite a bit since we now don't set errors on fences which are already completed even if we haven't realized that they are already completed yet. > In any case, there must not be signaled fences in nouveau's pending- > list. They must be removed immediately once they signal, and this must > not race. Why actually? As far as I can see the pending list is not for the unsignaled fences, but rather the pending interrupt processing. So having signaled fences on the pending list is perfectly possible. Regards, Christian. > >> >> >>> >>> I'm running out of ideas. What I'm wondering if we couldn't just >>> remove >>> performance hacky fastpath functions such as >>> nouveau_fence_is_signaled() completely. It seems redundant to me. >>> >> >> That would work for me as well. > I'll test this approach. Seems a bit like the nuclear approach, but if > it works we'd at least clean up a lot of this mess. > > > P. > > >> >> >>> >>> >>> Or we might add locking to it, but IDK what was achieved with RCU >>> here. >>> In any case it's definitely bad that Nouveau has so many redundant >>> and >>> half-redundant mechanisms. >>> >> >> Yeah, agree messing with the locks even more won't help us here. >> >> Regards, >> Christian. >> >> >>> >>> >>> >>> P. >>> >>> >>>> >>>> >>>> P. >>>> >>>> >>>>> >>>>> Regards, >>>>> Christian. >>>>> >>>>> >>>>>> >>>>>> P. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>>> >>>>>>> Regards, >>>>>>> Christian. >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> Replace the call to dma_fence_is_signaled() with >>>>>>>> nouveau_fence_base_is_signaled(). >>>>>>>> >>>>>>>> Cc: <stable(a)vger.kernel.org> # 4.10+, precise commit not >>>>>>>> to >>>>>>>> be >>>>>>>> determined >>>>>>>> Signed-off-by: Philipp Stanner <phasta(a)kernel.org> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c >>>>>>>> b/drivers/gpu/drm/nouveau/nouveau_fence.c >>>>>>>> index 7cc84472cece..33535987d8ed 100644 >>>>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_fence.c >>>>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c >>>>>>>> @@ -274,7 +274,7 @@ nouveau_fence_done(struct >>>>>>>> nouveau_fence >>>>>>>> *fence) >>>>>>>> nvif_event_block(&fctx->event); >>>>>>>> spin_unlock_irqrestore(&fctx->lock, >>>>>>>> flags); >>>>>>>> } >>>>>>>> - return dma_fence_is_signaled(&fence->base); >>>>>>>> + return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >>>>>>>> &fence- >>>>>>>> >>>>>>>>> >>>>>>>>> base.flags); >>>>>>>>> >>>>>>>> >>>>>>>> } >>>>>>>> >>>>>>>> static long >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >>

2 weeks

Re: [PATCH 0/3] uio/dma-buf: Give UIO users access to DMA addresses.

by Christian König

Hi Bastien, Am 11.04.25 um 10:14 schrieb Bastien Curutchet: > Hi Christian, > > On 4/11/25 9:30 AM, Christian König wrote: >> Hi Thomas, >> >> Am 10.04.25 um 21:43 schrieb Thomas Petazzoni: >>> Hello Christian, >>> >>> Thanks for your feedback! >>> >>> On Thu, 10 Apr 2025 18:29:12 +0200 >>> Christian König<christian.koenig(a)amd.com> wrote: >>> >>>>> Many UIO users performing DMA from their UIO device need to access the >>>>> DMA addresses of the allocated buffers. There are out-of-tree drivers >>>>> that allow to do it but nothing in the mainline. >>>> Well that basically disqualifies this patch set in the first paragraph. >>>> >>>> To justify some kernel change we always need an in kernel user of the >>>> interface, since this is purely for out-of-tree drivers this is a >>>> no-go to begin with. >>> I'm not sure to understand your comment here. This patch series is >>> about extending the UIO interface... which is a user-space interface. >>> So obviously it has no "in-kernel user" because it's meant to be used >>> from user-space. Could you clarify what you meant here? >> >> Bastien wrote about "out-of-tree drivers" which is something the upstream kernel explicitly does not support. >> > > Sorry maybe it wasn't clear, but what I meant is that the goal of this series is to replace 'out-of-tree drivers' with something upstream. Ah! Yeah that wasn't really clear from the description. But anyway please note that when you want to create new UAPI you need to provide an open source user of it. E.g. link to a repository or something similar in the covert letter should do it. >> Well why do you then want to use DMA-buf in the first place? As far as I know what you describe can perfectly be done with the normal UIO memory management interfaces. >> >> DMA-buf is only interesting when you actually want to share something in between devices or between applications. >> > > I wanted to use DMA-buf because it allows to dynamically allocate/release coherent buffers from userspace. UIO doesn't provide such interface. > I'm aware that exposing DMA addresses to userspace isn't a good practice. That's why this series create a new heap specific to UIO that will be the only one implementing the new ioctl. I don't know the UIO interfaces that well, but that is pretty clearly an abuse of DMA-buf and won't fly at all. If you want coherent memory for your device you should use dma_alloc_coherent() for that. Regards, Christian. > > > Best regards, > Bastien > > >

2 weeks

Re: [PATCH 1/3] drm/nouveau: Prevent signaled fences in pending list

by Christian König

Am 11.04.25 um 11:29 schrieb Philipp Stanner: > [SNIP] > It could be, however, that at the same moment nouveau_fence_signal() is > removing that entry, holding the appropriate lock. > > So we have a race. Again. Ah, yes of course. If signaled is called with or without the lock is actually undetermined. > You see, fixing things in Nouveau is difficult :) > It gets more difficult if you want to clean it up "properly", so it > conforms to rules such as those from dma_fence. > > I have now provided two fixes that both work, but you are not satisfied > with from the dma_fence-maintainer's perspective. I understand that, > but please also understand that it's actually not my primary task to > work on Nouveau. I just have to fix this bug to move on with my > scheduler work. Well I'm happy with whatever solution as long as it works, but as far as I can see the approach with the callback simply doesn't. You just can't drop the fence reference for the list from the callback. > So if you have another idea, feel free to share it. But I'd like to > know how we can go on here. Well the fence code actually works, doesn't it? The problem is rather that setting the error throws a warning because it doesn't expect signaled fences on the pending list. Maybe we should fix that instead. > I'm running out of ideas. What I'm wondering if we couldn't just remove > performance hacky fastpath functions such as > nouveau_fence_is_signaled() completely. It seems redundant to me. That would work for me as well. > > Or we might add locking to it, but IDK what was achieved with RCU here. > In any case it's definitely bad that Nouveau has so many redundant and > half-redundant mechanisms. Yeah, agree messing with the locks even more won't help us here. Regards, Christian. > > > P. > >> >> P. >> >>> Regards, >>> Christian. >>> >>>> P. >>>> >>>> >>>> >>>>> Regards, >>>>> Christian. >>>>> >>>>>> Replace the call to dma_fence_is_signaled() with >>>>>> nouveau_fence_base_is_signaled(). >>>>>> >>>>>> Cc: <stable(a)vger.kernel.org> # 4.10+, precise commit not to >>>>>> be >>>>>> determined >>>>>> Signed-off-by: Philipp Stanner <phasta(a)kernel.org> >>>>>> --- >>>>>> drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c >>>>>> b/drivers/gpu/drm/nouveau/nouveau_fence.c >>>>>> index 7cc84472cece..33535987d8ed 100644 >>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_fence.c >>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c >>>>>> @@ -274,7 +274,7 @@ nouveau_fence_done(struct nouveau_fence >>>>>> *fence) >>>>>> nvif_event_block(&fctx->event); >>>>>> spin_unlock_irqrestore(&fctx->lock, flags); >>>>>> } >>>>>> - return dma_fence_is_signaled(&fence->base); >>>>>> + return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence- >>>>>>> base.flags); >>>>>> } >>>>>> >>>>>> static long

2 weeks, 1 day

Re: [PATCH 0/3] uio/dma-buf: Give UIO users access to DMA addresses.

by Christian König

Hi Thomas, Am 10.04.25 um 21:43 schrieb Thomas Petazzoni: > Hello Christian, > > Thanks for your feedback! > > On Thu, 10 Apr 2025 18:29:12 +0200 > Christian König <christian.koenig(a)amd.com> wrote: > >>> Many UIO users performing DMA from their UIO device need to access the >>> DMA addresses of the allocated buffers. There are out-of-tree drivers >>> that allow to do it but nothing in the mainline. >> Well that basically disqualifies this patch set in the first paragraph. >> >> To justify some kernel change we always need an in kernel user of the >> interface, since this is purely for out-of-tree drivers this is a >> no-go to begin with. > I'm not sure to understand your comment here. This patch series is > about extending the UIO interface... which is a user-space interface. > So obviously it has no "in-kernel user" because it's meant to be used > from user-space. Could you clarify what you meant here? Bastien wrote about "out-of-tree drivers" which is something the upstream kernel explicitly does not support. When you make that UIO API and have an open source userspace driver then that is probably a good justification to do this. What the kernel community tries to prevent here is that people start using the UAPI to write closed source drivers in userspace. >> What you could potentially do is to create an UIO driver which >> imports DMA-bufs, pins them and then provide the DMA addresses to >> userspace. >> >> But please be aware that DMA-fences are fundamentally incompatible >> with UIO. So you won't be able to do any form of synchronization >> which probably makes the implementation pretty limited. > Could you clarify why DMA-fences would be needed here, and for what > synchronization? In general DMA-buf is an interface which enables you do share device specific buffers between different drivers as well as between userspace processes. For this to work with for example cameras, GPUs or RDMA NICs you need to some kind of synchronization primitive, e.g. device A can only starts it's DMA transaction when device B has completed his. The problem is that this synchronization approach is fundamentally incompatible with UIO. See here for more details: https://www.kernel.org/doc/html/latest/driver-api/dma-buf.html#indefinite-d… > The DMA buffers allocated here are DMA coherent buffers. So the > user-space application that uses UIO will allocate such buffers once at > application start, retrieve their DMA address, and then program DMA > transfers as it sees fit using the memory-mapped registers accessible > through UIO. I'm not sure which synchronization you are referring to. > We are not "chaining" DMA transfers, with for example a camera > interface feeding its DMA buffers to an ISP or something like that. The > typical use case here is some IP block in an FPGA that does DMA > transfers to transfer data to/from some sort of specialized I/O > interface. We get an interrupt when the transfer is done, and we can > re-use the buffer for the next transfer. Well why do you then want to use DMA-buf in the first place? As far as I know what you describe can perfectly be done with the normal UIO memory management interfaces. DMA-buf is only interesting when you actually want to share something in between devices or between applications. Regards, Christian. > If you could clarify here as well, it would definitely help in > understanding the shortcomings/limitations. > > Thanks a lot! > > Thomas Petazzoni

2 weeks, 1 day

Re: [PATCH 0/3] uio/dma-buf: Give UIO users access to DMA addresses.

by Christian König

Am 10.04.25 um 16:53 schrieb Bastien Curutchet: > Hi all, > > Many UIO users performing DMA from their UIO device need to access the > DMA addresses of the allocated buffers. There are out-of-tree drivers > that allow to do it but nothing in the mainline. Well that basically disqualifies this patch set in the first paragraph. To justify some kernel change we always need an in kernel user of the interface, since this is purely for out-of-tree drivers this is a no-go to begin with. > I know DMA shouldn't be handled by userspace but, IMHO, since UIO > drivers exist, it would be better if they offered a way of doing this. Leaking DMA addresses to userspace is usually seen as quite some security hole, but on the other hand with UIO you don't have much other choice. > This patch series use the dma-heap framework which already allows > userspace to allocate DMA buffers. I tried to avoid 'polluting' the > existing heaps to prevent inappropriate uses of this new feature by > introducing a new UIO heap, which is the only one implementing this > behavior. Yeah, that won't fly at all. What you could potentially do is to create an UIO driver which imports DMA-bufs, pins them and then provide the DMA addresses to userspace. But please be aware that DMA-fences are fundamentally incompatible with UIO. So you won't be able to do any form of synchronization which probably makes the implementation pretty limited. Regards, Christian. > > PATCH 1 allows the creation of heaps that don't implement map/unmap_buf > operations as UIO heap doesn't use them. > PATCH 2 adds the DMA_BUF_IOCTL_GET_DMA_ADDR which transmits the DMA > addresses to userspace. > PATCH 3 implements the UIO heap. > > It has been tested with the uio_pci_generic driver on a PowerPC. > > Signed-off-by: Bastien Curutchet <bastien.curutchet(a)bootlin.com> > --- > Bastien Curutchet (3): > dma-buf: Allow heap that doesn't provide map_buf/unmap_buf > dma-buf: Add DMA_BUF_IOCTL_GET_DMA_ADDR > uio: Add UIO_DMABUF_HEAP > > drivers/dma-buf/dma-buf.c | 29 +++++++++-- > drivers/uio/Kconfig | 9 ++++ > drivers/uio/Makefile | 1 + > drivers/uio/uio.c | 4 ++ > drivers/uio/uio_heap.c | 120 +++++++++++++++++++++++++++++++++++++++++++ > include/linux/dma-buf.h | 1 + > include/linux/uio_driver.h | 2 + > include/uapi/linux/dma-buf.h | 1 + > 8 files changed, 164 insertions(+), 3 deletions(-) > --- > base-commit: 5f13fa25acaa4f586aaed12efcf7436e004eeaf2 > change-id: 20250408-uio-dma-9b011e9e7f0b > > Best regards,

2 weeks, 1 day

← Newer
1
2
3
4
5
6
7
8
9
10
Older →

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig April 2025