Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

7 participants
7098 discussions

[PATCH v2] dma-buf: Document how exporters and importers agree on mapping lifetime

by Leon Romanovsky

From: Leon Romanovsky <leonro(a)nvidia.com> Nothing in DMA-buf declares whether a mapping may be moved or taken away. Nearly every callback on both sides is optional, so the answer follows from which ones are implemented and from whether dma_buf_pin() succeeds. Documentation/ says none of this, and the rules sit in the kdoc of dma_buf_ops.pin, dma_buf_attach_ops.invalidate_mappings and dma_buf_invalidate_mappings(), which a driver author has to know by name before finding them. Describe the three flows an importer has to handle, the callbacks each one asks of both sides, and dma_buf_pin() as the runtime negotiation. Signed-off-by: Leon Romanovsky <leonro(a)nvidia.com> --- Let's try this variant from importer POV. Thanks Changes in v2: - Split pci p2p documentation patch to separate series - Improved dma-buf documentation patch. - Link to v1: https://patch.msgid.link/20260825-document-dma-buf-v1-0-5ecfb3e1371c@nvidia… --- Documentation/driver-api/dma-buf.rst | 6 ++++ drivers/dma-buf/dma-buf.c | 69 +++++++++++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst index 2f36c21d9948..39c201f38aa6 100644 --- a/Documentation/driver-api/dma-buf.rst +++ b/Documentation/driver-api/dma-buf.rst @@ -113,6 +113,12 @@ Basic Operation and Device DMA Access .. kernel-doc:: drivers/dma-buf/dma-buf.c :doc: dma buf device access +Mapping Lifetime Negotiation +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +.. kernel-doc:: drivers/dma-buf/dma-buf.c + :doc: mapping lifetime negotiation + CPU Access to DMA Buffer Objects ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index d504c636dc29..aa47142cd9ed 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -684,7 +684,74 @@ static struct file *dma_buf_getfile(size_t size, int flags) * reference acquired with dma_buf_get() by calling dma_buf_put(). * * For the detailed semantics exporters are expected to implement see - * &dma_buf_ops. + * &dma_buf_ops. Whether the exporter may still move or take away the backing + * storage after step 3 depends on what both sides implement. See the mapping + * lifetime negotiation section below. + */ + +/** + * DOC: mapping lifetime negotiation + * + * Nearly everything in DMA-buf is optional. No flag or enum says whether the + * exporter may move or take away the backing storage while an importer holds + * a mapping. Each side simply implements the callbacks it can offer, and + * dma_buf_pin() settles the result at runtime. + * + * Out of that optionality the importer sees three main flows. They are named + * from the importer's point of view, because each one demands a different + * capability of its hardware: + * + * - Pinned: the memory is never taken away. The importer offers no way + * to stop DMA. + * - Revoked: the storage never moves, but the exporter may take it away. The + * importer must be able to stop DMA, and may hit user visible errors while + * doing so. + * - Movable: the exporter may relocate the storage at any time. The importer + * must be able to pause DMA, and must raise no error while the storage is + * moving. + * + * An importer reaches its flow like this: + * + * 1. Attach with dma_buf_dynamic_attach(). Leaving out the optional + * &dma_buf_attach_ops.invalidate_mappings callback pins the buffer for as + * long as the attachment exists. + * 2. Call dma_buf_pin() under the reservation lock. + * 3. On failure, run the movable flow or give up. + * 4. On success, the flow is the revoked one if the optional + * &dma_buf_attach_ops.invalidate_mappings is implemented, and the pinned + * one if it is not. + * + * Pinned flow: + * + * - Exporter: implement &dma_buf_ops.pin and &dma_buf_ops.unpin to hold the + * storage still on request. An exporter whose storage never moves implements + * neither, and dma_buf_pin() then succeeds on its own. An exporter which + * refuses to be pinned implements &dma_buf_ops.pin and fails it. + * - Importer: nothing more. The mapping stays valid until the importer unmaps. + * + * Revoked flow: + * + * - Exporter: answer dma_buf_pin() as above. Call + * dma_buf_invalidate_mappings() when the storage goes away and fail + * &dma_buf_ops.map_dma_buf while it is gone. The two waits which complete a + * revocation are described in dma_buf_invalidate_mappings(). + * - Importer: &dma_buf_attach_ops.invalidate_mappings has to unmap within + * bounded time and drop the pin. + * + * Movable flow: + * + * - Exporter: call dma_buf_invalidate_mappings() before each move, then wait + * for the &dma_buf.resv fences. &dma_buf_ops.pin and &dma_buf_ops.unpin play + * no part here. + * - Importer: hold no pin. &dma_buf_attach_ops.invalidate_mappings drops the + * cached mapping and has to lead to dma_buf_unmap_attachment() within + * bounded time. It need not stop the hardware, because access runs until the + * importer's &dma_buf.resv fences retire. The importer maps again before the + * next DMA. + * + * &dma_buf_ops.attach is the best place for an exporter to turn an importer + * away, because the importer can still fall back to another flow and attach + * again. */ /** --- base-commit: 63367df6e7255067ad6a83abe0d2799dfa491876 change-id: 20260820-document-dma-buf-3f8b41e32f57 Best regards, -- Leon Romanovsky <leonro(a)nvidia.com>

6 hours, 7 minutes

[PATCH 0/2] Document the DMA-buf mapping lifetime negotiation

by Leon Romanovsky

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>

2 days, 12 hours

Re: [PATCH] Documentation/gpu: Fix brackets

by Randy Dunlap

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

3 days, 2 hours

Re: [PATCH v8 2/2] dma-buf: Split sgl by largest page-aligned chunk

by Jason Gunthorpe

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

3 days, 2 hours

Re: [PATCH v8 0/2] dma-buf: Fix silent overflow and alignment

by Christian König

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 >

4 days, 1 hour

Re: [PATCH v5 4/5] accel/amdxdna: refuse to flush an imported BO

by Lizhi Hou

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

4 days, 3 hours

Re: [PATCH v8 2/2] dma-buf: Split sgl by largest page-aligned chunk

by Jason Gunthorpe

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

4 days, 6 hours

Re: [PATCH v4 1/2] drm: Add common drm_user_fence helper

by Christian König

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

1 week, 2 days

Re: [PATCH v4 1/2] drm: Add common drm_user_fence helper

by Christian König

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

1 week, 2 days

Re: [PATCH v4 1/2] drm: Add common drm_user_fence helper

by Matthew Brost

On Fri, Aug 28, 2026 at 12:01:02PM +0530, Srinivasan Shanmugam wrote: > Introduce a common DRM user fence helper providing the kref-managed, > MM-borrowing dma-fence-callback-to-workqueue pattern used by drivers > that must access userspace memory from a kthread context when a GPU > fence signals. > > XE uses this pattern (xe_sync.c) to write a fence completion value > to a userspace VA. AMDGPU will use the same pattern to signal a > per-queue eventfd from a user-queue EOP fence callback. > > The helper provides: > - struct drm_user_fence: embeddable base structure > - struct drm_user_fence_ops: worker/destroy callbacks > - drm_user_fence_init(): initialize and grab the process MM > - drm_user_fence_get/put(): reference counting > - drm_user_fence_add_callback(): attach to a dma-fence > > The worker callback receives a bool indicating whether the process > MM was successfully obtained, allowing drivers to handle the > unavailable-MM case (log, skip the userspace write, etc.) without > duplicating the mmget/kthread_use_mm/mmput boilerplate. > > Suggested-by: Christian König <christian.koenig(a)amd.com> > Cc: Alex Deucher <alexander.deucher(a)amd.com> > Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> > Cc: Maxime Ripard <mripard(a)kernel.org> > Cc: Thomas Zimmermann <tzimmermann(a)suse.de> > Cc: David Airlie <airlied(a)gmail.com> > Cc: Simona Vetter <simona(a)ffwll.ch> > Cc: Sumit Semwal <sumit.semwal(a)linaro.org> > Cc: Matthew Brost <matthew.brost(a)intel.com> First off, I'm supportive of the idea of a common DRM layer for user fences and updating Xe accordingly. This isn't a complete review, but here's a quick initial suggestion. Also, by the way, you're still fighting our CI [1]. Feel free to keep hammering on it, as that's what it's there for. iirc if kunit fails as in this case, nothing else will run. Ask AI and should be able to get instructions on how to build our kunit + run it (it doesn't require Intel hardware in a lot of cases). [1] https://patchwork.freedesktop.org/series/172930/ > Cc: Thomas Hellström <thomas.hellstrom(a)linux.intel.com> > Cc: dri-devel(a)lists.freedesktop.org > Cc: intel-xe(a)lists.freedesktop.org > Cc: linux-media(a)vger.kernel.org > Cc: linaro-mm-sig(a)lists.linaro.org > Cc: linux-kernel(a)vger.kernel.org > Cc: amd-gfx(a)lists.freedesktop.org > Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam(a)amd.com> > --- > v4: > - Check cancel_work_sync() return value in drm_user_fence_cancel_sync() > and call drm_user_fence_put() if work was dequeued, fixing a memory > leak of the drm_user_fence, mm_struct and stored dma_fence when a > pending work item is cancelled. (Sashiko review) > > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/drm_user_fence.c | 223 +++++++++++++++++++++++++++++++ > include/drm/drm_user_fence.h | 76 +++++++++++ > 3 files changed, 300 insertions(+) > create mode 100644 drivers/gpu/drm/drm_user_fence.c > create mode 100644 include/drm/drm_user_fence.h > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index e97faabcd783..52de1f474535 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -69,6 +69,7 @@ drm-y := \ > drm_syncobj.o \ > drm_sysfs.o \ > drm_trace_points.o \ > + drm_user_fence.o \ > drm_vblank.o \ > drm_vblank_work.o \ > drm_vma_manager.o \ > diff --git a/drivers/gpu/drm/drm_user_fence.c b/drivers/gpu/drm/drm_user_fence.c > new file mode 100644 > index 000000000000..cdc47d092cbb > --- /dev/null > +++ b/drivers/gpu/drm/drm_user_fence.c > @@ -0,0 +1,223 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright © 2024 The Linux Foundation > + * > + * Common DRM user fence helper. > + * > + * When a GPU dma-fence signals, drivers often need to write a value to a > + * userspace VA or notify userspace via an eventfd. Both operations require > + * a valid process MM, which is not available in IRQ context. > + * > + * This helper queues a work item on fence signal. The work item borrows the > + * process MM via kthread_use_mm() and calls ops->worker(), which the driver > + * implements to perform the actual userspace access. > + */ > + > +#include <linux/kthread.h> > +#include <linux/sched/mm.h> > +#include <linux/workqueue.h> > + > +#include <drm/drm_user_fence.h> > + > +static void drm_user_fence_destroy(struct kref *kref) > +{ > + struct drm_user_fence *ufence = > + container_of(kref, struct drm_user_fence, refcount); > + > + /* Release the extra reference stored for cancel() */ > + if (ufence->fence) > + dma_fence_put(ufence->fence); > + > + mmdrop(ufence->mm); > + ufence->ops->destroy(ufence); > +} > + > +/** > + * drm_user_fence_get - Acquire a reference to a user fence > + * @ufence: user fence > + */ > +void drm_user_fence_get(struct drm_user_fence *ufence) > +{ > + kref_get(&ufence->refcount); > +} > +EXPORT_SYMBOL_GPL(drm_user_fence_get); > + > +/** > + * drm_user_fence_put - Release a reference to a user fence > + * @ufence: user fence > + */ > +void drm_user_fence_put(struct drm_user_fence *ufence) > +{ > + kref_put(&ufence->refcount, drm_user_fence_destroy); > +} > +EXPORT_SYMBOL_GPL(drm_user_fence_put); > + > +static void drm_user_fence_work(struct work_struct *w) > +{ > + struct drm_user_fence *ufence = > + container_of(w, struct drm_user_fence, work); > + bool mm_ok = false; > + > + if (mmget_not_zero(ufence->mm)) { > + kthread_use_mm(ufence->mm); > + mm_ok = true; > + } > + > + ufence->ops->worker(ufence, mm_ok); > + > + if (mm_ok) { > + kthread_unuse_mm(ufence->mm); > + mmput(ufence->mm); > + } > + > + drm_user_fence_put(ufence); > +} > + > +static void drm_user_fence_cb(struct dma_fence *fence, struct dma_fence_cb *cb) > +{ > + struct drm_user_fence *ufence = > + container_of(cb, struct drm_user_fence, cb); > + > + queue_work(ufence->wq, &ufence->work); > + /* > + * Put the transferred reference from add_callback. The stored > + * reference in ufence->fence is released in drm_user_fence_destroy(). > + */ > + dma_fence_put(fence); > +} > + > +/** > + * drm_user_fence_init - Initialize a user fence > + * @ufence: user fence to initialize > + * @wq: workqueue to run the worker on (must be ordered if sequencing matters) > + * @ops: driver operations > + * > + * Must be called from process context with a valid current->mm. > + * Grabs a reference to current->mm via mmgrab(). > + */ > +void drm_user_fence_init(struct drm_user_fence *ufence, > + struct workqueue_struct *wq, > + const struct drm_user_fence_ops *ops) > +{ > + kref_init(&ufence->refcount); > + ufence->mm = current->mm; > + mmgrab(ufence->mm); > + ufence->wq = wq; > + ufence->ops = ops; > + ufence->fence = NULL; > + INIT_WORK(&ufence->work, drm_user_fence_work); > +} > +EXPORT_SYMBOL_GPL(drm_user_fence_init); > + > +/** > + * drm_user_fence_add_callback - Attach a user fence to a dma-fence > + * @ufence: user fence > + * @fence: dma-fence to watch; ownership of this reference is transferred > + * to the callback — caller must NOT put it afterward. > + * > + * When @fence signals, a work item is queued that calls ops->worker() with > + * the process MM active. If @fence has already signaled the work item is > + * queued immediately. > + * > + * An additional reference to @fence is stored internally in @ufence to > + * allow drm_user_fence_cancel() to be called safely without the caller > + * needing to hold a separate fence reference. > + * > + * On any return value the caller's fence reference is consumed. > + * > + * Return: 0 on success, negative errno on error. > + */ > +int drm_user_fence_add_callback(struct drm_user_fence *ufence, > + struct dma_fence *fence) > +{ > + int err; > + > + drm_user_fence_get(ufence); > + > + /* Extra ref stored for cancel() — lives until drm_user_fence_destroy() */ > + ufence->fence = dma_fence_get(fence); > + > + err = dma_fence_add_callback(fence, &ufence->cb, drm_user_fence_cb); > + if (err == -ENOENT) { > + /* fence already signaled — queue work and release transferred ref */ > + queue_work(ufence->wq, &ufence->work); > + dma_fence_put(fence); > + err = 0; > + } else if (err) { > + dma_fence_put(ufence->fence); > + ufence->fence = NULL; > + drm_user_fence_put(ufence); > + dma_fence_put(fence); > + } > + /* on success: transferred ref goes to drm_user_fence_cb */ > + > + return err; > +} > +EXPORT_SYMBOL_GPL(drm_user_fence_add_callback); > + > +/** > + * drm_user_fence_cancel - Cancel a pending user fence callback > + * @ufence: user fence > + * > + * Attempts to remove the pending callback before driver context teardown. > + * Must be called before the driver tears down its workqueue or ops. > + * The caller must hold a reference to @ufence across this call. > + * > + * If the callback has already fired this returns false and no additional > + * action is needed — the callback handles its own reference. > + * > + * If removal succeeds the callback reference is released internally. > + * The caller must still release its own separate reference via > + * drm_user_fence_put() when done with the object. > + * > + * This function is safe to call from atomic context as it only acquires > + * the dma-fence spinlock internally. If the caller also needs to wait > + * for the worker to finish, use drm_user_fence_cancel_sync() instead, > + * which may sleep. > + * > + * Return: true if callback was removed, false if it had already fired. > + */ > +bool drm_user_fence_cancel(struct drm_user_fence *ufence) > +{ > + struct dma_fence *fence = ufence->fence; > + > + if (!fence) > + return false; > + > + if (dma_fence_remove_callback(fence, &ufence->cb)) { > + /* > + * Callback will not fire — release the transferred reference > + * that would have been put by drm_user_fence_cb(). The stored > + * reference in ufence->fence is released in destroy(). > + */ > + dma_fence_put(fence); > + drm_user_fence_put(ufence); > + return true; > + } > + > + /* Callback already fired — it handled its own cleanup */ > + return false; > +} > +EXPORT_SYMBOL_GPL(drm_user_fence_cancel); > + > +/** > + * drm_user_fence_cancel_sync - Cancel callback and wait for worker to finish > + * @ufence: user fence > + * > + * Calls drm_user_fence_cancel() then cancel_work_sync() to guarantee > + * the worker has fully completed before returning. > + * > + * This function may sleep. Must not be called from atomic or interrupt > + * context. Use drm_user_fence_cancel() instead when sleeping is not > + * allowed. > + * > + * Drivers must call this during teardown before freeing any resources > + * accessed by ops->worker(). > + */ > +void drm_user_fence_cancel_sync(struct drm_user_fence *ufence) > +{ > + drm_user_fence_cancel(ufence); > + if (cancel_work_sync(&ufence->work)) > + drm_user_fence_put(ufence); > +} > +EXPORT_SYMBOL_GPL(drm_user_fence_cancel_sync); > diff --git a/include/drm/drm_user_fence.h b/include/drm/drm_user_fence.h > new file mode 100644 > index 000000000000..02a02266ab93 > --- /dev/null > +++ b/include/drm/drm_user_fence.h > @@ -0,0 +1,75 @@ > +/* SPDX-License-Identifier: MIT */ > +/* > + * Copyright © 2024 The Linux Foundation > + */ > + > +#ifndef __DRM_USER_FENCE_H__ > +#define __DRM_USER_FENCE_H__ > + > +#include <linux/dma-fence.h> > +#include <linux/kref.h> > +#include <linux/workqueue.h> > + > +struct drm_user_fence; > + > +/** > + * struct drm_user_fence_ops - driver callbacks for a DRM user fence > + */ > +struct drm_user_fence_ops { > + /** > + * @worker: Called from workqueue context. > + * > + * If @mm_ok is true, kthread_use_mm() is active and userspace memory > + * (copy_to_user, eventfd_signal, etc.) may be accessed safely. > + * If @mm_ok is false, the process MM was already gone; the driver > + * should log a warning and skip the userspace write. > + * > + * wake_up() or other post-signal housekeeping should also happen here. > + */ > + void (*worker)(struct drm_user_fence *ufence, bool mm_ok); > + > + /** > + * @destroy: Called when the last reference is dropped. > + * Free the containing structure here. > + */ > + void (*destroy)(struct drm_user_fence *ufence); > +}; > + > +/** > + * 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. Matt > + /** @refcount: Reference count. */ > + struct kref refcount; > + /** @mm: Process MM grabbed at init time. */ > + struct mm_struct *mm; > + /** @work: Work item queued when the dma-fence signals. */ > + struct work_struct work; > + /** @cb: dma-fence callback. */ > + struct dma_fence_cb cb; > + /** > + * @fence: Extra reference held for safe cancel(). Set during > + * add_callback, released in destroy(). > + */ > + struct dma_fence *fence; > + /** @wq: Workqueue to run @work on. */ > + struct workqueue_struct *wq; > + /** @ops: Driver operations. */ > + const struct drm_user_fence_ops *ops; > +}; > + > +void drm_user_fence_init(struct drm_user_fence *ufence, > + struct workqueue_struct *wq, > + const struct drm_user_fence_ops *ops); > +void drm_user_fence_get(struct drm_user_fence *ufence); > +void drm_user_fence_put(struct drm_user_fence *ufence); > +int drm_user_fence_add_callback(struct drm_user_fence *ufence, > + struct dma_fence *fence); > +bool drm_user_fence_cancel(struct drm_user_fence *ufence); > +void drm_user_fence_cancel_sync(struct drm_user_fence *ufence); > + > +#endif /* __DRM_USER_FENCE_H__ */ > -- > 2.34.1 >

1 week, 2 days

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig