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

[Felix Kuehling]

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....@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.

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