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

[Felix Kuehling]

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.


> +	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.

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. */