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

[Felix Kuehling <felix.kuehling@xxxxxxx>]

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.

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