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

[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