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);
> + 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);
> + 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;
One more thought: You have a bunch of pcs_entries pointing to pdd. What
mechanism guarantees, that the pcs_entries are destroyed before the pdd
on process termination? I think kfd_pc_sampling_exit doesn't run during
process termination, because it deals with per-device data structures,
not per-process data structures.
Should the IDR be in the PDD rather than the device? In that case you
wouldn't even need the pdd pointer in struct pcs_entry.
I see you have a patch much later in the series "drm/amdkfd: add pc
sampling release when process release". I'd prefer this squashed here
and in the patches that add the start/stop functions.
Regards,
Felix
> + user_args->trace_id = (uint32_t)i;
> +
> + 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. */