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
