On Mon, 2026-08-31 at 12:36 +0000, SHANMUGAM, SRINIVASAN wrote: > AMD General > > > -----Original Message----- > > From: Thomas Hellström <[email protected]> > > Sent: Monday, August 31, 2026 5:52 PM > > To: SHANMUGAM, SRINIVASAN <[email protected]>; > > Matthew Brost <[email protected]>; Koenig, Christian > > <[email protected]>; Deucher, Alexander > > <[email protected]> > > Cc: [email protected]; Mika Kuoppala > > <[email protected]>; Maarten Lankhorst > > <[email protected]>; > > [email protected]; intel- > > [email protected] > > Subject: Re: [PATCH v5 0/4] drm: Add common > > drm_work_fence/drm_user_fence > > helpers and convert XE > > > > On Mon, 2026-08-31 at 11:13 +0000, SHANMUGAM, SRINIVASAN wrote: > > > AMD General > > > > > > > -----Original Message----- > > > > From: Thomas Hellström <[email protected]> > > > > Sent: Monday, August 31, 2026 3:46 PM > > > > To: SHANMUGAM, SRINIVASAN <[email protected]>; > > Matthew > > > > Brost <[email protected]>; Koenig, Christian > > > > <[email protected]>; Deucher, Alexander > > > > <[email protected]> > > > > Cc: [email protected]; Mika Kuoppala > > > > <[email protected]>; Maarten Lankhorst > > > > <[email protected]>; > > > > [email protected]; intel- > > > > [email protected] > > > > Subject: Re: [PATCH v5 0/4] drm: Add common > > > > drm_work_fence/drm_user_fence helpers and convert XE > > > > > > > > On Mon, 2026-08-31 at 11:11 +0530, Srinivasan Shanmugam wrote: > > > > > When a GPU dma-fence signals, drivers often need to perform > > > > > work > > > > > that cannot run in IRQ context. This pattern is currently > > > > > open-coded in multiple drivers. > > > > > > > > > > This series introduces two layered helpers: > > > > > > > > > > Patch 1 introduces drm_work_fence — a generic embeddable base > > > > > structure that handles the dma-fence-callback-to-workqueue > > > > > pattern. > > > > > Any driver needing deferred fence work can use this directly. > > > > > > > > > > Patch 2 introduces drm_user_fence — a thin layer on top of > > > > > drm_work_fence that adds kthread_use_mm() support for drivers > > > > > that > > > > > need to access userspace memory when a fence signals. > > > > > > > > > > Patch 3 converts XE to use drm_user_fence. XE continues to > > > > > write a > > > > > fence completion value to a userspace VA using the new > > > > > helper. > > > > > > > > > > Patch 4 adds optional per-signal compare functionality to > > > > > drm_user_fence. > > > > > When cmp_addr is set, the worker is called only if the value > > > > > at > > > > > cmp_addr satisfies the configured comparison. This enables > > > > > AMDGPU's EOP eventfd per-signal filtering without open-coding > > > > > the > > > > > read+compare > > > > > pattern. > > > > > > > > > > A follow-on patch (not in this series) will wire AMDGPU's > > > > > render- > > > > > node EOP eventfd signaling path to drm_work_fence. > > > > > > > > > > v5: > > > > > - Split drm_user_fence into drm_work_fence (generic) and > > > > > drm_user_fence > > > > > (MM-borrowing subclass) per Matthew Brost's suggestion. > > > > > - Add per-signal compare functionality > > > > > (drm_user_fence_set_compare()) > > > > > per Christian König's suggestion. > > > > > - Use mmput_async() instead of mmput() to avoid potential > > > > > deadlock in > > > > > MMU notifier release path. (Sashiko review) > > > > > > > > > > Suggested-by: Matthew Brost <[email protected]> > > > > > Suggested-by: Christian König <[email protected]> > > > > > Cc: Mika Kuoppala <[email protected]> > > > > > Cc: Thomas Hellström <[email protected]> > > > > > Cc: Maarten Lankhorst <[email protected]> > > > > > Cc: [email protected] > > > > > Cc: [email protected] > > > > > Cc: [email protected] > > > > > > > > I think the get_user() and put_user() of 64-bit values in drm > > > > (driver > > > > common) code is not safe for typical use-cases on 32-bit > > > > systems. > > > > For xe we > > > > officially don't (yet at least) support 32-bit systems so hence > > > > the > > > > code is a bit sloppy but for drm helpers I'm not sure we can > > > > get > > > > away with this. At least not without some form of warning or > > > > assert. > > > > > > > > I think to make 32-bit systems 64-bit user-fence safe, we would > > > > need > > > > to user > > > > pin_user_pages() combined with cmpxchg64() and a similar > > > > cmpxchg > > > > operation on the user-space side. > > > > > > Hi Thomas, > > > > > > Thanks for the review. > > > > > > For the 32-bit safety concern on get_user() of u64 values — since > > > no > > > current GPU driver supports 32-bit user fences (XE explicitly > > > excludes > > > 32-bit, and AMDGPU targets modern hardware), would adding a > > > BUILD_BUG_ON or IS_ENABLED(CONFIG_64BIT) guard in > > > drm_user_fence_set_compare() be acceptable for now? > > > > > > If a 32-bit driver ever needs this in the future, we can follow > > > up > > > with pin_user_pages() + cmpxchg64() for proper atomic access. > > > > > > Does that approach work for you? > > > > Xe supports building on 32-bit but not running. Can we use a > > drm_WARN_ON_ONCE(!IS_ENABLED(CONFIG_64BIT)) or similar somewhere? > > Perhaps that was your second suggestion? > > Hi Thomas, > > Yes, that matches our suggestion. We will add: > > WARN_ON_ONCE(!IS_ENABLED(CONFIG_64BIT)); > > in drm_user_fence_set_compare(). We cannot use drm_WARN_ON_ONCE() > since drm_user_fence has no struct drm_device * reference. > > Is plain WARN_ON_ONCE acceptable, or should we add a drm_device > pointer to drm_user_fence_set_compare() to use drm_WARN_ON_ONCE()?
For this purpose, IMO WARN_ON_ONCE() is fine. Not sure if drm has a general recommendation to add a device pointer, though. Thanks, Thomas > > Thanks, > Srini
