Re: [PATCH v4 3/9] drm/amdkfd: Add SMI add event helper

[Felix Kuehling <felix.kuehling@xxxxxxx>]

Am 2022-02-25 um 13:06 schrieb Philip Yang:
> To remove duplicate code, unify event message format and simplify new
> event add in the following patches.
>
> Signed-off-by: Philip Yang <Philip.Yang@xxxxxxx>

This patch is

Reviewed-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>

I think you can submit patch 1 and patch 3 independently of the rest of 
the series, since they are just a fix and a cleanup of the existing code 
and do not change the API.

Regards,
  Felix


> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c | 66 +++++++--------------
>   1 file changed, 21 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
> index 6b743068057d..2e0a26159662 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
> @@ -189,22 +189,28 @@ static void add_event_to_kfifo(pid_t pid, struct kfd_dev *dev,
>   	rcu_read_unlock();
>   }
>   
> -void kfd_smi_event_update_gpu_reset(struct kfd_dev *dev, bool post_reset)
> +static void kfd_smi_event_add(pid_t pid, struct kfd_dev *dev,
> +			      unsigned int event, char *fmt, ...)
>   {
> -	/*
> -	 * GpuReset msg = Reset seq number (incremented for
> -	 * every reset message sent before GPU reset).
> -	 * 1 byte event + 1 byte space + 8 bytes seq num +
> -	 * 1 byte \n + 1 byte \0 = 12
> -	 */
>   	char fifo_in[KFD_SMI_EVENT_MSG_SIZE];
>   	int len;
> -	unsigned int event;
> +	va_list args;
>   
>   	if (list_empty(&dev->smi_clients))
>   		return;
>   
> -	memset(fifo_in, 0x0, sizeof(fifo_in));
> +	len = snprintf(fifo_in, sizeof(fifo_in), "%x ", event);
> +
> +	va_start(args, fmt);
> +	len += vsnprintf(fifo_in + len, sizeof(fifo_in) - len, fmt, args);
> +	va_end(args);
> +
> +	add_event_to_kfifo(pid, dev, event, fifo_in, len);
> +}
> +
> +void kfd_smi_event_update_gpu_reset(struct kfd_dev *dev, bool post_reset)
> +{
> +	unsigned int event;
>   
>   	if (post_reset) {
>   		event = KFD_SMI_EVENT_GPU_POST_RESET;
> @@ -212,48 +218,20 @@ void kfd_smi_event_update_gpu_reset(struct kfd_dev *dev, bool post_reset)
>   		event = KFD_SMI_EVENT_GPU_PRE_RESET;
>   		++(dev->reset_seq_num);
>   	}
> -
> -	len = snprintf(fifo_in, sizeof(fifo_in), "%x %x\n", event,
> -						dev->reset_seq_num);
> -
> -	add_event_to_kfifo(0, dev, event, fifo_in, len);
> +	kfd_smi_event_add(0, dev, event, "%x\n", dev->reset_seq_num);
>   }
>   
>   void kfd_smi_event_update_thermal_throttling(struct kfd_dev *dev,
>   					     uint64_t throttle_bitmask)
>   {
> -	/*
> -	 * ThermalThrottle msg = throttle_bitmask(8):
> -	 * 			 thermal_interrupt_count(16):
> -	 * 1 byte event + 1 byte space + 16 byte throttle_bitmask +
> -	 * 1 byte : + 16 byte thermal_interupt_counter + 1 byte \n +
> -	 * 1 byte \0 = 37
> -	 */
> -	char fifo_in[KFD_SMI_EVENT_MSG_SIZE];
> -	int len;
> -
> -	if (list_empty(&dev->smi_clients))
> -		return;
> -
> -	len = snprintf(fifo_in, sizeof(fifo_in), "%x %llx:%llx\n",
> -		       KFD_SMI_EVENT_THERMAL_THROTTLE, throttle_bitmask,
> -		       amdgpu_dpm_get_thermal_throttling_counter(dev->adev));
> -
> -	add_event_to_kfifo(0, dev, KFD_SMI_EVENT_THERMAL_THROTTLE, fifo_in, len);
> +	kfd_smi_event_add(0, dev, KFD_SMI_EVENT_THERMAL_THROTTLE, "%llx:%llx\n",
> +			  throttle_bitmask,
> +			  amdgpu_dpm_get_thermal_throttling_counter(dev->adev));
>   }
>   
>   void kfd_smi_event_update_vmfault(struct kfd_dev *dev, uint16_t pasid)
>   {
>   	struct amdgpu_task_info task_info;
> -	/* VmFault msg = (hex)uint32_pid(8) + :(1) + task name(16) = 25 */
> -	/* 1 byte event + 1 byte space + 25 bytes msg + 1 byte \n +
> -	 * 1 byte \0 = 29
> -	 */
> -	char fifo_in[KFD_SMI_EVENT_MSG_SIZE];
> -	int len;
> -
> -	if (list_empty(&dev->smi_clients))
> -		return;
>   
>   	memset(&task_info, 0, sizeof(struct amdgpu_task_info));
>   	amdgpu_vm_get_task_info(dev->adev, pasid, &task_info);
> @@ -261,10 +239,8 @@ void kfd_smi_event_update_vmfault(struct kfd_dev *dev, uint16_t pasid)
>   	if (!task_info.pid)
>   		return;
>   
> -	len = snprintf(fifo_in, sizeof(fifo_in), "%x %x:%s\n", KFD_SMI_EVENT_VMFAULT,
> -		task_info.pid, task_info.task_name);
> -
> -	add_event_to_kfifo(0, dev, KFD_SMI_EVENT_VMFAULT, fifo_in, len);
> +	kfd_smi_event_add(0, dev, KFD_SMI_EVENT_VMFAULT, "%x:%s\n",
> +			  task_info.pid, task_info.task_name);
>   }
>   
>   int kfd_smi_event_open(struct kfd_dev *dev, uint32_t *fd)