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

[James Zhu]

On 2023-11-22 16:51, Felix Kuehling wrote:
>
> 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) {
>         ...
>     }
> [JZ[ Sure
>
>> +        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.
[JZ[ Sure
>
>
>> +    }
>> +
>> +    /* 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;
> [JZ[ Sure
> 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 {