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