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

[Felix Kuehling <felix.kuehling@amd.com>]

Am 2022-11-24 um 09:51 schrieb Kim, Jonathan:
> [Public]
>
>> -----Original Message-----
>> From: Kuehling, Felix <Felix.Kuehling@amd.com>
>> Sent: November 22, 2022 7:45 PM
>> To: Kim, Jonathan <Jonathan.Kim@amd.com>; amd-
>> gfx@lists.freedesktop.org
>> 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@amd.com>
>>>
>>> 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@amd.com>
>>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>> Signed-off-by: Jonathan Kim <jonathan.kim@amd.com>
>>> ---
>>>    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