[PATCH 06/24] drm/amdkfd: add trace_id return

[James Zhu]

On 2023-11-22 16:56, Felix Kuehling wrote:
>
> On 2023-11-03 09:11, James Zhu wrote:
>> Add trace_id return for new pc sampling creation per device,
>> Use IDR to quickly locate pc_sampling_entry for reference.
>>
>> Signed-off-by: James Zhu <James.Zhu at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdkfd/kfd_device.c      |  2 ++
>>   drivers/gpu/drm/amd/amdkfd/kfd_pc_sampling.c | 20 +++++++++++++++++++-
>>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h        |  6 ++++++
>>   3 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>> index 0e24e011f66b..bcaeedac8fe0 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>> @@ -536,10 +536,12 @@ static void kfd_smi_init(struct kfd_node *dev)
>>   static void kfd_pc_sampling_init(struct kfd_node *dev)
>>   {
>>       mutex_init(&dev->pcs_data.mutex);
>> + idr_init_base(&dev->pcs_data.hosttrap_entry.base.pc_sampling_idr, 1);
>>   }
>>     static void kfd_pc_sampling_exit(struct kfd_node *dev)
>>   {
>> + idr_destroy(&dev->pcs_data.hosttrap_entry.base.pc_sampling_idr);
>>       mutex_destroy(&dev->pcs_data.mutex);
>>   }
>>   diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_pc_sampling.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_pc_sampling.c
>> index f0d910ee730c..4c9fc48e1a6a 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_pc_sampling.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_pc_sampling.c
>> @@ -99,6 +99,7 @@ static int kfd_pc_sample_create(struct 
>> kfd_process_device *pdd,
>>   {
>>       struct kfd_pc_sample_info *supported_format = NULL;
>>       struct kfd_pc_sample_info user_info;
>> +    struct pc_sampling_entry *pcs_entry;
>>       int ret;
>>       int i;
>>   @@ -140,7 +141,19 @@ static int kfd_pc_sample_create(struct 
>> kfd_process_device *pdd,
>>           return ret ? ret : -EEXIST;
>>       }
>>   -    /* TODO: add trace_id return */
>> +    pcs_entry = kvzalloc(sizeof(*pcs_entry), GFP_KERNEL);
>
> I don't see a reason to use kvzalloc here. You know the size of the 
> structure, so kzalloc should be perfectly fine.
[JZ] Sure, will change to kzalloc
>
>
>> +    if (!pcs_entry) {
>> +        mutex_unlock(&pdd->dev->pcs_data.mutex);
>> +        return -ENOMEM;
>> +    }
>> +
>> +    i = 
>> idr_alloc_cyclic(&pdd->dev->pcs_data.hosttrap_entry.base.pc_sampling_idr,
>> +                pcs_entry, 1, 0, GFP_KERNEL);
>> +    if (i < 0) {
>> +        mutex_unlock(&pdd->dev->pcs_data.mutex);
>> +        kvfree(pcs_entry);
>
> kfree
>
>
>> +        return i;
>> +    }
>>         if (!pdd->dev->pcs_data.hosttrap_entry.base.use_count)
>> memcpy(&pdd->dev->pcs_data.hosttrap_entry.base.pc_sample_info,
>> @@ -149,6 +162,11 @@ static int kfd_pc_sample_create(struct 
>> kfd_process_device *pdd,
>>       pdd->dev->pcs_data.hosttrap_entry.base.use_count++;
>>       mutex_unlock(&pdd->dev->pcs_data.mutex);
>>   +    pcs_entry->pdd = pdd;
>> +    user_args->trace_id = (uint32_t)i;
>
> I suspect this should be done inside the lock. You don't want someone 
> looking up the pcs_entry before it has been initialized.
[JZ]pcs_entry is for this pc sampling process, and it has 
kfd_process->mutex protected,
>
> Regards,
>   Felix
>
>
>> +
>> +    pr_debug("alloc pcs_entry = %p, trace_id = 0x%x on gpu 0x%x", 
>> pcs_entry, i, pdd->dev->id);
>> +
>>       return 0;
>>   }
>>   diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> index 81c925fb2952..642558026d16 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> @@ -258,6 +258,7 @@ struct kfd_dev;
>>     struct kfd_dev_pc_sampling_data {
>>       uint32_t use_count;         /* Num of PC sampling sessions */
>> +    struct idr pc_sampling_idr;
>>       struct kfd_pc_sample_info pc_sample_info;
>>   };
>>   @@ -743,6 +744,11 @@ enum kfd_pdd_bound {
>>    */
>>   #define SDMA_ACTIVITY_DIVISOR  100
>>   +struct pc_sampling_entry {
>> +    bool enabled;
>> +    struct kfd_process_device *pdd;
>> +};
>> +
>>   /* Data that is per-process-per device. */
>>   struct kfd_process_device {
>>       /* The device that owns this data. */