Re: [PATCH 17/29] drm/amdkfd: Add debug trap enabled flag to TMA

Felix Kuehling <felix.kuehling@xxxxxxx> · Thu, 24 Nov 2022 11:23:48 -0500

Am 2022-11-24 um 09:51 schrieb Kim, Jonathan:
[Public]

-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling@xxxxxxx>
Sent: November 22, 2022 7:45 PM
To: Kim, Jonathan <Jonathan.Kim@xxxxxxx>; amd-
gfx@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH 17/29] drm/amdkfd: Add debug trap enabled flag to
TMA


On 2022-10-31 12:23, Jonathan Kim wrote:
From: Jay Cornwall <jay.cornwall@xxxxxxx>

Trap handler behavior will differ when a debugger is attached.

Make the debug trap flag available in the trap handler TMA.
Update it when the debug trap ioctl is invoked.

v3: Rebase for upstream

v2:
Add missing debug flag setup on APUs

Signed-off-by: Jay Cornwall <jay.cornwall@xxxxxxx>
Reviewed-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>
Signed-off-by: Jonathan Kim <jonathan.kim@xxxxxxx>
---
   drivers/gpu/drm/amd/amdkfd/kfd_debug.c   |  4 ++++
   drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  2 ++
   drivers/gpu/drm/amd/amdkfd/kfd_process.c | 16 ++++++++++++++++
   3 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
index ae6e701a2656..d4f87f2adada 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
@@ -193,6 +193,8 @@ void kfd_dbg_trap_deactivate(struct kfd_process
*target, bool unwind, int unwind
             if (unwind && count == unwind_count)
                     break;

+           kfd_process_set_trap_debug_flag(&pdd->qpd, false);
+
             /* GFX off is already disabled by debug activate if not RLC
restore supported. */
             if (kfd_dbg_is_rlc_restore_supported(pdd->dev))
                     amdgpu_gfx_off_ctrl(pdd->dev->adev, false);
@@ -278,6 +280,8 @@ int kfd_dbg_trap_activate(struct kfd_process
*target)
             if (kfd_dbg_is_rlc_restore_supported(pdd->dev))
                     amdgpu_gfx_off_ctrl(pdd->dev->adev, true);

+           kfd_process_set_trap_debug_flag(&pdd->qpd, true);
+
             r = debug_refresh_runlist(pdd->dev->dqm);
             if (r) {
                     target->runtime_info.runtime_state =
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 9690a2adb9ed..82b28588ab72 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -1101,6 +1101,8 @@ int kfd_init_apertures(struct kfd_process
*process);
   void kfd_process_set_trap_handler(struct qcm_process_device *qpd,
                               uint64_t tba_addr,
                               uint64_t tma_addr);
+void kfd_process_set_trap_debug_flag(struct qcm_process_device
*qpd,
+                                bool enabled);

   /* CWSR initialization */
   int kfd_process_init_cwsr_apu(struct kfd_process *process, struct file
*filep);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 59c4c38833b6..d62e0c62df76 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -1252,6 +1252,8 @@ int kfd_process_init_cwsr_apu(struct
kfd_process *p, struct file *filep)
             memcpy(qpd->cwsr_kaddr, dev->cwsr_isa, dev-
cwsr_isa_size);

+           kfd_process_set_trap_debug_flag(qpd, p-
debug_trap_enabled);
+
             qpd->tma_addr = qpd->tba_addr +
KFD_CWSR_TMA_OFFSET;
             pr_debug("set tba :0x%llx, tma:0x%llx, cwsr_kaddr:%p for
pqm.\n",
                     qpd->tba_addr, qpd->tma_addr, qpd->cwsr_kaddr);
@@ -1288,6 +1290,9 @@ static int
kfd_process_device_init_cwsr_dgpu(struct kfd_process_device *pdd)
     memcpy(qpd->cwsr_kaddr, dev->cwsr_isa, dev->cwsr_isa_size);

+   kfd_process_set_trap_debug_flag(&pdd->qpd,
+                                   pdd->process-
debug_trap_enabled);
+
     qpd->tma_addr = qpd->tba_addr + KFD_CWSR_TMA_OFFSET;
     pr_debug("set tba :0x%llx, tma:0x%llx, cwsr_kaddr:%p for pqm.\n",
              qpd->tba_addr, qpd->tma_addr, qpd->cwsr_kaddr);
@@ -1374,6 +1379,17 @@ bool kfd_process_xnack_mode(struct
kfd_process *p, bool supported)
     return true;
   }

+void kfd_process_set_trap_debug_flag(struct qcm_process_device
*qpd,
+                                bool enabled)
+{
+   /* If TMA doesn't exist then flag will be set during allocation. */
I would expect a change to the TMA allocation function, but that isn't
in this patch?
The TMA is allocated under kfd_process_init_cwsr_* and CWSR enabled is a pre-condition for the 1st level trap handler loading.
The lack of context in the patch for those functions may be hiding that fact.
Is the placement of this comment misleading?  Maybe it should go in kfd_dbg_trap_activate when kfd_process_set_trap_debug_flag is called?
Or should it just be removed since the combined calls within initialization of CWSR + debug enable seem complete for enablement?

I think the comment is fine. I was sort of expecting to see the 
corresponding change in the TMA allocation in the same patch. So my 
question is just lack of context. If that change in the TMA allocation 
got squashed into another patch in the series, maybe it would make sense 
to move it into this patch instead.

Regards,
  Felix



Thanks,

Jon

Regards,
    Felix

+   if (qpd->cwsr_kaddr) {
+           uint64_t *tma =
+                   (uint64_t *)(qpd->cwsr_kaddr +
KFD_CWSR_TMA_OFFSET);
+           tma[2] = enabled;
+   }
+}
+
   /*
    * On return the kfd_process is fully operational and will be freed when
the
    * mm is released






