Re: [PATCH v3] drm/xe/ufence: Signal ufence immediately when possible

[Nirmoy Das <nirmoy.das@intel.com>]

On 10/18/2024 4:23 PM, Matthew Auld wrote:
> On 18/10/2024 13:47, Nirmoy Das wrote:
>> If the backing fence is signaled then signal ufence immediately.
>> This should reduce load from the xe ordered_wq and also won't block
>> signaling a ufence which doesn't require any serialization.
>>
>> v2: fix system_wq typo
>> v3: signal immediately instead of queuing in system_wq (Matt B)
>>
>> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1630
>> Cc: Matthew Auld <matthew.auld@intel.com>
>> gc: Matthew Brost <matthew.brost@intel.com>
>
> s/gc/Cc
>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_sync.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_sync.c b/drivers/gpu/drm/xe/xe_sync.c
>> index c6cf227ead40..069c1e4ebea5 100644
>> --- a/drivers/gpu/drm/xe/xe_sync.c
>> +++ b/drivers/gpu/drm/xe/xe_sync.c
>> @@ -72,10 +72,8 @@ static struct xe_user_fence *user_fence_create(struct xe_device *xe, u64 addr,
>>       return ufence;
>>   }
>>   -static void user_fence_worker(struct work_struct *w)
>> +static void signal_user_fence(struct xe_user_fence *ufence)
>>   {
>> -    struct xe_user_fence *ufence = container_of(w, struct xe_user_fence, worker);
>> -
>>       if (mmget_not_zero(ufence->mm)) {
>>           kthread_use_mm(ufence->mm);
>>           if (copy_to_user(ufence->addr, &ufence->value, sizeof(ufence->value)))
>
> This can end up in a CPU fault handler? There might be some locking issues if caller is say holding dma-resv. For example the caller in xe_exec which is holding dma-resv. If it can indeed hit this path, then we might get some splats/deadlocks, I think.


What is the connection between writting into ufence addr  and dma-resv  ? Trying to understand this locking problem.


it looks like I have to use a worker anyway to do kthread_use_mm(), https://intel-gfx-ci.01.org/tree/intel-xe/xe-pw-140169v1/bat-atsm-2/igt@xe_exec_balancer@no-exec-cm-virtual-basic.html


Regards,

Nirmoy

>
>> @@ -89,6 +87,14 @@ static void user_fence_worker(struct work_struct *w)
>>       user_fence_put(ufence);
>>   }
>>   +static void user_fence_worker(struct work_struct *w)
>> +{
>> +    struct xe_user_fence *ufence = container_of(w, struct xe_user_fence,
>> +                            worker);
>> +
>> +    signal_user_fence(ufence);
>> +}
>> +
>>   static void kick_ufence(struct xe_user_fence *ufence, struct dma_fence *fence)
>>   {
>>       INIT_WORK(&ufence->worker, user_fence_worker);
>> @@ -236,7 +242,8 @@ void xe_sync_entry_signal(struct xe_sync_entry *sync, struct dma_fence *fence)
>>           err = dma_fence_add_callback(fence, &sync->ufence->cb,
>>                            user_fence_cb);
>>           if (err == -ENOENT) {
>> -            kick_ufence(sync->ufence, fence);
>> +            /* signal the ufence immediately if fence is already signalled */
>> +            signal_user_fence(sync->ufence);
>>           } else if (err) {
>>               XE_WARN_ON("failed to add user fence");
>>               user_fence_put(sync->ufence);