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

Re: [PATCH] dma-buf: insert memory barrier before updating num_fences

by Christian König

On 5/13/25 04:06, Hyejeong Choi wrote: > smp_store_mb() inserts memory barrier after storing operation. > It is different with what the comment is originally aiming so Null > pointer dereference can be happened if memory update is reordered. > > Signed-off-by: Hyejeong Choi <hjeong.choi(a)samsung.com> I've reviewed, add CC stable and Fixes tags and pushed it to drm-misc-fixes. Thanks, Christian. > --- > drivers/dma-buf/dma-resv.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index 5f8d010516f0..b1ef4546346d 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c > @@ -320,8 +320,9 @@ void dma_resv_add_fence(struct dma_resv *obj, struct dma_fence *fence, > count++; > > dma_resv_list_set(fobj, i, fence, usage); > - /* pointer update must be visible before we extend the num_fences */ > - smp_store_mb(fobj->num_fences, count); > + /* fence update must be visible before we extend the num_fences */ > + smp_wmb(); > + fobj->num_fences = count; > } > EXPORT_SYMBOL(dma_resv_add_fence); > > >

7 months

1
0
0 0

Re: [PATCH 2/2] dmabuf/heaps: implement DMA_BUF_IOCTL_RW_FILE for system_heap

by Christian König

On 5/14/25 13:02, wangtao wrote: >> -----Original Message----- >> From: Christian König <christian.koenig(a)amd.com> >> Sent: Tuesday, May 13, 2025 9:18 PM >> To: wangtao <tao.wangtao(a)honor.com>; sumit.semwal(a)linaro.org; >> benjamin.gaignard(a)collabora.com; Brian.Starkey(a)arm.com; >> jstultz(a)google.com; tjmercier(a)google.com >> Cc: linux-media(a)vger.kernel.org; dri-devel(a)lists.freedesktop.org; linaro- >> mm-sig(a)lists.linaro.org; linux-kernel(a)vger.kernel.org; >> wangbintian(BintianWang) <bintian.wang(a)honor.com>; yipengxiang >> <yipengxiang(a)honor.com>; <liulu.liu(a)honor.com>; <feng.han(a)honor.com> >> Subject: Re: [PATCH 2/2] dmabuf/heaps: implement >> DMA_BUF_IOCTL_RW_FILE for system_heap >> >> On 5/13/25 14:30, wangtao wrote: >>>> -----Original Message----- >>>> From: Christian König <christian.koenig(a)amd.com> >>>> Sent: Tuesday, May 13, 2025 7:32 PM >>>> To: wangtao <tao.wangtao(a)honor.com>; sumit.semwal(a)linaro.org; >>>> benjamin.gaignard(a)collabora.com; Brian.Starkey(a)arm.com; >>>> jstultz(a)google.com; tjmercier(a)google.com >>>> Cc: linux-media(a)vger.kernel.org; dri-devel(a)lists.freedesktop.org; >>>> linaro- mm-sig(a)lists.linaro.org; linux-kernel(a)vger.kernel.org; >>>> wangbintian(BintianWang) <bintian.wang(a)honor.com>; yipengxiang >>>> <yipengxiang(a)honor.com>; <liulu.liu(a)honor.com>; >>>> <feng.han(a)honor.com> >>>> Subject: Re: [PATCH 2/2] dmabuf/heaps: implement >>>> DMA_BUF_IOCTL_RW_FILE for system_heap >>>> >>>> On 5/13/25 11:28, wangtao wrote: >>>>> Support direct file I/O operations for system_heap dma-buf objects. >>>>> Implementation includes: >>>>> 1. Convert sg_table to bio_vec >>>> >>>> That is usually illegal for DMA-bufs. >>> [wangtao] The term 'convert' is misleading in this context. The appropriate >> phrasing should be: Construct bio_vec from sg_table. >> >> Well it doesn't matter what you call it. Touching the page inside an sg table of >> a DMA-buf is illegal, we even have code to actively prevent that. > [wangtao] For a driver using DMA-buf: Don't touch pages in the sg_table. But the system heap exporter (sg_table owner) should be allowed to use them. Good point that might be possible. > If a driver takes ownership via dma_buf_map_attachment or similar calls, the exporter must stop using the sg_table. > User-space programs should call DMA_BUF_IOCTL_RW_FILE only when the DMA-buf is not attached. > The exporter must check ownership (e.g., ensure no map_dma_buf/vmap is active) and block new calls during operations. > I'll add these checks in patch v2. > >> >> Once more: This approach was already rejected multiple times! Please use >> udmabuf instead! >> >> The hack you came up here is simply not necessary. > [wangtao] Many people need DMA-buf direct I/O. I tried it 2 years ago. My method is simpler, uses less CPU/power, and performs better: I don't think that this is a valid argument. > - Speed: 3418 MB/s vs. 2073 MB/s (udmabuf) at 1GHz CPU. > - udmabuf wastes half its CPU time on __get_user_pages. > - Creating 32x32MB DMA-bufs + reading 1GB file takes 346 ms vs. 1145 ms for udmabuf (10x slower) vs. 1503 ms for DMA-buf normal. Why would using udmabuf be slower here? > udmabuf is slightly faster but not enough. Switching to udmabuf is easy for small apps but hard in complex systems without major benefits. Yeah, but your approach here is a rather clear hack. Using udmabuf is much more cleaner and generally accepted by everybody now. As far as I can see I have to reject your approach here. Regards, Christian. >> >> Regards, >> Christian. >> >> >>> Appreciate your feedback. >>>> >>>> Regards, >>>> Christian. >>>> >>>>> 2. Set IOCB_DIRECT when O_DIRECT is supported 3. Invoke >>>>> vfs_iocb_iter_read()/vfs_iocb_iter_write() for actual I/O >>>>> >>>>> Performance metrics (UFS 4.0 device @4GB/s, Arm64 CPU @1GHz): >>>>> >>>>> | Metric | 1MB | 8MB | 64MB | 1024MB | 3072MB | >>>>> |--------------------|-------:|-------:|--------:|---------:|------- >>>>> |--------------------|-- >>>>> |--------------------|:| >>>>> | Buffer Read (us) | 1658 | 9028 | 69295 | 1019783 | 2978179 | >>>>> | Direct Read (us) | 707 | 2647 | 18689 | 299627 | 937758 | >>>>> | Buffer Rate (MB/s) | 603 | 886 | 924 | 1004 | 1032 | >>>>> | Direct Rate (MB/s) | 1414 | 3022 | 3425 | 3418 | 3276 | >>>>> >>>>> Signed-off-by: wangtao <tao.wangtao(a)honor.com> >>>>> --- >>>>> drivers/dma-buf/heaps/system_heap.c | 118 >>>>> ++++++++++++++++++++++++++++ >>>>> 1 file changed, 118 insertions(+) >>>>> >>>>> diff --git a/drivers/dma-buf/heaps/system_heap.c >>>>> b/drivers/dma-buf/heaps/system_heap.c >>>>> index 26d5dc89ea16..f7b71b9843aa 100644 >>>>> --- a/drivers/dma-buf/heaps/system_heap.c >>>>> +++ b/drivers/dma-buf/heaps/system_heap.c >>>>> @@ -20,6 +20,8 @@ >>>>> #include <linux/scatterlist.h> >>>>> #include <linux/slab.h> >>>>> #include <linux/vmalloc.h> >>>>> +#include <linux/bvec.h> >>>>> +#include <linux/uio.h> >>>>> >>>>> static struct dma_heap *sys_heap; >>>>> >>>>> @@ -281,6 +283,121 @@ static void system_heap_vunmap(struct >> dma_buf >>>> *dmabuf, struct iosys_map *map) >>>>> iosys_map_clear(map); >>>>> } >>>>> >>>>> +static struct bio_vec *system_heap_init_bvec(struct >>>> system_heap_buffer *buffer, >>>>> + size_t offset, size_t len, int *nr_segs) { >>>>> + struct sg_table *sgt = &buffer->sg_table; >>>>> + struct scatterlist *sg; >>>>> + size_t length = 0; >>>>> + unsigned int i, k = 0; >>>>> + struct bio_vec *bvec; >>>>> + size_t sg_left; >>>>> + size_t sg_offset; >>>>> + size_t sg_len; >>>>> + >>>>> + bvec = kvcalloc(sgt->nents, sizeof(*bvec), GFP_KERNEL); >>>>> + if (!bvec) >>>>> + return NULL; >>>>> + >>>>> + for_each_sg(sgt->sgl, sg, sgt->nents, i) { >>>>> + length += sg->length; >>>>> + if (length <= offset) >>>>> + continue; >>>>> + >>>>> + sg_left = length - offset; >>>>> + sg_offset = sg->offset + sg->length - sg_left; >>>>> + sg_len = min(sg_left, len); >>>>> + >>>>> + bvec[k].bv_page = sg_page(sg); >>>>> + bvec[k].bv_len = sg_len; >>>>> + bvec[k].bv_offset = sg_offset; >>>>> + k++; >>>>> + >>>>> + offset += sg_len; >>>>> + len -= sg_len; >>>>> + if (len <= 0) >>>>> + break; >>>>> + } >>>>> + >>>>> + *nr_segs = k; >>>>> + return bvec; >>>>> +} >>>>> + >>>>> +static int system_heap_rw_file(struct system_heap_buffer *buffer, >>>>> +bool >>>> is_read, >>>>> + bool direct_io, struct file *filp, loff_t file_offset, >>>>> + size_t buf_offset, size_t len) >>>>> +{ >>>>> + struct bio_vec *bvec; >>>>> + int nr_segs = 0; >>>>> + struct iov_iter iter; >>>>> + struct kiocb kiocb; >>>>> + ssize_t ret = 0; >>>>> + >>>>> + if (direct_io) { >>>>> + if (!(filp->f_mode & FMODE_CAN_ODIRECT)) >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + bvec = system_heap_init_bvec(buffer, buf_offset, len, &nr_segs); >>>>> + if (!bvec) >>>>> + return -ENOMEM; >>>>> + >>>>> + iov_iter_bvec(&iter, is_read ? ITER_DEST : ITER_SOURCE, bvec, >>>> nr_segs, len); >>>>> + init_sync_kiocb(&kiocb, filp); >>>>> + kiocb.ki_pos = file_offset; >>>>> + if (direct_io) >>>>> + kiocb.ki_flags |= IOCB_DIRECT; >>>>> + >>>>> + while (kiocb.ki_pos < file_offset + len) { >>>>> + if (is_read) >>>>> + ret = vfs_iocb_iter_read(filp, &kiocb, &iter); >>>>> + else >>>>> + ret = vfs_iocb_iter_write(filp, &kiocb, &iter); >>>>> + if (ret <= 0) >>>>> + break; >>>>> + } >>>>> + >>>>> + kvfree(bvec); >>>>> + return ret < 0 ? ret : 0; >>>>> +} >>>>> + >>>>> +static int system_heap_dma_buf_rw_file(struct dma_buf *dmabuf, >>>>> + struct dma_buf_rw_file *back) >>>>> +{ >>>>> + struct system_heap_buffer *buffer = dmabuf->priv; >>>>> + int ret = 0; >>>>> + __u32 op = back->flags & DMA_BUF_RW_FLAGS_OP_MASK; >>>>> + bool direct_io = back->flags & DMA_BUF_RW_FLAGS_DIRECT; >>>>> + struct file *filp; >>>>> + >>>>> + if (op != DMA_BUF_RW_FLAGS_READ && op != >>>> DMA_BUF_RW_FLAGS_WRITE) >>>>> + return -EINVAL; >>>>> + if (direct_io) { >>>>> + if (!PAGE_ALIGNED(back->file_offset) || >>>>> + !PAGE_ALIGNED(back->buf_offset) || >>>>> + !PAGE_ALIGNED(back->buf_len)) >>>>> + return -EINVAL; >>>>> + } >>>>> + if (!back->buf_len || back->buf_len > dmabuf->size || >>>>> + back->buf_offset >= dmabuf->size || >>>>> + back->buf_offset + back->buf_len > dmabuf->size) >>>>> + return -EINVAL; >>>>> + if (back->file_offset + back->buf_len < back->file_offset) >>>>> + return -EINVAL; >>>>> + >>>>> + filp = fget(back->fd); >>>>> + if (!filp) >>>>> + return -EBADF; >>>>> + >>>>> + mutex_lock(&buffer->lock); >>>>> + ret = system_heap_rw_file(buffer, op == >>>> DMA_BUF_RW_FLAGS_READ, direct_io, >>>>> + filp, back->file_offset, back->buf_offset, back- >>>>> buf_len); >>>>> + mutex_unlock(&buffer->lock); >>>>> + >>>>> + fput(filp); >>>>> + return ret; >>>>> +} >>>>> + >>>>> static void system_heap_dma_buf_release(struct dma_buf *dmabuf) { >>>>> struct system_heap_buffer *buffer = dmabuf->priv; @@ -308,6 >>>> +425,7 >>>>> @@ static const struct dma_buf_ops system_heap_buf_ops = { >>>>> .mmap = system_heap_mmap, >>>>> .vmap = system_heap_vmap, >>>>> .vunmap = system_heap_vunmap, >>>>> + .rw_file = system_heap_dma_buf_rw_file, >>>>> .release = system_heap_dma_buf_release, }; >>>>> >>> >

7 months

1
0
0 0

Re: [PATCH 1/7] drm/shmem-helper: Add lockdep asserts to vmap/vunmap

by Christian König

Am 18.03.25 um 20:22 schrieb Daniel Almeida: > From: Asahi Lina <lina(a)asahilina.net> > > Since commit 21aa27ddc582 ("drm/shmem-helper: Switch to reservation > lock"), the drm_gem_shmem_vmap and drm_gem_shmem_vunmap functions > require that the caller holds the DMA reservation lock for the object. > Add lockdep assertions to help validate this. > > Signed-off-by: Asahi Lina <lina(a)asahilina.net> > Signed-off-by: Daniel Almeida <daniel.almeida(a)collabora.com> Oh, yeah that is certainly a good idea. Reviewed-by: Christian König <christian.koenig(a)amd.com> > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > index 5ab351409312b5a0de542df2b636278d6186cb7b..ec89e9499f5f02a2a35713669bf649dd2abb9938 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -338,6 +338,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem, > struct drm_gem_object *obj = &shmem->base; > int ret = 0; > > + dma_resv_assert_held(obj->resv); > + > if (obj->import_attach) { > ret = dma_buf_vmap(obj->import_attach->dmabuf, map); > if (!ret) { > @@ -404,6 +406,8 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem, > { > struct drm_gem_object *obj = &shmem->base; > > + dma_resv_assert_held(obj->resv); > + > if (obj->import_attach) { > dma_buf_vunmap(obj->import_attach->dmabuf, map); > } else { >

7 months, 1 week

2
1
0 0

[PATCH bpf-next v6 0/5] Replace CONFIG_DMABUF_SYSFS_STATS with BPF

by T.J. Mercier

Until CONFIG_DMABUF_SYSFS_STATS was added [1] it was only possible to perform per-buffer accounting with debugfs which is not suitable for production environments. Eventually we discovered the overhead with per-buffer sysfs file creation/removal was significantly impacting allocation and free times, and exacerbated kernfs lock contention. [2] dma_buf_stats_setup() is responsible for 39% of single-page buffer creation duration, or 74% of single-page dma_buf_export() duration when stressing dmabuf allocations and frees. I prototyped a change from per-buffer to per-exporter statistics with a RCU protected list of exporter allocations that accommodates most (but not all) of our use-cases and avoids almost all of the sysfs overhead. While that adds less overhead than per-buffer sysfs, and less even than the maintenance of the dmabuf debugfs_list, it's still *additional* overhead on top of the debugfs_list and doesn't give us per-buffer info. This series uses the existing dmabuf debugfs_list to implement a BPF dmabuf iterator, which adds no overhead to buffer allocation/free and provides per-buffer info. The list has been moved outside of CONFIG_DEBUG_FS scope so that it is always populated. The BPF program loaded by userspace that extracts per-buffer information gets to define its own interface which avoids the lack of ABI stability with debugfs. This will allow us to replace our use of CONFIG_DMABUF_SYSFS_STATS, and the plan is to remove it from the kernel after the next longterm stable release. [1] https://lore.kernel.org/linux-media/20201210044400.1080308-1-hridya@google.… [2] https://lore.kernel.org/all/20220516171315.2400578-1-tjmercier@google.com v1: https://lore.kernel.org/all/20250414225227.3642618-1-tjmercier@google.com v1 -> v2: Make the DMA buffer list independent of CONFIG_DEBUG_FS per Christian König Add CONFIG_DMA_SHARED_BUFFER check to kernel/bpf/Makefile per kernel test robot Use BTF_ID_LIST_SINGLE instead of BTF_ID_LIST_GLOBAL_SINGLE per Song Liu Fixup comment style, mixing code/declarations, and use ASSERT_OK_FD in selftest per Song Liu Add BPF_ITER_RESCHED feature to bpf_dmabuf_reg_info per Alexei Starovoitov Add open-coded iterator and selftest per Alexei Starovoitov Add a second test buffer from the system dmabuf heap to selftests Use the BPF program we'll use in production for selftest per Alexei Starovoitov https://r.android.com/c/platform/system/bpfprogs/+/3616123/2/dmabufIter.c https://r.android.com/c/platform/system/memory/libmeminfo/+/3614259/1/libdm… v2: https://lore.kernel.org/all/20250504224149.1033867-1-tjmercier@google.com v2 -> v3: Rebase onto bpf-next/master Move get_next_dmabuf() into drivers/dma-buf/dma-buf.c, along with the new get_first_dmabuf(). This avoids having to expose the dmabuf list and mutex to the rest of the kernel, and keeps the dmabuf mutex operations near each other in the same file. (Christian König) Add Christian's RB to dma-buf: Rename debugfs symbols Drop RFC: dma-buf: Remove DMA-BUF statistics v3: https://lore.kernel.org/all/20250507001036.2278781-1-tjmercier@google.com v3 -> v4: Fix selftest BPF program comment style (not kdoc) per Alexei Starovoitov Fix dma-buf.c kdoc comment style per Alexei Starovoitov Rename get_first_dmabuf / get_next_dmabuf to dma_buf_iter_begin / dma_buf_iter_next per Christian König Add Christian's RB to bpf: Add dmabuf iterator v4: https://lore.kernel.org/all/20250508182025.2961555-1-tjmercier@google.com v4 -> v5: Add Christian's Acks to all patches Add Song Liu's Acks Move BTF_ID_LIST_SINGLE and DEFINE_BPF_ITER_FUNC closer to usage per Song Liu Fix open-coded iterator comment style per Song Liu Move iterator termination check to its own subtest per Song Liu Rework selftest buffer creation per Song Liu Fix spacing in sanitize_string per BPF CI v5: https://lore.kernel.org/all/20250512174036.266796-1-tjmercier@google.com v5 -> v6: Song Liu: Init test buffer FDs to -1 Zero-init udmabuf_create for future proofing Bail early for iterator fd/FILE creation failure Dereference char ptr to check for NUL in sanitize_string() Move map insertion from create_test_buffers() to test_dmabuf_iter() Add ACK to selftests/bpf: Add test for open coded dmabuf_iter T.J. Mercier (5): dma-buf: Rename debugfs symbols bpf: Add dmabuf iterator bpf: Add open coded dmabuf iterator selftests/bpf: Add test for dmabuf_iter selftests/bpf: Add test for open coded dmabuf_iter drivers/dma-buf/dma-buf.c | 98 ++++-- include/linux/dma-buf.h | 4 +- kernel/bpf/Makefile | 3 + kernel/bpf/dmabuf_iter.c | 150 +++++++++ kernel/bpf/helpers.c | 5 + .../testing/selftests/bpf/bpf_experimental.h | 5 + tools/testing/selftests/bpf/config | 3 + .../selftests/bpf/prog_tests/dmabuf_iter.c | 285 ++++++++++++++++++ .../testing/selftests/bpf/progs/dmabuf_iter.c | 91 ++++++ 9 files changed, 622 insertions(+), 22 deletions(-) create mode 100644 kernel/bpf/dmabuf_iter.c create mode 100644 tools/testing/selftests/bpf/prog_tests/dmabuf_iter.c create mode 100644 tools/testing/selftests/bpf/progs/dmabuf_iter.c base-commit: 43745d11bfd9683abdf08ad7a5cc403d6a9ffd15 -- 2.49.0.1045.g170613ef41-goog

7 months, 1 week

1
5
0 0

Re: [PATCH 1/2] dmabuf: add DMA_BUF_IOCTL_RW_FILE

by Christian König

On 5/13/25 11:27, wangtao wrote: > Add DMA_BUF_IOCTL_RW_FILE to save/restore data from/to a dma-buf. Similar approach where rejected before in favor of using udmabuf. Is there any reason you can't use that approach as well? Regards, Christian. > > Signed-off-by: wangtao <tao.wangtao(a)honor.com> > --- > drivers/dma-buf/dma-buf.c | 8 ++++++++ > include/linux/dma-buf.h | 3 +++ > include/uapi/linux/dma-buf.h | 29 +++++++++++++++++++++++++++++ > 3 files changed, 40 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 5baa83b85515..95d8b0158ffd 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -460,6 +460,7 @@ static long dma_buf_ioctl(struct file *file, > struct dma_buf *dmabuf; > struct dma_buf_sync sync; > enum dma_data_direction direction; > + struct dma_buf_rw_file kfile; > int ret; > > dmabuf = file->private_data; > @@ -504,6 +505,13 @@ static long dma_buf_ioctl(struct file *file, > return dma_buf_import_sync_file(dmabuf, (const void __user *)arg); > #endif > > + case DMA_BUF_IOCTL_RW_FILE: > + if (copy_from_user(&kfile, (void __user *) arg, sizeof(kfile))) > + return -EFAULT; > + if (!dmabuf->ops->rw_file) > + return -EINVAL; > + return dmabuf->ops->rw_file(dmabuf, &kfile); > + > default: > return -ENOTTY; > } > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index 36216d28d8bd..de236ba2094b 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -22,6 +22,7 @@ > #include <linux/fs.h> > #include <linux/dma-fence.h> > #include <linux/wait.h> > +#include <uapi/linux/dma-buf.h> > > struct device; > struct dma_buf; > @@ -285,6 +286,8 @@ 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 (*rw_file)(struct dma_buf *dmabuf, struct dma_buf_rw_file *file); > }; > > /** > diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h > index 5a6fda66d9ad..ec9164b7b753 100644 > --- a/include/uapi/linux/dma-buf.h > +++ b/include/uapi/linux/dma-buf.h > @@ -167,6 +167,29 @@ struct dma_buf_import_sync_file { > __s32 fd; > }; > > +/** > + * struct dma_buf_rw_file - read/write file associated with a dma-buf > + * > + * Userspace can performs a DMA_BUF_IOCTL_BACK to save data from a dma-buf or > + * restore data to a dma-buf. > + */ > +struct dma_buf_rw_file { > + > + /** @flags: Flags indicating read/write for this dma-buf. */ > + __u32 flags; > + /** @fd: File descriptor of the file associated with this dma-buf. */ > + __s32 fd; > + /** @file_offset: Offset within the file where this dma-buf starts. > + * > + * Offset and Length must be page-aligned for direct I/O. > + */ > + __u64 file_offset; > + /** @buf_offset: Offset within this dma-buf where the read/write starts. */ > + __u64 buf_offset; > + /** @buf_len: Length of this dma-buf read/write. */ > + __u64 buf_len; > +}; > + > #define DMA_BUF_BASE 'b' > #define DMA_BUF_IOCTL_SYNC _IOW(DMA_BUF_BASE, 0, struct dma_buf_sync) > > @@ -179,4 +202,10 @@ struct dma_buf_import_sync_file { > #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_RW_FLAGS_OP_MASK (0xFF << 0) > +#define DMA_BUF_RW_FLAGS_READ (1 << 0) /* Restore dma-buf data */ > +#define DMA_BUF_RW_FLAGS_WRITE (2 << 0) /* Save dma-buf data */ > +#define DMA_BUF_RW_FLAGS_DIRECT (1u << 31) /* Direct read/write file */ > +#define DMA_BUF_IOCTL_RW_FILE _IOW(DMA_BUF_BASE, 4, struct dma_buf_rw_file) > + > #endif

7 months, 1 week

2
2
0 0

Re: [RFC v2 10/13] dma-fence: Add safe access helpers and document the rules

by Rob Clark

On Fri, May 9, 2025 at 8:34 AM Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> wrote: > > Dma-fence objects currently suffer from a potential use after free problem > where fences exported to userspace and other drivers can outlive the > exporting driver, or the associated data structures. > > The discussion on how to address this concluded that adding reference > counting to all the involved objects is not desirable, since it would need > to be very wide reaching and could cause unloadable drivers if another > entity would be holding onto a signaled fence reference potentially > indefinitely. > > This patch enables the safe access by introducing and documenting a > contract between fence exporters and users. It documents a set of > contraints and adds helpers which a) drivers with potential to suffer from > the use after free must use and b) users of the dma-fence API must use as > well. > > Premise of the design has multiple sides: > > 1. Drivers (fence exporters) MUST ensure a RCU grace period between > signalling a fence and freeing the driver private data associated with it. > > The grace period does not have to follow the signalling immediately but > HAS to happen before data is freed. > > 2. Users of the dma-fence API marked with such requirement MUST contain > the complete access to the data within a single code block guarded by the > new dma_fence_access_begin() and dma_fence_access_end() helpers. > > The combination of the two ensures that whoever sees the > DMA_FENCE_FLAG_SIGNALED_BIT not set is guaranteed to have access to a > valid fence->lock and valid data potentially accessed by the fence->ops > virtual functions, until the call to dma_fence_access_end(). > > 3. Module unload (fence->ops) disappearing is for now explicitly not > handled. That would required a more complex protection, possibly needing > SRCU instead of RCU to handle callers such as dma_fence_wait_timeout(), > where race between dma_fence_enable_sw_signaling, signalling, and > dereference of fence->ops->wait() would need a sleeping SRCU context. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> > --- > drivers/dma-buf/dma-fence.c | 69 +++++++++++++++++++++++++++++++++++++ > include/linux/dma-fence.h | 32 ++++++++++++----- > 2 files changed, 93 insertions(+), 8 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index dc2456f68685..cfe1d7b79c22 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -533,6 +533,7 @@ void dma_fence_release(struct kref *kref) > struct dma_fence *fence = > container_of(kref, struct dma_fence, refcount); > > + dma_fence_access_begin(); > trace_dma_fence_destroy(fence); > > if (WARN(!list_empty(&fence->cb_list) && > @@ -560,6 +561,8 @@ void dma_fence_release(struct kref *kref) > fence->ops->release(fence); > else > dma_fence_free(fence); > + > + dma_fence_access_end(); > } > EXPORT_SYMBOL(dma_fence_release); > > @@ -982,11 +985,13 @@ EXPORT_SYMBOL(dma_fence_set_deadline); > */ > void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq) > { > + dma_fence_access_begin(); > seq_printf(seq, "%s %s seq %llu %ssignalled\n", > dma_fence_driver_name(fence), > dma_fence_timeline_name(fence), > fence->seqno, > dma_fence_is_signaled(fence) ? "" : "un"); > + dma_fence_access_end(); > } > EXPORT_SYMBOL(dma_fence_describe); > > @@ -1033,3 +1038,67 @@ dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops, > __set_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags); > } > EXPORT_SYMBOL(dma_fence_init64); > + > +/** > + * dma_fence_driver_name - Access the driver name > + * @fence: the fence to query > + * > + * Returns a driver name backing the dma-fence implementation. > + * > + * IMPORTANT CONSIDERATION: > + * Dma-fence contract stipulates that access to driver provided data (data not > + * directly embedded into the object itself), such as the &dma_fence.lock and > + * memory potentially accessed by the &dma_fence.ops functions, is forbidden > + * after the fence has been signalled. Drivers are allowed to free that data, > + * and some do. > + * > + * To allow safe access drivers are mandated to guarantee a RCU grace period > + * between signalling the fence and freeing said data. > + * > + * As such access to the driver name is only valid inside a RCU locked section. > + * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded > + * by the &dma_fence_access_being and &dma_fence_access_end pair. > + */ > +const char *dma_fence_driver_name(struct dma_fence *fence) > +{ > + RCU_LOCKDEP_WARN(!rcu_read_lock_held(), > + "rcu_read_lock() required for safe access to returned string"); > + > + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) > + return fence->ops->get_driver_name(fence); > + else > + return "detached-driver"; > +} > +EXPORT_SYMBOL(dma_fence_driver_name); > + > +/** > + * dma_fence_timeline_name - Access the timeline name > + * @fence: the fence to query > + * > + * Returns a timeline name provided by the dma-fence implementation. > + * > + * IMPORTANT CONSIDERATION: > + * Dma-fence contract stipulates that access to driver provided data (data not > + * directly embedded into the object itself), such as the &dma_fence.lock and > + * memory potentially accessed by the &dma_fence.ops functions, is forbidden > + * after the fence has been signalled. Drivers are allowed to free that data, > + * and some do. > + * > + * To allow safe access drivers are mandated to guarantee a RCU grace period > + * between signalling the fence and freeing said data. > + * > + * As such access to the driver name is only valid inside a RCU locked section. > + * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded > + * by the &dma_fence_access_being and &dma_fence_access_end pair. > + */ > +const char *dma_fence_timeline_name(struct dma_fence *fence) > +{ > + RCU_LOCKDEP_WARN(!rcu_read_lock_held(), > + "rcu_read_lock() required for safe access to returned string"); > + > + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) > + return fence->ops->get_driver_name(fence); > + else > + return "signaled-timeline"; This means that trace_dma_fence_signaled() will get the wrong timeline/driver name, which probably screws up perfetto and maybe other tools. Maybe it would work well enough just to move the trace_dma_fence_signaled() call ahead of the test_and_set_bit()? Idk if some things will start getting confused if they see that trace multiple times. Maybe a better solution would be to add a new bit for this purpose, which is set after the tracepoint in dma_fence_signal_timestamp_locked(). (trace_dma_fence_destroy() will I guess be messed up regardless.. it doesn't look like perfetto cares about this tracepoint, so maybe that is ok. It doesn't seem so useful.) BR, -R > +} > +EXPORT_SYMBOL(dma_fence_timeline_name); > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index c814a86087f8..c8a9915eb360 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -387,15 +387,31 @@ bool dma_fence_remove_callback(struct dma_fence *fence, > struct dma_fence_cb *cb); > void dma_fence_enable_sw_signaling(struct dma_fence *fence); > > -static inline const char *dma_fence_driver_name(struct dma_fence *fence) > -{ > - return fence->ops->get_driver_name(fence); > -} > +/** > + * DOC: Safe external access to driver provided object members > + * > + * All data not stored directly in the dma-fence object, such as the > + * &dma_fence.lock and memory potentially accessed by functions in the > + * &dma_fence.ops table, MUST NOT be accessed after the fence has been signalled > + * because after that point drivers are allowed to free it. > + * > + * All code accessing that data via the dma-fence API (or directly, which is > + * discouraged), MUST make sure to contain the complete access within a > + * &dma_fence_access_begin and &dma_fence_access_end pair. > + * > + * Some dma-fence API handles this automatically, while other, as for example > + * &dma_fence_driver_name and &dma_fence_timeline_name, leave that > + * responsibility to the caller. > + * > + * To enable this scheme to work drivers MUST ensure a RCU grace period elapses > + * between signalling the fence and freeing the said data. > + * > + */ > +#define dma_fence_access_begin rcu_read_lock > +#define dma_fence_access_end rcu_read_unlock > > -static inline const char *dma_fence_timeline_name(struct dma_fence *fence) > -{ > - return fence->ops->get_timeline_name(fence); > -} > +const char *dma_fence_driver_name(struct dma_fence *fence); > +const char *dma_fence_timeline_name(struct dma_fence *fence); > > /** > * dma_fence_is_signaled_locked - Return an indication if the fence > -- > 2.48.0 >

7 months, 1 week

1
0
0 0

Re: [PATCH 2/2] dmabuf/heaps: implement DMA_BUF_IOCTL_RW_FILE for system_heap

by Christian König

On 5/13/25 14:30, wangtao wrote: >> -----Original Message----- >> From: Christian König <christian.koenig(a)amd.com> >> Sent: Tuesday, May 13, 2025 7:32 PM >> To: wangtao <tao.wangtao(a)honor.com>; sumit.semwal(a)linaro.org; >> benjamin.gaignard(a)collabora.com; Brian.Starkey(a)arm.com; >> jstultz(a)google.com; tjmercier(a)google.com >> Cc: linux-media(a)vger.kernel.org; dri-devel(a)lists.freedesktop.org; linaro- >> mm-sig(a)lists.linaro.org; linux-kernel(a)vger.kernel.org; >> wangbintian(BintianWang) <bintian.wang(a)honor.com>; yipengxiang >> <yipengxiang(a)honor.com>; liulu 00013167 <liulu.liu(a)honor.com>; hanfeng >> 00012985 <feng.han(a)honor.com> >> Subject: Re: [PATCH 2/2] dmabuf/heaps: implement >> DMA_BUF_IOCTL_RW_FILE for system_heap >> >> On 5/13/25 11:28, wangtao wrote: >>> Support direct file I/O operations for system_heap dma-buf objects. >>> Implementation includes: >>> 1. Convert sg_table to bio_vec >> >> That is usually illegal for DMA-bufs. > [wangtao] The term 'convert' is misleading in this context. The appropriate phrasing should be: Construct bio_vec from sg_table. Well it doesn't matter what you call it. Touching the page inside an sg table of a DMA-buf is illegal, we even have code to actively prevent that. Once more: This approach was already rejected multiple times! Please use udmabuf instead! The hack you came up here is simply not necessary. Regards, Christian. > Appreciate your feedback. >> >> Regards, >> Christian. >> >>> 2. Set IOCB_DIRECT when O_DIRECT is supported 3. Invoke >>> vfs_iocb_iter_read()/vfs_iocb_iter_write() for actual I/O >>> >>> Performance metrics (UFS 4.0 device @4GB/s, Arm64 CPU @1GHz): >>> >>> | Metric | 1MB | 8MB | 64MB | 1024MB | 3072MB | >>> |--------------------|-------:|-------:|--------:|---------:|--------- >>> |--------------------|:| >>> | Buffer Read (us) | 1658 | 9028 | 69295 | 1019783 | 2978179 | >>> | Direct Read (us) | 707 | 2647 | 18689 | 299627 | 937758 | >>> | Buffer Rate (MB/s) | 603 | 886 | 924 | 1004 | 1032 | >>> | Direct Rate (MB/s) | 1414 | 3022 | 3425 | 3418 | 3276 | >>> >>> Signed-off-by: wangtao <tao.wangtao(a)honor.com> >>> --- >>> drivers/dma-buf/heaps/system_heap.c | 118 >>> ++++++++++++++++++++++++++++ >>> 1 file changed, 118 insertions(+) >>> >>> diff --git a/drivers/dma-buf/heaps/system_heap.c >>> b/drivers/dma-buf/heaps/system_heap.c >>> index 26d5dc89ea16..f7b71b9843aa 100644 >>> --- a/drivers/dma-buf/heaps/system_heap.c >>> +++ b/drivers/dma-buf/heaps/system_heap.c >>> @@ -20,6 +20,8 @@ >>> #include <linux/scatterlist.h> >>> #include <linux/slab.h> >>> #include <linux/vmalloc.h> >>> +#include <linux/bvec.h> >>> +#include <linux/uio.h> >>> >>> static struct dma_heap *sys_heap; >>> >>> @@ -281,6 +283,121 @@ static void system_heap_vunmap(struct dma_buf >> *dmabuf, struct iosys_map *map) >>> iosys_map_clear(map); >>> } >>> >>> +static struct bio_vec *system_heap_init_bvec(struct >> system_heap_buffer *buffer, >>> + size_t offset, size_t len, int *nr_segs) { >>> + struct sg_table *sgt = &buffer->sg_table; >>> + struct scatterlist *sg; >>> + size_t length = 0; >>> + unsigned int i, k = 0; >>> + struct bio_vec *bvec; >>> + size_t sg_left; >>> + size_t sg_offset; >>> + size_t sg_len; >>> + >>> + bvec = kvcalloc(sgt->nents, sizeof(*bvec), GFP_KERNEL); >>> + if (!bvec) >>> + return NULL; >>> + >>> + for_each_sg(sgt->sgl, sg, sgt->nents, i) { >>> + length += sg->length; >>> + if (length <= offset) >>> + continue; >>> + >>> + sg_left = length - offset; >>> + sg_offset = sg->offset + sg->length - sg_left; >>> + sg_len = min(sg_left, len); >>> + >>> + bvec[k].bv_page = sg_page(sg); >>> + bvec[k].bv_len = sg_len; >>> + bvec[k].bv_offset = sg_offset; >>> + k++; >>> + >>> + offset += sg_len; >>> + len -= sg_len; >>> + if (len <= 0) >>> + break; >>> + } >>> + >>> + *nr_segs = k; >>> + return bvec; >>> +} >>> + >>> +static int system_heap_rw_file(struct system_heap_buffer *buffer, bool >> is_read, >>> + bool direct_io, struct file *filp, loff_t file_offset, >>> + size_t buf_offset, size_t len) >>> +{ >>> + struct bio_vec *bvec; >>> + int nr_segs = 0; >>> + struct iov_iter iter; >>> + struct kiocb kiocb; >>> + ssize_t ret = 0; >>> + >>> + if (direct_io) { >>> + if (!(filp->f_mode & FMODE_CAN_ODIRECT)) >>> + return -EINVAL; >>> + } >>> + >>> + bvec = system_heap_init_bvec(buffer, buf_offset, len, &nr_segs); >>> + if (!bvec) >>> + return -ENOMEM; >>> + >>> + iov_iter_bvec(&iter, is_read ? ITER_DEST : ITER_SOURCE, bvec, >> nr_segs, len); >>> + init_sync_kiocb(&kiocb, filp); >>> + kiocb.ki_pos = file_offset; >>> + if (direct_io) >>> + kiocb.ki_flags |= IOCB_DIRECT; >>> + >>> + while (kiocb.ki_pos < file_offset + len) { >>> + if (is_read) >>> + ret = vfs_iocb_iter_read(filp, &kiocb, &iter); >>> + else >>> + ret = vfs_iocb_iter_write(filp, &kiocb, &iter); >>> + if (ret <= 0) >>> + break; >>> + } >>> + >>> + kvfree(bvec); >>> + return ret < 0 ? ret : 0; >>> +} >>> + >>> +static int system_heap_dma_buf_rw_file(struct dma_buf *dmabuf, >>> + struct dma_buf_rw_file *back) >>> +{ >>> + struct system_heap_buffer *buffer = dmabuf->priv; >>> + int ret = 0; >>> + __u32 op = back->flags & DMA_BUF_RW_FLAGS_OP_MASK; >>> + bool direct_io = back->flags & DMA_BUF_RW_FLAGS_DIRECT; >>> + struct file *filp; >>> + >>> + if (op != DMA_BUF_RW_FLAGS_READ && op != >> DMA_BUF_RW_FLAGS_WRITE) >>> + return -EINVAL; >>> + if (direct_io) { >>> + if (!PAGE_ALIGNED(back->file_offset) || >>> + !PAGE_ALIGNED(back->buf_offset) || >>> + !PAGE_ALIGNED(back->buf_len)) >>> + return -EINVAL; >>> + } >>> + if (!back->buf_len || back->buf_len > dmabuf->size || >>> + back->buf_offset >= dmabuf->size || >>> + back->buf_offset + back->buf_len > dmabuf->size) >>> + return -EINVAL; >>> + if (back->file_offset + back->buf_len < back->file_offset) >>> + return -EINVAL; >>> + >>> + filp = fget(back->fd); >>> + if (!filp) >>> + return -EBADF; >>> + >>> + mutex_lock(&buffer->lock); >>> + ret = system_heap_rw_file(buffer, op == >> DMA_BUF_RW_FLAGS_READ, direct_io, >>> + filp, back->file_offset, back->buf_offset, back- >>> buf_len); >>> + mutex_unlock(&buffer->lock); >>> + >>> + fput(filp); >>> + return ret; >>> +} >>> + >>> static void system_heap_dma_buf_release(struct dma_buf *dmabuf) { >>> struct system_heap_buffer *buffer = dmabuf->priv; @@ -308,6 >> +425,7 >>> @@ static const struct dma_buf_ops system_heap_buf_ops = { >>> .mmap = system_heap_mmap, >>> .vmap = system_heap_vmap, >>> .vunmap = system_heap_vunmap, >>> + .rw_file = system_heap_dma_buf_rw_file, >>> .release = system_heap_dma_buf_release, }; >>> >

7 months, 1 week

1
0
0 0

Re: [PATCH 2/2] dmabuf/heaps: implement DMA_BUF_IOCTL_RW_FILE for system_heap

by Christian König

On 5/13/25 11:28, wangtao wrote: > Support direct file I/O operations for system_heap dma-buf objects. > Implementation includes: > 1. Convert sg_table to bio_vec That is usually illegal for DMA-bufs. Regards, Christian. > 2. Set IOCB_DIRECT when O_DIRECT is supported > 3. Invoke vfs_iocb_iter_read()/vfs_iocb_iter_write() for actual I/O > > Performance metrics (UFS 4.0 device @4GB/s, Arm64 CPU @1GHz): > > | Metric | 1MB | 8MB | 64MB | 1024MB | 3072MB | > |--------------------|-------:|-------:|--------:|---------:|---------:| > | Buffer Read (us) | 1658 | 9028 | 69295 | 1019783 | 2978179 | > | Direct Read (us) | 707 | 2647 | 18689 | 299627 | 937758 | > | Buffer Rate (MB/s) | 603 | 886 | 924 | 1004 | 1032 | > | Direct Rate (MB/s) | 1414 | 3022 | 3425 | 3418 | 3276 | > > Signed-off-by: wangtao <tao.wangtao(a)honor.com> > --- > drivers/dma-buf/heaps/system_heap.c | 118 ++++++++++++++++++++++++++++ > 1 file changed, 118 insertions(+) > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > index 26d5dc89ea16..f7b71b9843aa 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -20,6 +20,8 @@ > #include <linux/scatterlist.h> > #include <linux/slab.h> > #include <linux/vmalloc.h> > +#include <linux/bvec.h> > +#include <linux/uio.h> > > static struct dma_heap *sys_heap; > > @@ -281,6 +283,121 @@ static void system_heap_vunmap(struct dma_buf *dmabuf, struct iosys_map *map) > iosys_map_clear(map); > } > > +static struct bio_vec *system_heap_init_bvec(struct system_heap_buffer *buffer, > + size_t offset, size_t len, int *nr_segs) > +{ > + struct sg_table *sgt = &buffer->sg_table; > + struct scatterlist *sg; > + size_t length = 0; > + unsigned int i, k = 0; > + struct bio_vec *bvec; > + size_t sg_left; > + size_t sg_offset; > + size_t sg_len; > + > + bvec = kvcalloc(sgt->nents, sizeof(*bvec), GFP_KERNEL); > + if (!bvec) > + return NULL; > + > + for_each_sg(sgt->sgl, sg, sgt->nents, i) { > + length += sg->length; > + if (length <= offset) > + continue; > + > + sg_left = length - offset; > + sg_offset = sg->offset + sg->length - sg_left; > + sg_len = min(sg_left, len); > + > + bvec[k].bv_page = sg_page(sg); > + bvec[k].bv_len = sg_len; > + bvec[k].bv_offset = sg_offset; > + k++; > + > + offset += sg_len; > + len -= sg_len; > + if (len <= 0) > + break; > + } > + > + *nr_segs = k; > + return bvec; > +} > + > +static int system_heap_rw_file(struct system_heap_buffer *buffer, bool is_read, > + bool direct_io, struct file *filp, loff_t file_offset, > + size_t buf_offset, size_t len) > +{ > + struct bio_vec *bvec; > + int nr_segs = 0; > + struct iov_iter iter; > + struct kiocb kiocb; > + ssize_t ret = 0; > + > + if (direct_io) { > + if (!(filp->f_mode & FMODE_CAN_ODIRECT)) > + return -EINVAL; > + } > + > + bvec = system_heap_init_bvec(buffer, buf_offset, len, &nr_segs); > + if (!bvec) > + return -ENOMEM; > + > + iov_iter_bvec(&iter, is_read ? ITER_DEST : ITER_SOURCE, bvec, nr_segs, len); > + init_sync_kiocb(&kiocb, filp); > + kiocb.ki_pos = file_offset; > + if (direct_io) > + kiocb.ki_flags |= IOCB_DIRECT; > + > + while (kiocb.ki_pos < file_offset + len) { > + if (is_read) > + ret = vfs_iocb_iter_read(filp, &kiocb, &iter); > + else > + ret = vfs_iocb_iter_write(filp, &kiocb, &iter); > + if (ret <= 0) > + break; > + } > + > + kvfree(bvec); > + return ret < 0 ? ret : 0; > +} > + > +static int system_heap_dma_buf_rw_file(struct dma_buf *dmabuf, > + struct dma_buf_rw_file *back) > +{ > + struct system_heap_buffer *buffer = dmabuf->priv; > + int ret = 0; > + __u32 op = back->flags & DMA_BUF_RW_FLAGS_OP_MASK; > + bool direct_io = back->flags & DMA_BUF_RW_FLAGS_DIRECT; > + struct file *filp; > + > + if (op != DMA_BUF_RW_FLAGS_READ && op != DMA_BUF_RW_FLAGS_WRITE) > + return -EINVAL; > + if (direct_io) { > + if (!PAGE_ALIGNED(back->file_offset) || > + !PAGE_ALIGNED(back->buf_offset) || > + !PAGE_ALIGNED(back->buf_len)) > + return -EINVAL; > + } > + if (!back->buf_len || back->buf_len > dmabuf->size || > + back->buf_offset >= dmabuf->size || > + back->buf_offset + back->buf_len > dmabuf->size) > + return -EINVAL; > + if (back->file_offset + back->buf_len < back->file_offset) > + return -EINVAL; > + > + filp = fget(back->fd); > + if (!filp) > + return -EBADF; > + > + mutex_lock(&buffer->lock); > + ret = system_heap_rw_file(buffer, op == DMA_BUF_RW_FLAGS_READ, direct_io, > + filp, back->file_offset, back->buf_offset, back->buf_len); > + mutex_unlock(&buffer->lock); > + > + fput(filp); > + return ret; > +} > + > static void system_heap_dma_buf_release(struct dma_buf *dmabuf) > { > struct system_heap_buffer *buffer = dmabuf->priv; > @@ -308,6 +425,7 @@ static const struct dma_buf_ops system_heap_buf_ops = { > .mmap = system_heap_mmap, > .vmap = system_heap_vmap, > .vunmap = system_heap_vunmap, > + .rw_file = system_heap_dma_buf_rw_file, > .release = system_heap_dma_buf_release, > }; >

7 months, 1 week

1
0
0 0

Re: [PATCH bpf-next v5 5/5] selftests/bpf: Add test for open coded dmabuf_iter

by T.J. Mercier

On Mon, May 12, 2025 at 1:30 PM Song Liu <song(a)kernel.org> wrote: > > On Mon, May 12, 2025 at 10:41 AM T.J. Mercier <tjmercier(a)google.com> wrote: > > > > Use the same test buffers as the traditional iterator and a new BPF map > > to verify the test buffers can be found with the open coded dmabuf > > iterator. > > > > Signed-off-by: T.J. Mercier <tjmercier(a)google.com> > > Acked-by: Christian König <christian.koenig(a)amd.com> > > Acked-by: Song Liu <song(a)kernel.org> Thanks, I'll send v6 this afternoon or tomorrow morning with all changes. > With a nitpick below. > > [...] > > > > > -static int create_test_buffers(void) > > +static int create_test_buffers(int map_fd) > > { > > + bool f = false; > > + > > udmabuf = create_udmabuf(); > > sysheap_dmabuf = create_sys_heap_dmabuf(); > > > > if (udmabuf < 0 || sysheap_dmabuf < 0) > > return -1; > > > > - return 0; > > + return bpf_map_update_elem(map_fd, udmabuf_test_buffer_name, &f, BPF_ANY) || > > + bpf_map_update_elem(map_fd, sysheap_test_buffer_name, &f, BPF_ANY); > > nit: Instead of passing map_fd in here, we can just call > bpf_map_update_elem() in test_dmabuf_iter() > > [...] >

7 months, 1 week

1
0
0 0

Re: [PATCH bpf-next v5 4/5] selftests/bpf: Add test for dmabuf_iter

by T.J. Mercier

On Mon, May 12, 2025 at 11:58 AM Song Liu <song(a)kernel.org> wrote: > > On Mon, May 12, 2025 at 10:41 AM T.J. Mercier <tjmercier(a)google.com> wrote: > [...] > > + > > +static int udmabuf; > > static int udmabuf = -1; > > > +static const char udmabuf_test_buffer_name[DMA_BUF_NAME_LEN] = "udmabuf_test_buffer_for_iter"; > > +static size_t udmabuf_test_buffer_size; > > +static int sysheap_dmabuf; > > static int sysheap_dmabuf = -1; We don't really need the -1 since the rework in this version initializes them to -1 on failure, but I'll add it anyway. > > +static const char sysheap_test_buffer_name[DMA_BUF_NAME_LEN] = "sysheap_test_buffer_for_iter"; > > +static size_t sysheap_test_buffer_size; > > + > > +static int create_udmabuf(void) > > +{ > > + struct udmabuf_create create; > > nit: zero initialize create to be future proof. Ok, done. > > > + int dev_udmabuf, memfd, local_udmabuf; > > + > > + udmabuf_test_buffer_size = 10 * getpagesize(); > > [...] > > > +static void subtest_dmabuf_iter_check_default_iter(struct dmabuf_iter *skel) > > +{ > > + bool found_test_sysheap_dmabuf = false; > > + bool found_test_udmabuf = false; > > + struct DmabufInfo bufinfo; > > + size_t linesize = 0; > > + char *line = NULL; > > + FILE *iter_file; > > + int iter_fd, f = INODE; > > + > > + iter_fd = bpf_iter_create(bpf_link__fd(skel->links.dmabuf_collector)); > > + ASSERT_OK_FD(iter_fd, "iter_create"); > > Should we check ASSERT_OK_FD() and exit early on > failures? We should fallthrough on error, but let's add the early out. > > > + > > + iter_file = fdopen(iter_fd, "r"); > > + ASSERT_OK_PTR(iter_file, "fdopen"); > > Same here. > [...] > > +/* > > + * Fields output by this iterator are delimited by newlines. Convert any > > + * newlines in user-provided printed strings to spaces. > > + */ > > +static void sanitize_string(char *src, size_t size) > > +{ > > + for (char *c = src; c && (size_t)(c - src) < size; ++c) > > Should this be: > > for (char *c = src; *c && (size_t)(c - src) < size; ++c) > > ? Yes! Thanks. > Thanks, > Song > > [...]

7 months, 1 week

1
0
0 0

Re: [RFC v2 04/13] dma-fence: Move array and chain checks to flags

by Christian König

On 5/12/25 11:14, Tvrtko Ursulin wrote: > > On 12/05/2025 09:19, Christian König wrote: >> On 5/9/25 17:33, Tvrtko Ursulin wrote: >>> With the goal of reducing the need for drivers to touch fence->ops, we >>> add explicit flags for struct dma_fence_array and struct dma_fence_chain >>> and make the respective helpers (dma_fence_is_array() and >>> dma_fence_is_chain()) use them. >>> >>> This also allows us to remove the exported symbols for the respective >>> operation tables. >> >> That looks like overkill to me. We don't de-reference the ops for the check, instead just the values are compared. >> >> Since the array and chain are always build in that should be completely unproblematic for driver unload. > > You are right this is not strictly needed. Idea was just to reduce any access to ops as much as we can and this fell under that scope. > > Another benefit one could perhaps argue is two fewer EXPORT_SYMBOLs, which is perhaps a little bit cleaner design (less exporting of implementation details to the outside), but it is not a super strong argument. I would rather say that using the symbols improves things. Background is that otherwise every driver could accidentally or because of malicious intend set this flag. The symbol is not so easily changeable. Regards, Christian. > > If we will not be going for this one then I would be taking 1/13 via drm-intel-gt-next. > > Regards, > > Tvrtko > >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> >>> --- >>> drivers/dma-buf/dma-fence-array.c | 2 +- >>> drivers/dma-buf/dma-fence-chain.c | 2 +- >>> include/linux/dma-fence.h | 9 ++++----- >>> 3 files changed, 6 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c >>> index 6657d4b30af9..daf444f5d228 100644 >>> --- a/drivers/dma-buf/dma-fence-array.c >>> +++ b/drivers/dma-buf/dma-fence-array.c >>> @@ -167,7 +167,6 @@ const struct dma_fence_ops dma_fence_array_ops = { >>> .release = dma_fence_array_release, >>> .set_deadline = dma_fence_array_set_deadline, >>> }; >>> -EXPORT_SYMBOL(dma_fence_array_ops); >>> /** >>> * dma_fence_array_alloc - Allocate a custom fence array >>> @@ -207,6 +206,7 @@ void dma_fence_array_init(struct dma_fence_array *array, >>> spin_lock_init(&array->lock); >>> dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock, >>> context, seqno); >>> + __set_bit(DMA_FENCE_FLAG_ARRAY_BIT, &array->base.flags); >>> init_irq_work(&array->work, irq_dma_fence_array_work); >>> atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); >>> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c >>> index a8a90acf4f34..f4abe41fb092 100644 >>> --- a/drivers/dma-buf/dma-fence-chain.c >>> +++ b/drivers/dma-buf/dma-fence-chain.c >>> @@ -225,7 +225,6 @@ const struct dma_fence_ops dma_fence_chain_ops = { >>> .release = dma_fence_chain_release, >>> .set_deadline = dma_fence_chain_set_deadline, >>> }; >>> -EXPORT_SYMBOL(dma_fence_chain_ops); >>> /** >>> * dma_fence_chain_init - initialize a fence chain >>> @@ -263,6 +262,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain, >>> dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock, >>> context, seqno); >>> + __set_bit(DMA_FENCE_FLAG_CHAIN_BIT, &chain->base.flags); >>> /* >>> * Chaining dma_fence_chain container together is only allowed through >>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h >>> index ac6535716dbe..5bafd0a5f1f1 100644 >>> --- a/include/linux/dma-fence.h >>> +++ b/include/linux/dma-fence.h >>> @@ -98,6 +98,8 @@ struct dma_fence { >>> enum dma_fence_flag_bits { >>> DMA_FENCE_FLAG_SEQNO64_BIT, >>> + DMA_FENCE_FLAG_ARRAY_BIT, >>> + DMA_FENCE_FLAG_CHAIN_BIT, >>> DMA_FENCE_FLAG_SIGNALED_BIT, >>> DMA_FENCE_FLAG_TIMESTAMP_BIT, >>> DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, >>> @@ -632,9 +634,6 @@ struct dma_fence *dma_fence_get_stub(void); >>> struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp); >>> u64 dma_fence_context_alloc(unsigned num); >>> -extern const struct dma_fence_ops dma_fence_array_ops; >>> -extern const struct dma_fence_ops dma_fence_chain_ops; >>> - >>> /** >>> * dma_fence_is_array - check if a fence is from the array subclass >>> * @fence: the fence to test >>> @@ -643,7 +642,7 @@ extern const struct dma_fence_ops dma_fence_chain_ops; >>> */ >>> static inline bool dma_fence_is_array(struct dma_fence *fence) >>> { >>> - return fence->ops == &dma_fence_array_ops; >>> + return test_bit(DMA_FENCE_FLAG_ARRAY_BIT, &fence->flags); >>> } >>> /** >>> @@ -654,7 +653,7 @@ static inline bool dma_fence_is_array(struct dma_fence *fence) >>> */ >>> static inline bool dma_fence_is_chain(struct dma_fence *fence) >>> { >>> - return fence->ops == &dma_fence_chain_ops; >>> + return test_bit(DMA_FENCE_FLAG_CHAIN_BIT, &fence->flags); >>> } >>> /** >> >

7 months, 1 week

1
0
0 0

[PATCH bpf-next v5 0/5] Replace CONFIG_DMABUF_SYSFS_STATS with BPF

by T.J. Mercier

Until CONFIG_DMABUF_SYSFS_STATS was added [1] it was only possible to perform per-buffer accounting with debugfs which is not suitable for production environments. Eventually we discovered the overhead with per-buffer sysfs file creation/removal was significantly impacting allocation and free times, and exacerbated kernfs lock contention. [2] dma_buf_stats_setup() is responsible for 39% of single-page buffer creation duration, or 74% of single-page dma_buf_export() duration when stressing dmabuf allocations and frees. I prototyped a change from per-buffer to per-exporter statistics with a RCU protected list of exporter allocations that accommodates most (but not all) of our use-cases and avoids almost all of the sysfs overhead. While that adds less overhead than per-buffer sysfs, and less even than the maintenance of the dmabuf debugfs_list, it's still *additional* overhead on top of the debugfs_list and doesn't give us per-buffer info. This series uses the existing dmabuf debugfs_list to implement a BPF dmabuf iterator, which adds no overhead to buffer allocation/free and provides per-buffer info. The list has been moved outside of CONFIG_DEBUG_FS scope so that it is always populated. The BPF program loaded by userspace that extracts per-buffer information gets to define its own interface which avoids the lack of ABI stability with debugfs. This will allow us to replace our use of CONFIG_DMABUF_SYSFS_STATS, and the plan is to remove it from the kernel after the next longterm stable release. [1] https://lore.kernel.org/linux-media/20201210044400.1080308-1-hridya@google.… [2] https://lore.kernel.org/all/20220516171315.2400578-1-tjmercier@google.com v1: https://lore.kernel.org/all/20250414225227.3642618-1-tjmercier@google.com v1 -> v2: Make the DMA buffer list independent of CONFIG_DEBUG_FS per Christian König Add CONFIG_DMA_SHARED_BUFFER check to kernel/bpf/Makefile per kernel test robot Use BTF_ID_LIST_SINGLE instead of BTF_ID_LIST_GLOBAL_SINGLE per Song Liu Fixup comment style, mixing code/declarations, and use ASSERT_OK_FD in selftest per Song Liu Add BPF_ITER_RESCHED feature to bpf_dmabuf_reg_info per Alexei Starovoitov Add open-coded iterator and selftest per Alexei Starovoitov Add a second test buffer from the system dmabuf heap to selftests Use the BPF program we'll use in production for selftest per Alexei Starovoitov https://r.android.com/c/platform/system/bpfprogs/+/3616123/2/dmabufIter.c https://r.android.com/c/platform/system/memory/libmeminfo/+/3614259/1/libdm… v2: https://lore.kernel.org/all/20250504224149.1033867-1-tjmercier@google.com v2 -> v3: Rebase onto bpf-next/master Move get_next_dmabuf() into drivers/dma-buf/dma-buf.c, along with the new get_first_dmabuf(). This avoids having to expose the dmabuf list and mutex to the rest of the kernel, and keeps the dmabuf mutex operations near each other in the same file. (Christian König) Add Christian's RB to dma-buf: Rename debugfs symbols Drop RFC: dma-buf: Remove DMA-BUF statistics v3: https://lore.kernel.org/all/20250507001036.2278781-1-tjmercier@google.com v3 -> v4: Fix selftest BPF program comment style (not kdoc) per Alexei Starovoitov Fix dma-buf.c kdoc comment style per Alexei Starovoitov Rename get_first_dmabuf / get_next_dmabuf to dma_buf_iter_begin / dma_buf_iter_next per Christian König Add Christian's RB to bpf: Add dmabuf iterator v4: https://lore.kernel.org/all/20250508182025.2961555-1-tjmercier@google.com v4 -> v5: Add Christian's Acks to all patches Add Song Liu's Acks Move BTF_ID_LIST_SINGLE and DEFINE_BPF_ITER_FUNC closer to usage per Song Liu Fix open-coded iterator comment style per Song Liu Move iterator termination check to its own subtest per Song Liu Rework selftest buffer creation per Song Liu Fix spacing in sanitize_string per BPF CI T.J. Mercier (5): dma-buf: Rename debugfs symbols bpf: Add dmabuf iterator bpf: Add open coded dmabuf iterator selftests/bpf: Add test for dmabuf_iter selftests/bpf: Add test for open coded dmabuf_iter drivers/dma-buf/dma-buf.c | 98 +++++-- include/linux/dma-buf.h | 4 +- kernel/bpf/Makefile | 3 + kernel/bpf/dmabuf_iter.c | 150 ++++++++++ kernel/bpf/helpers.c | 5 + .../testing/selftests/bpf/bpf_experimental.h | 5 + tools/testing/selftests/bpf/config | 3 + .../selftests/bpf/prog_tests/dmabuf_iter.c | 276 ++++++++++++++++++ .../testing/selftests/bpf/progs/dmabuf_iter.c | 91 ++++++ 9 files changed, 613 insertions(+), 22 deletions(-) create mode 100644 kernel/bpf/dmabuf_iter.c create mode 100644 tools/testing/selftests/bpf/prog_tests/dmabuf_iter.c create mode 100644 tools/testing/selftests/bpf/progs/dmabuf_iter.c base-commit: 43745d11bfd9683abdf08ad7a5cc403d6a9ffd15 -- 2.49.0.1045.g170613ef41-goog

7 months, 1 week

1
5
0 0

Re: [RFC PATCH 00/12] Private MMIO support for private assigned dev

by Jason Gunthorpe

On Mon, May 12, 2025 at 07:30:21PM +1000, Alexey Kardashevskiy wrote: > > > I'm surprised by this.. iommufd shouldn't be doing PCI stuff, it is > > > just about managing the translation control of the device. > > > > I have a little difficulty to understand. Is TSM bind PCI stuff? To me > > it is. Host sends PCI TDISP messages via PCI DOE to put the device in > > TDISP LOCKED state, so that device behaves differently from before. Then > > why put it in IOMMUFD? > > > "TSM bind" sets up the CPU side of it, it binds a VM to a piece of > IOMMU on the host CPU. The device does not know about the VM, it > just enables/disables encryption by a request from the CPU (those > start/stop interface commands). And IOMMUFD won't be doing DOE, the > platform driver (such as AMD CCP) will. Nothing to do for VFIO here. > > We probably should notify VFIO about the state transition but I do > not know VFIO would want to do in response. We have an awkward fit for what CCA people are doing to the various Linux APIs. Looking somewhat maximally across all the arches a "bind" for a CC vPCI device creation operation does: - Setup the CPU page tables for the VM to have access to the MMIO - Revoke hypervisor access to the MMIO - Setup the vIOMMU to understand the vPCI device - Take over control of some of the IOVA translation, at least for T=1, and route to the the vIOMMU - Register the vPCI with any attestation functions the VM might use - Do some DOE stuff to manage/validate TDSIP/etc So we have interactions of things controlled by PCI, KVM, VFIO, and iommufd all mushed together. iommufd is the only area that already has a handle to all the required objects: - The physical PCI function - The CC vIOMMU object - The KVM FD - The CC vPCI object Which is why I have been thinking it is the right place to manage this. It doesn't mean that iommufd is suddenly doing PCI stuff, no, that stays in VFIO. > > > So your issue is you need to shoot down the dmabuf during vPCI device > > > destruction? > > > > I assume "vPCI device" refers to assigned device in both shared mode & > > prvate mode. So no, I need to shoot down the dmabuf during TSM unbind, > > a.k.a. when assigned device is converting from private to shared. > > Then recover the dmabuf after TSM unbind. The device could still work > > in VM in shared mode. What are you trying to protect with this? Is there some intelism where you can't have references to encrypted MMIO pages? > > What I really want is, one SW component to manage MMIO dmabuf, secure > > iommu & TSM bind/unbind. So easier coordinate these 3 operations cause > > these ops are interconnected according to secure firmware's requirement. > > This SW component is QEMU. It knows about FLRs and other config > space things, it can destroy all these IOMMUFD objects and talk to > VFIO too, I've tried, so far it is looking easier to manage. Thanks, Yes, qemu should be sequencing this. The kernel only needs to enforce any rules required to keep the system from crashing. Jason

7 months, 1 week

1
0
0 0

Re: [PATCH] dma-buf: insert memory barrier before updating num_fences

by Christian König

On 5/12/25 13:12, Hyejeong Choi wrote: > smp_store_mb() inserts memory barrier after storing operation. > It is different with what the comment is originally aiming so Null > pointer dereference can be happened if memory update is reordered. > > Signed-off-by: Hyejeong Choi <hjeong.choi(a)samsung.com> > --- > drivers/dma-buf/dma-resv.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index 5f8d010516f0..52af5c7430da 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c > @@ -320,8 +320,9 @@ void dma_resv_add_fence(struct dma_resv *obj, struct dma_fence *fence, > count++; > > dma_resv_list_set(fobj, i, fence, usage); > - /* pointer update must be visible before we extend the num_fences */ > - smp_store_mb(fobj->num_fences, count); > + /* fence update must be visible before we extend the num_fences */ > + smp_wmb(); > + WRITE_ONCE(fobj->num_fences, count); The WRITE_ONCE isn't necessary since smp_wmb() implies a compiler barrier, but apart from that really good catch. Can you modify the patch and re-send? I will be pushing it to -fixes ASAP. Regards, Christian. > } > EXPORT_SYMBOL(dma_resv_add_fence); > > >

7 months, 1 week

1
0
0 0

Re: [RFC v2 09/13] drm/i915: Use dma-fence driver and timeline name helpers

by Christian König

On 5/9/25 17:33, Tvrtko Ursulin wrote: > Access the dma-fence internals via the previously added helpers. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> > --- > drivers/gpu/drm/i915/gt/intel_gt_requests.c | 4 ++-- > drivers/gpu/drm/i915/i915_request.c | 2 +- > drivers/gpu/drm/i915/i915_sw_fence.c | 4 ++-- > 3 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c > index d1a382dfaa1d..ae3557ed6c1e 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c > @@ -252,8 +252,8 @@ void intel_gt_watchdog_work(struct work_struct *work) > struct dma_fence *f = &rq->fence; > > pr_notice("Fence expiration time out i915-%s:%s:%llx!\n", > - f->ops->get_driver_name(f), > - f->ops->get_timeline_name(f), > + dma_fence_driver_name(f), > + dma_fence_timeline_name(f), > f->seqno); > i915_request_cancel(rq, -EINTR); > } > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c > index c3d27eadc0a7..4874c4f1e4ab 100644 > --- a/drivers/gpu/drm/i915/i915_request.c > +++ b/drivers/gpu/drm/i915/i915_request.c > @@ -2184,7 +2184,7 @@ void i915_request_show(struct drm_printer *m, > const char *prefix, > int indent) > { > - const char *name = rq->fence.ops->get_timeline_name((struct dma_fence *)&rq->fence); > + const char *name = dma_fence_timeline_name((struct dma_fence *)&rq->fence); > char buf[80] = ""; > int x = 0; > > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c > index 1d4cc91c0e40..e51ca7e50a4e 100644 > --- a/drivers/gpu/drm/i915/i915_sw_fence.c > +++ b/drivers/gpu/drm/i915/i915_sw_fence.c > @@ -435,8 +435,8 @@ static void timer_i915_sw_fence_wake(struct timer_list *t) > return; > > pr_notice("Asynchronous wait on fence %s:%s:%llx timed out (hint:%ps)\n", > - cb->dma->ops->get_driver_name(cb->dma), > - cb->dma->ops->get_timeline_name(cb->dma), > + dma_fence_driver_name(cb->dma), > + dma_fence_timeline_name(cb->dma), > cb->dma->seqno, > i915_sw_fence_debug_hint(fence)); >

7 months, 1 week

1
0
0 0

Re: [RFC v2 08/13] drm/amdgpu: Use dma-fence driver and timeline name helpers

by Christian König

On 5/9/25 17:33, Tvrtko Ursulin wrote: > Access the dma-fence internals via the previously added helpers. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h > index 11dd2e0f7979..8e5bf179a6c8 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h > @@ -33,7 +33,7 @@ > #define TRACE_INCLUDE_FILE amdgpu_trace > > #define AMDGPU_JOB_GET_TIMELINE_NAME(job) \ > - job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished) > + dma_fence_timeline_name(&job->base.s_fence->finished) I think you can nuke the macro now as well, the new function name is short enough. Christian. > > TRACE_EVENT(amdgpu_device_rreg, > TP_PROTO(unsigned did, uint32_t reg, uint32_t value),

7 months, 1 week

1
0
0 0

Re: [RFC v2 07/13] sync_file: Use dma-fence driver and timeline name helpers

by Christian König

On 5/9/25 17:33, Tvrtko Ursulin wrote: > Access the dma-fence internals via the previously added helpers. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> > --- > drivers/dma-buf/sync_file.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > index d9b1c1b2a72b..212df4b849fe 100644 > --- a/drivers/dma-buf/sync_file.c > +++ b/drivers/dma-buf/sync_file.c > @@ -137,8 +137,8 @@ char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len) > struct dma_fence *fence = sync_file->fence; > > snprintf(buf, len, "%s-%s%llu-%lld", > - fence->ops->get_driver_name(fence), > - fence->ops->get_timeline_name(fence), > + dma_fence_driver_name(fence), > + dma_fence_timeline_name(fence), > fence->context, > fence->seqno); > } > @@ -262,9 +262,9 @@ static long sync_file_ioctl_merge(struct sync_file *sync_file, > static int sync_fill_fence_info(struct dma_fence *fence, > struct sync_fence_info *info) > { > - strscpy(info->obj_name, fence->ops->get_timeline_name(fence), > + strscpy(info->obj_name, dma_fence_timeline_name(fence), > sizeof(info->obj_name)); > - strscpy(info->driver_name, fence->ops->get_driver_name(fence), > + strscpy(info->driver_name, dma_fence_driver_name(fence), > sizeof(info->driver_name)); > > info->status = dma_fence_get_status(fence);

7 months, 1 week

1
0
0 0

Re: [RFC v2 06/13] dma-fence: Use driver and timeline name helpers internally

by Christian König

On 5/9/25 17:33, Tvrtko Ursulin wrote: > Make the implementation use the previously added helper for accessing the > driver and timeline name. This will enable more coverage later when > asserts will be added into them. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> I would squash that together with the previous patch. Since both patches are for the same component it doesn't make much sense to separate them. Anyway Reviewed-by: Christian König <christian.koenig(a)amd.com> > --- > drivers/dma-buf/dma-fence.c | 9 +++++---- > include/trace/events/dma_fence.h | 4 ++-- > 2 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index 33661658f684..dc2456f68685 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -538,8 +538,8 @@ void dma_fence_release(struct kref *kref) > if (WARN(!list_empty(&fence->cb_list) && > !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags), > "Fence %s:%s:%llx:%llx released with pending signals!\n", > - fence->ops->get_driver_name(fence), > - fence->ops->get_timeline_name(fence), > + dma_fence_driver_name(fence), > + dma_fence_timeline_name(fence), > fence->context, fence->seqno)) { > unsigned long flags; > > @@ -983,8 +983,9 @@ EXPORT_SYMBOL(dma_fence_set_deadline); > void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq) > { > seq_printf(seq, "%s %s seq %llu %ssignalled\n", > - fence->ops->get_driver_name(fence), > - fence->ops->get_timeline_name(fence), fence->seqno, > + dma_fence_driver_name(fence), > + dma_fence_timeline_name(fence), > + fence->seqno, > dma_fence_is_signaled(fence) ? "" : "un"); > } > EXPORT_SYMBOL(dma_fence_describe); > diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h > index a4de3df8500b..84c83074ee81 100644 > --- a/include/trace/events/dma_fence.h > +++ b/include/trace/events/dma_fence.h > @@ -16,8 +16,8 @@ DECLARE_EVENT_CLASS(dma_fence, > TP_ARGS(fence), > > TP_STRUCT__entry( > - __string(driver, fence->ops->get_driver_name(fence)) > - __string(timeline, fence->ops->get_timeline_name(fence)) > + __string(driver, dma_fence_driver_name(fence)) > + __string(timeline, dma_fence_timeline_name(fence)) > __field(unsigned int, context) > __field(unsigned int, seqno) > ),

7 months, 1 week

1
0
0 0

Re: [RFC v2 05/13] dma-fence: Add helpers for accessing driver and timeline name

by Christian König

On 5/9/25 17:33, Tvrtko Ursulin wrote: > Add some helpers in order to enable preventing dma-fence users accessing > the implementation details directly. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> > --- > include/linux/dma-fence.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index 5bafd0a5f1f1..c814a86087f8 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -387,6 +387,16 @@ bool dma_fence_remove_callback(struct dma_fence *fence, > struct dma_fence_cb *cb); > void dma_fence_enable_sw_signaling(struct dma_fence *fence); > > +static inline const char *dma_fence_driver_name(struct dma_fence *fence) > +{ > + return fence->ops->get_driver_name(fence); > +} > + > +static inline const char *dma_fence_timeline_name(struct dma_fence *fence) > +{ > + return fence->ops->get_timeline_name(fence); > +} > + > /** > * dma_fence_is_signaled_locked - Return an indication if the fence > * is signaled yet.

7 months, 1 week

1
0
0 0

Re: [RFC v2 04/13] dma-fence: Move array and chain checks to flags

by Christian König

On 5/9/25 17:33, Tvrtko Ursulin wrote: > With the goal of reducing the need for drivers to touch fence->ops, we > add explicit flags for struct dma_fence_array and struct dma_fence_chain > and make the respective helpers (dma_fence_is_array() and > dma_fence_is_chain()) use them. > > This also allows us to remove the exported symbols for the respective > operation tables. That looks like overkill to me. We don't de-reference the ops for the check, instead just the values are compared. Since the array and chain are always build in that should be completely unproblematic for driver unload. Regards, Christian. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> > --- > drivers/dma-buf/dma-fence-array.c | 2 +- > drivers/dma-buf/dma-fence-chain.c | 2 +- > include/linux/dma-fence.h | 9 ++++----- > 3 files changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c > index 6657d4b30af9..daf444f5d228 100644 > --- a/drivers/dma-buf/dma-fence-array.c > +++ b/drivers/dma-buf/dma-fence-array.c > @@ -167,7 +167,6 @@ const struct dma_fence_ops dma_fence_array_ops = { > .release = dma_fence_array_release, > .set_deadline = dma_fence_array_set_deadline, > }; > -EXPORT_SYMBOL(dma_fence_array_ops); > > /** > * dma_fence_array_alloc - Allocate a custom fence array > @@ -207,6 +206,7 @@ void dma_fence_array_init(struct dma_fence_array *array, > spin_lock_init(&array->lock); > dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock, > context, seqno); > + __set_bit(DMA_FENCE_FLAG_ARRAY_BIT, &array->base.flags); > init_irq_work(&array->work, irq_dma_fence_array_work); > > atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); > diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c > index a8a90acf4f34..f4abe41fb092 100644 > --- a/drivers/dma-buf/dma-fence-chain.c > +++ b/drivers/dma-buf/dma-fence-chain.c > @@ -225,7 +225,6 @@ const struct dma_fence_ops dma_fence_chain_ops = { > .release = dma_fence_chain_release, > .set_deadline = dma_fence_chain_set_deadline, > }; > -EXPORT_SYMBOL(dma_fence_chain_ops); > > /** > * dma_fence_chain_init - initialize a fence chain > @@ -263,6 +262,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain, > > dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock, > context, seqno); > + __set_bit(DMA_FENCE_FLAG_CHAIN_BIT, &chain->base.flags); > > /* > * Chaining dma_fence_chain container together is only allowed through > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index ac6535716dbe..5bafd0a5f1f1 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -98,6 +98,8 @@ struct dma_fence { > > enum dma_fence_flag_bits { > DMA_FENCE_FLAG_SEQNO64_BIT, > + DMA_FENCE_FLAG_ARRAY_BIT, > + DMA_FENCE_FLAG_CHAIN_BIT, > DMA_FENCE_FLAG_SIGNALED_BIT, > DMA_FENCE_FLAG_TIMESTAMP_BIT, > DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, > @@ -632,9 +634,6 @@ struct dma_fence *dma_fence_get_stub(void); > struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp); > u64 dma_fence_context_alloc(unsigned num); > > -extern const struct dma_fence_ops dma_fence_array_ops; > -extern const struct dma_fence_ops dma_fence_chain_ops; > - > /** > * dma_fence_is_array - check if a fence is from the array subclass > * @fence: the fence to test > @@ -643,7 +642,7 @@ extern const struct dma_fence_ops dma_fence_chain_ops; > */ > static inline bool dma_fence_is_array(struct dma_fence *fence) > { > - return fence->ops == &dma_fence_array_ops; > + return test_bit(DMA_FENCE_FLAG_ARRAY_BIT, &fence->flags); > } > > /** > @@ -654,7 +653,7 @@ static inline bool dma_fence_is_array(struct dma_fence *fence) > */ > static inline bool dma_fence_is_chain(struct dma_fence *fence) > { > - return fence->ops == &dma_fence_chain_ops; > + return test_bit(DMA_FENCE_FLAG_CHAIN_BIT, &fence->flags); > } > > /**

7 months, 1 week

1
0
0 0

Re: [RFC v2 03/13] dma-fence: Use a flag for 64-bit seqnos

by Christian König

On 5/9/25 17:33, Tvrtko Ursulin wrote: > With the goal of reducing the need for drivers to touch (and dereference) > fence->ops, we move the 64-bit seqnos flag from struct dma_fence_ops to > the fence->flags. > > Drivers which were setting this flag are changed to use new > dma_fence_init64() instead of dma_fence_init(). > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> My idea was to copy the flag from the ops during init, but that should work as well. Reviewed-by: Christian König <christian.koenig(a)amd.com> > --- > drivers/dma-buf/dma-fence-chain.c | 5 ++--- > drivers/dma-buf/dma-fence.c | 9 +++++++++ > drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c | 5 ++--- > include/linux/dma-fence.h | 6 +++++- > 4 files changed, 18 insertions(+), 7 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c > index 90424f23fd73..a8a90acf4f34 100644 > --- a/drivers/dma-buf/dma-fence-chain.c > +++ b/drivers/dma-buf/dma-fence-chain.c > @@ -218,7 +218,6 @@ static void dma_fence_chain_set_deadline(struct dma_fence *fence, > } > > const struct dma_fence_ops dma_fence_chain_ops = { > - .use_64bit_seqno = true, > .get_driver_name = dma_fence_chain_get_driver_name, > .get_timeline_name = dma_fence_chain_get_timeline_name, > .enable_signaling = dma_fence_chain_enable_signaling, > @@ -262,8 +261,8 @@ void dma_fence_chain_init(struct dma_fence_chain *chain, > seqno = max(prev->seqno, seqno); > } > > - dma_fence_init(&chain->base, &dma_fence_chain_ops, > - &chain->lock, context, seqno); > + dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock, > + context, seqno); > > /* > * Chaining dma_fence_chain container together is only allowed through > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index f0cdd3e99d36..33661658f684 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -1023,3 +1023,12 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, > trace_dma_fence_init(fence); > } > EXPORT_SYMBOL(dma_fence_init); > + > +void > +dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops, > + spinlock_t *lock, u64 context, u64 seqno) > +{ > + dma_fence_init(fence, ops, lock, context, seqno); > + __set_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags); > +} > +EXPORT_SYMBOL(dma_fence_init64); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c > index 51cddfa3f1e8..5d26797356a3 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c > @@ -71,7 +71,6 @@ static void amdgpu_tlb_fence_work(struct work_struct *work) > } > > static const struct dma_fence_ops amdgpu_tlb_fence_ops = { > - .use_64bit_seqno = true, > .get_driver_name = amdgpu_tlb_fence_get_driver_name, > .get_timeline_name = amdgpu_tlb_fence_get_timeline_name > }; > @@ -101,8 +100,8 @@ void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct amdgpu_vm *vm > INIT_WORK(&f->work, amdgpu_tlb_fence_work); > spin_lock_init(&f->lock); > > - dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock, > - vm->tlb_fence_context, atomic64_read(&vm->tlb_seq)); > + dma_fence_init64(&f->base, &amdgpu_tlb_fence_ops, &f->lock, > + vm->tlb_fence_context, atomic64_read(&vm->tlb_seq)); > > /* TODO: We probably need a separate wq here */ > dma_fence_get(&f->base); > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index 48b5202c531d..ac6535716dbe 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -97,6 +97,7 @@ struct dma_fence { > }; > > enum dma_fence_flag_bits { > + DMA_FENCE_FLAG_SEQNO64_BIT, > DMA_FENCE_FLAG_SIGNALED_BIT, > DMA_FENCE_FLAG_TIMESTAMP_BIT, > DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, > @@ -262,6 +263,9 @@ struct dma_fence_ops { > void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, > spinlock_t *lock, u64 context, u64 seqno); > > +void dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops, > + spinlock_t *lock, u64 context, u64 seqno); > + > void dma_fence_release(struct kref *kref); > void dma_fence_free(struct dma_fence *fence); > void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq); > @@ -454,7 +458,7 @@ static inline bool __dma_fence_is_later(struct dma_fence *fence, u64 f1, u64 f2) > * 32bit sequence numbers. Use a 64bit compare when the driver says to > * do so. > */ > - if (fence->ops->use_64bit_seqno) > + if (test_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags)) > return f1 > f2; > > return (int)(lower_32_bits(f1) - lower_32_bits(f2)) > 0;

7 months, 1 week

1
0
0 0

Re: [RFC v2 02/13] dma-fence: Change signature of __dma_fence_is_later

by Christian König

On 5/9/25 17:33, Tvrtko Ursulin wrote: > With the goal of reducing the need for drivers to touch (and dereference) > fence->ops, we change the prototype of __dma_fence_is_later() to take > fence instead of fence->ops. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> > --- > drivers/dma-buf/dma-fence-chain.c | 2 +- > drivers/dma-buf/sw_sync.c | 2 +- > drivers/gpu/drm/xe/xe_hw_fence.c | 2 +- > drivers/gpu/drm/xe/xe_sched_job.c | 14 ++++++++------ > include/linux/dma-fence.h | 9 ++++----- > 5 files changed, 15 insertions(+), 14 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c > index 9663ba1bb6ac..90424f23fd73 100644 > --- a/drivers/dma-buf/dma-fence-chain.c > +++ b/drivers/dma-buf/dma-fence-chain.c > @@ -252,7 +252,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain, > chain->prev_seqno = 0; > > /* Try to reuse the context of the previous chain node. */ > - if (prev_chain && __dma_fence_is_later(seqno, prev->seqno, prev->ops)) { > + if (prev_chain && __dma_fence_is_later(prev, seqno, prev->seqno)) { > context = prev->context; > chain->prev_seqno = prev->seqno; > } else { > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c > index 4f27ee93a00c..3c20f1d31cf5 100644 > --- a/drivers/dma-buf/sw_sync.c > +++ b/drivers/dma-buf/sw_sync.c > @@ -170,7 +170,7 @@ static bool timeline_fence_signaled(struct dma_fence *fence) > { > struct sync_timeline *parent = dma_fence_parent(fence); > > - return !__dma_fence_is_later(fence->seqno, parent->value, fence->ops); > + return !__dma_fence_is_later(fence, fence->seqno, parent->value); > } > > static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t deadline) > diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c > index 0b4f12be3692..03eb8c6d1616 100644 > --- a/drivers/gpu/drm/xe/xe_hw_fence.c > +++ b/drivers/gpu/drm/xe/xe_hw_fence.c > @@ -165,7 +165,7 @@ static bool xe_hw_fence_signaled(struct dma_fence *dma_fence) > u32 seqno = xe_map_rd(xe, &fence->seqno_map, 0, u32); > > return dma_fence->error || > - !__dma_fence_is_later(dma_fence->seqno, seqno, dma_fence->ops); > + !__dma_fence_is_later(dma_fence, dma_fence->seqno, seqno); > } > > static bool xe_hw_fence_enable_signaling(struct dma_fence *dma_fence) > diff --git a/drivers/gpu/drm/xe/xe_sched_job.c b/drivers/gpu/drm/xe/xe_sched_job.c > index 1905ca590965..f0a6ce610948 100644 > --- a/drivers/gpu/drm/xe/xe_sched_job.c > +++ b/drivers/gpu/drm/xe/xe_sched_job.c > @@ -216,15 +216,17 @@ void xe_sched_job_set_error(struct xe_sched_job *job, int error) > > bool xe_sched_job_started(struct xe_sched_job *job) > { > + struct dma_fence *fence = dma_fence_chain_contained(job->fence); > struct xe_lrc *lrc = job->q->lrc[0]; > > - return !__dma_fence_is_later(xe_sched_job_lrc_seqno(job), > - xe_lrc_start_seqno(lrc), > - dma_fence_chain_contained(job->fence)->ops); > + return !__dma_fence_is_later(fence, > + xe_sched_job_lrc_seqno(job), > + xe_lrc_start_seqno(lrc)); > } > > bool xe_sched_job_completed(struct xe_sched_job *job) > { > + struct dma_fence *fence = dma_fence_chain_contained(job->fence); > struct xe_lrc *lrc = job->q->lrc[0]; > > /* > @@ -232,9 +234,9 @@ bool xe_sched_job_completed(struct xe_sched_job *job) > * parallel handshake is done. > */ > > - return !__dma_fence_is_later(xe_sched_job_lrc_seqno(job), > - xe_lrc_seqno(lrc), > - dma_fence_chain_contained(job->fence)->ops); > + return !__dma_fence_is_later(fence, > + xe_sched_job_lrc_seqno(job), > + xe_lrc_seqno(lrc)); > } > > void xe_sched_job_arm(struct xe_sched_job *job) > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index b12776883d14..48b5202c531d 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -441,21 +441,20 @@ dma_fence_is_signaled(struct dma_fence *fence) > > /** > * __dma_fence_is_later - return if f1 is chronologically later than f2 > + * @fence: fence in whose context to do the comparison > * @f1: the first fence's seqno > * @f2: the second fence's seqno from the same context > - * @ops: dma_fence_ops associated with the seqno > * > * Returns true if f1 is chronologically later than f2. Both fences must be > * from the same context, since a seqno is not common across contexts. > */ > -static inline bool __dma_fence_is_later(u64 f1, u64 f2, > - const struct dma_fence_ops *ops) > +static inline bool __dma_fence_is_later(struct dma_fence *fence, u64 f1, u64 f2) > { > /* This is for backward compatibility with drivers which can only handle > * 32bit sequence numbers. Use a 64bit compare when the driver says to > * do so. > */ > - if (ops->use_64bit_seqno) > + if (fence->ops->use_64bit_seqno) > return f1 > f2; > > return (int)(lower_32_bits(f1) - lower_32_bits(f2)) > 0; > @@ -475,7 +474,7 @@ static inline bool dma_fence_is_later(struct dma_fence *f1, > if (WARN_ON(f1->context != f2->context)) > return false; > > - return __dma_fence_is_later(f1->seqno, f2->seqno, f1->ops); > + return __dma_fence_is_later(f1, f1->seqno, f2->seqno); > } > > /**

7 months, 1 week

1
0
0 0

Re: [RFC v2 01/13] drm/i915: Use provided dma_fence_is_chain

by Christian König

On 5/9/25 17:47, Matthew Brost wrote: > On Fri, May 09, 2025 at 04:33:40PM +0100, Tvrtko Ursulin wrote: >> Replace open-coded helper with the subsystem one. >> > > You probably can just send this one by itself as it good cleanup and > independent. > > Reviewed-by: Matthew Brost <matthew.brost(a)intel.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> Any objections that I start to push those patches to drm-misc-next or do you want to take this one through the i915 branch? Regards, Christian. > >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_wait.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c >> index 7127e90c1a8f..991666fd9f85 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c >> @@ -106,11 +106,6 @@ static void fence_set_priority(struct dma_fence *fence, >> rcu_read_unlock(); >> } >> >> -static inline bool __dma_fence_is_chain(const struct dma_fence *fence) >> -{ >> - return fence->ops == &dma_fence_chain_ops; >> -} >> - >> void i915_gem_fence_wait_priority(struct dma_fence *fence, >> const struct i915_sched_attr *attr) >> { >> @@ -126,7 +121,7 @@ void i915_gem_fence_wait_priority(struct dma_fence *fence, >> >> for (i = 0; i < array->num_fences; i++) >> fence_set_priority(array->fences[i], attr); >> - } else if (__dma_fence_is_chain(fence)) { >> + } else if (dma_fence_is_chain(fence)) { >> struct dma_fence *iter; >> >> /* The chain is ordered; if we boost the last, we boost all */ >> -- >> 2.48.0 >>

7 months, 1 week

1
0
0 0

Re: [PATCH bpf-next v4 5/5] selftests/bpf: Add test for open coded dmabuf_iter

by T.J. Mercier

On Fri, May 9, 2025 at 2:58 PM Song Liu <song(a)kernel.org> wrote: > > On Fri, May 9, 2025 at 2:43 PM T.J. Mercier <tjmercier(a)google.com> wrote: > > > [...] > > > > > > Personally, I would prefer we just merge all the logic of > > > create_udmabuf() and create_sys_heap_dmabuf() > > > into create_test_buffers(). > > > > That's a lot of different stuff to put in one place. How about > > returning file descriptors from the buffer create functions while > > having them clean up after themselves: > > I do like this version better. Some nitpicks though. > > > > > -static int memfd, udmabuf; > > +static int udmabuf; > > About this, and ... > > > static const char udmabuf_test_buffer_name[DMA_BUF_NAME_LEN] = > > "udmabuf_test_buffer_for_iter"; > > static size_t udmabuf_test_buffer_size; > > static int sysheap_dmabuf; > > static const char sysheap_test_buffer_name[DMA_BUF_NAME_LEN] = > > "sysheap_test_buffer_for_iter"; > > static size_t sysheap_test_buffer_size; > > > > -static int create_udmabuf(int map_fd) > > +static int create_udmabuf(void) > > { > > struct udmabuf_create create; > > - int dev_udmabuf; > > - bool f = false; > > + int dev_udmabuf, memfd, udmabuf; > .. here. > > It is not ideal to have a global udmabuf and a local udmabuf. > If we want the global version, let's rename the local one. Ok let me change up the name of the aliasing variable to local_udmabuf. > [...] > > > > > static int create_test_buffers(int map_fd) > > { > > - int ret; > > + bool f = false; > > + > > + udmabuf = create_udmabuf(); > > + sysheap_dmabuf = create_sys_heap_dmabuf(); > > > > - ret = create_udmabuf(map_fd); > > - if (ret) > > - return ret; > > + if (udmabuf < 0 || sysheap_dmabuf < 0) > > + return -1; > > We also need destroy_test_buffers() on the error path here, > or at the caller. The caller does currently check to decide if it should bother running the tests or not, and calls destroy_test_buffers() if not. > > - return create_sys_heap_dmabuf(map_fd); > > + return bpf_map_update_elem(map_fd, udmabuf_test_buffer_name, > > &f, BPF_ANY) || > > + bpf_map_update_elem(map_fd, sysheap_test_buffer_name, > > &f, BPF_ANY); > > } > > > > static void destroy_test_buffers(void) > > { > > close(udmabuf); > > - close(memfd); > > close(sysheap_dmabuf); > > For the two global fds, let's reset them to -1 right after close(). > > Thanks, > Song Will do, thanks.

7 months, 1 week

1
0
0 0

Re: [PATCH bpf-next v4 5/5] selftests/bpf: Add test for open coded dmabuf_iter

by T.J. Mercier

On Fri, May 9, 2025 at 11:46 AM Song Liu <song(a)kernel.org> wrote: > > On Thu, May 8, 2025 at 11:21 AM T.J. Mercier <tjmercier(a)google.com> wrote: > > > > Use the same test buffers as the traditional iterator and a new BPF map > > to verify the test buffers can be found with the open coded dmabuf > > iterator. > > The way we split 4/5 and 5/5 makes the code tricker to follow. I guess > the motivation is to back port default iter along to older kernels. But I > think we can still make the code cleaner. > > > > > Signed-off-by: T.J. Mercier <tjmercier(a)google.com> > > --- > [...] > > > > > -static int create_udmabuf(void) > > +static int create_udmabuf(int map_fd) > > { > > struct udmabuf_create create; > > int dev_udmabuf; > > + bool f = false; > > > > udmabuf_test_buffer_size = 10 * getpagesize(); > > > > @@ -63,10 +64,10 @@ static int create_udmabuf(void) > > if (!ASSERT_OK(ioctl(udmabuf, DMA_BUF_SET_NAME_B, udmabuf_test_buffer_name), "name")) > > return 1; > > > > - return 0; > > + return bpf_map_update_elem(map_fd, udmabuf_test_buffer_name, &f, BPF_ANY); > > We don't really need this bpf_map_update_elem() inside > create_udmabuf(), right? > > > } > > > > -static int create_sys_heap_dmabuf(void) > > +static int create_sys_heap_dmabuf(int map_fd) > > { > > sysheap_test_buffer_size = 20 * getpagesize(); > > > > @@ -77,6 +78,7 @@ static int create_sys_heap_dmabuf(void) > > .heap_flags = 0, > > }; > > int heap_fd, ret; > > + bool f = false; > > > > if (!ASSERT_LE(sizeof(sysheap_test_buffer_name), DMA_BUF_NAME_LEN, "NAMETOOLONG")) > > return 1; > > @@ -95,18 +97,18 @@ static int create_sys_heap_dmabuf(void) > > if (!ASSERT_OK(ioctl(sysheap_dmabuf, DMA_BUF_SET_NAME_B, sysheap_test_buffer_name), "name")) > > return 1; > > > > - return 0; > > + return bpf_map_update_elem(map_fd, sysheap_test_buffer_name, &f, BPF_ANY); > > Same for this bpf_map_update_elem(), we can call this directly from > create_test_buffers(). > > > } > > > > -static int create_test_buffers(void) > > +static int create_test_buffers(int map_fd) > > { > > int ret; > > > > - ret = create_udmabuf(); > > + ret = create_udmabuf(map_fd); > > if (ret) > > return ret; > > > > - return create_sys_heap_dmabuf(); > > + return create_sys_heap_dmabuf(map_fd); > > Personally, I would prefer we just merge all the logic of > create_udmabuf() and create_sys_heap_dmabuf() > into create_test_buffers(). That's a lot of different stuff to put in one place. How about returning file descriptors from the buffer create functions while having them clean up after themselves: -static int memfd, udmabuf; +static int udmabuf; static const char udmabuf_test_buffer_name[DMA_BUF_NAME_LEN] = "udmabuf_test_buffer_for_iter"; static size_t udmabuf_test_buffer_size; static int sysheap_dmabuf; static const char sysheap_test_buffer_name[DMA_BUF_NAME_LEN] = "sysheap_test_buffer_for_iter"; static size_t sysheap_test_buffer_size; -static int create_udmabuf(int map_fd) +static int create_udmabuf(void) { struct udmabuf_create create; - int dev_udmabuf; - bool f = false; + int dev_udmabuf, memfd, udmabuf; udmabuf_test_buffer_size = 10 * getpagesize(); if (!ASSERT_LE(sizeof(udmabuf_test_buffer_name), DMA_BUF_NAME_LEN, "NAMETOOLONG")) - return 1; + return -1; memfd = memfd_create("memfd_test", MFD_ALLOW_SEALING); if (!ASSERT_OK_FD(memfd, "memfd_create")) - return 1; + return -1; if (!ASSERT_OK(ftruncate(memfd, udmabuf_test_buffer_size), "ftruncate")) - return 1; + goto close_memfd; if (!ASSERT_OK(fcntl(memfd, F_ADD_SEALS, F_SEAL_SHRINK), "seal")) - return 1; + goto close_memfd; dev_udmabuf = open("/dev/udmabuf", O_RDONLY); if (!ASSERT_OK_FD(dev_udmabuf, "open udmabuf")) - return 1; + goto close_memfd; create.memfd = memfd; create.flags = UDMABUF_FLAGS_CLOEXEC; @@ -59,15 +58,21 @@ static int create_udmabuf(int map_fd) udmabuf = ioctl(dev_udmabuf, UDMABUF_CREATE, &create); close(dev_udmabuf); if (!ASSERT_OK_FD(udmabuf, "udmabuf_create")) - return 1; + goto close_memfd; if (!ASSERT_OK(ioctl(udmabuf, DMA_BUF_SET_NAME_B, udmabuf_test_buffer_name), "name")) - return 1; + goto close_udmabuf; + + return udmabuf; - return bpf_map_update_elem(map_fd, udmabuf_test_buffer_name, &f, BPF_ANY); +close_udmabuf: + close(udmabuf); +close_memfd: + close(memfd); + return -1; } -static int create_sys_heap_dmabuf(int map_fd) +static int create_sys_heap_dmabuf(void) { sysheap_test_buffer_size = 20 * getpagesize(); @@ -78,43 +83,46 @@ static int create_sys_heap_dmabuf(int map_fd) .heap_flags = 0, }; int heap_fd, ret; - bool f = false; if (!ASSERT_LE(sizeof(sysheap_test_buffer_name), DMA_BUF_NAME_LEN, "NAMETOOLONG")) - return 1; + return -1; heap_fd = open("/dev/dma_heap/system", O_RDONLY); if (!ASSERT_OK_FD(heap_fd, "open dma heap")) - return 1; + return -1; ret = ioctl(heap_fd, DMA_HEAP_IOCTL_ALLOC, &data); close(heap_fd); if (!ASSERT_OK(ret, "syheap alloc")) - return 1; + return -1; - sysheap_dmabuf = data.fd; + if (!ASSERT_OK(ioctl(data.fd, DMA_BUF_SET_NAME_B, sysheap_test_buffer_name), "name")) + goto close_sysheap_dmabuf; - if (!ASSERT_OK(ioctl(sysheap_dmabuf, DMA_BUF_SET_NAME_B, sysheap_test_buffer_name), "name")) - return 1; + return data.fd; - return bpf_map_update_elem(map_fd, sysheap_test_buffer_name, &f, BPF_ANY); +close_sysheap_dmabuf: + close(data.fd); + return -1; } static int create_test_buffers(int map_fd) { - int ret; + bool f = false; + + udmabuf = create_udmabuf(); + sysheap_dmabuf = create_sys_heap_dmabuf(); - ret = create_udmabuf(map_fd); - if (ret) - return ret; + if (udmabuf < 0 || sysheap_dmabuf < 0) + return -1; - return create_sys_heap_dmabuf(map_fd); + return bpf_map_update_elem(map_fd, udmabuf_test_buffer_name, &f, BPF_ANY) || + bpf_map_update_elem(map_fd, sysheap_test_buffer_name, &f, BPF_ANY); } static void destroy_test_buffers(void) { close(udmabuf); - close(memfd); close(sysheap_dmabuf); }

7 months, 1 week

1
0
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig