Re: [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....@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. */