Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

29 participants
7092 discussions

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

12 hours, 19 minutes

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

13 hours, 13 minutes

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 >

14 hours, 14 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>

1 day, 2 hours

Re: [PATCH v3 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO

by Lizhi Hou

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)

2 days, 4 hours

Re: [BUG] WARNING: refcount bug in sync_timeline_signal

by Christian König

Hi Jaeyoung, the SW_SYNC component is pretty much deprecated, only kept for user space unit testing and not maintained any more. Background is that even if you fix all those known bugs you can still hard crash the Linux kernel with it. See the big warning in th KConfig and the kernel tainting when that component is enabled as well. So if you have patches which fixes things I'm happy to take them, but nobody will investigate in bug reports on that component any more. Regards, Christian. On 8/24/26 17:56, Jaeyoung Chung wrote: > Hello, > > We found a "WARNING: refcount bug in sync_timeline_signal" on Linux v7.2. > The issue was found by our own race fuzzer. We have not analyzed the root cause, > so we do not have a proposed fix to offer. > > To reproduce the race reliably, we applied the delay patch below to the > kernel and ran the C reproducer as root inside an x86_64 QEMU guest. The > crash log we observed, the delay patch and the reproducer are all included > below. > > The following kernel config options are required to reproduce the issue: > CONFIG_SW_SYNC=y > CONFIG_SYNC_FILE=y > CONFIG_DMA_SHARED_BUFFER=y > CONFIG_DEBUG_FS=y > CONFIG_KASAN=y > > We hope this report is useful. Please let us know if any further > information would help. > > Reported-by: Eulgyu Kim <eulgyukim(a)snu.ac.kr> > Reported-by: Jaeyoung Chung <jjy600901(a)snu.ac.kr> > > Kernel delay patch: > ================================================================== > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c > index 8df20b0218a9..d30ce9d915ba 100644 > --- a/drivers/dma-buf/sw_sync.c > +++ b/drivers/dma-buf/sw_sync.c > @@ -8,6 +8,7 @@ > #include <linux/file.h> > #include <linux/fs.h> > #include <linux/uaccess.h> > +#include <linux/delay.h> > #include <linux/panic.h> > #include <linux/slab.h> > #include <linux/sync_file.h> > @@ -215,6 +216,10 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc) > > spin_lock_irq(&obj->lock); > > + if (strncmp(current->comm, "syzrepro1", 9) == 0) { > + mdelay(50); > + } > + > obj->value += inc; > > list_for_each_entry_safe(pt, next, &obj->pt_list, link) { > ================================================================== > > C reproducer: > ================================================================== > #define _GNU_SOURCE > #include <errno.h> > #include <fcntl.h> > #include <pthread.h> > #include <sched.h> > #include <stdint.h> > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > #include <sys/ioctl.h> > #include <sys/mount.h> > #include <sys/prctl.h> > #include <sys/stat.h> > #include <time.h> > #include <unistd.h> > > #define SYSCHK(x) ({ long __r = (long)(x); if (__r == -1L) { perror(#x); exit(1); } __r; }) > > #define SW_SYNC_IOC_CREATE_FENCE 0xc0285700u > #define SW_SYNC_IOC_INC 0x40045701u > #define SWSYNC "/sys/kernel/debug/sync/sw_sync" > #define BIAS_US 2000 > > struct fence_data { > uint32_t value; > char name[32]; > int32_t fence; > }; > > static volatile int g_round, g_stop, g_tl_fd = -1, g_fence_fd = -1; > static volatile int g_armed, g_done0, g_done1; > > static void spin_us(long us) > { > struct timespec a, b; > > clock_gettime(CLOCK_MONOTONIC, &a); > do { > clock_gettime(CLOCK_MONOTONIC, &b); > __asm__ __volatile__("pause" ::: "memory"); > } while ((b.tv_sec - a.tv_sec) * 1000000L + (b.tv_nsec - a.tv_nsec) / 1000L < us); > } > > static void pin_cpu(int cpu) > { > cpu_set_t set; > > CPU_ZERO(&set); > CPU_SET(cpu, &set); > sched_setaffinity(0, sizeof(set), &set); > } > > static int wait_round(int *last) > { > while (g_round == *last && !g_stop) > sched_yield(); > if (g_stop) > return 0; > *last = g_round; > return 1; > } > > /* T1: advances the timeline, signalling the pt */ > static void *thr_inc(void *a) > { > unsigned int v = 1; > int last = 0; > > (void)a; > prctl(PR_SET_NAME, "syzrepro1", 0, 0, 0); > pin_cpu(0); > while (wait_round(&last)) { > g_armed = 1; > __sync_synchronize(); > ioctl(g_tl_fd, SW_SYNC_IOC_INC, &v); > __sync_synchronize(); > g_done1 = 1; > } > return NULL; > } > > /* T0: closes the fence mid-signal, dropping the pt's last reference */ > static void *thr_close(void *a) > { > int last = 0; > > (void)a; > prctl(PR_SET_NAME, "syzrepro0", 0, 0, 0); > pin_cpu(1); > while (wait_round(&last)) { > while (!g_armed && !g_stop) > __asm__ __volatile__("pause" ::: "memory"); > if (g_stop) > break; > spin_us(BIAS_US); > close(g_fence_fd); > __sync_synchronize(); > g_done0 = 1; > } > return NULL; > } > > static int open_sw_sync(void) > { > int fd = open(SWSYNC, O_RDWR); > > if (fd >= 0 || errno != ENOENT) > return fd; > mkdir("/sys/kernel/debug", 0755); > mount("debugfs", "/sys/kernel/debug", "debugfs", 0, NULL); > return open(SWSYNC, O_RDWR); > } > > int main(void) > { > pthread_t t0, t1; > long i; > > close(SYSCHK(open_sw_sync())); > pthread_create(&t1, NULL, thr_inc, NULL); > pthread_create(&t0, NULL, thr_close, NULL); > > for (i = 0; i < 200 && !g_stop; i++) { > struct fence_data d = { .value = 1, .name = "syzrepro_pt", .fence = -1 }; > int tl = SYSCHK(open_sw_sync()), w; > > SYSCHK(ioctl(tl, SW_SYNC_IOC_CREATE_FENCE, &d)); > g_tl_fd = tl; > g_fence_fd = d.fence; > g_armed = g_done0 = g_done1 = 0; > __sync_synchronize(); > g_round = i + 1; > > for (w = 0; w < 5000 && !(g_done0 && g_done1); w++) { > struct timespec ts = { 0, 1000000 }; > > nanosleep(&ts, NULL); > } > close(tl); > } > > g_stop = 1; > __sync_synchronize(); > g_round++; > pthread_join(t1, NULL); > pthread_join(t0, NULL); > return 0; > } > ================================================================== > > Crash log: > ================================================================== > refcount_t: addition on 0; use-after-free. > WARNING: lib/refcount.c:25 at refcount_warn_saturate+0x76/0xd0 lib/refcount.c:25, CPU#0: syzrepro1/402 > Modules linked in: > CPU: 0 UID: 0 PID: 402 Comm: syzrepro1 Tainted: G L 7.2.0-dirty #2 PREEMPT > Tainted: [L]=SOFTLOCKUP > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014 > RIP: 0010:refcount_warn_saturate+0x76/0xd0 lib/refcount.c:25 > Code: 3d ff e4 bb 04 67 48 0f b9 3a eb 4d 85 db 74 2f 83 fb 01 75 38 48 8d 3d f8 e4 bb 04 67 48 0f b9 3a eb 36 48 8d 3d fa e4 bb 04 <67> 48 0f b9 3a eb 28 48 8d 3d fc e4 bb 04 67 48 0f b9 3a eb 1a 48 > RSP: 0018:ffff88810cd47d68 EFLAGS: 00010046 > RAX: 0000000000000000 RBX: 0000000000000002 RCX: dffffc0000000000 > RDX: 0000000000000001 RSI: 0000000000000004 RDI: ffffffff9be2c840 > RBP: ffff888102a5dc40 R08: ffff888102a5d73b R09: 1ffff1102054bae7 > R10: dffffc0000000000 R11: ffffed102054bae8 R12: dffffc0000000000 > R13: ffff888102a5d740 R14: ffff888102a5d738 R15: ffff888102a5d738 > FS: 00007ae687b8f6c0(0000) GS:ffff88817d75f000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000578dd65f7e58 CR3: 000000010b72c000 CR4: 00000000000006f0 > Call Trace: > <TASK> > __refcount_add include/linux/refcount.h:-1 [inline] > __refcount_inc include/linux/refcount.h:366 [inline] > refcount_inc include/linux/refcount.h:383 [inline] > kref_get include/linux/kref.h:45 [inline] > dma_fence_get include/linux/dma-fence.h:317 [inline] > sync_timeline_signal+0x383/0x550 drivers/dma-buf/sw_sync.c:229 > sw_sync_ioctl_inc drivers/dma-buf/sw_sync.c:410 [inline] > sw_sync_ioctl+0x140/0x980 drivers/dma-buf/sw_sync.c:475 > vfs_ioctl fs/ioctl.c:51 [inline] > __do_sys_ioctl fs/ioctl.c:597 [inline] > __se_sys_ioctl+0xb6/0x100 fs/ioctl.c:583 > do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline] > do_syscall_64+0xf7/0x370 arch/x86/entry/syscall_64.c:94 > entry_SYSCALL_64_after_hwframe+0x76/0x7e > RIP: 0033:0x7ae687c90d6b > Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1c 48 8b 44 24 18 64 48 2b 04 25 28 00 00 > RSP: 002b:00007ae687b8edd0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 > RAX: ffffffffffffffda RBX: 00007ae687b8ee3c RCX: 00007ae687c90d6b > RDX: 00007ae687b8ee38 RSI: 0000000040045701 RDI: 0000000000000003 > RBP: 0000000000000000 R08: 0000000000000000 R09: 00007ffcdb19abc7 > R10: 0000000000000000 R11: 0000000000000246 R12: ffffffffffffff80 > R13: 0000000000000000 R14: 00007ffcdb19aad0 R15: 00007ae68738f000 > </TASK> > ---------------- > Code disassembly (best guess): > 0: 3d ff e4 bb 04 cmp $0x4bbe4ff,%eax > 5: 67 48 0f b9 3a ud1 (%edx),%rdi > a: eb 4d jmp 0x59 > c: 85 db test %ebx,%ebx > e: 74 2f je 0x3f > 10: 83 fb 01 cmp $0x1,%ebx > 13: 75 38 jne 0x4d > 15: 48 8d 3d f8 e4 bb 04 lea 0x4bbe4f8(%rip),%rdi # 0x4bbe514 > 1c: 67 48 0f b9 3a ud1 (%edx),%rdi > 21: eb 36 jmp 0x59 > 23: 48 8d 3d fa e4 bb 04 lea 0x4bbe4fa(%rip),%rdi # 0x4bbe524 > * 2a: 67 48 0f b9 3a ud1 (%edx),%rdi <-- trapping instruction > 2f: eb 28 jmp 0x59 > 31: 48 8d 3d fc e4 bb 04 lea 0x4bbe4fc(%rip),%rdi # 0x4bbe534 > 38: 67 48 0f b9 3a ud1 (%edx),%rdi > 3d: eb 1a jmp 0x59 > 3f: 48 rex.W > ================================================================== > >

2 days, 12 hours

Re: [PATCH v2 09/15] accel/qda: Add DMA-backed GEM objects and memory manager integration

by Christian König

On 8/24/26 17:07, Rob Clark wrote: > On Mon, Aug 24, 2026 at 7:31 AM Christian König > <christian.koenig(a)amd.com> wrote: >> >> On 8/17/26 06:47, Ekansh Gupta wrote: >>> Introduce DMA-coherent buffer management for the QDA driver, wiring >>> together the GEM subsystem, the IOMMU memory manager, and a DMA >>> allocation backend. >>> >>> qda_gem.c / qda_gem.h >>> Implements the GEM object lifecycle for QDA buffers. Each buffer is >>> represented by a qda_gem_obj which embeds a drm_gem_object and >>> carries the kernel virtual address, DMA address, and a pointer to >>> the IOMMU device that performed the allocation. The .free callback >>> delegates to the memory manager, and the .mmap callback uses >>> dma_mmap_coherent() via the DMA backend. >>> >>> qda_memory_dma.c / qda_memory_dma.h >>> DMA coherent allocation backend. qda_dma_alloc() calls >>> dma_alloc_coherent() on the CB device and encodes the stream ID >>> (SID) in the upper 32 bits of the returned DMA address, following >>> the Qualcomm FastRPC convention for IOMMU address space tagging. >>> qda_dma_free() strips the SID prefix before calling >>> dma_free_coherent(). >>> >>> qda_memory_manager.c >>> Adds process-to-device assignment: each DRM file (process) is >>> assigned one IOMMU context bank device for the lifetime of the >>> session. qda_memory_manager_assign_device() first checks whether >>> the process already has a device (reusing it with a refcount >>> increment), then falls back to claiming an unassigned device. >>> qda_memory_manager_alloc() and qda_memory_manager_free() delegate >>> to the DMA backend after resolving the correct CB device for the >>> calling process. >> >> Oh, stuff like that is usually a pretty big NO-GO now. >> >> AMD has made the same mistake with KFD and it resulted in a massive chaos. >> >> The DRM file is the driver context your process uses and even if there are multiple DRM files for the same PID you should absolute *NOT* share anything between them. >> >> The general rule of thumb is to not attach anything to the process using the DRM file descriptor. > > A couple other things to keep in mind: > > 1. drm device fd can be passed across processes > 2. virtgpu/virglrenderer drm native ctx is a single process that opens > the drm device file N times on behalf of N guest userspace processes. > > All that to say, Christian is right here.. drm_file is your context, > pgtables should be attached to the drm_file (potentially via > drm_gpuvm.. which you perhaps want to use). Some additional points you should probably keep in mind: 1. In some configurations the DRM fd is not opened by the application but rather by the display server. So you see for example X/XWayland opening the fd. 2. For gdb plugins you now seriously want to use pidfd_getfd() to grab the DRM fd from the debugged process into your debugger process. Hacks with the PID or TGID are something which needs in deep review for correctness. Regards, Christian. > > BR, > -R > >> Regards, >> Christian. >> >>> >>> qda_drv.c / qda_drv.h >>> qda_file_priv gains an assigned_iommu_dev pointer and a pid field. >>> The .postclose callback decrements the IOMMU device refcount and >>> clears the process assignment when the last reference is dropped. >>> >>> Assisted-by: Claude:claude-sonnet-5 >>> Signed-off-by: Ekansh Gupta <ekansh.gupta(a)oss.qualcomm.com> >>> --- >>> Changes in v2: >>> - Adapt to the dynamically-sized device array introduced in patch 07 >>> (kcalloc'd from DT node count, replaces fixed QDA_IOMMU_DEVICES_MAX) >>> - Protect register/unregister with the process_assignment_lock mutex so >>> the device-assignment and device-registration paths are serialised >>> - No functional changes requested by reviewers on this patch >>> --- >>> drivers/accel/qda/Makefile | 2 + >>> drivers/accel/qda/qda_drv.c | 4 + >>> drivers/accel/qda/qda_drv.h | 4 + >>> drivers/accel/qda/qda_gem.c | 134 ++++++++++++++++++ >>> drivers/accel/qda/qda_gem.h | 52 +++++++ >>> drivers/accel/qda/qda_memory_dma.c | 82 +++++++++++ >>> drivers/accel/qda/qda_memory_dma.h | 17 +++ >>> drivers/accel/qda/qda_memory_manager.c | 239 ++++++++++++++++++++++++++++++++- >>> drivers/accel/qda/qda_memory_manager.h | 30 +++++ >>> 9 files changed, 559 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/accel/qda/Makefile b/drivers/accel/qda/Makefile >>> index b658dad35fee..a46ddceecfc5 100644 >>> --- a/drivers/accel/qda/Makefile >>> +++ b/drivers/accel/qda/Makefile >>> @@ -8,7 +8,9 @@ obj-$(CONFIG_DRM_ACCEL_QDA) := qda.o >>> qda-y := \ >>> qda_cb.o \ >>> qda_drv.o \ >>> + qda_gem.o \ >>> qda_ioctl.o \ >>> + qda_memory_dma.o \ >>> qda_memory_manager.o \ >>> qda_rpmsg.o >>> >>> diff --git a/drivers/accel/qda/qda_drv.c b/drivers/accel/qda/qda_drv.c >>> index e1fd8bfa12d7..2b14dab95507 100644 >>> --- a/drivers/accel/qda/qda_drv.c >>> +++ b/drivers/accel/qda/qda_drv.c >>> @@ -21,6 +21,7 @@ static int qda_open(struct drm_device *dev, struct drm_file *file) >>> if (!qda_file_priv) >>> return -ENOMEM; >>> >>> + qda_file_priv->pid = current->pid; >>> qda_file_priv->qda_dev = qda_dev_from_drm(dev); >>> file->driver_priv = qda_file_priv; >>> >>> @@ -31,6 +32,9 @@ static void qda_postclose(struct drm_device *dev, struct drm_file *file) >>> { >>> struct qda_file_priv *qda_file_priv = file->driver_priv; >>> >>> + if (qda_file_priv->assigned_iommu_dev) >>> + qda_memory_manager_release_device(qda_file_priv->assigned_iommu_dev); >>> + >>> kfree(qda_file_priv); >>> file->driver_priv = NULL; >>> } >>> diff --git a/drivers/accel/qda/qda_drv.h b/drivers/accel/qda/qda_drv.h >>> index 690a833d732b..2fe58f3efec0 100644 >>> --- a/drivers/accel/qda/qda_drv.h >>> +++ b/drivers/accel/qda/qda_drv.h >>> @@ -24,6 +24,10 @@ >>> struct qda_file_priv { >>> /** @qda_dev: Back-pointer to device structure */ >>> struct qda_dev *qda_dev; >>> + /** @assigned_iommu_dev: IOMMU device assigned to this process */ >>> + struct qda_iommu_device *assigned_iommu_dev; >>> + /** @pid: Process ID for tracking */ >>> + pid_t pid; >>> }; >>> >>> /** >>> diff --git a/drivers/accel/qda/qda_gem.c b/drivers/accel/qda/qda_gem.c >>> new file mode 100644 >>> index 000000000000..66e78013a726 >>> --- /dev/null >>> +++ b/drivers/accel/qda/qda_gem.c >>> @@ -0,0 +1,134 @@ >>> +// SPDX-License-Identifier: GPL-2.0-only >>> +// Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. >>> +#include <drm/drm_gem.h> >>> +#include <drm/drm_prime.h> >>> +#include <drm/drm_print.h> >>> +#include <linux/slab.h> >>> +#include <linux/dma-mapping.h> >>> +#include "qda_drv.h" >>> +#include "qda_gem.h" >>> +#include "qda_memory_manager.h" >>> +#include "qda_memory_dma.h" >>> + >>> +/** >>> + * qda_gem_free_object() - Free a GEM object and its associated resources >>> + * @gem_obj: DRM GEM object to free >>> + */ >>> +void qda_gem_free_object(struct drm_gem_object *gem_obj) >>> +{ >>> + struct qda_gem_obj *qda_gem_obj = to_qda_gem_obj(gem_obj); >>> + struct qda_dev *qdev = qda_dev_from_drm(gem_obj->dev); >>> + >>> + if (qda_gem_obj->virt && qdev->iommu_mgr) >>> + qda_memory_manager_free(qdev->iommu_mgr, qda_gem_obj); >>> + >>> + drm_gem_object_release(gem_obj); >>> + kfree(qda_gem_obj); >>> +} >>> + >>> +/** >>> + * qda_gem_mmap_obj() - Map a GEM object into userspace >>> + * @drm_obj: DRM GEM object to map >>> + * @vma: Virtual memory area to map into >>> + * >>> + * Return: 0 on success, negative error code on failure >>> + */ >>> +int qda_gem_mmap_obj(struct drm_gem_object *drm_obj, struct vm_area_struct *vma) >>> +{ >>> + struct qda_gem_obj *qda_gem_obj = to_qda_gem_obj(drm_obj); >>> + int ret; >>> + >>> + /* The fake offset is only used to find the object, not to index it */ >>> + vma->vm_pgoff = 0; >>> + >>> + ret = qda_dma_mmap(qda_gem_obj, vma); >>> + if (ret) >>> + return ret; >>> + >>> + vm_flags_set(vma, VM_DONTEXPAND | VM_DONTDUMP); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct drm_gem_object_funcs qda_gem_object_funcs = { >>> + .free = qda_gem_free_object, >>> + .mmap = qda_gem_mmap_obj, >>> +}; >>> + >>> +/** >>> + * qda_gem_alloc_object() - Allocate a new QDA GEM object >>> + * @drm_dev: DRM device >>> + * @aligned_size: Size of the object in bytes (must be page-aligned) >>> + * >>> + * Return: Pointer to the new GEM object, or ERR_PTR on failure >>> + */ >>> +struct qda_gem_obj *qda_gem_alloc_object(struct drm_device *drm_dev, size_t aligned_size) >>> +{ >>> + struct qda_gem_obj *qda_gem_obj; >>> + int ret; >>> + >>> + qda_gem_obj = kzalloc_obj(*qda_gem_obj); >>> + if (!qda_gem_obj) >>> + return ERR_PTR(-ENOMEM); >>> + >>> + ret = drm_gem_object_init(drm_dev, &qda_gem_obj->base, aligned_size); >>> + if (ret) { >>> + kfree(qda_gem_obj); >>> + return ERR_PTR(ret); >>> + } >>> + >>> + qda_gem_obj->base.funcs = &qda_gem_object_funcs; >>> + qda_gem_obj->size = aligned_size; >>> + >>> + return qda_gem_obj; >>> +} >>> + >>> +void qda_gem_cleanup_object(struct qda_gem_obj *qda_gem_obj) >>> +{ >>> + drm_gem_object_release(&qda_gem_obj->base); >>> + kfree(qda_gem_obj); >>> +} >>> + >>> +int qda_gem_create_handle(struct drm_file *file_priv, struct drm_gem_object *gem_obj, u32 *handle) >>> +{ >>> + int ret; >>> + >>> + ret = drm_gem_handle_create(file_priv, gem_obj, handle); >>> + drm_gem_object_put(gem_obj); >>> + >>> + return ret; >>> +} >>> + >>> +/** >>> + * qda_gem_create_object() - Allocate and initialize a GEM object with DMA backing >>> + * @drm_dev: DRM device >>> + * @iommu_mgr: Memory manager to use for DMA allocation >>> + * @size: Requested size in bytes >>> + * @file_priv: DRM file private data for process association >>> + * >>> + * Return: Pointer to the base DRM GEM object on success, ERR_PTR on failure >>> + */ >>> +struct drm_gem_object *qda_gem_create_object(struct drm_device *drm_dev, >>> + struct qda_memory_manager *iommu_mgr, size_t size, >>> + struct drm_file *file_priv) >>> +{ >>> + struct qda_gem_obj *qda_gem_obj; >>> + size_t aligned_size; >>> + int ret; >>> + >>> + aligned_size = PAGE_ALIGN(size); >>> + if (!aligned_size) >>> + return ERR_PTR(-EINVAL); >>> + >>> + qda_gem_obj = qda_gem_alloc_object(drm_dev, aligned_size); >>> + if (IS_ERR(qda_gem_obj)) >>> + return ERR_CAST(qda_gem_obj); >>> + >>> + ret = qda_memory_manager_alloc(iommu_mgr, qda_gem_obj, file_priv); >>> + if (ret) { >>> + qda_gem_cleanup_object(qda_gem_obj); >>> + return ERR_PTR(ret); >>> + } >>> + >>> + return &qda_gem_obj->base; >>> +} >>> diff --git a/drivers/accel/qda/qda_gem.h b/drivers/accel/qda/qda_gem.h >>> new file mode 100644 >>> index 000000000000..afd7c9b49549 >>> --- /dev/null >>> +++ b/drivers/accel/qda/qda_gem.h >>> @@ -0,0 +1,52 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>> +/* >>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. >>> + */ >>> +#ifndef __QDA_GEM_H__ >>> +#define __QDA_GEM_H__ >>> + >>> +#include <linux/dma-mapping.h> >>> +#include <drm/drm_device.h> >>> +#include <drm/drm_gem.h> >>> +#include "qda_memory_manager.h" >>> + >>> +/** >>> + * struct qda_gem_obj - QDA GEM buffer object >>> + * >>> + * Represents a GEM buffer object that can be allocated by the driver >>> + * or imported from another driver via DMA-BUF. >>> + */ >>> +struct qda_gem_obj { >>> + /** @base: DRM GEM object base — must be first member */ >>> + struct drm_gem_object base; >>> + /** @iommu_dev: IOMMU context bank device that performed the allocation */ >>> + struct qda_iommu_device *iommu_dev; >>> + /** @virt: Kernel virtual address of the allocated DMA memory */ >>> + void *virt; >>> + /** @dma_addr: DMA address (with SID encoded in upper 32 bits) */ >>> + dma_addr_t dma_addr; >>> + /** @size: Size of the buffer in bytes */ >>> + size_t size; >>> +}; >>> + >>> +/** >>> + * to_qda_gem_obj - Cast a drm_gem_object pointer to qda_gem_obj >>> + * @gem_obj: Pointer to the embedded drm_gem_object >>> + */ >>> +#define to_qda_gem_obj(gem_obj) container_of(gem_obj, struct qda_gem_obj, base) >>> + >>> +/* GEM object lifecycle */ >>> +struct drm_gem_object *qda_gem_create_object(struct drm_device *drm_dev, >>> + struct qda_memory_manager *iommu_mgr, >>> + size_t size, struct drm_file *file_priv); >>> +void qda_gem_free_object(struct drm_gem_object *gem_obj); >>> +int qda_gem_mmap_obj(struct drm_gem_object *gem_obj, struct vm_area_struct *vma); >>> + >>> +/* Internal helpers (also used by PRIME import) */ >>> +struct qda_gem_obj *qda_gem_alloc_object(struct drm_device *drm_dev, size_t aligned_size); >>> +void qda_gem_cleanup_object(struct qda_gem_obj *qda_gem_obj); >>> + >>> +/* Utility functions */ >>> +int qda_gem_create_handle(struct drm_file *file_priv, struct drm_gem_object *gem_obj, u32 *handle); >>> + >>> +#endif /* __QDA_GEM_H__ */ >>> diff --git a/drivers/accel/qda/qda_memory_dma.c b/drivers/accel/qda/qda_memory_dma.c >>> new file mode 100644 >>> index 000000000000..7072ed8edf0b >>> --- /dev/null >>> +++ b/drivers/accel/qda/qda_memory_dma.c >>> @@ -0,0 +1,82 @@ >>> +// SPDX-License-Identifier: GPL-2.0-only >>> +// Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. >>> +#include <linux/slab.h> >>> +#include <linux/dma-mapping.h> >>> +#include "qda_drv.h" >>> +#include "qda_gem.h" >>> +#include "qda_memory_dma.h" >>> + >>> +/* >>> + * The DSP identifies the IOMMU context bank from the upper bits of the address, >>> + * so the stream ID is folded into every address handed to it. The DMA API only >>> + * ever sees the plain 32-bit address, which is why the context bank devices are >>> + * created with a 32-bit DMA mask. >>> + */ >>> +static dma_addr_t get_actual_dma_addr(struct qda_gem_obj *gem_obj) >>> +{ >>> + return gem_obj->dma_addr - ((u64)gem_obj->iommu_dev->sid << 32); >>> +} >>> + >>> +static void setup_gem_object(struct qda_gem_obj *gem_obj, void *virt, >>> + dma_addr_t dma_addr, struct qda_iommu_device *iommu_dev) >>> +{ >>> + gem_obj->virt = virt; >>> + gem_obj->dma_addr = dma_addr; >>> + gem_obj->iommu_dev = iommu_dev; >>> +} >>> + >>> +static void cleanup_gem_object_fields(struct qda_gem_obj *gem_obj) >>> +{ >>> + gem_obj->virt = NULL; >>> + gem_obj->dma_addr = 0; >>> + gem_obj->iommu_dev = NULL; >>> +} >>> + >>> +/** >>> + * qda_dma_alloc() - Allocate DMA coherent memory for a GEM object >>> + * @iommu_dev: Pointer to the QDA IOMMU device structure >>> + * @gem_obj: Pointer to GEM object to allocate memory for >>> + * @size: Size of memory to allocate in bytes >>> + * >>> + * Return: 0 on success, negative error code on failure >>> + */ >>> +int qda_dma_alloc(struct qda_iommu_device *iommu_dev, >>> + struct qda_gem_obj *gem_obj, size_t size) >>> +{ >>> + dma_addr_t dma_addr; >>> + void *virt; >>> + >>> + virt = dma_alloc_coherent(iommu_dev->dev, size, &dma_addr, GFP_KERNEL); >>> + if (!virt) >>> + return -ENOMEM; >>> + >>> + dma_addr += ((u64)iommu_dev->sid << 32); >>> + setup_gem_object(gem_obj, virt, dma_addr, iommu_dev); >>> + >>> + return 0; >>> +} >>> + >>> +/** >>> + * qda_dma_free() - Free DMA coherent memory for a GEM object >>> + * @gem_obj: Pointer to GEM object to free memory for >>> + */ >>> +void qda_dma_free(struct qda_gem_obj *gem_obj) >>> +{ >>> + dma_free_coherent(gem_obj->iommu_dev->dev, gem_obj->size, >>> + gem_obj->virt, get_actual_dma_addr(gem_obj)); >>> + >>> + cleanup_gem_object_fields(gem_obj); >>> +} >>> + >>> +/** >>> + * qda_dma_mmap() - Map DMA memory into userspace >>> + * @gem_obj: Pointer to GEM object containing DMA memory >>> + * @vma: Virtual memory area to map into >>> + * >>> + * Return: 0 on success, negative error code on failure >>> + */ >>> +int qda_dma_mmap(struct qda_gem_obj *gem_obj, struct vm_area_struct *vma) >>> +{ >>> + return dma_mmap_coherent(gem_obj->iommu_dev->dev, vma, gem_obj->virt, >>> + get_actual_dma_addr(gem_obj), gem_obj->size); >>> +} >>> diff --git a/drivers/accel/qda/qda_memory_dma.h b/drivers/accel/qda/qda_memory_dma.h >>> new file mode 100644 >>> index 000000000000..99352a99dc33 >>> --- /dev/null >>> +++ b/drivers/accel/qda/qda_memory_dma.h >>> @@ -0,0 +1,17 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>> +/* >>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. >>> + */ >>> + >>> +#ifndef __QDA_MEMORY_DMA_H__ >>> +#define __QDA_MEMORY_DMA_H__ >>> + >>> +#include <linux/dma-mapping.h> >>> +#include "qda_memory_manager.h" >>> + >>> +int qda_dma_alloc(struct qda_iommu_device *iommu_dev, >>> + struct qda_gem_obj *gem_obj, size_t size); >>> +void qda_dma_free(struct qda_gem_obj *gem_obj); >>> +int qda_dma_mmap(struct qda_gem_obj *gem_obj, struct vm_area_struct *vma); >>> + >>> +#endif /* __QDA_MEMORY_DMA_H__ */ >>> diff --git a/drivers/accel/qda/qda_memory_manager.c b/drivers/accel/qda/qda_memory_manager.c >>> index b1a80ee77c35..d47c7419e11d 100644 >>> --- a/drivers/accel/qda/qda_memory_manager.c >>> +++ b/drivers/accel/qda/qda_memory_manager.c >>> @@ -1,14 +1,26 @@ >>> // SPDX-License-Identifier: GPL-2.0-only >>> // Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. >>> >>> +#include <linux/refcount.h> >>> #include <linux/slab.h> >>> +#include <linux/spinlock.h> >>> +#include <drm/drm_file.h> >>> +#include <drm/drm_print.h> >>> #include "qda_drv.h" >>> +#include "qda_gem.h" >>> #include "qda_memory_manager.h" >>> +#include "qda_memory_dma.h" >>> + >>> +static void iommu_device_free(struct kref *ref) >>> +{ >>> + kfree(container_of(ref, struct qda_iommu_device, ref)); >>> +} >>> >>> static void cleanup_all_memory_devices(struct qda_memory_manager *mem_mgr) >>> { >>> int i; >>> >>> + mutex_lock(&mem_mgr->process_assignment_lock); >>> for (i = 0; i < mem_mgr->num_devices; i++) { >>> struct qda_iommu_device *iommu_dev = mem_mgr->devices[i]; >>> >>> @@ -16,9 +28,215 @@ static void cleanup_all_memory_devices(struct qda_memory_manager *mem_mgr) >>> continue; >>> >>> mem_mgr->devices[i] = NULL; >>> - kfree(iommu_dev); >>> + kref_put(&iommu_dev->ref, iommu_device_free); >>> } >>> mem_mgr->num_devices = 0; >>> + mutex_unlock(&mem_mgr->process_assignment_lock); >>> +} >>> + >>> +static void init_iommu_device_fields(struct qda_iommu_device *iommu_dev) >>> +{ >>> + spin_lock_init(&iommu_dev->lock); >>> + refcount_set(&iommu_dev->users, 0); >>> + kref_init(&iommu_dev->ref); >>> + iommu_dev->assigned_pid = 0; >>> + iommu_dev->assigned_file_priv = NULL; >>> +} >>> + >>> +static struct qda_iommu_device *find_device_for_pid(struct qda_memory_manager *mem_mgr, >>> + pid_t pid) >>> +{ >>> + struct qda_iommu_device *found_dev = NULL; >>> + unsigned long flags; >>> + int i; >>> + >>> + for (i = 0; i < mem_mgr->num_devices; i++) { >>> + struct qda_iommu_device *iommu_dev = mem_mgr->devices[i]; >>> + >>> + if (!iommu_dev) >>> + continue; >>> + >>> + spin_lock_irqsave(&iommu_dev->lock, flags); >>> + if (iommu_dev->assigned_pid == pid) { >>> + found_dev = iommu_dev; >>> + refcount_inc(&found_dev->users); >>> + kref_get(&found_dev->ref); >>> + spin_unlock_irqrestore(&iommu_dev->lock, flags); >>> + break; >>> + } >>> + spin_unlock_irqrestore(&iommu_dev->lock, flags); >>> + } >>> + >>> + return found_dev; >>> +} >>> + >>> +static struct qda_iommu_device *assign_available_device_to_pid(struct qda_memory_manager *mem_mgr, >>> + pid_t pid, >>> + struct drm_file *file_priv) >>> +{ >>> + struct qda_iommu_device *selected_dev = NULL; >>> + unsigned long flags; >>> + int i; >>> + >>> + for (i = 0; i < mem_mgr->num_devices; i++) { >>> + struct qda_iommu_device *iommu_dev = mem_mgr->devices[i]; >>> + >>> + if (!iommu_dev) >>> + continue; >>> + >>> + spin_lock_irqsave(&iommu_dev->lock, flags); >>> + if (iommu_dev->assigned_pid == 0) { >>> + iommu_dev->assigned_pid = pid; >>> + iommu_dev->assigned_file_priv = file_priv; >>> + selected_dev = iommu_dev; >>> + refcount_set(&selected_dev->users, 1); >>> + kref_get(&selected_dev->ref); >>> + spin_unlock_irqrestore(&iommu_dev->lock, flags); >>> + break; >>> + } >>> + spin_unlock_irqrestore(&iommu_dev->lock, flags); >>> + } >>> + >>> + return selected_dev; >>> +} >>> + >>> +static struct qda_iommu_device *get_process_iommu_device(struct qda_memory_manager *mem_mgr, >>> + struct drm_file *file_priv) >>> +{ >>> + struct qda_file_priv *qda_priv; >>> + >>> + if (!file_priv || !file_priv->driver_priv) >>> + return NULL; >>> + >>> + qda_priv = (struct qda_file_priv *)file_priv->driver_priv; >>> + return qda_priv->assigned_iommu_dev; >>> +} >>> + >>> +/** >>> + * qda_memory_manager_assign_device() - Assign an IOMMU device to a process >>> + * @mem_mgr: Pointer to memory manager >>> + * @file_priv: DRM file private data for process association >>> + * >>> + * On success the caller owns a reference on the device and must release it >>> + * with qda_memory_manager_release_device(). >>> + * >>> + * Return: 0 on success, negative error code on failure >>> + */ >>> +int qda_memory_manager_assign_device(struct qda_memory_manager *mem_mgr, >>> + struct drm_file *file_priv) >>> +{ >>> + struct qda_file_priv *qda_priv; >>> + struct qda_iommu_device *selected_dev = NULL; >>> + int ret = 0; >>> + pid_t current_pid; >>> + >>> + if (!file_priv || !file_priv->driver_priv) >>> + return -EINVAL; >>> + >>> + qda_priv = (struct qda_file_priv *)file_priv->driver_priv; >>> + current_pid = qda_priv->pid; >>> + >>> + mutex_lock(&mem_mgr->process_assignment_lock); >>> + >>> + if (qda_priv->assigned_iommu_dev) { >>> + ret = 0; >>> + goto unlock_and_return; >>> + } >>> + >>> + selected_dev = find_device_for_pid(mem_mgr, current_pid); >>> + >>> + if (selected_dev) { >>> + qda_priv->assigned_iommu_dev = selected_dev; >>> + goto unlock_and_return; >>> + } >>> + >>> + selected_dev = assign_available_device_to_pid(mem_mgr, current_pid, file_priv); >>> + >>> + if (!selected_dev) { >>> + drm_dbg_driver(file_priv->minor->dev, "No context bank available for PID %d\n", >>> + current_pid); >>> + ret = -EBUSY; >>> + goto unlock_and_return; >>> + } >>> + >>> + qda_priv->assigned_iommu_dev = selected_dev; >>> + >>> +unlock_and_return: >>> + mutex_unlock(&mem_mgr->process_assignment_lock); >>> + >>> + return ret; >>> +} >>> + >>> +/** >>> + * qda_memory_manager_release_device() - Release a process assignment >>> + * @iommu_dev: Device previously obtained from qda_memory_manager_assign_device() >>> + * >>> + * Drops the caller's reference. Once the last process using the device is >>> + * gone the assignment is cleared so the device can serve another process. >>> + */ >>> +void qda_memory_manager_release_device(struct qda_iommu_device *iommu_dev) >>> +{ >>> + unsigned long flags; >>> + >>> + if (refcount_dec_and_test(&iommu_dev->users)) { >>> + spin_lock_irqsave(&iommu_dev->lock, flags); >>> + iommu_dev->assigned_pid = 0; >>> + iommu_dev->assigned_file_priv = NULL; >>> + spin_unlock_irqrestore(&iommu_dev->lock, flags); >>> + } >>> + >>> + kref_put(&iommu_dev->ref, iommu_device_free); >>> +} >>> + >>> +static struct qda_iommu_device *get_or_assign_iommu_device(struct qda_memory_manager *mem_mgr, >>> + struct drm_file *file_priv) >>> +{ >>> + struct qda_iommu_device *iommu_dev; >>> + int ret; >>> + >>> + iommu_dev = get_process_iommu_device(mem_mgr, file_priv); >>> + if (iommu_dev) >>> + return iommu_dev; >>> + >>> + ret = qda_memory_manager_assign_device(mem_mgr, file_priv); >>> + if (ret) >>> + return NULL; >>> + >>> + iommu_dev = get_process_iommu_device(mem_mgr, file_priv); >>> + if (iommu_dev) >>> + return iommu_dev; >>> + >>> + return NULL; >>> +} >>> + >>> +/** >>> + * qda_memory_manager_alloc() - Allocate memory for a GEM object >>> + * @mem_mgr: Pointer to memory manager >>> + * @gem_obj: Pointer to GEM object to allocate memory for >>> + * @file_priv: DRM file private data for process association >>> + * >>> + * Return: 0 on success, negative error code on failure >>> + */ >>> +int qda_memory_manager_alloc(struct qda_memory_manager *mem_mgr, struct qda_gem_obj *gem_obj, >>> + struct drm_file *file_priv) >>> +{ >>> + struct qda_iommu_device *selected_dev; >>> + >>> + selected_dev = get_or_assign_iommu_device(mem_mgr, file_priv); >>> + if (!selected_dev) >>> + return -EBUSY; >>> + >>> + return qda_dma_alloc(selected_dev, gem_obj, gem_obj->size); >>> +} >>> + >>> +/** >>> + * qda_memory_manager_free() - Free memory for a GEM object >>> + * @mem_mgr: Pointer to memory manager >>> + * @gem_obj: Pointer to GEM object to free memory for >>> + */ >>> +void qda_memory_manager_free(struct qda_memory_manager *mem_mgr, struct qda_gem_obj *gem_obj) >>> +{ >>> + qda_dma_free(gem_obj); >>> } >>> >>> /** >>> @@ -31,13 +249,21 @@ static void cleanup_all_memory_devices(struct qda_memory_manager *mem_mgr) >>> int qda_memory_manager_register_device(struct qda_memory_manager *mem_mgr, >>> struct qda_iommu_device *iommu_dev) >>> { >>> - if (mem_mgr->num_devices >= mem_mgr->max_devices) >>> - return -ENOSPC; >>> + int ret = 0; >>> >>> + mutex_lock(&mem_mgr->process_assignment_lock); >>> + if (mem_mgr->num_devices >= mem_mgr->max_devices) { >>> + ret = -ENOSPC; >>> + goto out; >>> + } >>> + >>> + init_iommu_device_fields(iommu_dev); >>> iommu_dev->id = mem_mgr->num_devices; >>> mem_mgr->devices[mem_mgr->num_devices++] = iommu_dev; >>> >>> - return 0; >>> +out: >>> + mutex_unlock(&mem_mgr->process_assignment_lock); >>> + return ret; >>> } >>> >>> /** >>> @@ -52,14 +278,16 @@ void qda_memory_manager_unregister_device(struct qda_memory_manager *mem_mgr, >>> { >>> int i; >>> >>> + mutex_lock(&mem_mgr->process_assignment_lock); >>> for (i = 0; i < mem_mgr->num_devices; i++) { >>> if (mem_mgr->devices[i] == iommu_dev) { >>> mem_mgr->devices[i] = NULL; >>> break; >>> } >>> } >>> + mutex_unlock(&mem_mgr->process_assignment_lock); >>> >>> - kfree(iommu_dev); >>> + kref_put(&iommu_dev->ref, iommu_device_free); >>> } >>> >>> /** >>> @@ -80,6 +308,7 @@ int qda_memory_manager_init(struct qda_memory_manager *mem_mgr, int max_devices) >>> >>> mem_mgr->num_devices = 0; >>> mem_mgr->max_devices = max_devices; >>> + mutex_init(&mem_mgr->process_assignment_lock); >>> >>> return 0; >>> } >>> diff --git a/drivers/accel/qda/qda_memory_manager.h b/drivers/accel/qda/qda_memory_manager.h >>> index 7e38c8a18284..0949c0c213f6 100644 >>> --- a/drivers/accel/qda/qda_memory_manager.h >>> +++ b/drivers/accel/qda/qda_memory_manager.h >>> @@ -7,25 +7,44 @@ >>> #define __QDA_MEMORY_MANAGER_H__ >>> >>> #include <linux/device.h> >>> +#include <linux/kref.h> >>> +#include <linux/mutex.h> >>> +#include <linux/refcount.h> >>> +#include <linux/spinlock.h> >>> +#include <drm/drm_file.h> >>> >>> /* Forward declarations */ >>> struct qda_dev; >>> +struct qda_gem_obj; >>> >>> /** >>> * struct qda_iommu_device - IOMMU device instance for memory management >>> * >>> * Represents a single IOMMU-enabled device managed by the memory manager. >>> * Each device can be assigned to a specific process session. >>> + * >>> + * The object outlives its registration: open files keep a reference through >>> + * @ref, so a device can be unregistered while a process still holds it. >>> */ >>> struct qda_iommu_device { >>> /** @dev: Pointer to the underlying device */ >>> struct device *dev; >>> /** @qdev: Back-pointer to the parent QDA device */ >>> struct qda_dev *qdev; >>> + /** @assigned_file_priv: DRM file private data for the assigned process */ >>> + struct drm_file *assigned_file_priv; >>> /** @id: Unique identifier assigned by the memory manager */ >>> u32 id; >>> /** @sid: Stream ID for IOMMU transactions */ >>> u32 sid; >>> + /** @assigned_pid: Process ID of the process assigned to this device */ >>> + pid_t assigned_pid; >>> + /** @users: Number of open files sharing this device */ >>> + refcount_t users; >>> + /** @ref: Reference count controlling when the object is freed */ >>> + struct kref ref; >>> + /** @lock: Spinlock protecting concurrent access to device */ >>> + spinlock_t lock; >>> }; >>> >>> /** >>> @@ -42,6 +61,8 @@ struct qda_memory_manager { >>> int num_devices; >>> /** @max_devices: Capacity of the @devices array */ >>> int max_devices; >>> + /** @process_assignment_lock: Mutex protecting process-to-device assignments */ >>> + struct mutex process_assignment_lock; >>> }; >>> >>> int qda_memory_manager_init(struct qda_memory_manager *mem_mgr, int max_devices); >>> @@ -51,5 +72,14 @@ int qda_memory_manager_register_device(struct qda_memory_manager *mem_mgr, >>> struct qda_iommu_device *iommu_dev); >>> void qda_memory_manager_unregister_device(struct qda_memory_manager *mem_mgr, >>> struct qda_iommu_device *iommu_dev); >>> +int qda_memory_manager_assign_device(struct qda_memory_manager *mem_mgr, >>> + struct drm_file *file_priv); >>> +void qda_memory_manager_release_device(struct qda_iommu_device *iommu_dev); >>> + >>> +int qda_memory_manager_alloc(struct qda_memory_manager *mem_mgr, >>> + struct qda_gem_obj *gem_obj, >>> + struct drm_file *file_priv); >>> +void qda_memory_manager_free(struct qda_memory_manager *mem_mgr, >>> + struct qda_gem_obj *gem_obj); >>> >>> #endif /* __QDA_MEMORY_MANAGER_H__ */ >>> >>

3 days, 7 hours

Re: [PATCH v19 00/40] DEPT(DEPendency Tracker)

by Byungchul Park

On Tue, Aug 25, 2026 at 03:10:28PM +0200, David Hildenbrand (Arm) wrote: > >> As the saying goes, it's hard to teach old dogs new tricks, but in the end > >> taking care of two dogs is likely harder than only a single dog? :) > > > > Or it's better to train a unmatured hunting dog than to teach a pet dog, > > how to hunt, for better hunting, even though the pet dog might > > eventually become able to. > > I'm not sure whether "pet dog" is the right analogy to lockdep in that scenario, > though ;) Ah. Okay. I will be careful in using terms. I wanted to tell DEPT has its strength, lockdep has its own, and they are different. Byungchul > > -- > Cheers, > > David

3 days, 8 hours

Re: [PATCH v19 00/40] DEPT(DEPendency Tracker)

by Byungchul Park

On Fri, Aug 21, 2026 at 07:48:40PM +1000, NeilBrown wrote: > On Fri, 21 Aug 2026, David Hildenbrand (Arm) wrote: > > [...] > > > > > Exactly. That's why we use classification e.g. lock class - DEPT also > > > makes use of the concept. > > > > > > DEPT doesn't use a full map in each page but uses a minimum space for a > > > timestamp in each to track when each starts to wait so as to use the > > > recorded timestamp when the event occurs e.g. folio_unlock(). > > > > Thanks for that information! > > > > > > > >> Given that lockdep is a debug feature, and we will at some point allocate struct > > >> folio separately, I assume we could just squeeze a "struct lockdep_map" in there > > >> in such debug configs and the world would not collapse. > > > > > > That's a good news for lockdep. (And even for DEPT :) > > > > He :) Where do you currently store the additional per-page information? > > lockdep doesn't need to store per-page information. Possibly DEPT > doesn't either. Class can be stored in global map, but DEPT needs to keep a timestamp in each page to track when a potential-wait e.g. folio_lock() has been started and to refer to the information on its event e.g. folio_unlock(). > lockdep needs one lockdep_map for each lock class. It would make sense > for all folio locks to use the same global lockdep_map. Exactly. Only considering classes, you are right. > Each specific lock is known to lockdep as a task which holds the lock, a > lockdep_map which represents the class of locks, and subclass number > which allows a given task to hold multiple locks of the same class > providing it declare (e.g. with spin_lock_nested() etc). Right, lockdep works that way. > > >> Doing that today (one "struct lockdep_map" in each "struct page") wouldn't work > > >> as mm_zero_struct_page() would not expect such large "struct page". But > > >> conceptually, for a debug kernel with a special CONFIG_LOCKDEP_PAGE_LOCK, maybe > > >> that would already be ok and we could just do that (and optimize it as we > > >> allocate folios separately). > > > > > > Sounds great. > > > > > >> Not that it's ideal, but for a debug feature to at least check PG_lock, probably > > >> an easier way to achieve it than some completely new infrastructure. > > > > > > I understand what you are going to tell. > > > > > > However, it's worth noting that lockdep tracks dependencies basically > > > based on **lock acqusition orders** in the system. To make it track > > > even rwlock and general synchronization mechanism as well, lockdep has > > > no choice but to get more complicated. > > > > Well, yes, sure :) > > > > > > > > Focusing on only the dependency checking, the most parts of lockdep are > > > for the tricky things, so the reusable parts are not that big. > > > > > >> Now, Willy said "locking rules don't really apply to individual folios", I > > >> wonder if that could just help to also let lockdep check PG_lock with less > > >> metadata? (didn't fully wrap my head around the implications) > > > > > > That's what DEPT did and what brought external wgen introduced in DEPT. > > > I was considering the exactly same thing :) > > > > > > Again, lockdep that tracks lock acquisition orders can't do that. > > > > > >> [1] > > >> https://lore.kernel.org/all/aR3WHf9QZ_dizNun@casper.infradead.org/?utm_sour… > > >> > > >> > > >> It's your guiding example, that's why I mention it. You do mention other wait > > >> cases here, I don't know anything about them, but for folios it's really just > > >> "we used a single bit so far" AFAIKs. > > > > > > It doesn't matter whether it's implemented using bit or not. folio lock > > > is quite special since it's allowed to be released other than the > > > acquisition context that makes lockdep impossible to track them. > > > > Does that really make lockdep *impossible* to track them? IOW, there is no way > > to extend lockdep to support lock release in different context? > > Yes and no.... > > lockdep has no knowledge of control flows moving across threads in the > way that I assume DEPT does. But it should be possible to tell it. > > If you have some code that takes a lock and then hands it off to > another thread, at the hand-off point you call > lock_map_release(&the_lock_map) > > This says "no task owns this lock any more". > > maybe you put the folio which is locked on a queue or an lru or > whatever. > > There is no way to say "that queue owns this lock". Maybe that could > usefully be added - assuming coherent semantics can be designed. It's certainly useful to track who owns the lock, but the essence, when it comes to deadlock detection, lies elsewhere. DEPT is based on the essence, that is, a deadlock comes from waits that are never awakened. > Somewhere else some other task takes responsibility for that folio and > the lock. maybe it dequeues a page, or maybe an lru callback gives the > locked page to some code. > That code then calls > lock_map_acquire_try(&the_lock_map) > > This says "this task is now holding this lock" (or more accurately "now > holding a lock of this class"). > Note the "_try" - that says that the task didn't have to wait for the > lock, it just got it for free, which in fact it did. > > Now if that task takes some other lock, lockdep will see a dependency > between the page lock and the new lock, and will accept or reject it as > you would expect. It's an interesting approach if your goal is to track the owenership, but if the goal is for tracking dependencies.. well.. I'm not sure. Byungchul > So you definitely *can* send lock dependency information between tasks > with lockdep. I have only tried it in extremely simple cases where a > single object is being locked by one task and unlocked by another - no > queues or lists. > There may be - and probably are - more complexities involved with > locking folios and passing them around. Maybe it is so complex that you > need all the support that DEPT provides. But I'd like to see a coherent > explanation of how the functionality offered by DEPT is clearly better. > > NeilBrown > > > > > I guess there is a way, but the question is at which price (I seriously have no > > idea, maybe this was already discussed and people have a pointer for me). > > > > > > > >> [...] > > >> > > >>> > > >>> Q. Why not build DEPT into lockdep? > > >>> > > >>> A. Lockdep is stable, battle-tested code. I chose separation because > > >>> while DEPT borrows BFS and hashing ideas, the wait/event model > > >>> requires rebuilding from scratch. Lockdep was designed for lock > > >>> acquisition order — retrofitting it would risk its stability. > > >> > > >> Why can't this just be some configurable extension to lockdep > > >> (CONFIG_LOCKDEP_XYZ) until the feature is stable and can unconditionally be > > >> enabled along with it? > > > > > > Answered? > > > > Not quite. I don't understand why this must be a completely separate machinery, > > even if, conceptually, it would do more than traditional lockdep. > > > > Is it either DEPT or LOCKDEP in current configurations? Can both run at the same > > time? > > > > > > > >> I don't quite buy the "would risk its stability" argument. A lot of stuff we do > > >> "risks stability", every day :) > > > > > > That's awsome anyway :) > > > > > >> Is there another good reason (incompatible with X, dangerous with Y, cinfusing > > >> Z) why this really must be a separate thing? > > > > > > Roughly: > > > > > > 1. Similar or less effort is needed for the new one - retrofitting > > > lockdep is not easy and big changes are required since the > > > reusable parts are not that big. > > > > > > 2. Even though you didn't agree, retrofitting it would risk its > > > stability. > > > > Well, I don't buy the stability argument, really :) > > > > Retrofitting effort for lockdep is an interesting point, though. Lockdep > > maintainers would have to make the call here regarding direction and feasibility. > > > > [...] > > > > >> But I am not a locking maintainer. I think there was plenty of discussion in the > > >> past, so I might just be raising points that were already discussed in the past, > > >> but I really just read some random pieces of earlier discussions. (ideally > > >> previous discussions would be summarized here) > > >> > > >> Long story short: we are now in v19 and I think there was pushback in the past. > > >> Did the opinion of locking maintainers change, or is there a way forward to > > >> integrate this in a way that would make locking maintainers accept this? > > > > > > One of locking maintainers who I met in an LPC told me that he agrees > > > with the direction of DEPT and supports DEPT, not officially tho. > > > > Hm. > > > > > > > > What he and other people are concerning w.r.t DEPT the most is, false > > > positives, which is the most important issue for now. > > > > Thanks for highlighting that. What's the main reason for false positives? Is it > > something conceptual that is mostly impossible to solve, or rather just > > implementation work to cover all edge cases? > > > > > > > > At the same time, I think the most important thing is to make DEPT > > > useful in practice especially with folio locks involved. Actually, I'm > > > planning to share DEPT's true reports periodically to LKML and work with > > > people who believe DEPT can make things better. > > > > > > Any advices will be welcome. Thanks for your opinions. > > > > I think we must come to some conclusion on how to proceed with DEPT. I see the > > following options: > > > > (1) Don't merge it and carry it OOT. Shame if it delivers real value. > > > > (2) Merge it (after proper review and acks from relevant maintainers ;) ), > > keeping it entirely separate from lockdep. > > > > (3) Integrate it with lockdep on a high level, giving us a single locking > > dependency checker, but mostly letting dept have a separate implementation. > > Look into possible merging afterwards. > > > > (4) Retrofit and extend lockdep to really have one mechanism. > > > > As the saying goes, it's hard to teach old dogs new tricks, but in the end > > taking care of two dogs is likely harder than only a single dog? :) > > > > I tend to favor (4) (or 3 with possible future work to achieve 4), but I am not > > a locking maintainer, so really they have to voice what to do. > > > > I do see value in DEPT (even if the folio lock might be handled differently). > > > > -- > > Cheers, > > > > David > > >

3 days, 8 hours

Re: [PATCH v5 05/10] mm/cma: Introduce cma_alloc_at() API

by Marek Szyprowski

On 20.08.2026 20:04, David Hildenbrand (Arm) wrote: >> /* This part must be outside protection */> diff --git a/mm/cma.c b/mm/cma.c >> index a10ea37a261d..1e1ebae79090 100644 >> --- a/mm/cma.c >> +++ b/mm/cma.c >> @@ -936,6 +936,141 @@ struct page *cma_alloc_frozen_compound(struct cma *cma, unsigned int order) >> return __cma_alloc_frozen(cma, 1 << order, order, gfp); >> } >> +static int cma_range_alloc_at(struct cma *cma, struct cma_memrange *cmr, >> + unsigned long offset, unsigned long count, >> + struct page **pagep, gfp_t gfp) >> +{ >> + struct page *page = NULL; >> + unsigned long pfn; >> + int ret = -EBUSY; >> + >> + spin_lock_irq(&cma->lock); >> + >> + /* >> + * If the request is larger than the available number of pages, stop >> + * right away. >> + */ >> + if (count > cma->available_count) >> + goto unlock; >> + >> + ret = bitmap_allocate(cmr->bitmap, offset, count); >> + if (ret < 0) >> + goto unlock; >> + >> + pfn = cmr->base_pfn + offset; >> + page = pfn_to_page(pfn); >> + >> + /* >> + * Do not hand out page ranges that are not contiguous, so >> + * callers can just iterate the pages without having to worry >> + * about these corner cases. >> + */ >> + if (!page_range_contiguous(page, count)) { >> + pr_warn_ratelimited("%s: %s: skipping non-contiguous area [0x%lx-0x%lx]", >> + __func__, cma->name, pfn, pfn + count - 1); >> + ret = -EBUSY; >> + goto clear; >> + } >> + >> + cma->available_count -= count; >> + >> + /* >> + * It's safe to drop the lock here. We've marked this region for >> + * our exclusive use. If the migration fails we will take the >> + * lock again and unmark it. >> + */ >> + spin_unlock_irq(&cma->lock); >> + >> + mutex_lock(&cma->alloc_mutex); >> + ret = alloc_contig_frozen_range(pfn, pfn + count, ACR_FLAGS_CMA, gfp); >> + mutex_unlock(&cma->alloc_mutex); >> + > There is quite some code duplication with cma_range_alloc(). Please try harder > to factor common code out and reuse it. > > > > ... > Also here, way too much code duplication with __cma_alloc_frozen(). > > There must be a better way :) Maybe the existing 'alloc' functions could call 'alloc_at' variant internally (with locks adjusted and moved out). > (I really prefer this direction of the patch set) Definitely, I also like this approach much more than previous attempt with 'dynamically' instantiated cma regions. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland

3 days, 9 hours

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig