Re: [PATCH v4 3/5] drm/amdkfd: set activated flag true when event age unmatchs

[James Zhu <jamesz@amd.com>]

On 2023-06-09 16:22, Felix Kuehling wrote:
>
> On 2023-06-09 16:13, James Zhu wrote:
>> Set waiter's activated flag true when event age unmatchs with 
>> last_event_age.
>>
>> -v4: add event type check
>>
>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdkfd/kfd_events.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> index c7689181cc22..2cc1a7e976f4 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> @@ -41,6 +41,7 @@ struct kfd_event_waiter {
>>       wait_queue_entry_t wait;
>>       struct kfd_event *event; /* Event to wait for */
>>       bool activated;         /* Becomes true when event is signaled */
>> +    bool event_age_enabled;  /* set to true when last_event_age is 
>> non-zero */
>>   };
>>     /*
>> @@ -797,9 +798,9 @@ static struct kfd_event_waiter 
>> *alloc_event_waiters(uint32_t num_events)
>>     static int init_event_waiter(struct kfd_process *p,
>>           struct kfd_event_waiter *waiter,
>> -        uint32_t event_id)
>> +        struct kfd_event_data *event_data)
>>   {
>> -    struct kfd_event *ev = lookup_event_by_id(p, event_id);
>> +    struct kfd_event *ev = lookup_event_by_id(p, event_data->event_id);
>>         if (!ev)
>>           return -EINVAL;
>> @@ -808,6 +809,13 @@ static int init_event_waiter(struct kfd_process *p,
>>       waiter->event = ev;
>>       waiter->activated = ev->signaled;
>>       ev->signaled = ev->signaled && !ev->auto_reset;
>> +
>> +    /* last_event_age = 0 reserved for backward compatible */
>> +    waiter->event_age_enabled = 
>> !!event_data->signal_event_data.last_event_age;
>
> This should also be inside the "if (waiter->event->type == 
> KFD_EVENT_TYPE_SIGNAL)". I'd do something like this:
>
>     if (waiter->event->type == KFD_EVENT_TYPE_SIGNAL &&
>         event_data->signal_event_data.last_event_age) {
>         waiter->event_age_enabled = true;
>         if (ev->event_age != 
> event_data->signal_event_data.last_event_age)
>             waiter->activated = true;
>     }
>
> You don't need WRITE_ONCE here because there can be no concurrent 
> access before you add the waiter to the wait queue.
[JZ] Yes, this looks better.
>
> Regards,
>   Felix
>
>
>> +    if (waiter->event->type == KFD_EVENT_TYPE_SIGNAL && 
>> waiter->event_age_enabled &&
>> +        ev->event_age != event_data->signal_event_data.last_event_age)
>> +        WRITE_ONCE(waiter->activated, true);
>> +
>>       if (!waiter->activated)
>>           add_wait_queue(&ev->wq, &waiter->wait);
>>       spin_unlock(&ev->lock);
>> @@ -948,8 +956,7 @@ int kfd_wait_on_events(struct kfd_process *p,
>>               goto out_unlock;
>>           }
>>   -        ret = init_event_waiter(p, &event_waiters[i],
>> -                    event_data.event_id);
>> +        ret = init_event_waiter(p, &event_waiters[i], &event_data);
>>           if (ret)
>>               goto out_unlock;
>>       }