- Linaro-mm-sig - lists.linaro.org

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

by Christoph Hellwig

On Mon, Apr 14, 2025 at 10:24:55AM +0200, Thomas Petazzoni wrote: > What this patch series is about is to add new user-space interface to > extend the existing UIO subsystem. Which as I explained to you is fundamentally broken and unsafe. If you need to do DMA from userspae you need to use vfio/iommufd. > I am not sure how this can work in our use-case. We have a very simple > set of IP blocks implemented in a FPGA, some of those IP blocks are > able to perform DMA operations. The register of those IP blocks are > mapped into a user-space application using the existing, accepted > upstream, UIO subsystem. Some of those registers allow to program DMA > transfers. So far, we can do all what we need, except program those DMA > transfers. Lots of people are having the same issue, and zillions of > ugly out-of-tree solutions flourish all over, and we're trying to see > if we can constructively find a solution that would be acceptable > upstream to resolve this use-case. Our platform is an old PowerPC with > no IOMMU. Then your driver design can't work and you need to replace it with a proper in-kernel driver.

1 month, 3 weeks

1
0
0 0

Re: [PATCH v7 3/4] drm/panthor: Label all kernel BO's

by Steven Price

On 11/04/2025 16:03, Adrián Larumbe wrote: > Kernel BO's aren't exposed to UM, so labelling them is the responsibility > of the driver itself. This kind of tagging will prove useful in further > commits when want to expose these objects through DebugFS. > > Expand panthor_kernel_bo_create() interface to take a NULL-terminated NIT: s/NULL/NUL/ > string. No bounds checking is done because all label strings are given > as statically-allocated literals, but if a more complex kernel BO naming > scheme with explicit memory allocation and formatting was desired in the > future, this would have to change. I'm not sure I follow why bounds-checking would be an issue for strings from the kernel. But as you state these are all literals so definitely not a problem. > > Signed-off-by: Adrián Larumbe <adrian.larumbe(a)collabora.com> > Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> > Reviewed-by: Liviu Dudau <liviu.dudau(a)arm.com> Reviewed-by: Steven Price <steven.price(a)arm.com> > --- > drivers/gpu/drm/panthor/panthor_fw.c | 8 +++++--- > drivers/gpu/drm/panthor/panthor_gem.c | 4 +++- > drivers/gpu/drm/panthor/panthor_gem.h | 2 +- > drivers/gpu/drm/panthor/panthor_heap.c | 6 ++++-- > drivers/gpu/drm/panthor/panthor_sched.c | 9 ++++++--- > 5 files changed, 19 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c > index 0f52766a3120..a7fdc4d8020d 100644 > --- a/drivers/gpu/drm/panthor/panthor_fw.c > +++ b/drivers/gpu/drm/panthor/panthor_fw.c > @@ -449,7 +449,8 @@ panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev, > DRM_PANTHOR_BO_NO_MMAP, > DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC | > DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED, > - PANTHOR_VM_KERNEL_AUTO_VA); > + PANTHOR_VM_KERNEL_AUTO_VA, > + "Queue FW interface"); > if (IS_ERR(mem)) > return mem; > > @@ -481,7 +482,8 @@ panthor_fw_alloc_suspend_buf_mem(struct panthor_device *ptdev, size_t size) > return panthor_kernel_bo_create(ptdev, panthor_fw_vm(ptdev), size, > DRM_PANTHOR_BO_NO_MMAP, > DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC, > - PANTHOR_VM_KERNEL_AUTO_VA); > + PANTHOR_VM_KERNEL_AUTO_VA, > + "FW suspend buffer"); > } > > static int panthor_fw_load_section_entry(struct panthor_device *ptdev, > @@ -601,7 +603,7 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev, > section->mem = panthor_kernel_bo_create(ptdev, panthor_fw_vm(ptdev), > section_size, > DRM_PANTHOR_BO_NO_MMAP, > - vm_map_flags, va); > + vm_map_flags, va, "FW section"); > if (IS_ERR(section->mem)) > return PTR_ERR(section->mem); > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c > index af0ac17f357f..3c5fc854356e 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.c > +++ b/drivers/gpu/drm/panthor/panthor_gem.c > @@ -82,7 +82,7 @@ void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo) > struct panthor_kernel_bo * > panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm, > size_t size, u32 bo_flags, u32 vm_map_flags, > - u64 gpu_va) > + u64 gpu_va, const char *name) > { > struct drm_gem_shmem_object *obj; > struct panthor_kernel_bo *kbo; > @@ -106,6 +106,8 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm, > kbo->obj = &obj->base; > bo->flags = bo_flags; > > + panthor_gem_kernel_bo_set_label(kbo, name); > + > /* The system and GPU MMU page size might differ, which becomes a > * problem for FW sections that need to be mapped at explicit address > * since our PAGE_SIZE alignment might cover a VA range that's > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h > index beba066b4974..62aea06dbc6d 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.h > +++ b/drivers/gpu/drm/panthor/panthor_gem.h > @@ -153,7 +153,7 @@ panthor_kernel_bo_vunmap(struct panthor_kernel_bo *bo) > struct panthor_kernel_bo * > panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm, > size_t size, u32 bo_flags, u32 vm_map_flags, > - u64 gpu_va); > + u64 gpu_va, const char *name); > > void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo); > > diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c > index 3bdf61c14264..d236e9ceade4 100644 > --- a/drivers/gpu/drm/panthor/panthor_heap.c > +++ b/drivers/gpu/drm/panthor/panthor_heap.c > @@ -151,7 +151,8 @@ static int panthor_alloc_heap_chunk(struct panthor_heap_pool *pool, > chunk->bo = panthor_kernel_bo_create(pool->ptdev, pool->vm, heap->chunk_size, > DRM_PANTHOR_BO_NO_MMAP, > DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC, > - PANTHOR_VM_KERNEL_AUTO_VA); > + PANTHOR_VM_KERNEL_AUTO_VA, > + "Tiler heap chunk"); > if (IS_ERR(chunk->bo)) { > ret = PTR_ERR(chunk->bo); > goto err_free_chunk; > @@ -555,7 +556,8 @@ panthor_heap_pool_create(struct panthor_device *ptdev, struct panthor_vm *vm) > pool->gpu_contexts = panthor_kernel_bo_create(ptdev, vm, bosize, > DRM_PANTHOR_BO_NO_MMAP, > DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC, > - PANTHOR_VM_KERNEL_AUTO_VA); > + PANTHOR_VM_KERNEL_AUTO_VA, > + "Heap pool"); > if (IS_ERR(pool->gpu_contexts)) { > ret = PTR_ERR(pool->gpu_contexts); > goto err_destroy_pool; > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c > index 446ec780eb4a..43ee57728de5 100644 > --- a/drivers/gpu/drm/panthor/panthor_sched.c > +++ b/drivers/gpu/drm/panthor/panthor_sched.c > @@ -3332,7 +3332,8 @@ group_create_queue(struct panthor_group *group, > DRM_PANTHOR_BO_NO_MMAP, > DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC | > DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED, > - PANTHOR_VM_KERNEL_AUTO_VA); > + PANTHOR_VM_KERNEL_AUTO_VA, > + "CS ring buffer"); > if (IS_ERR(queue->ringbuf)) { > ret = PTR_ERR(queue->ringbuf); > goto err_free_queue; > @@ -3362,7 +3363,8 @@ group_create_queue(struct panthor_group *group, > DRM_PANTHOR_BO_NO_MMAP, > DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC | > DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED, > - PANTHOR_VM_KERNEL_AUTO_VA); > + PANTHOR_VM_KERNEL_AUTO_VA, > + "Group job stats"); > > if (IS_ERR(queue->profiling.slots)) { > ret = PTR_ERR(queue->profiling.slots); > @@ -3493,7 +3495,8 @@ int panthor_group_create(struct panthor_file *pfile, > DRM_PANTHOR_BO_NO_MMAP, > DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC | > DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED, > - PANTHOR_VM_KERNEL_AUTO_VA); > + PANTHOR_VM_KERNEL_AUTO_VA, > + "Group sync objects"); > if (IS_ERR(group->syncobjs)) { > ret = PTR_ERR(group->syncobjs); > goto err_put_group;

1 month, 3 weeks

1
0
0 0

Re: [PATCH v7 2/4] drm/panthor: Add driver IOCTL for setting BO labels

by Steven Price

On 11/04/2025 16:03, Adrián Larumbe wrote: > Allow UM to label a BO for which it possesses a DRM handle. > > Signed-off-by: Adrián Larumbe <adrian.larumbe(a)collabora.com> > Reviewed-by: Liviu Dudau <liviu.dudau(a)arm.com> > Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> Reviewed-by: Steven Price <steven.price(a)arm.com> Although very minor NITs below which you can consider. > --- > drivers/gpu/drm/panthor/panthor_drv.c | 42 ++++++++++++++++++++++++++- > drivers/gpu/drm/panthor/panthor_gem.h | 2 ++ > include/uapi/drm/panthor_drm.h | 23 +++++++++++++++ > 3 files changed, 66 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > index 06fe46e32073..983b24f1236c 100644 > --- a/drivers/gpu/drm/panthor/panthor_drv.c > +++ b/drivers/gpu/drm/panthor/panthor_drv.c > @@ -1331,6 +1331,44 @@ static int panthor_ioctl_vm_get_state(struct drm_device *ddev, void *data, > return 0; > } > > +static int panthor_ioctl_bo_set_label(struct drm_device *ddev, void *data, > + struct drm_file *file) > +{ > + struct drm_panthor_bo_set_label *args = data; > + struct drm_gem_object *obj; > + const char *label; > + int ret = 0; > + > + obj = drm_gem_object_lookup(file, args->handle); > + if (!obj) > + return -ENOENT; > + > + if (args->size && args->label) { > + if (args->size > PANTHOR_BO_LABEL_MAXLEN) { > + ret = -E2BIG; > + goto err_label; > + } > + > + label = strndup_user(u64_to_user_ptr(args->label), args->size); > + if (IS_ERR(label)) { > + ret = PTR_ERR(label); > + goto err_label; > + } > + } else if (args->size && !args->label) { > + ret = -EINVAL; > + goto err_label; > + } else { > + label = NULL; > + } > + > + panthor_gem_bo_set_label(obj, label); > + > +err_label: > + drm_gem_object_put(obj); > + > + return ret; > +} > + > static int > panthor_open(struct drm_device *ddev, struct drm_file *file) > { > @@ -1400,6 +1438,7 @@ static const struct drm_ioctl_desc panthor_drm_driver_ioctls[] = { > PANTHOR_IOCTL(TILER_HEAP_CREATE, tiler_heap_create, DRM_RENDER_ALLOW), > PANTHOR_IOCTL(TILER_HEAP_DESTROY, tiler_heap_destroy, DRM_RENDER_ALLOW), > PANTHOR_IOCTL(GROUP_SUBMIT, group_submit, DRM_RENDER_ALLOW), > + PANTHOR_IOCTL(BO_SET_LABEL, bo_set_label, DRM_RENDER_ALLOW), > }; > > static int panthor_mmap(struct file *filp, struct vm_area_struct *vma) > @@ -1509,6 +1548,7 @@ static void panthor_debugfs_init(struct drm_minor *minor) > * - 1.2 - adds DEV_QUERY_GROUP_PRIORITIES_INFO query > * - adds PANTHOR_GROUP_PRIORITY_REALTIME priority > * - 1.3 - adds DRM_PANTHOR_GROUP_STATE_INNOCENT flag > + * - 1.4 - adds DRM_IOCTL_PANTHOR_BO_SET_LABEL ioctl > */ > static const struct drm_driver panthor_drm_driver = { > .driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ | > @@ -1522,7 +1562,7 @@ static const struct drm_driver panthor_drm_driver = { > .name = "panthor", > .desc = "Panthor DRM driver", > .major = 1, > - .minor = 3, > + .minor = 4, > > .gem_create_object = panthor_gem_create_object, > .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h > index af0d77338860..beba066b4974 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.h > +++ b/drivers/gpu/drm/panthor/panthor_gem.h > @@ -13,6 +13,8 @@ > > struct panthor_vm; > > +#define PANTHOR_BO_LABEL_MAXLEN PAGE_SIZE > + > /** > * struct panthor_gem_object - Driver specific GEM object. > */ > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h > index 97e2c4510e69..12b1994499a9 100644 > --- a/include/uapi/drm/panthor_drm.h > +++ b/include/uapi/drm/panthor_drm.h > @@ -127,6 +127,9 @@ enum drm_panthor_ioctl_id { > > /** @DRM_PANTHOR_TILER_HEAP_DESTROY: Destroy a tiler heap. */ > DRM_PANTHOR_TILER_HEAP_DESTROY, > + > + /** @DRM_PANTHOR_BO_SET_LABEL: Label a BO. */ > + DRM_PANTHOR_BO_SET_LABEL, > }; > > /** > @@ -977,6 +980,24 @@ struct drm_panthor_tiler_heap_destroy { > __u32 pad; > }; > > +/** > + * struct drm_panthor_bo_set_label - Arguments passed to DRM_IOCTL_PANTHOR_BO_SET_LABEL > + */ > +struct drm_panthor_bo_set_label { > + /** @handle: Handle of the buffer object to label. */ > + __u32 handle; > + > + /** > + * @size: Length of the label, including the NULL terminator. > + * > + * Cannot be greater than the OS page size. > + */ > + __u32 size; > + > + /** @label: User pointer to a NULL-terminated string */ > + __u64 label; > +}; First very minor NIT: * NULL is a pointer, i.e. (void*)0 * NUL is the ASCII code point '\0'. So it's a NUL-terminated string. Second NIT: We don't actually need 'size' - since the string is NUL-terminated we can just strndup_user(__user_pointer__, PAGE_SIZE). As things stand we validate that strlen(label) < size <= PAGE_SIZE - which is a little odd (user space might as well just pass PAGE_SIZE rather than calculate the actual length). Thanks, Steve > + > /** > * DRM_IOCTL_PANTHOR() - Build a Panthor IOCTL number > * @__access: Access type. Must be R, W or RW. > @@ -1019,6 +1040,8 @@ enum { > DRM_IOCTL_PANTHOR(WR, TILER_HEAP_CREATE, tiler_heap_create), > DRM_IOCTL_PANTHOR_TILER_HEAP_DESTROY = > DRM_IOCTL_PANTHOR(WR, TILER_HEAP_DESTROY, tiler_heap_destroy), > + DRM_IOCTL_PANTHOR_BO_SET_LABEL = > + DRM_IOCTL_PANTHOR(WR, BO_SET_LABEL, bo_set_label), > }; > > #if defined(__cplusplus)

1 month, 3 weeks

1
0
0 0

Re: [PATCH v7 1/4] drm/panthor: Introduce BO labeling

by Steven Price

Hi Adrián, On 11/04/2025 16:03, Adrián Larumbe wrote: > Add a new character string Panthor BO field, and a function that allows > setting it from within the driver. > > Driver takes care of freeing the string when it's replaced or no longer > needed at object destruction time, but allocating it is the responsibility > of callers. > > Signed-off-by: Adrián Larumbe <adrian.larumbe(a)collabora.com> > Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> > --- > drivers/gpu/drm/panthor/panthor_gem.c | 39 +++++++++++++++++++++++++++ > drivers/gpu/drm/panthor/panthor_gem.h | 17 ++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c > index 8244a4e6c2a2..af0ac17f357f 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.c > +++ b/drivers/gpu/drm/panthor/panthor_gem.c > @@ -2,6 +2,7 @@ > /* Copyright 2019 Linaro, Ltd, Rob Herring <robh(a)kernel.org> */ > /* Copyright 2023 Collabora ltd. */ > > +#include <linux/cleanup.h> > #include <linux/dma-buf.h> > #include <linux/dma-mapping.h> > #include <linux/err.h> > @@ -18,6 +19,14 @@ static void panthor_gem_free_object(struct drm_gem_object *obj) > struct panthor_gem_object *bo = to_panthor_bo(obj); > struct drm_gem_object *vm_root_gem = bo->exclusive_vm_root_gem; > > + /* > + * Label might have been allocated with kstrdup_const(), > + * we need to take that into account when freeing the memory > + */ > + kfree_const(bo->label.str); > + > + mutex_destroy(&bo->label.lock); > + > drm_gem_free_mmap_offset(&bo->base.base); > mutex_destroy(&bo->gpuva_list_lock); > drm_gem_shmem_free(&bo->base); > @@ -196,6 +205,7 @@ struct drm_gem_object *panthor_gem_create_object(struct drm_device *ddev, size_t > obj->base.map_wc = !ptdev->coherent; > mutex_init(&obj->gpuva_list_lock); > drm_gem_gpuva_set_lock(&obj->base.base, &obj->gpuva_list_lock); > + mutex_init(&obj->label.lock); > > return &obj->base.base; > } > @@ -247,3 +257,32 @@ panthor_gem_create_with_handle(struct drm_file *file, > > return ret; > } > + > +void > +panthor_gem_bo_set_label(struct drm_gem_object *obj, const char *label) > +{ > + struct panthor_gem_object *bo = to_panthor_bo(obj); > + const char *old_label; > + > + scoped_guard(mutex, &bo->label.lock) { > + old_label = bo->label.str; > + bo->label.str = label; > + } > + > + kfree(old_label); Shouldn't this be kfree_const()? I suspect as things stand we can't trigger this bug but it's inconsistent. > +} > + > +void > +panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const char *label) > +{ > + const char *str; > + > + str = kstrdup_const(label, GFP_KERNEL); > + if (!str) { In the next patch you permit user space to clear the label (args->size==0) which means label==NULL and AFAICT kstrdup_const() will return NULL in this case triggering this warning. Thanks, Steve > + /* Failing to allocate memory for a label isn't a fatal condition */ > + drm_warn(bo->obj->dev, "Not enough memory to allocate BO label"); > + return; > + } > + > + panthor_gem_bo_set_label(bo->obj, str); > +} > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h > index 1a363bb814f4..af0d77338860 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.h > +++ b/drivers/gpu/drm/panthor/panthor_gem.h > @@ -46,6 +46,20 @@ struct panthor_gem_object { > > /** @flags: Combination of drm_panthor_bo_flags flags. */ > u32 flags; > + > + /** > + * @label: BO tagging fields. The label can be assigned within the > + * driver itself or through a specific IOCTL. > + */ > + struct { > + /** > + * @label.str: Pointer to NULL-terminated string, > + */ > + const char *str; > + > + /** @lock.str: Protects access to the @label.str field. */ > + struct mutex lock; > + } label; > }; > > /** > @@ -91,6 +105,9 @@ panthor_gem_create_with_handle(struct drm_file *file, > struct panthor_vm *exclusive_vm, > u64 *size, u32 flags, uint32_t *handle); > > +void panthor_gem_bo_set_label(struct drm_gem_object *obj, const char *label); > +void panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const char *label); > + > static inline u64 > panthor_kernel_bo_gpuva(struct panthor_kernel_bo *bo) > {

1 month, 3 weeks

1
0
0 0

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

by Christian König

Am 14.04.25 um 10:24 schrieb Thomas Petazzoni: > Hello Christoph, > > On Sun, 13 Apr 2025 22:55:02 -0700 > Christoph Hellwig <hch(a)infradead.org> wrote: > >> 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. > It is impressive how "out-of-tree" triggers kernel maintainers, and > then end up not even reading what the patch series is all about (but I > forgive you, because it triggers me in the same way). > > This patch series is NOT about adding new kernel APIs meant to be used > by out-of-tree drivers, which of course would be bad, and we would have > never proposed. > > What this patch series is about is to add new user-space interface to > extend the existing UIO subsystem. What my colleague Bastien was > mentioning about out-of-tree drivers is that there are a LOT of > out-of-tree drivers extending UIO to allow user-space applications to > do DMA, and our proposal is that considering how many people need that > and implement ugly out-of-tree drivers to solve this issue, we suggest > the mainline kernel should have a built in solution. > > Please re-read again, and realize that we are NOT adding new kernel > APIs for out-of-tree drivers. > >> FYI the proper and safe way to do DMA from userspace is to use >> vfio/iommufd. > I am not sure how this can work in our use-case. We have a very simple > set of IP blocks implemented in a FPGA, some of those IP blocks are > able to perform DMA operations. The register of those IP blocks are > mapped into a user-space application using the existing, accepted > upstream, UIO subsystem. Some of those registers allow to program DMA > transfers. So far, we can do all what we need, except program those DMA > transfers. Lots of people are having the same issue, and zillions of > ugly out-of-tree solutions flourish all over, and we're trying to see > if we can constructively find a solution that would be acceptable > upstream to resolve this use-case. Our platform is an old PowerPC with > no IOMMU. Maybe I should try to better explain the concern here. The question is "Where is the source code of your FPGA driver?". I mean that you are trying to replace the out-of-tree solution is rather welcomed, but the out-of-tree solutions are out-of-tree because they don't fit with requirements to be added to the core Linux tree. And one of those requirements is that you need to provide the source code of the userspace user of this interface, in this case here that is your FPGA driver. An MIT/X11 license is usually sufficient, GPL is of course seen as better and it must not be a toy application, but rather the real thing. And that is what people usually don't want and that's why no in-tree solution exists for this. Regards, Christian. > > Note that the safety argument doesn't hold: UIO already allows to map > registers into user-space applications, which can already program DMA > transfers to arbitrary locations in memory. The safety comes from the > fact that such UIO devices are associated with permissions that the > system administrator controls, to decide which applications are safe > enough to be granted access to those UIO devices. Therefore, providing > access through UIO of the physical address of well-defined DMA buffers > properly allocated through a sane API is not reducing the safety of > anything compared to what UIO already allows today. > > Best regards, > > Thomas

1 month, 3 weeks

1
0
0 0

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 month, 3 weeks

1
0
0 0

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 month, 3 weeks

1
0
0 0

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 */ > > /**

1 month, 3 weeks

1
0
0 0

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

1 month, 3 weeks

1
0
0 0

[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

1 month, 3 weeks

2
1
0 0

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 >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >>

1 month, 3 weeks

1
0
0 0

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 > > >

1 month, 3 weeks

1
0
0 0

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

1 month, 3 weeks

1
0
0 0

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

1 month, 3 weeks

1
0
0 0

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,

1 month, 4 weeks

1
0
0 0

Re: [PATCH v6 4/4] drm/panthor: show device-wide list of DRM GEM objects over DebugFS

by Liviu Dudau

On Wed, Apr 09, 2025 at 10:22:22PM +0100, Adrián Larumbe wrote: > Add a device DebugFS file that displays a complete list of all the DRM > GEM objects that are exposed to UM through a DRM handle. > > Since leaking object identifiers that might belong to a different NS is > inadmissible, this functionality is only made available in debug builds > with DEBUGFS support enabled. > > File format is that of a table, with each entry displaying a variety of > fields with information about each GEM object. > > Each GEM object entry in the file displays the following information > fields: Client PID, BO's global name, reference count, BO virtual size, > BO resize size, VM address in its DRM-managed range, BO label and a GEM > state flags. > > There's also a usage flags field for the type of BO, which tells us > whether it's a kernel BO and/or mapped onto the FW's address space. > > Signed-off-by: Adrián Larumbe <adrian.larumbe(a)collabora.com> Reviewed-by: Liviu Dudau <liviu.dudau(a)arm.com> Best regards, Liviu > --- > drivers/gpu/drm/panthor/panthor_device.c | 5 + > drivers/gpu/drm/panthor/panthor_device.h | 11 ++ > drivers/gpu/drm/panthor/panthor_drv.c | 26 ++++ > drivers/gpu/drm/panthor/panthor_gem.c | 186 +++++++++++++++++++++++ > drivers/gpu/drm/panthor/panthor_gem.h | 59 +++++++ > 5 files changed, 287 insertions(+) > > diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c > index a9da1d1eeb70..b776e1a2e4f3 100644 > --- a/drivers/gpu/drm/panthor/panthor_device.c > +++ b/drivers/gpu/drm/panthor/panthor_device.c > @@ -184,6 +184,11 @@ int panthor_device_init(struct panthor_device *ptdev) > if (ret) > return ret; > > +#ifdef CONFIG_DEBUG_FS > + drmm_mutex_init(&ptdev->base, &ptdev->gems.lock); > + INIT_LIST_HEAD(&ptdev->gems.node); > +#endif > + > atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_SUSPENDED); > p = alloc_page(GFP_KERNEL | __GFP_ZERO); > if (!p) > diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h > index da6574021664..86206a961b38 100644 > --- a/drivers/gpu/drm/panthor/panthor_device.h > +++ b/drivers/gpu/drm/panthor/panthor_device.h > @@ -205,6 +205,17 @@ struct panthor_device { > > /** @fast_rate: Maximum device clock frequency. Set by DVFS */ > unsigned long fast_rate; > + > +#ifdef CONFIG_DEBUG_FS > + /** @gems: Device-wide list of GEM objects owned by at least one file. */ > + struct { > + /** @gems.lock: Protects the device-wide list of GEM objects. */ > + struct mutex lock; > + > + /** @node: Used to keep track of all the device's DRM objects */ > + struct list_head node; > + } gems; > +#endif > }; > > struct panthor_gpu_usage { > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > index 983b24f1236c..4d3f2eb29a47 100644 > --- a/drivers/gpu/drm/panthor/panthor_drv.c > +++ b/drivers/gpu/drm/panthor/panthor_drv.c > @@ -1535,9 +1535,35 @@ static const struct file_operations panthor_drm_driver_fops = { > }; > > #ifdef CONFIG_DEBUG_FS > +static int panthor_gems_show(struct seq_file *m, void *data) > +{ > + struct drm_info_node *node = m->private; > + struct drm_device *dev = node->minor->dev; > + struct panthor_device *ptdev = container_of(dev, struct panthor_device, base); > + > + panthor_gem_debugfs_print_bos(ptdev, m); > + > + return 0; > +} > + > + > +static struct drm_info_list panthor_debugfs_list[] = { > + {"gems", panthor_gems_show, 0, NULL}, > +}; > + > +static int panthor_gems_debugfs_init(struct drm_minor *minor) > +{ > + drm_debugfs_create_files(panthor_debugfs_list, > + ARRAY_SIZE(panthor_debugfs_list), > + minor->debugfs_root, minor); > + > + return 0; > +} > + > static void panthor_debugfs_init(struct drm_minor *minor) > { > panthor_mmu_debugfs_init(minor); > + panthor_gems_debugfs_init(minor); > } > #endif > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c > index 3c5fc854356e..ca9baa7b43da 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.c > +++ b/drivers/gpu/drm/panthor/panthor_gem.c > @@ -11,14 +11,51 @@ > #include <drm/panthor_drm.h> > > #include "panthor_device.h" > +#include "panthor_fw.h" > #include "panthor_gem.h" > #include "panthor_mmu.h" > > +#ifdef CONFIG_DEBUG_FS > +static void panthor_gem_debugfs_bo_add(struct panthor_device *ptdev, > + struct panthor_gem_object *bo) > +{ > + INIT_LIST_HEAD(&bo->debugfs.node); > + > + bo->debugfs.creator.tgid = current->group_leader->pid; > + get_task_comm(bo->debugfs.creator.process_name, current->group_leader); > + > + mutex_lock(&ptdev->gems.lock); > + list_add_tail(&bo->debugfs.node, &ptdev->gems.node); > + mutex_unlock(&ptdev->gems.lock); > +} > + > +static void panthor_gem_debugfs_bo_rm(struct panthor_gem_object *bo) > +{ > + struct panthor_device *ptdev = container_of(bo->base.base.dev, > + struct panthor_device, base); > + > + if (list_empty(&bo->debugfs.node)) > + return; > + > + mutex_lock(&ptdev->gems.lock); > + list_del_init(&bo->debugfs.node); > + mutex_unlock(&ptdev->gems.lock); > +} > + > +#else > +static void panthor_gem_debugfs_bo_add(struct panthor_device *ptdev, > + struct panthor_gem_object *bo) > +{} > +static void panthor_gem_debugfs_bo_rm(struct panthor_gem_object *bo) {} > +#endif > + > static void panthor_gem_free_object(struct drm_gem_object *obj) > { > struct panthor_gem_object *bo = to_panthor_bo(obj); > struct drm_gem_object *vm_root_gem = bo->exclusive_vm_root_gem; > > + panthor_gem_debugfs_bo_rm(bo); > + > /* > * Label might have been allocated with kstrdup_const(), > * we need to take that into account when freeing the memory > @@ -87,6 +124,7 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm, > struct drm_gem_shmem_object *obj; > struct panthor_kernel_bo *kbo; > struct panthor_gem_object *bo; > + u32 debug_flags = PANTHOR_DEBUGFS_GEM_USAGE_FLAG_KERNEL; > int ret; > > if (drm_WARN_ON(&ptdev->base, !vm)) > @@ -106,7 +144,11 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm, > kbo->obj = &obj->base; > bo->flags = bo_flags; > > + if (vm == panthor_fw_vm(ptdev)) > + debug_flags |= PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED; > + > panthor_gem_kernel_bo_set_label(kbo, name); > + panthor_gem_debugfs_bo_set_mask(to_panthor_bo(kbo->obj), debug_flags); > > /* The system and GPU MMU page size might differ, which becomes a > * problem for FW sections that need to be mapped at explicit address > @@ -209,6 +251,8 @@ struct drm_gem_object *panthor_gem_create_object(struct drm_device *ddev, size_t > drm_gem_gpuva_set_lock(&obj->base.base, &obj->gpuva_list_lock); > mutex_init(&obj->label.lock); > > + panthor_gem_debugfs_bo_add(ptdev, obj); > + > return &obj->base.base; > } > > @@ -257,6 +301,12 @@ panthor_gem_create_with_handle(struct drm_file *file, > /* drop reference from allocate - handle holds it now. */ > drm_gem_object_put(&shmem->base); > > + /* > + * No explicit flags are needed in the call below, since the > + * function internally sets the INITIALIZED bit for us. > + */ > + panthor_gem_debugfs_bo_set_mask(bo, 0); > + > return ret; > } > > @@ -288,3 +338,139 @@ panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const char *label) > > panthor_gem_bo_set_label(bo->obj, str); > } > + > +#ifdef CONFIG_DEBUG_FS > +static void > +panthor_gem_debugfs_format_flags(char flags_str[], int flags_len, > + const char * const names[], u32 name_count, > + u32 flags) > +{ > + bool first = true; > + int offset = 0; > + > +#define ACC_FLAGS(...) \ > + ({ \ > + offset += snprintf(flags_str + offset, flags_len - offset, ##__VA_ARGS__); \ > + if (offset == flags_len) \ > + return; \ > + }) > + > + ACC_FLAGS("%c", '('); > + > + if (!flags) > + ACC_FLAGS("%s", "none"); > + > + while (flags) { > + u32 bit = fls(flags) - 1; > + u32 idx = bit + 1; > + > + if (!first) > + ACC_FLAGS("%s", ","); > + > + if (idx >= name_count || !names[idx]) > + ACC_FLAGS("unknown-bit%d", bit); > + else > + ACC_FLAGS("%s", names[idx]); > + > + first = false; > + flags &= ~BIT(bit); > + } > + > + ACC_FLAGS("%c", ')'); > + > +#undef ACC_FLAGS > +} > + > +struct gem_size_totals { > + size_t size; > + size_t resident; > + size_t reclaimable; > +}; > + > +static void panthor_gem_debugfs_bo_print(struct panthor_gem_object *bo, > + struct seq_file *m, > + struct gem_size_totals *totals) > +{ > + unsigned int refcount = kref_read(&bo->base.base.refcount); > + char creator_info[32] = {}; > + size_t resident_size; > + char gem_state_str[24] = {}; > + char gem_usage_str[24] = {}; > + u32 gem_usage_flags = bo->debugfs.flags & (u32)~PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED; > + u32 gem_state_flags = 0; > + > + static const char * const gem_state_flags_names[] = { > + [PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED] = "imported", > + [PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED] = "exported", > + }; > + > + static const char * const gem_usage_flags_names[] = { > + [PANTHOR_DEBUGFS_GEM_USAGE_FLAG_KERNEL] = "kernel", > + [PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED] = "fw-mapped", > + }; > + > + /* Skip BOs being destroyed. */ > + if (!refcount) > + return; > + > + resident_size = bo->base.pages != NULL ? bo->base.base.size : 0; > + > + snprintf(creator_info, sizeof(creator_info), > + "%s/%d", bo->debugfs.creator.process_name, bo->debugfs.creator.tgid); > + seq_printf(m, "%-32s%-16d%-16d%-16zd%-16zd%-16lx", > + creator_info, > + bo->base.base.name, > + refcount, > + bo->base.base.size, > + resident_size, > + drm_vma_node_start(&bo->base.base.vma_node)); > + > + > + if (bo->base.base.import_attach != NULL) > + gem_state_flags |= PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED; > + if (bo->base.base.dma_buf != NULL) > + gem_state_flags |= PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED; > + > + panthor_gem_debugfs_format_flags(gem_state_str, sizeof(gem_state_str), > + gem_state_flags_names, ARRAY_SIZE(gem_state_flags_names), > + gem_state_flags); > + panthor_gem_debugfs_format_flags(gem_usage_str, sizeof(gem_usage_str), > + gem_usage_flags_names, ARRAY_SIZE(gem_usage_flags_names), > + gem_usage_flags); > + > + seq_printf(m, "%-24s%-24s", gem_state_str, gem_usage_str); > + > + scoped_guard(mutex, &bo->label.lock) { > + seq_printf(m, "%s", bo->label.str ? : ""); > + } > + > + seq_puts(m, "\n"); > + > + totals->size += bo->base.base.size; > + totals->resident += resident_size; > + if (bo->base.madv > 0) > + totals->reclaimable += resident_size; > +} > + > +void panthor_gem_debugfs_print_bos(struct panthor_device *ptdev, > + struct seq_file *m) > +{ > + struct gem_size_totals totals = {0}; > + struct panthor_gem_object *bo; > + > + seq_puts(m, "created-by global-name refcount size resident-size file-offset state usage label\n"); > + seq_puts(m, "---------------------------------------------------------------------------------------------------------------------------------------------------------------------\n"); > + > + scoped_guard(mutex, &ptdev->gems.lock) { > + list_for_each_entry(bo, &ptdev->gems.node, debugfs.node) { > + if (bo->debugfs.flags & PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED) > + panthor_gem_debugfs_bo_print(bo, m, &totals); > + } > + > + } > + > + seq_puts(m, "=====================================================================================================================================================================\n"); > + seq_printf(m, "Total size: %zd, Total resident: %zd, Total reclaimable: %zd\n", > + totals.size, totals.resident, totals.reclaimable); > +} > +#endif > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h > index 62aea06dbc6d..8c56e0c0dc9c 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.h > +++ b/drivers/gpu/drm/panthor/panthor_gem.h > @@ -15,6 +15,48 @@ struct panthor_vm; > > #define PANTHOR_BO_LABEL_MAXLEN PAGE_SIZE > > +enum panthor_debugfs_gem_state_flags { > + /** @PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED: GEM BO is PRIME imported. */ > + PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED = BIT(0), > + > + /** @PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED: GEM BO is PRIME exported. */ > + PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED = BIT(1), > +}; > + > +enum panthor_debugfs_gem_usage_flags { > + /** @PANTHOR_DEBUGFS_GEM_USAGE_FLAG_KERNEL: BO is for kernel use only. */ > + PANTHOR_DEBUGFS_GEM_USAGE_FLAG_KERNEL = BIT(0), > + > + /** @PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED: BO is mapped on the FW VM. */ > + PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED = BIT(1), > + > + /** @PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED: BO is ready for DebugFS display. */ > + PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED = BIT(31), > +}; > + > +/** > + * struct panthor_gem_debugfs - GEM object's DebugFS list information > + */ > +struct panthor_gem_debugfs { > + /** > + * @node: Node used to insert the object in the device-wide list of > + * GEM objects, to display information about it through a DebugFS file. > + */ > + struct list_head node; > + > + /** @creator: Information about the UM process which created the GEM. */ > + struct { > + /** @creator.process_name: Group leader name in owning thread's process */ > + char process_name[TASK_COMM_LEN]; > + > + /** @creator.tgid: PID of the thread's group leader within its process */ > + pid_t tgid; > + } creator; > + > + /** @flags: Combination of panthor_debugfs_gem_usage_flags flags */ > + u32 flags; > +}; > + > /** > * struct panthor_gem_object - Driver specific GEM object. > */ > @@ -62,6 +104,10 @@ struct panthor_gem_object { > /** @lock.str: Protects access to the @label.str field. */ > struct mutex lock; > } label; > + > +#ifdef CONFIG_DEBUG_FS > + struct panthor_gem_debugfs debugfs; > +#endif > }; > > /** > @@ -157,4 +203,17 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm, > > void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo); > > +#ifdef CONFIG_DEBUG_FS > +void panthor_gem_debugfs_print_bos(struct panthor_device *pfdev, > + struct seq_file *m); > +static inline void > +panthor_gem_debugfs_bo_set_mask(struct panthor_gem_object *bo, u32 type_mask) > +{ > + bo->debugfs.flags = type_mask | PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED; > +} > + > +#else > +void panthor_gem_debugfs_bo_set_mask(struct panthor_gem_object *bo, u32 type_mask) {}; > +#endif > + > #endif /* __PANTHOR_GEM_H__ */ > -- > 2.48.1 > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯

1 month, 4 weeks

1
0
0 0

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

by Christian König

Am 10.04.25 um 15:09 schrieb Philipp Stanner: > On Thu, 2025-04-10 at 14:58 +0200, Christian König wrote: >> Am 10.04.25 um 11:24 schrieb Philipp Stanner: >>> Nouveau currently relies on the assumption that dma_fences will >>> only >>> ever get signaled through nouveau_fence_signal(), which takes care >>> of >>> removing a signaled fence from the list nouveau_fence_chan.pending. >>> >>> This self-imposed rule is violated in nouveau_fence_done(), where >>> dma_fence_is_signaled() (somewhat surprisingly, considering its >>> name) >>> can signal the fence without removing it from the list. This >>> enables >>> accesses to already signaled fences through the list, which is a >>> bug. >>> >>> In particular, it can race with nouveau_fence_context_kill(), which >>> would then attempt to set an error code on an already signaled >>> fence, >>> which is illegal. >>> >>> In nouveau_fence_done(), the call to nouveau_fence_update() already >>> ensures to signal all ready fences. Thus, the signaling potentially >>> performed by dma_fence_is_signaled() is actually not necessary. >> Ah, I now got what you are trying to do here! But that won't help. >> >> The problem is it is perfectly valid for somebody external (e.g. >> other driver, TTM etc...) to call dma_fence_is_signaled() on a >> nouveau fence. >> >> This will then in turn still signal the fence and leave it on the >> pending list and creating the problem you have. > Good to hear – precisely that then is the use case for a dma_fence > callback! ^_^ It guarantees that, no matter who signals a fence, no > matter at what place, a certain action will always be performed. > > I can't think of any other mechanism which could guarantee that a > signaled fence immediately gets removed from nouveau's pending list, > other than the callbacks. > > But seriously, I don't think that anyone does this currently, nor do I > think that anyone could get away with doing it without the entire > computer burning down. Yeah, I don't think that this is possible at the moment. When you do stuff like that from the provider side you will always run into lifetime issues because in the signaling from interrupt case you then drop the last reference before the signaling is completed. How about the attached (not even compile tested) patch? I think it should fix the issue. 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

1 month, 4 weeks

1
0
0 0

Re: [PATCH v6 3/4] drm/panthor: Label all kernel BO's

by Liviu Dudau

On Wed, Apr 09, 2025 at 10:22:21PM +0100, Adrián Larumbe wrote: > Kernel BO's aren't exposed to UM, so labelling them is the responsibility > of the driver itself. This kind of tagging will prove useful in further > commits when want to expose these objects through DebugFS. > > Expand panthor_kernel_bo_create() interface to take a NULL-terminated > string. No bounds checking is done because all label strings are given > as statically-allocated literals, but if a more complex kernel BO naming > scheme with explicit memory allocation and formatting was desired in the > future, this would have to change. > > Signed-off-by: Adrián Larumbe <adrian.larumbe(a)collabora.com> > Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> > --- > drivers/gpu/drm/panthor/panthor_fw.c | 8 +++++--- > drivers/gpu/drm/panthor/panthor_gem.c | 4 +++- > drivers/gpu/drm/panthor/panthor_gem.h | 2 +- > drivers/gpu/drm/panthor/panthor_heap.c | 6 ++++-- > drivers/gpu/drm/panthor/panthor_sched.c | 9 ++++++--- > 5 files changed, 19 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c > index 0f52766a3120..a7fdc4d8020d 100644 > --- a/drivers/gpu/drm/panthor/panthor_fw.c > +++ b/drivers/gpu/drm/panthor/panthor_fw.c > @@ -449,7 +449,8 @@ panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev, > DRM_PANTHOR_BO_NO_MMAP, > DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC | > DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED, > - PANTHOR_VM_KERNEL_AUTO_VA); > + PANTHOR_VM_KERNEL_AUTO_VA, > + "Queue FW interface"); > if (IS_ERR(mem)) > return mem; > > @@ -481,7 +482,8 @@ panthor_fw_alloc_suspend_buf_mem(struct panthor_device *ptdev, size_t size) > return panthor_kernel_bo_create(ptdev, panthor_fw_vm(ptdev), size, > DRM_PANTHOR_BO_NO_MMAP, > DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC, > - PANTHOR_VM_KERNEL_AUTO_VA); > + PANTHOR_VM_KERNEL_AUTO_VA, > + "FW suspend buffer"); > } > > static int panthor_fw_load_section_entry(struct panthor_device *ptdev, > @@ -601,7 +603,7 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev, > section->mem = panthor_kernel_bo_create(ptdev, panthor_fw_vm(ptdev), > section_size, > DRM_PANTHOR_BO_NO_MMAP, > - vm_map_flags, va); > + vm_map_flags, va, "FW section"); Nit: we could add the section->name if available and if we want a more detailed label, but it is not critical. Reviewed-by: Liviu Dudau <liviu.dudau(a)arm.com> Best regards, Liviu > if (IS_ERR(section->mem)) > return PTR_ERR(section->mem); > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c > index af0ac17f357f..3c5fc854356e 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.c > +++ b/drivers/gpu/drm/panthor/panthor_gem.c > @@ -82,7 +82,7 @@ void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo) > struct panthor_kernel_bo * > panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm, > size_t size, u32 bo_flags, u32 vm_map_flags, > - u64 gpu_va) > + u64 gpu_va, const char *name) > { > struct drm_gem_shmem_object *obj; > struct panthor_kernel_bo *kbo; > @@ -106,6 +106,8 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm, > kbo->obj = &obj->base; > bo->flags = bo_flags; > > + panthor_gem_kernel_bo_set_label(kbo, name); > + > /* The system and GPU MMU page size might differ, which becomes a > * problem for FW sections that need to be mapped at explicit address > * since our PAGE_SIZE alignment might cover a VA range that's > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h > index beba066b4974..62aea06dbc6d 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.h > +++ b/drivers/gpu/drm/panthor/panthor_gem.h > @@ -153,7 +153,7 @@ panthor_kernel_bo_vunmap(struct panthor_kernel_bo *bo) > struct panthor_kernel_bo * > panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm, > size_t size, u32 bo_flags, u32 vm_map_flags, > - u64 gpu_va); > + u64 gpu_va, const char *name); > > void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo); > > diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c > index 3bdf61c14264..d236e9ceade4 100644 > --- a/drivers/gpu/drm/panthor/panthor_heap.c > +++ b/drivers/gpu/drm/panthor/panthor_heap.c > @@ -151,7 +151,8 @@ static int panthor_alloc_heap_chunk(struct panthor_heap_pool *pool, > chunk->bo = panthor_kernel_bo_create(pool->ptdev, pool->vm, heap->chunk_size, > DRM_PANTHOR_BO_NO_MMAP, > DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC, > - PANTHOR_VM_KERNEL_AUTO_VA); > + PANTHOR_VM_KERNEL_AUTO_VA, > + "Tiler heap chunk"); > if (IS_ERR(chunk->bo)) { > ret = PTR_ERR(chunk->bo); > goto err_free_chunk; > @@ -555,7 +556,8 @@ panthor_heap_pool_create(struct panthor_device *ptdev, struct panthor_vm *vm) > pool->gpu_contexts = panthor_kernel_bo_create(ptdev, vm, bosize, > DRM_PANTHOR_BO_NO_MMAP, > DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC, > - PANTHOR_VM_KERNEL_AUTO_VA); > + PANTHOR_VM_KERNEL_AUTO_VA, > + "Heap pool"); > if (IS_ERR(pool->gpu_contexts)) { > ret = PTR_ERR(pool->gpu_contexts); > goto err_destroy_pool; > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c > index 446ec780eb4a..43ee57728de5 100644 > --- a/drivers/gpu/drm/panthor/panthor_sched.c > +++ b/drivers/gpu/drm/panthor/panthor_sched.c > @@ -3332,7 +3332,8 @@ group_create_queue(struct panthor_group *group, > DRM_PANTHOR_BO_NO_MMAP, > DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC | > DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED, > - PANTHOR_VM_KERNEL_AUTO_VA); > + PANTHOR_VM_KERNEL_AUTO_VA, > + "CS ring buffer"); > if (IS_ERR(queue->ringbuf)) { > ret = PTR_ERR(queue->ringbuf); > goto err_free_queue; > @@ -3362,7 +3363,8 @@ group_create_queue(struct panthor_group *group, > DRM_PANTHOR_BO_NO_MMAP, > DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC | > DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED, > - PANTHOR_VM_KERNEL_AUTO_VA); > + PANTHOR_VM_KERNEL_AUTO_VA, > + "Group job stats"); > > if (IS_ERR(queue->profiling.slots)) { > ret = PTR_ERR(queue->profiling.slots); > @@ -3493,7 +3495,8 @@ int panthor_group_create(struct panthor_file *pfile, > DRM_PANTHOR_BO_NO_MMAP, > DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC | > DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED, > - PANTHOR_VM_KERNEL_AUTO_VA); > + PANTHOR_VM_KERNEL_AUTO_VA, > + "Group sync objects"); > if (IS_ERR(group->syncobjs)) { > ret = PTR_ERR(group->syncobjs); > goto err_put_group; > -- > 2.48.1 > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯

1 month, 4 weeks

1
0
0 0

Re: [PATCH v6 1/4] drm/panthor: Introduce BO labeling

by Liviu Dudau

On Wed, Apr 09, 2025 at 10:22:19PM +0100, Adrián Larumbe wrote: > Add a new character string Panthor BO field, and a function that allows > setting it from within the driver. > > Driver takes care of freeing the string when it's replaced or no longer > needed at object destruction time, but allocating it is the responsibility > of callers. > > Signed-off-by: Adrián Larumbe <adrian.larumbe(a)collabora.com> > Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> Reviewed-by: Liviu Dudau <liviu.dudau(a)arm.com> Best regards, Liviu > --- > drivers/gpu/drm/panthor/panthor_gem.c | 39 +++++++++++++++++++++++++++ > drivers/gpu/drm/panthor/panthor_gem.h | 17 ++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c > index 8244a4e6c2a2..af0ac17f357f 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.c > +++ b/drivers/gpu/drm/panthor/panthor_gem.c > @@ -2,6 +2,7 @@ > /* Copyright 2019 Linaro, Ltd, Rob Herring <robh(a)kernel.org> */ > /* Copyright 2023 Collabora ltd. */ > > +#include <linux/cleanup.h> > #include <linux/dma-buf.h> > #include <linux/dma-mapping.h> > #include <linux/err.h> > @@ -18,6 +19,14 @@ static void panthor_gem_free_object(struct drm_gem_object *obj) > struct panthor_gem_object *bo = to_panthor_bo(obj); > struct drm_gem_object *vm_root_gem = bo->exclusive_vm_root_gem; > > + /* > + * Label might have been allocated with kstrdup_const(), > + * we need to take that into account when freeing the memory > + */ > + kfree_const(bo->label.str); > + > + mutex_destroy(&bo->label.lock); > + > drm_gem_free_mmap_offset(&bo->base.base); > mutex_destroy(&bo->gpuva_list_lock); > drm_gem_shmem_free(&bo->base); > @@ -196,6 +205,7 @@ struct drm_gem_object *panthor_gem_create_object(struct drm_device *ddev, size_t > obj->base.map_wc = !ptdev->coherent; > mutex_init(&obj->gpuva_list_lock); > drm_gem_gpuva_set_lock(&obj->base.base, &obj->gpuva_list_lock); > + mutex_init(&obj->label.lock); > > return &obj->base.base; > } > @@ -247,3 +257,32 @@ panthor_gem_create_with_handle(struct drm_file *file, > > return ret; > } > + > +void > +panthor_gem_bo_set_label(struct drm_gem_object *obj, const char *label) > +{ > + struct panthor_gem_object *bo = to_panthor_bo(obj); > + const char *old_label; > + > + scoped_guard(mutex, &bo->label.lock) { > + old_label = bo->label.str; > + bo->label.str = label; > + } > + > + kfree(old_label); > +} > + > +void > +panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const char *label) > +{ > + const char *str; > + > + str = kstrdup_const(label, GFP_KERNEL); > + if (!str) { > + /* Failing to allocate memory for a label isn't a fatal condition */ > + drm_warn(bo->obj->dev, "Not enough memory to allocate BO label"); > + return; > + } > + > + panthor_gem_bo_set_label(bo->obj, str); > +} > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h > index 1a363bb814f4..af0d77338860 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.h > +++ b/drivers/gpu/drm/panthor/panthor_gem.h > @@ -46,6 +46,20 @@ struct panthor_gem_object { > > /** @flags: Combination of drm_panthor_bo_flags flags. */ > u32 flags; > + > + /** > + * @label: BO tagging fields. The label can be assigned within the > + * driver itself or through a specific IOCTL. > + */ > + struct { > + /** > + * @label.str: Pointer to NULL-terminated string, > + */ > + const char *str; > + > + /** @lock.str: Protects access to the @label.str field. */ > + struct mutex lock; > + } label; > }; > > /** > @@ -91,6 +105,9 @@ panthor_gem_create_with_handle(struct drm_file *file, > struct panthor_vm *exclusive_vm, > u64 *size, u32 flags, uint32_t *handle); > > +void panthor_gem_bo_set_label(struct drm_gem_object *obj, const char *label); > +void panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const char *label); > + > static inline u64 > panthor_kernel_bo_gpuva(struct panthor_kernel_bo *bo) > { > -- > 2.48.1 > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯

1 month, 4 weeks

1
0
0 0

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

by Christian König

Am 10.04.25 um 11:24 schrieb Philipp Stanner: > Nouveau currently relies on the assumption that dma_fences will only > ever get signaled through nouveau_fence_signal(), which takes care of > removing a signaled fence from the list nouveau_fence_chan.pending. > > This self-imposed rule is violated in nouveau_fence_done(), where > dma_fence_is_signaled() (somewhat surprisingly, considering its name) > can signal the fence without removing it from the list. This enables > accesses to already signaled fences through the list, which is a bug. > > In particular, it can race with nouveau_fence_context_kill(), which > would then attempt to set an error code on an already signaled fence, > which is illegal. > > In nouveau_fence_done(), the call to nouveau_fence_update() already > ensures to signal all ready fences. Thus, the signaling potentially > performed by dma_fence_is_signaled() is actually not necessary. Ah, I now got what you are trying to do here! But that won't help. The problem is it is perfectly valid for somebody external (e.g. other driver, TTM etc...) to call dma_fence_is_signaled() on a nouveau fence. This will then in turn still signal the fence and leave it on the pending list and creating the problem you have. 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

1 month, 4 weeks

1
0
0 0

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

by Christian König

Am 10.04.25 um 14:21 schrieb Danilo Krummrich: > On Thu, Apr 10, 2025 at 02:13:34PM +0200, Christian König wrote: >> Am 10.04.25 um 11:24 schrieb Philipp Stanner: >>> Nouveau currently relies on the assumption that dma_fences will only >>> ever get signaled through nouveau_fence_signal(), which takes care of >>> removing a signaled fence from the list nouveau_fence_chan.pending. >>> >>> This self-imposed rule is violated in nouveau_fence_done(), where >>> dma_fence_is_signaled() (somewhat surprisingly, considering its name) >>> can signal the fence without removing it from the list. This enables >>> accesses to already signaled fences through the list, which is a bug. >>> >>> In particular, it can race with nouveau_fence_context_kill(), which >>> would then attempt to set an error code on an already signaled fence, >>> which is illegal. >>> >>> In nouveau_fence_done(), the call to nouveau_fence_update() already >>> ensures to signal all ready fences. Thus, the signaling potentially >>> performed by dma_fence_is_signaled() is actually not necessary. >>> >>> 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); >> See the code above that: >> >> if (fence->base.ops == &nouveau_fence_ops_legacy || >> fence->base.ops == &nouveau_fence_ops_uevent) { > I think this check is a bit pointless given that fence is already a struct > nouveau_fence. :) Oh, good point. I totally missed that. In this case that indeed doesn't make any sense at all. (Unless somebody just blindly upcasted the structure, but I really hope that this isn't the case here). Regards, Christian.

1 month, 4 weeks

1
0
0 0

Re: [PATCH 0/3] drm/nouveau: Fix & improve nouveau_fence_done()

by Christian König

Am 10.04.25 um 11:51 schrieb Philipp Stanner: > On Thu, 2025-04-10 at 11:24 +0200, Philipp Stanner wrote: >> Contains two patches improving nouveau_fence_done(), and one >> addressing >> an actual bug (race): > Oops, that's the wrong calltrace. Here we go: > > [ 85.791794] Call Trace: [ 85.791796] <TASK> [ 85.791797] ? nouveau_fence_context_kill (/home/imperator/linux/./include/linux/dma-fence.h:587 (discriminator 9) /home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_fence.c:94 (discriminator 9)) nouveau [ 85.791874] ? __warn.cold (/home/imperator/linux/kernel/panic.c:748) [ 85.791878] ? nouveau_fence_context_kill (/home/imperator/linux/./include/linux/dma-fence.h:587 (discriminator 9) /home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_fence.c:94 (discriminator 9)) nouveau [ 85.791950] ? report_bug (/home/imperator/linux/lib/bug.c:180 /home/imperator/linux/lib/bug.c:219) [ 85.791953] ? handle_bug (/home/imperator/linux/arch/x86/kernel/traps.c:260) [ 85.791956] ? exc_invalid_op (/home/imperator/linux/arch/x86/kernel/traps.c:309 (discriminator 1)) [ 85.791957] ? asm_exc_invalid_op (/home/imperator/linux/./arch/x86/include/asm/idtentry.h:621) [ 85.791960] ? nouveau_fence_context_kill (/home/imperator/linux/./include/linux/dma-fence.h:587 (discriminator 9) /home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_fence.c:94 (discriminator 9)) nouveau [ 85.792028] drm_sched_fini.cold (/home/imperator/linux/./include/trace/../../drivers/gpu/drm/scheduler/gpu_scheduler_trace.h:72 (discriminator 1)) gpu_sched [ 85.792033] ? drm_sched_entity_kill.part.0 (/home/imperator/linux/drivers/gpu/drm/scheduler/sched_entity.c:243 (discriminator 2)) gpu_sched [ 85.792037] nouveau_sched_destroy (/home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_sched.c:509 /home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_sched.c:518) nouveau [ 85.792122] nouveau_abi16_chan_fini.isra.0 (/home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_abi16.c:188) nouveau [ 85.792191] nouveau_abi16_fini (/home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_abi16.c:224 (discriminator 3)) nouveau [ 85.792263] nouveau_drm_postclose (/home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_drm.c:1240) nouveau [ 85.792349] drm_file_free (/home/imperator/linux/drivers/gpu/drm/drm_file.c:255) [ 85.792353] drm_release (/home/imperator/linux/./arch/x86/include/asm/atomic.h:67 (discriminator 1) /home/imperator/linux/./include/linux/atomic/atomic-arch-fallback.h:2278 (discriminator 1) /home/imperator/linux/./include/linux/atomic/atomic-instrumented.h:1384 (discriminator 1) /home/imperator/linux/drivers/gpu/drm/drm_file.c:428 (discriminator 1)) [ 85.792355] __fput (/home/imperator/linux/fs/file_table.c:464) [ 85.792357] task_work_run (/home/imperator/linux/kernel/task_work.c:227) [ 85.792360] do_exit (/home/imperator/linux/kernel/exit.c:939) [ 85.792362] do_group_exit (/home/imperator/linux/kernel/exit.c:1069) [ 85.792364] get_signal (/home/imperator/linux/kernel/signal.c:3036) [ 85.792366] arch_do_signal_or_restart (/home/imperator/linux/./arch/x86/include/asm/syscall.h:38 /home/imperator/linux/arch/x86/kernel/signal.c:264 /home/imperator/linux/arch/x86/kernel/signal.c:339) [ 85.792369] syscall_exit_to_user_mode (/home/imperator/linux/kernel/entry/common.c:113 /home/imperator/linux/./include/linux/entry-common.h:329 /home/imperator/linux/kernel/entry/common.c:207 /home/imperator/linux/kernel/entry/common.c:218) [ 85.792372] do_syscall_64 (/home/imperator/linux/./arch/x86/include/asm/cpufeature.h:172 /home/imperator/linux/arch/x86/entry/common.c:98) [ 85.792373] ? syscall_exit_to_user_mode_prepare (/home/imperator/linux/./include/linux/audit.h:357 /home/imperator/linux/kernel/entry/common.c:166 /home/imperator/linux/kernel/entry/common.c:200) [ 85.792376] ? syscall_exit_to_user_mode (/home/imperator/linux/./arch/x86/include/asm/paravirt.h:686 /home/imperator/linux/./include/linux/entry-common.h:232 /home/imperator/linux/kernel/entry/common.c:206 /home/imperator/linux/kernel/entry/common.c:218) [ 85.792377] ? do_syscall_64 (/home/imperator/linux/./arch/x86/include/asm/cpufeature.h:172 /home/imperator/linux/arch/x86/entry/common.c:98) [ 85.792378] entry_SYSCALL_64_after_hwframe (/home/imperator/linux/arch/x86/entry/entry_64.S:130) [ 85.792381] RIP: 0033:0x7ff950b6af70 [ 85.792383] Code: Unable to access opcode bytes at 0x7ff950b6af46. objdump: '/tmp/tmp.sfPRl5k2te.o': No such file Code starting with the faulting instruction =========================================== [ 85.792383] RSP: 002b:00007ff93cdfb6f0 EFLAGS: 00000293 ORIG_RAX: 000000000000010f [ 85.792385] RAX: fffffffffffffdfe RBX: 000055d386d61870 RCX: 00007ff950b6af70 [ 85.792386] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 00007ff928000b90 [ 85.792387] RBP: 00007ff93cdfb740 R08: 0000000000000008 R09: 0000000000000000 [ 85.792388] R10: 0000000000000000 R11: 0000000000000293 R12: 0000000000000001 [ 85.792388] R13: 0000000000000000 R14: 0000000000000000 R15: 00007ff951b10b40 [ 85.792390] </TASK> [ 85.792391] ---[ end trace 0000000000000000 ]--- I think I understand the problem now as well, but that backtrace is completely mangled in the mail. It would be nice if you could send that out again. Thanks, Christian. > > By the way, for reference: > I did try whether it could be done to have nouveau_fence_signal() > incorporated into nouveau_fence_update() and nouveau_fence_done(). > This, however, would then cause a race with the list_del() in > nouveau_fence_no_signaling(), WARNing because of the list poison. > > So the "solution" space is: > * A cleanup callback on the dma_fence. > * Keeping the current race or > * replacing it with another race with another function. > * Just preventing nouveau_fence_done() from signaling fences other > than through nouveau_fence_update/signal > > The later seems clearly like the cleanest solution to me. Alternative > would be a work-intensive rework of all the misdesigns broken in > nouveau_fence.c > > > P. > >> [ 39.848463] WARNING: CPU: 21 PID: 1734 at >> drivers/gpu/drm/nouveau/nouveau_fence.c:509 >> nouveau_fence_no_signaling+0xac/0xd0 [nouveau] >> [ 39.848551] Modules linked in: snd_seq_dummy snd_hrtimer >> nf_conntrack_netbios_ns nf_conntrack_broadcast nft_fib_inet >> nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_ine >> t nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat >> nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set >> nf_tables qrtr sunrpc snd_sof_pci_intel_ >> tgl snd_sof_pci_intel_cnl snd_sof_intel_hda_generic snd_sof_pci >> snd_sof_xtensa_dsp snd_sof_intel_hda_common snd_soc_hdac_hda >> snd_sof_intel_hda snd_sof snd_sof_utils snd >> _soc_acpi_intel_match snd_soc_acpi snd_soc_acpi_intel_sdca_quirks >> snd_sof_intel_hda_mlink snd_soc_sdca snd_soc_avs snd_ctl_led >> snd_soc_hda_codec intel_rapl_msr snd_hda_ >> codec_realtek snd_hda_ext_core intel_rapl_common >> snd_hda_codec_generic snd_soc_core snd_hda_scodec_component >> intel_uncore_frequency intel_uncore_frequency_common snd_hd >> a_codec_hdmi intel_ifs snd_compress i10nm_edac skx_edac_common nfit >> snd_hda_intel snd_intel_dspcfg libnvdimm snd_hda_codec binfmt_misc >> snd_hwdep snd_hda_core snd_seq sn >> d_seq_device dell_wmi >> [ 39.848575] dell_pc x86_pkg_temp_thermal spi_nor platform_profile >> sparse_keymap intel_powerclamp dax_hmem snd_pcm cxl_acpi coretemp >> cxl_port iTCO_wdt mtd rapl intel >> _pmc_bxt pmt_telemetry cxl_core dell_wmi_sysman pmt_class >> iTCO_vendor_support snd_timer isst_if_mmio vfat intel_cstate >> dell_smbios dcdbas fat dell_wmi_ddv dell_smm_hwmo >> n dell_wmi_descriptor firmware_attributes_class wmi_bmof intel_uncore >> einj pcspkr isst_if_mbox_pci atlantic snd isst_if_common intel_vsec >> e1000e macsec mei_me i2c_i801 >> spi_intel_pci soundcore i2c_smbus spi_intel mei joydev loop nfnetlink >> zram nouveau drm_ttm_helper ttm polyval_clmulni iaa_crypto gpu_sched >> polyval_generic rtsx_pci_sdmm >> c ghash_clmulni_intel i2c_algo_bit mmc_core drm_gpuvm sha512_ssse3 >> nvme drm_exec drm_display_helper sha256_ssse3 idxd sha1_ssse3 cec >> nvme_core idxd_bus rtsx_pci nvme_au >> th pinctrl_alderlake ip6_tables ip_tables fuse >> [ 39.848603] CPU: 21 UID: 42 PID: 1734 Comm: gnome-shell Tainted: >> G W 6.14.0-rc4+ #11 >> [ 39.848605] Tainted: [W]=WARN >> [ 39.848606] Hardware name: Dell Inc. Precision 7960 Tower/01G0M6, >> BIOS 2.7.0 12/17/2024 >> [ 39.848607] RIP: 0010:nouveau_fence_no_signaling+0xac/0xd0 >> [nouveau] >> [ 39.848688] Code: db 74 17 48 8d 7b 38 b8 ff ff ff ff f0 0f c1 43 >> 38 83 f8 01 74 29 85 c0 7e 17 31 c0 5b 5d c3 cc cc cc cc e8 76 b2 c5 >> f0 eb 96 <0f> 0b e9 67 ff ff f >> f be 03 00 00 00 e8 83 76 33 f1 31 c0 eb dd e8 >> [ 39.848690] RSP: 0018:ff1cc1ffc5c039f0 EFLAGS: 00010046 >> [ 39.848691] RAX: 0000000000000001 RBX: ff175a3b504da980 RCX: >> ff175a3b4801e008 >> [ 39.848692] RDX: ff175a3b43e7bad0 RSI: ffffffffc09d3fda RDI: >> ff175a3b504da980 >> [ 39.848693] RBP: ff175a3b504da9c0 R08: ffffffffc09e39df R09: >> 0000000000000001 >> [ 39.848694] R10: 0000000000000001 R11: 0000000000000000 R12: >> ff175a3b6d97de00 >> [ 39.848695] R13: 0000000000000246 R14: ff1cc1ffc5c03c60 R15: >> 0000000000000001 >> [ 39.848696] FS: 00007fc5477846c0(0000) GS:ff175a5a50280000(0000) >> knlGS:0000000000000000 >> [ 39.848698] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> [ 39.848699] CR2: 000055cb7613d1a8 CR3: 000000012e5ce004 CR4: >> 0000000000f71ef0 >> [ 39.848700] DR0: 0000000000000000 DR1: 0000000000000000 DR2: >> 0000000000000000 >> [ 39.848701] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: >> 0000000000000400 >> [ 39.848702] PKRU: 55555554 >> [ 39.848703] Call Trace: >> [ 39.848704] <TASK> >> [ 39.848705] ? nouveau_fence_no_signaling+0xac/0xd0 [nouveau] >> [ 39.848782] ? __warn.cold+0x93/0xfa >> [ 39.848785] ? nouveau_fence_no_signaling+0xac/0xd0 [nouveau] >> [ 39.848861] ? report_bug+0xff/0x140 >> [ 39.848863] ? handle_bug+0x58/0x90 >> [ 39.848865] ? exc_invalid_op+0x17/0x70 >> [ 39.848866] ? asm_exc_invalid_op+0x1a/0x20 >> [ 39.848870] ? nouveau_fence_no_signaling+0xac/0xd0 [nouveau] >> [ 39.848943] nouveau_fence_enable_signaling+0x32/0x80 [nouveau] >> [ 39.849016] ? __pfx_nouveau_fence_cleanup_cb+0x10/0x10 [nouveau] >> [ 39.849088] __dma_fence_enable_signaling+0x33/0xc0 >> [ 39.849090] dma_fence_add_callback+0x4b/0xd0 >> [ 39.849093] nouveau_fence_emit+0xa3/0x260 [nouveau] >> [ 39.849166] nouveau_fence_new+0x7d/0xf0 [nouveau] >> [ 39.849242] nouveau_gem_ioctl_pushbuf+0xe8f/0x1300 [nouveau] >> [ 39.849338] ? __pfx_nouveau_gem_ioctl_pushbuf+0x10/0x10 [nouveau] >> [ 39.849431] drm_ioctl_kernel+0xad/0x100 >> [ 39.849433] drm_ioctl+0x288/0x550 >> [ 39.849435] ? __pfx_nouveau_gem_ioctl_pushbuf+0x10/0x10 [nouveau] >> [ 39.849526] nouveau_drm_ioctl+0x57/0xb0 [nouveau] >> [ 39.849620] __x64_sys_ioctl+0x94/0xc0 >> [ 39.849621] do_syscall_64+0x82/0x160 >> [ 39.849623] ? drm_ioctl+0x2b7/0x550 >> [ 39.849625] ? __pfx_nouveau_gem_ioctl_pushbuf+0x10/0x10 [nouveau] >> [ 39.849719] ? ktime_get_mono_fast_ns+0x38/0xd0 >> [ 39.849721] ? __pm_runtime_suspend+0x69/0xc0 >> [ 39.849724] ? syscall_exit_to_user_mode_prepare+0x15e/0x1a0 >> [ 39.849726] ? syscall_exit_to_user_mode+0x10/0x200 >> [ 39.849729] ? do_syscall_64+0x8e/0x160 >> [ 39.849730] ? exc_page_fault+0x7e/0x1a0 >> [ 39.849733] entry_SYSCALL_64_after_hwframe+0x76/0x7e >> [ 39.849735] RIP: 0033:0x7fc5576fe0ad >> [ 39.849736] Code: 04 25 28 00 00 00 48 89 45 c8 31 c0 48 8d 45 10 >> c7 45 b0 10 00 00 00 48 89 45 b8 48 8d 45 d0 48 89 45 c0 b8 10 00 00 >> 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1a 48 8b 45 c8 64 48 2b 04 25 28 >> 00 00 00 >> [ 39.849737] RSP: 002b:00007ffc002688a0 EFLAGS: 00000246 ORIG_RAX: >> 0000000000000010 >> [ 39.849739] RAX: ffffffffffffffda RBX: 000055cb74e316c0 RCX: >> 00007fc5576fe0ad >> [ 39.849740] RDX: 00007ffc00268960 RSI: 00000000c0406481 RDI: >> 000000000000000e >> [ 39.849741] RBP: 00007ffc002688f0 R08: 0000000000000000 R09: >> 000055cb74e35560 >> [ 39.849742] R10: 0000000000000014 R11: 0000000000000246 R12: >> 00007ffc00268960 >> [ 39.849744] R13: 00000000c0406481 R14: 000000000000000e R15: >> 000055cb74e3cd10 >> [ 39.849746] </TASK> >> [ 39.849746] ---[ end trace 0000000000000000 ]--- >> [ 39.849776] ------------[ cut here ]------------ >> >> >> This is the first WARN_ON() in dma_fence_set_error(), called by >> nouveau_fence_context_kill(). >> >> It's rare, but it is a bug, or rather: the archetype of a race, since >> (as Christian pointed out) nouveau_fence_update() later at some point >> will remove the signaled fence (by signaling it again). >> >> >> P. >> >> >> Philipp Stanner (3): >> drm/nouveau: Prevent signaled fences in pending list >> drm/nouveau: Remove surplus if-branch >> drm/nouveau: Add helper to check base fence >> >> drivers/gpu/drm/nouveau/nouveau_fence.c | 32 ++++++++++++++--------- >> -- >> 1 file changed, 18 insertions(+), 14 deletions(-) >>

1 month, 4 weeks

1
0
0 0

Re: [PATCH 2/3] drm/nouveau: Remove surplus if-branch

by Christian König

Am 10.04.25 um 11:24 schrieb Philipp Stanner: > nouveau_fence_done() contains an if-branch which checks for the > existence of either of two fence backend ops. Those two are the only > backend ops existing in Nouveau, however; and at least one backend ops > must be in use for the entire driver to be able to work. The if branch > is, therefore, surplus. > > Remove the if-branch. What happens here is that nouveau checks if the fence comes from itself or some external source. So when you remove that check you potentially illegally uses nouveau_fctx() on a non-nouveau fence. Regards, Christian. > > Signed-off-by: Philipp Stanner <phasta(a)kernel.org> > --- > drivers/gpu/drm/nouveau/nouveau_fence.c | 24 +++++++++++------------- > 1 file changed, 11 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c > index 33535987d8ed..db6f4494405c 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > @@ -259,21 +259,19 @@ nouveau_fence_emit(struct nouveau_fence *fence) > bool > nouveau_fence_done(struct nouveau_fence *fence) > { > - if (fence->base.ops == &nouveau_fence_ops_legacy || > - fence->base.ops == &nouveau_fence_ops_uevent) { > - struct nouveau_fence_chan *fctx = nouveau_fctx(fence); > - struct nouveau_channel *chan; > - unsigned long flags; > + struct nouveau_fence_chan *fctx = nouveau_fctx(fence); > + struct nouveau_channel *chan; > + unsigned long flags; > > - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags)) > - return true; > + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags)) > + return true; > + > + spin_lock_irqsave(&fctx->lock, flags); > + chan = rcu_dereference_protected(fence->channel, lockdep_is_held(&fctx->lock)); > + if (chan && nouveau_fence_update(chan, fctx)) > + nvif_event_block(&fctx->event); > + spin_unlock_irqrestore(&fctx->lock, flags); > > - spin_lock_irqsave(&fctx->lock, flags); > - chan = rcu_dereference_protected(fence->channel, lockdep_is_held(&fctx->lock)); > - if (chan && nouveau_fence_update(chan, fctx)) > - nvif_event_block(&fctx->event); > - spin_unlock_irqrestore(&fctx->lock, flags); > - } > return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags); > } >

1 month, 4 weeks

1
0
0 0

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

by Christian König

Am 10.04.25 um 11:24 schrieb Philipp Stanner: > Nouveau currently relies on the assumption that dma_fences will only > ever get signaled through nouveau_fence_signal(), which takes care of > removing a signaled fence from the list nouveau_fence_chan.pending. > > This self-imposed rule is violated in nouveau_fence_done(), where > dma_fence_is_signaled() (somewhat surprisingly, considering its name) > can signal the fence without removing it from the list. This enables > accesses to already signaled fences through the list, which is a bug. > > In particular, it can race with nouveau_fence_context_kill(), which > would then attempt to set an error code on an already signaled fence, > which is illegal. > > In nouveau_fence_done(), the call to nouveau_fence_update() already > ensures to signal all ready fences. Thus, the signaling potentially > performed by dma_fence_is_signaled() is actually not necessary. > > 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); See the code above that: if (fence->base.ops == &nouveau_fence_ops_legacy || fence->base.ops == &nouveau_fence_ops_uevent) { .... Nouveau first tests if it's one of it's own fences, and if yes does some special handling. E.g. checking the fence status bits etc... So this dma_fence_is_signaled() is for all non-nouveau fences and then not touching the internal flags is perfectly correct as far as I can see. Regards, Christian. > } > > static long

1 month, 4 weeks

1
0
0 0

Re: [PATCH 1/2] dma-fence: Rename dma_fence_is_signaled()

by Christian König

Am 09.04.25 um 17:04 schrieb Philipp Stanner: > On Wed, 2025-04-09 at 16:10 +0200, Christian König wrote: >>> I only see improvement by making things more obvious. >>> >>> In any case, how would you call a wrapper that just does >>> test_bit(IS_SIGNALED, …) ? >> Broken, that was very intentionally removed quite shortly after we >> created the framework. >> >> We have a few cases were implementations do check that for their >> fences, but consumers should never be allowed to touch such >> internals. > There is theory and there is practice. In practice, those internals are > being used by Nouveau, i915, Xe, vmgfx and radeon. What do you mean? I only skimmed over the use cases, but as far as I can see those are all valid. You can test the flag if you know what the fence means to you, that is not a problem at all. > So it seems that we failed quite a bit at communicating clearly how the > interface should be used. > > And, to repeat myself, with both name and docu of that function, I > think it is very easy to misunderstand what it's doing. You say that it > shouldn't matter – and maybe that's true, in theory. In practice, it > does matter. In practice, APIs get misused and have side-effects. And > making that harder is desirable. That sounds like I didn't used the right wording. It *must* not matter to the consumer. See the purpose of the DMA-fence framework is to make it irrelevant for the consumer how the provider has implemented it's fences. This means that things like if polling or interrupt driven signaling is used, 32bit vs 64bit seq numbers, etc... should all be hidden by the framework from the consumer of the fences. BTW I'm actually not sure if nouveau has a bug here. As far as I can see nouveau_fence_signal() will be called later eventually and do the necessary cleanup. But on the other hand it wouldn't surprise me if nouveau has a bug with that. The driver has been basically only barely maintained for quite a while. > In any case, I might have to add another such call to Nouveau, because > the solution preferred by you over the callback causes another race. > Certainly one could solve this in a clean way, but someone has to do > the work, and we're talking about more than a few hours here. Well this is not my preferred solution, it's just the technical correct solution as far as I can see. > In any case, be so kind and look at patch 2 and tell me there if you're > at least OK with making the documentation more detailed. As far as I can see that is clearly the wrong place to document that stuff. Regards, Christian. > > P.

1 month, 4 weeks

1
0
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig