Hi,
On 9/3/26 9:10 AM, Manuel Ebner wrote:
> Remove needless '(' and add missing ')'.
>
> Signed-off-by: Manuel Ebner <manuelebnerli(a)mailbox.org>
> ---
> Documentation/gpu/drm-vm-bind-locking.rst | 2 +-
> Documentation/gpu/nova/core/todo.rst | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
This patch probably should be 2 patches since these files have
different maintainers.
> diff --git a/Documentation/gpu/drm-vm-bind-locking.rst b/Documentation/gpu/drm-vm-bind-locking.rst
> index a345aa513..df6cf0132 100644
> --- a/Documentation/gpu/drm-vm-bind-locking.rst
> +++ b/Documentation/gpu/drm-vm-bind-locking.rst
> @@ -170,7 +170,7 @@ submission is therefore preceded with a re-validation section:
> validate_gem_bo(&gpu_vm_bo->gem_bo);
>
> // The following list iteration needs the Gem object's
> - // dma_resv to be held (it protects the gpu_vm_bo's list of
> + // dma_resv to be held. It protects the gpu_vm_bo's list of
> // gpu_vmas, but since local gem objects share the gpu_vm's
> // dma_resv, it is already held at this point.
I think that I must like parenthetical phrases more than you do. ;)
Acked-by: Randy Dunlap <rdunlap(a)infradead.org>
Thanks.
> for_each_gpu_vma_of_gpu_vm_bo(&gpu_vm_bo, &gpu_vma)
> diff --git a/Documentation/gpu/nova/core/todo.rst b/Documentation/gpu/nova/core/todo.rst
> index d5130b2b0..a01c362b1 100644
> --- a/Documentation/gpu/nova/core/todo.rst
> +++ b/Documentation/gpu/nova/core/todo.rst
> @@ -33,7 +33,7 @@ A good example from nova-core would be the ``Chipset`` enum type, which defines
> the value ``AD102``. When probing the GPU the value ``0x192`` can be read from a
> certain register indication the chipset AD102. Hence, the enum value ``AD102``
> should be derived from the number ``0x192``. Currently, nova-core uses a custom
> -implementation (``Chipset::from_u32`` for this.
> +implementation (``Chipset::from_u32``) for this.
>
> Instead, it would be desirable to have something like the ``FromPrimitive``
> trait [1] from the num crate.
--
~Randy
On Wed, Sep 02, 2026 at 07:07:50PM -0400, David Hu wrote:
> On Wed, Sep 2, 2026 at 8:08 AM Jason Gunthorpe <jgg(a)ziepe.ca> wrote:
> >
> > 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?
>
> Hi Jason,
>
> Thank you for the review. I think you are right on both counts.
> Regarding the IB side, the new SGL joining logic in
> `ib_umem_find_best_pgsz()` indeed avoids the issue. `mask |= va` is
> skipped for artifically split, contiguous SGLs. Since Christian has
> already pulled v8 into drm-misc-next, I won't spin a v9 to avoid
> creating unnecessary noise on the list. If you prefer a followup,
> please let me know.
Nope, I'm fine, it just explains why it wasn't seen in other
tested. You were backporting and mix&matched things. It confirms the
upstream kernel was fine from the start.
There are other importers besides RDMA, so I still think that this is
a good change regardless. Having each segment remain page aligned, and
staying away from ULONG_MAX that might trigger overflows is a friendly
and robust thing to do for less sophisticated importers.
Jason
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
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)
