[PATCH 05/24] drm/amdkfd: enable pc sampling create

[Felix Kuehling]

On 2023-11-03 09:11, James Zhu wrote:
> From: David Yat Sin <david.yatsin at amd.com>
>
> Enable pc sampling create.
>
> Co-developed-by: James Zhu <James.Zhu at amd.com>
> Signed-off-by: James Zhu <James.Zhu at amd.com>
> Signed-off-by: David Yat Sin <david.yatsin at amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_pc_sampling.c | 54 +++++++++++++++++++-
>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h        | 10 ++++
>   2 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_pc_sampling.c b/drivers/gpu/drm/amd/amdkfd/kfd_pc_sampling.c
> index 49fecbc7013e..f0d910ee730c 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_pc_sampling.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_pc_sampling.c
> @@ -97,7 +97,59 @@ static int kfd_pc_sample_stop(struct kfd_process_device *pdd)
>   static int kfd_pc_sample_create(struct kfd_process_device *pdd,
>   					struct kfd_ioctl_pc_sample_args __user *user_args)
>   {
> -	return -EINVAL;
> +	struct kfd_pc_sample_info *supported_format = NULL;
> +	struct kfd_pc_sample_info user_info;
> +	int ret;
> +	int i;
> +
> +	if (user_args->num_sample_info != 1)
> +		return -EINVAL;
> +
> +	ret = copy_from_user(&user_info, (void __user *) user_args->sample_info_ptr,
> +				sizeof(struct kfd_pc_sample_info));
> +	if (ret) {
> +		pr_debug("Failed to copy PC sampling info from user\n");
> +		return -EFAULT;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(supported_formats); i++) {
> +		if (KFD_GC_VERSION(pdd->dev) == supported_formats[i].ip_version
> +			&& user_info.method == supported_formats[i].sample_info->method
> +			&& user_info.type == supported_formats[i].sample_info->type
> +			&& user_info.value <= supported_formats[i].sample_info->value_max
> +			&& user_info.value >= supported_formats[i].sample_info->value_min) {
> +			supported_format =
> +				(struct kfd_pc_sample_info *)supported_formats[i].sample_info;
> +			break;
> +		}
> +	}
> +
> +	if (!supported_format) {
> +		pr_debug("Sampling format is not supported!");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	mutex_lock(&pdd->dev->pcs_data.mutex);
> +	if (pdd->dev->pcs_data.hosttrap_entry.base.use_count &&
> +		memcmp(&pdd->dev->pcs_data.hosttrap_entry.base.pc_sample_info,
> +				&user_info, sizeof(user_info))) {

I think you can compare structures in C. This would be more readable:

	if (pdd->dev->pcs_data.hosttrap_entry.base.use_count &&
	    pdd->dev->pcs_data.hosttrap_entry.base.pc_sample_info != user_info) {
		...
	}


> +		ret = copy_to_user((void __user *) user_args->sample_info_ptr,
> +			&pdd->dev->pcs_data.hosttrap_entry.base.pc_sample_info,
> +			sizeof(struct kfd_pc_sample_info));
> +		mutex_unlock(&pdd->dev->pcs_data.mutex);
> +		return ret ? ret : -EEXIST;

When copy_to_user fails, it returns the number of bytes not copied. 
That's not a useful return value here. This should be

		return ret ? -EFAULT : -EEXIST;

Also -EBUSY may be more appropriate than -EEXIST.


> +	}
> +
> +	/* TODO: add trace_id return */
> +
> +	if (!pdd->dev->pcs_data.hosttrap_entry.base.use_count)
> +		memcpy(&pdd->dev->pcs_data.hosttrap_entry.base.pc_sample_info,
> +				&user_info, sizeof(user_info));

I think you can assign structures in C. Just do

		pdd->dev->pcs_data.hosttrap_entry.base.pc_sample_info = user_info;

Regards,
   Felix


> +
> +	pdd->dev->pcs_data.hosttrap_entry.base.use_count++;
> +	mutex_unlock(&pdd->dev->pcs_data.mutex);
> +
> +	return 0;
>   }
>   
>   static int kfd_pc_sample_destroy(struct kfd_process_device *pdd, uint32_t trace_id)
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 4a0b66189c67..81c925fb2952 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -256,9 +256,19 @@ struct kfd_vmid_info {
>   
>   struct kfd_dev;
>   
> +struct kfd_dev_pc_sampling_data {
> +	uint32_t use_count;         /* Num of PC sampling sessions */
> +	struct kfd_pc_sample_info pc_sample_info;
> +};
> +
> +struct kfd_dev_pcs_hosttrap {
> +	struct kfd_dev_pc_sampling_data base;
> +};
> +
>   /* Per device PC Sampling data */
>   struct kfd_dev_pc_sampling {
>   	struct mutex mutex;
> +	struct kfd_dev_pcs_hosttrap hosttrap_entry;
>   };
>   
>   struct kfd_node {