On 2022-11-24 15:27, Kim, Jonathan wrote:
[AMD Official Use Only - General]-----Original Message----- From: Kuehling, Felix <Felix.Kuehling@xxxxxxx> Sent: November 24, 2022 11:24 AM To: Kim, Jonathan <Jonathan.Kim@xxxxxxx>; amd- gfx@xxxxxxxxxxxxxxxxxxxxx Subject: Re: [PATCH 17/29] drm/amdkfd: Add debug trap enabled flag to TMA 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.cb/drivers/gpu/drm/amd/amdkfd/kfd_debug.cindex 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(structkfd_process*target, bool unwind, int unwindif (unwind && count == unwind_count) break; + kfd_process_set_trap_debug_flag(&pdd->qpd, false); + /* GFX off is already disabled by debug activate if not RLCrestore 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.hb/drivers/gpu/drm/amd/amdkfd/kfd_priv.hindex 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.cb/drivers/gpu/drm/amd/amdkfd/kfd_process.cindex 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(structkfd_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 forpqm.\n",qpd->tba_addr, qpd->tma_addr, qpd->cwsr_kaddr); @@ -1288,6 +1290,9 @@ static intkfd_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(structkfd_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 isa 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 inkfd_dbg_trap_activate when kfd_process_set_trap_debug_flag is called?Or should it just be removed since the combined calls within initialization ofCWSR + 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.The change to set flag on TMA allocation is done in this patch as kfd_process_set_trap_debug_flag is now called in kfd_process_init_cwsr_*. To my knowledge, CWSR init and trap handler memory allocation should be atomic and that has been upstreamed for a while. Did you mean the user trap handler assignment? That should be independent from flagging.
OK, now I see where my confusion came from. I missed that ths same function was called from different places to enable the debug flag. The wording of the comment implied that this function is not used during TMA allocation and made me look for something accessing tma[2] directly somewhere else. The comment also talks only about setting the flag, while this function is also used to reset the flag. I think this comment would make more sense in kfd_dbg_trap_activate, where the context is "setting the flag when debugging is activated, potentially before the TMA is allocated".
Thanks, Felix
Thanks, JonRegards, FelixThanks, JonRegards, 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 whenthe* mm is released
