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

[Christian König]

On 8/28/26 10:31, SHANMUGAM, SRINIVASAN wrote:
> AMD General
> 
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig@xxxxxxx>
>> Sent: Friday, August 28, 2026 1:48 PM
>> To: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@xxxxxxx>;
>> Matthew Brost <matthew.brost@xxxxxxxxx>
>> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Maarten Lankhorst
>> <maarten.lankhorst@xxxxxxxxxxxxxxx>; Maxime Ripard <mripard@xxxxxxxxxx>;
>> Thomas Zimmermann <tzimmermann@xxxxxxx>; David Airlie
>> <airlied@xxxxxxxxx>; Simona Vetter <simona@xxxxxxxx>; Sumit Semwal
>> <sumit.semwal@xxxxxxxxxx>; Thomas Hellström
>> <thomas.hellstrom@xxxxxxxxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx; intel-
>> xe@xxxxxxxxxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx; linaro-mm-
>> sig@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>> 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