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

[James Zhu <jamesz@xxxxxxx>]

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@xxxxxxx>
> > ---
> >   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;
> >       }