On 9/1/26 19:08, David Hu wrote:
> From: David Hu <xuehaohu(a)google.com>
>
> This series address two related issues in scatter-gather mapping,
> specifically for the MMIO based dma-buf mapping. The fixes ensure
> sgt mapping is correct, and proper for large MMIO regions.
>
> Patch 1 fixes a silent integer overflow for mapping length exceeding 4G
> (Previously submitted as [PATCH v7] dma-buf: Fix silent overflow for
> phys vec to sgt)
> https://lore.kernel.org/all/20260609164047.486227-1-xuehaohu@google.com/
>
> Patch 2 Splits sgl by largest page aligned chunk
> (Previously submitted as [PATCH v3] dma-buf: Split sgl by largest page-aligned chunk)
> https://lore.kernel.org/all/20260722233806.3922093-1-dhu@x6u.co/
*sigh* such issues are exactly the reason why I didn't wanted the dma-mapping stuff inside DMA-buf. That clearly doesn't belong here.
I'm going to push those fixes to drm-misc-next now, but when there are more issues like that will mark the code as abandoned and not maintained.
Regards,
Christian.
>
> Changes in v8:
> - Combined the two patches into one unified series to avoid merge
> conflicts.
> - Collected Reviewed-by tag from Leon Romanovsky for Patch 2.
>
> David Hu (2):
> dma-buf: Fix silent overflow for phys vec to sgt
> dma-buf: Split sgl by largest page-aligned chunk
>
> drivers/dma-buf/dma-buf-mapping.c | 31 +++++++++++++++++++++----------
> 1 file changed, 21 insertions(+), 10 deletions(-)
>
> --
> 2.55.0.897.gb25b4bd76c-goog
>
Applied this to drm-misc-fixes.
On 8/19/26 15:44, Taimuraz Kaitmazov wrote:
> SYNC_BO clflushes an imported BO's scatterlist. An importer may not do
> that: the memory belongs to the exporter, and dma-buf gives the importer
> no interface to ask for maintenance on it. Refuse the request instead.
>
> is_import_bo() is (obj)->attach, which covers more than foreign buffers.
> A userptr BO arrives through a ubuf, and on a carveout device every share
> BO and the device heap arrive through a cbuf, so SYNC_BO answers
> -EOPNOTSUPP for those too, including the AMDXDNA_BO_DEV path that flushes
> through its heap.
>
> Only the ubuf case gives up maintenance it was getting: on a 64 MiB
> userptr BO a 4 KiB sync and a full sync both cost 659 us, this arm having
> ignored the range. amdxdna_cbuf_map() fills in only the DMA address and
> length, so drm_clflush_sg() already walks zero pages on carveout memory.
> Userspace maintains these through the mapping it already holds, as XRT's
> buffer::sync() does unless it is told to sync through the driver.
>
> Suggested-by: Lizhi Hou <lizhi.hou(a)amd.com>
> Signed-off-by: Taimuraz Kaitmazov <taimuraz(a)kaitmazov.com>
> Reviewed-by: Lizhi Hou <lizhi.hou(a)amd.com>
> ---
> drivers/accel/amdxdna/amdxdna_gem.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 0e0f844526ca..4be5298d1062 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -1224,6 +1224,9 @@ static int amdxdna_flush_bo(struct amdxdna_gem_obj *abo, u64 offset, u64 size)
> {
> u64 end;
>
> + if (is_import_bo(abo))
> + return -EOPNOTSUPP;
> +
> if (offset >= abo->mem.size)
> return -EINVAL;
>
> @@ -1234,9 +1237,7 @@ static int amdxdna_flush_bo(struct amdxdna_gem_obj *abo, u64 offset, u64 size)
> if (!size)
> return 0;
>
> - if (is_import_bo(abo))
> - drm_clflush_sg(abo->base.sgt);
> - else if (amdxdna_gem_vmap(abo))
> + if (amdxdna_gem_vmap(abo))
> drm_clflush_virt_range(amdxdna_gem_vmap(abo) + offset, size);
> else if (abo->base.pages)
> drm_clflush_pages(abo->base.pages, abo->mem.size >> PAGE_SHIFT);
On Tue, Sep 01, 2026 at 05:08:49PM +0000, David Hu wrote:
> From: David Hu <xuehaohu(a)google.com>
>
> Currently, `fill_sg_entry()` splits the scatterlist using `UINT_MAX`.
> This creates a non-page-aligned DMA length (`0xFFFFFFFF`) for the
> first entry, resulting in non-page-aligned DMA addresses for all
> subsequent entries.
This patch is fine, but pedenatically a scatterlist's entry limit
should be bounded to dma_get_max_seg_size(), though I don't think it
helps this. Operating scatterlists at the size limits has proven
problematic in a number of places already...
> In addition, a non-page-aligned sgl length will trigger an edge case
> in `ib_umem_find_best_pgsz()`. In case of a discontinuity in later
> buffers, we will have a `va` with lowest bit set to 1. That will lead
> to `ib_umem_find_best_pgsz()` always return 0, and break the promise
> to find best page size for the mapping on the NIC side.
That's an IB side bug, the newer logic that joins adjacent SGLs should
have avoided it?
Jason
DMA-buf lets an exporter pin, move or revoke the backing storage under
an importer, and which of the three applies is decided by the optional
callbacks each side implements and by whether dma_buf_pin() succeeds.
Nothing in Documentation/ describes that, and the single reference to
the mechanism still names move_notify(), removed in v7.1.
Thanks
Signed-off-by: Leon Romanovsky <leonro(a)nvidia.com>
---
Leon Romanovsky (2):
PCI/P2PDMA: Update DMABUF lifecycle docs after move_notify() rename
dma-buf: Document how exporters and importers agree on mapping lifetime
Documentation/driver-api/dma-buf.rst | 6 +++
Documentation/driver-api/pci/p2pdma.rst | 6 ++-
drivers/dma-buf/dma-buf.c | 85 ++++++++++++++++++++++++++++++++-
3 files changed, 94 insertions(+), 3 deletions(-)
---
base-commit: 8049741ac93acd3a590dac070e12571fddf0e294
change-id: 20260820-document-dma-buf-3f8b41e32f57
Best regards,
--
Leon Romanovsky <leonro(a)nvidia.com>
On 8/28/26 10:31, SHANMUGAM, SRINIVASAN wrote:
> AMD General
>
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig(a)amd.com>
>> Sent: Friday, August 28, 2026 1:48 PM
>> To: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM(a)amd.com>;
>> Matthew Brost <matthew.brost(a)intel.com>
>> Cc: Deucher, Alexander <Alexander.Deucher(a)amd.com>; Maarten Lankhorst
>> <maarten.lankhorst(a)linux.intel.com>; Maxime Ripard <mripard(a)kernel.org>;
>> Thomas Zimmermann <tzimmermann(a)suse.de>; David Airlie
>> <airlied(a)gmail.com>; Simona Vetter <simona(a)ffwll.ch>; Sumit Semwal
>> <sumit.semwal(a)linaro.org>; Thomas Hellström
>> <thomas.hellstrom(a)linux.intel.com>; dri-devel(a)lists.freedesktop.org; intel-
>> xe(a)lists.freedesktop.org; linux-media(a)vger.kernel.org; linaro-mm-
>> sig(a)lists.linaro.org; linux-kernel(a)vger.kernel.org; amd-gfx(a)lists.freedesktop.org
>> Subject: Re: [PATCH v4 1/2] drm: Add common drm_user_fence helper
>>
>> On 8/28/26 10:06, SHANMUGAM, SRINIVASAN wrote:
>> ...
>>>>> +/**
>>>>> + * struct drm_user_fence - embeddable DRM user fence
>>>>> + *
>>>>> + * Drivers embed this in their own structure and implement
>>>>> + * &drm_user_fence_ops. Call drm_user_fence_init() at creation and
>>>>> + * drm_user_fence_add_callback() to arm on a dma-fence.
>>>>> + * Call drm_user_fence_cancel_sync() before driver teardown.
>>>>> + */
>>>>> +struct drm_user_fence {
>>>>
>>>> Should this common layer be split into two distinct concepts?
>>>>
>>>> - drm_work_fence: 90% of what is here, minus the kthread_use_mm() and
>>>> mm-related code.
>>>> - drm_user_fence: a subclass of drm_work_fence that adds the
>>>> kthread_use_mm() and mm-related code.
>>>>
>>>> I suggest this because I was thinking about it the other day (I
>>>> forget the exact
>>>> context) and reconsidered a pattern where a fence signals and then I
>>>> need a worker because some work must be done outside of IRQ context.
>>>> A user fence is one example, since copy_to_user() can fault, which is
>>>> not allowed in IRQ context. At various times in Xe we've had multiple
>>>> patterns like this, although at the moment user fences are probably
>>>> the only case that requires it. If we looked across DRM as a whole, I suspect
>> we'd find this pattern open-coded in a number of places.
>>>>
>>>> Yes, drm_user_fence would be a very thin layer on top of
>>>> drm_work_fence, but I still see value in the split.
>>>
>>> Hi Matt,
>>>
>>> Thanks for the review and for being supportive of the idea.
>>>
>>> The split into drm_work_fence (general fence-to-workqueue pattern) and
>>> drm_user_fence (subclass adding kthread_use_mm) makes sense. I'll
>>> restructure v5 as follows:
>>>
>>> drm_work_fence: kref, work_struct, dma_fence_cb, stored fence ref,
>>> wq, ops — add_callback, cancel, cancel_sync
>>
>> Yeah, this pattern came up so often that I already considered adding it to the core
>> dma_fence framework.
>
> Hi Christian,
>
> Thanks for the feedback.
>
> On dma_fence_work: would you prefer I place the generic fence-to-work
> helper directly in the core dma_fence framework (drivers/dma-buf/),
> or is starting with drm_work_fence in DRM and promoting it later also
> acceptable?
Maybe ask AI to search for use cases. If you find something outside of drivers/gpu/drm then please place it under drivers/dma-buf.
If you don't find any existing use case drivers/gpu/drm should do as well.
Thanks,
Christian.
>
> I'll add the value comparison logic and will add a clear note that this cannot be
> used to implement dma_fence_ops.
>
> Thanks,
> Srini
On 8/28/26 10:06, SHANMUGAM, SRINIVASAN wrote:
...
>>> +/**
>>> + * struct drm_user_fence - embeddable DRM user fence
>>> + *
>>> + * Drivers embed this in their own structure and implement
>>> + * &drm_user_fence_ops. Call drm_user_fence_init() at creation and
>>> + * drm_user_fence_add_callback() to arm on a dma-fence.
>>> + * Call drm_user_fence_cancel_sync() before driver teardown.
>>> + */
>>> +struct drm_user_fence {
>>
>> Should this common layer be split into two distinct concepts?
>>
>> - drm_work_fence: 90% of what is here, minus the kthread_use_mm() and
>> mm-related code.
>> - drm_user_fence: a subclass of drm_work_fence that adds the
>> kthread_use_mm() and mm-related code.
>>
>> I suggest this because I was thinking about it the other day (I forget the exact
>> context) and reconsidered a pattern where a fence signals and then I need a worker
>> because some work must be done outside of IRQ context. A user fence is one
>> example, since copy_to_user() can fault, which is not allowed in IRQ context. At
>> various times in Xe we've had multiple patterns like this, although at the moment
>> user fences are probably the only case that requires it. If we looked across DRM as
>> a whole, I suspect we'd find this pattern open-coded in a number of places.
>>
>> Yes, drm_user_fence would be a very thin layer on top of drm_work_fence, but I still
>> see value in the split.
>
> Hi Matt,
>
> Thanks for the review and for being supportive of the idea.
>
> The split into drm_work_fence (general fence-to-workqueue pattern) and
> drm_user_fence (subclass adding kthread_use_mm) makes sense. I'll
> restructure v5 as follows:
>
> drm_work_fence: kref, work_struct, dma_fence_cb, stored fence ref,
> wq, ops — add_callback, cancel, cancel_sync
Yeah, this pattern came up so often that I already considered adding it to the core dma_fence framework.
So if you feel really brave make that a dma_fence_work helper. If I'm not completely mistaken AI should be able to find quite a number of use cases for that already.
>
> drm_user_fence: embeds drm_work_fence, adds mm_struct and the
> kthread_use_mm/mmput boilerplate, thin wrappers
>
> XE will continue to use drm_user_fence. For AMDGPU, The long-term
> per-signal filtering approach (reading the fence value via copy_from_user
> before signaling) will use drm_user_fence — further validating both
> layers of the split.
It would be really nice if we could move those compare functionality (>, <, !=, == etc...) XE has for the user value into the drm_user_fence handling as well.
We also need to add a heck of documentation that while this is able to consume dma_fences it *CAN'T* be used to implement dma_fence_ops. I had more than enough headache because of that.
Regards,
Christian.
>
> Regarding the CI failure — the root cause was a missing trailing newline
> at the end of xe_sync_types.h which caused the kunit build to fail with
> "unterminated #ifndef". I've set up kunit locally and confirmed the fix:
>
> Testing complete. Ran 588 tests: passed: 570, skipped: 18
> Elapsed time: 22.916s total, 3.949s configuring, 18.350s building,
> 0.601s running
>
> The 18 skipped tests require Intel hardware — expected. The CI fix will
> be included in v5 along with the drm_work_fence restructuring.
>
> Thanks,
> Srini
>
>>
>> Matt
On 8/13/26 09:46, Taimuraz Kaitmazov wrote:
> amdxdna_gem_obj_vmap() takes whatever dma_buf_vmap() returns and only
> rejects a NULL vaddr. iosys_map is discriminated by is_iomem, so an
> exporter answering with an I/O mapping leaves a void __iomem pointer in
> abo->mem.kva, which amdxdna_cmd_set_error() memsets and memcpys through.
>
> amdxdna_drm_va_tbl takes a dmabuf_fd, so such a BO can be any exporter's
> buffer. amdgpu cannot reach this: its .pin forces GTT for a non peer to
> peer attachment like ours. An exporter on drm_gem_prime_dmabuf_ops has
> no .pin, and drm_gem_ttm_vmap() answers iomem for a VRAM resident
> object, so an NPU paired with nouveau or radeon does.
>
> Refuse the mapping. vmw_gem_vmap() does the same; unlike that one this
> path is reachable from an unprivileged ioctl, so it does not warn.
>
> Signed-off-by: Taimuraz Kaitmazov <taimuraz(a)kaitmazov.com>
> ---
> drivers/accel/amdxdna/amdxdna_gem.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 1f190b319bb..b66ec9e4828 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -683,10 +683,16 @@ static int amdxdna_gem_obj_vmap(struct drm_gem_object *obj, struct iosys_map *ma
>
> dma_resv_assert_held(obj->resv);
>
> - if (is_import_bo(abo))
> + if (is_import_bo(abo)) {
> ret = dma_buf_vmap(abo->dma_buf, map);
> - else
> + /* Callers use mem.kva as an ordinary kernel address. */
> + if (!ret && map->is_iomem) {
> + dma_buf_vunmap(abo->dma_buf, map);
> + return -EOPNOTSUPP;
> + }
Thanks for the fix. The 'is_iomem' check should be moved to
amdxdna_gem_vmap() to cover all the cases.
Lizhi
> + } else {
> ret = drm_gem_shmem_object_vmap(obj, map);
> + }
> if (ret)
> return ret;
> if (!map->vaddr)