* [PATCH] drm/amdkfd: add mask for debug trap flag setting @ 2026-01-19 17:15 James Zhu 2026-01-19 19:28 ` Kim, Jonathan 2026-01-20 10:34 ` Six, Lancelot 0 siblings, 2 replies; 6+ messages in thread From: James Zhu @ 2026-01-19 17:15 UTC (permalink / raw) To: amd-gfx; +Cc: lancelot.six, Jonathan.Kim, jamesz to specify which bits are valid setting on flags. Signed-off-by: James Zhu <James.Zhu@amd.com> --- drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +- drivers/gpu/drm/amd/amdkfd/kfd_debug.c | 48 ++++++++++++------------ drivers/gpu/drm/amd/amdkfd/kfd_debug.h | 3 +- include/uapi/linux/kfd_ioctl.h | 3 +- 4 files changed, 30 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c index 79586abad7fd..fd43ef7c9076 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c @@ -3377,7 +3377,7 @@ static int kfd_ioctl_set_debug_trap(struct file *filep, struct kfd_process *p, v args->clear_node_address_watch.id); break; case KFD_IOC_DBG_TRAP_SET_FLAGS: - r = kfd_dbg_trap_set_flags(target, &args->set_flags.flags); + r = kfd_dbg_trap_set_flags(target, &args->set_flags); break; case KFD_IOC_DBG_TRAP_QUERY_DEBUG_EVENT: r = kfd_dbg_ev_query_debug_event(target, diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c index a04875236647..279160ca71a9 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c @@ -512,38 +512,42 @@ static void kfd_dbg_clear_process_address_watch(struct kfd_process *target) kfd_dbg_trap_clear_dev_address_watch(target->pdds[i], j); } -int kfd_dbg_trap_set_flags(struct kfd_process *target, uint32_t *flags) +int kfd_dbg_trap_set_flags(struct kfd_process *target, + struct kfd_ioctl_dbg_trap_set_flags_args *set_flags) { uint32_t prev_flags = target->dbg_flags; int i, r = 0, rewind_count = 0; + if (!((set_flags->flags ^ prev_flags) & set_flags->mask)) + return 0; + for (i = 0; i < target->n_pdds; i++) { struct kfd_topology_device *topo_dev = kfd_topology_device_by_id(target->pdds[i]->dev->id); uint32_t caps = topo_dev->node_props.capability; uint32_t caps2 = topo_dev->node_props.capability2; + struct amdgpu_device *adev = target->pdds[i]->dev->adev; - if (!(caps & HSA_CAP_TRAP_DEBUG_PRECISE_MEMORY_OPERATIONS_SUPPORTED) && - (*flags & KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP)) { - *flags = prev_flags; - return -EACCES; - } - - if (!(caps & HSA_CAP_TRAP_DEBUG_PRECISE_ALU_OPERATIONS_SUPPORTED) && - (*flags & KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP)) { - *flags = prev_flags; - return -EACCES; - } - - if (!(caps2 & HSA_CAP2_TRAP_DEBUG_LDS_OUT_OF_ADDR_RANGE_SUPPORTED) && - (*flags & KFD_DBG_TRAP_FLAG_LDS_OUT_OF_ADDR_RANGE)) { - *flags = prev_flags; + if (set_flags->mask == 0xFFFFFFFF && !set_flags->flags) + break; + if ((!(caps & HSA_CAP_TRAP_DEBUG_PRECISE_MEMORY_OPERATIONS_SUPPORTED) && + (set_flags->mask & KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP)) || + (!(caps & HSA_CAP_TRAP_DEBUG_PRECISE_ALU_OPERATIONS_SUPPORTED) && + (set_flags->mask & KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP)) || + (!(caps2 & HSA_CAP2_TRAP_DEBUG_LDS_OUT_OF_ADDR_RANGE_SUPPORTED) && + (set_flags->mask & KFD_DBG_TRAP_FLAG_LDS_OUT_OF_ADDR_RANGE))) { + set_flags->flags = prev_flags; + dev_dbg(adev->dev, "Debug Trap set mask 0x%x caps 0x%x caps2 0x%x", + set_flags->mask, caps, caps2); return -EACCES; } } - target->dbg_flags = *flags; - *flags = prev_flags; + target->dbg_flags ^= (target->dbg_flags ^ set_flags->flags) & set_flags->mask; + pr_debug("Debug Trap previous flags 0x%x set flags 0x%x set mask 0x%x target flags 0x%x", + prev_flags, set_flags->flags, set_flags->mask, target->dbg_flags); + + set_flags->flags = prev_flags; for (i = 0; i < target->n_pdds; i++) { struct kfd_process_device *pdd = target->pdds[i]; @@ -555,10 +559,8 @@ int kfd_dbg_trap_set_flags(struct kfd_process *target, uint32_t *flags) else r = kfd_dbg_set_mes_debug_mode(pdd, true); - if (r) { - target->dbg_flags = prev_flags; + if (r) break; - } rewind_count++; } @@ -596,7 +598,7 @@ void kfd_dbg_trap_deactivate(struct kfd_process *target, bool unwind, int unwind int i; if (!unwind) { - uint32_t flags = 0; + struct kfd_ioctl_dbg_trap_set_flags_args set_flags = {0, 0xFFFFFFFF}; int resume_count = resume_queues(target, 0, NULL); if (resume_count) @@ -606,7 +608,7 @@ void kfd_dbg_trap_deactivate(struct kfd_process *target, bool unwind, int unwind kfd_dbg_clear_process_address_watch(target); kfd_dbg_trap_set_wave_launch_mode(target, 0); - kfd_dbg_trap_set_flags(target, &flags); + kfd_dbg_trap_set_flags(target, &set_flags); } for (i = 0; i < target->n_pdds; i++) { diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.h b/drivers/gpu/drm/amd/amdkfd/kfd_debug.h index 894753818cba..34d27eb500a5 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.h +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.h @@ -62,7 +62,8 @@ int kfd_dbg_trap_set_dev_address_watch(struct kfd_process_device *pdd, uint32_t watch_address_mask, uint32_t *watch_id, uint32_t watch_mode); -int kfd_dbg_trap_set_flags(struct kfd_process *target, uint32_t *flags); +int kfd_dbg_trap_set_flags(struct kfd_process *target, + struct kfd_ioctl_dbg_trap_set_flags_args *set_flags); int kfd_dbg_trap_query_exception_info(struct kfd_process *target, uint32_t source_id, uint32_t exception_code, diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h index e9b756ca228c..0522fe7344e4 100644 --- a/include/uapi/linux/kfd_ioctl.h +++ b/include/uapi/linux/kfd_ioctl.h @@ -1515,6 +1515,7 @@ struct kfd_ioctl_dbg_trap_clear_node_address_watch_args { * Sets flags for wave behaviour. * * @flags (IN/OUT) - IN = flags to enable, OUT = flags previously enabled + * @mask (IN) - IN = specified debug trap operation bits on flag * * Generic errors apply (see kfd_dbg_trap_operations). * Return - 0 on SUCCESS. @@ -1522,7 +1523,7 @@ struct kfd_ioctl_dbg_trap_clear_node_address_watch_args { */ struct kfd_ioctl_dbg_trap_set_flags_args { __u32 flags; - __u32 pad; + __u32 mask; }; /** -- 2.34.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH] drm/amdkfd: add mask for debug trap flag setting 2026-01-19 17:15 [PATCH] drm/amdkfd: add mask for debug trap flag setting James Zhu @ 2026-01-19 19:28 ` Kim, Jonathan 2026-01-19 20:57 ` James Zhu 2026-01-20 10:34 ` Six, Lancelot 1 sibling, 1 reply; 6+ messages in thread From: Kim, Jonathan @ 2026-01-19 19:28 UTC (permalink / raw) To: Zhu, James, amd-gfx@lists.freedesktop.org; +Cc: Six, Lancelot [Public] > -----Original Message----- > From: Zhu, James <James.Zhu@amd.com> > Sent: Monday, January 19, 2026 12:15 PM > To: amd-gfx@lists.freedesktop.org > Cc: Six, Lancelot <Lancelot.Six@amd.com>; Kim, Jonathan > <Jonathan.Kim@amd.com>; Zhu, James <James.Zhu@amd.com> > Subject: [PATCH] drm/amdkfd: add mask for debug trap flag setting > > to specify which bits are valid setting on flags. Can you elaborate? > > Signed-off-by: James Zhu <James.Zhu@amd.com> > --- > drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +- > drivers/gpu/drm/amd/amdkfd/kfd_debug.c | 48 ++++++++++++------------ > drivers/gpu/drm/amd/amdkfd/kfd_debug.h | 3 +- > include/uapi/linux/kfd_ioctl.h | 3 +- > 4 files changed, 30 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > index 79586abad7fd..fd43ef7c9076 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > @@ -3377,7 +3377,7 @@ static int kfd_ioctl_set_debug_trap(struct file *filep, > struct kfd_process *p, v > args->clear_node_address_watch.id); > break; > case KFD_IOC_DBG_TRAP_SET_FLAGS: > - r = kfd_dbg_trap_set_flags(target, &args->set_flags.flags); > + r = kfd_dbg_trap_set_flags(target, &args->set_flags); > break; > case KFD_IOC_DBG_TRAP_QUERY_DEBUG_EVENT: > r = kfd_dbg_ev_query_debug_event(target, > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c > b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c > index a04875236647..279160ca71a9 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c > @@ -512,38 +512,42 @@ static void > kfd_dbg_clear_process_address_watch(struct kfd_process *target) > kfd_dbg_trap_clear_dev_address_watch(target- > >pdds[i], j); > } > > -int kfd_dbg_trap_set_flags(struct kfd_process *target, uint32_t *flags) > +int kfd_dbg_trap_set_flags(struct kfd_process *target, > + struct kfd_ioctl_dbg_trap_set_flags_args *set_flags) > { > uint32_t prev_flags = target->dbg_flags; > int i, r = 0, rewind_count = 0; > > + if (!((set_flags->flags ^ prev_flags) & set_flags->mask)) Does this block old debuggers from setting debug mode if pad is 0? > + return 0; > + > for (i = 0; i < target->n_pdds; i++) { > struct kfd_topology_device *topo_dev = > kfd_topology_device_by_id(target->pdds[i]- > >dev->id); > uint32_t caps = topo_dev->node_props.capability; > uint32_t caps2 = topo_dev->node_props.capability2; > + struct amdgpu_device *adev = target->pdds[i]->dev->adev; > > - if (!(caps & > HSA_CAP_TRAP_DEBUG_PRECISE_MEMORY_OPERATIONS_SUPPORTED) && > - (*flags & KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP)) { > - *flags = prev_flags; > - return -EACCES; > - } > - > - if (!(caps & > HSA_CAP_TRAP_DEBUG_PRECISE_ALU_OPERATIONS_SUPPORTED) && > - (*flags & KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP)) { > - *flags = prev_flags; > - return -EACCES; > - } > - > - if (!(caps2 & > HSA_CAP2_TRAP_DEBUG_LDS_OUT_OF_ADDR_RANGE_SUPPORTED) && > - (*flags & > KFD_DBG_TRAP_FLAG_LDS_OUT_OF_ADDR_RANGE)) { > - *flags = prev_flags; > + if (set_flags->mask == 0xFFFFFFFF && !set_flags->flags) > + break; Seems like this is only for a deactivation system call. Probably don't want to let users abuse this with a magic number then. Maybe breakout into new static call in deactivation or just device loop deactivate directly there since this logic is starting to get complicated. > + if ((!(caps & > HSA_CAP_TRAP_DEBUG_PRECISE_MEMORY_OPERATIONS_SUPPORTED) && > + (set_flags->mask & > KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP)) || > + (!(caps & > HSA_CAP_TRAP_DEBUG_PRECISE_ALU_OPERATIONS_SUPPORTED) && > + (set_flags->mask & KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP)) > || > + (!(caps2 & > HSA_CAP2_TRAP_DEBUG_LDS_OUT_OF_ADDR_RANGE_SUPPORTED) && > + (set_flags->mask & > KFD_DBG_TRAP_FLAG_LDS_OUT_OF_ADDR_RANGE))) { Please vertically indent align "caps" and "set_flags" underneath each other for legibility. > + set_flags->flags = prev_flags; > + dev_dbg(adev->dev, "Debug Trap set mask 0x%x caps > 0x%x caps2 0x%x", > + set_flags->mask, caps, caps2); > return -EACCES; > } > } > > - target->dbg_flags = *flags; > - *flags = prev_flags; > + target->dbg_flags ^= (target->dbg_flags ^ set_flags->flags) & set_flags- > >mask; I assume we can only set/unset 1 flag at a time, which is why this works. Otherwise, maybe I don't have enough background on this, but please do elaborate. > + pr_debug("Debug Trap previous flags 0x%x set flags 0x%x set mask 0x%x > target flags 0x%x", > + prev_flags, set_flags->flags, set_flags->mask, target- > >dbg_flags); > + > + set_flags->flags = prev_flags; > for (i = 0; i < target->n_pdds; i++) { > struct kfd_process_device *pdd = target->pdds[i]; > > @@ -555,10 +559,8 @@ int kfd_dbg_trap_set_flags(struct kfd_process *target, > uint32_t *flags) > else > r = kfd_dbg_set_mes_debug_mode(pdd, true); > > - if (r) { > - target->dbg_flags = prev_flags; Doesn't rewind require the previous value for something to rewind to? > + if (r) > break; > - } > > rewind_count++; > } > @@ -596,7 +598,7 @@ void kfd_dbg_trap_deactivate(struct kfd_process *target, > bool unwind, int unwind > int i; > > if (!unwind) { > - uint32_t flags = 0; > + struct kfd_ioctl_dbg_trap_set_flags_args set_flags = {0, > 0xFFFFFFFF}; Similar to 3 comments above. Just call a new static or loop directly to reset debug mode (i.e. all flags clear). i.e. you don't have to rewind on preemption failure here because deactivation itself is a rewind we're pretty much out of luck at this point if we fail. Jon > int resume_count = resume_queues(target, 0, NULL); > > if (resume_count) > @@ -606,7 +608,7 @@ void kfd_dbg_trap_deactivate(struct kfd_process *target, > bool unwind, int unwind > kfd_dbg_clear_process_address_watch(target); > kfd_dbg_trap_set_wave_launch_mode(target, 0); > > - kfd_dbg_trap_set_flags(target, &flags); > + kfd_dbg_trap_set_flags(target, &set_flags); > } > > for (i = 0; i < target->n_pdds; i++) { > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.h > b/drivers/gpu/drm/amd/amdkfd/kfd_debug.h > index 894753818cba..34d27eb500a5 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.h > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.h > @@ -62,7 +62,8 @@ int kfd_dbg_trap_set_dev_address_watch(struct > kfd_process_device *pdd, > uint32_t watch_address_mask, > uint32_t *watch_id, > uint32_t watch_mode); > -int kfd_dbg_trap_set_flags(struct kfd_process *target, uint32_t *flags); > +int kfd_dbg_trap_set_flags(struct kfd_process *target, > + struct kfd_ioctl_dbg_trap_set_flags_args *set_flags); > int kfd_dbg_trap_query_exception_info(struct kfd_process *target, > uint32_t source_id, > uint32_t exception_code, > diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h > index e9b756ca228c..0522fe7344e4 100644 > --- a/include/uapi/linux/kfd_ioctl.h > +++ b/include/uapi/linux/kfd_ioctl.h > @@ -1515,6 +1515,7 @@ struct > kfd_ioctl_dbg_trap_clear_node_address_watch_args { > * Sets flags for wave behaviour. > * > * @flags (IN/OUT) - IN = flags to enable, OUT = flags previously enabled > + * @mask (IN) - IN = specified debug trap operation bits on flag > * > * Generic errors apply (see kfd_dbg_trap_operations). > * Return - 0 on SUCCESS. > @@ -1522,7 +1523,7 @@ struct > kfd_ioctl_dbg_trap_clear_node_address_watch_args { > */ > struct kfd_ioctl_dbg_trap_set_flags_args { > __u32 flags; > - __u32 pad; > + __u32 mask; > }; > > /** > -- > 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amdkfd: add mask for debug trap flag setting 2026-01-19 19:28 ` Kim, Jonathan @ 2026-01-19 20:57 ` James Zhu 0 siblings, 0 replies; 6+ messages in thread From: James Zhu @ 2026-01-19 20:57 UTC (permalink / raw) To: Kim, Jonathan, Zhu, James, amd-gfx@lists.freedesktop.org; +Cc: Six, Lancelot [-- Attachment #1: Type: text/plain, Size: 10678 bytes --] Hi Jon, thanks! Answers are in line. Best Regards! James Zhu On 2026-01-19 14:28, Kim, Jonathan wrote: > [Public] > >> -----Original Message----- >> From: Zhu, James<James.Zhu@amd.com> >> Sent: Monday, January 19, 2026 12:15 PM >> To:amd-gfx@lists.freedesktop.org >> Cc: Six, Lancelot<Lancelot.Six@amd.com>; Kim, Jonathan >> <Jonathan.Kim@amd.com>; Zhu, James<James.Zhu@amd.com> >> Subject: [PATCH] drm/amdkfd: add mask for debug trap flag setting >> >> to specify which bits are valid setting on flags. > Can you elaborate? > Without mask setting, Need read current flags 1st, any update flags' specified bits, others bits keep unchanged, then send absolute flags value to system. mask can indicate which bits in flags are need to been touched, we can put value(0 or 1) into those specified bits, and send to system directly without read back operation. For example. flags = 0x2 mask =0x3, will keep MSB31-MSB2 unchanged, LSB1 set to 1,LSB0 set 0. Will update messages to: */apply masked bit update on debug trap flags/* */The implementation uses an XOR-AND-XOR pattern to atomically update bits within the 'set_flags->mask' range./* */Logic: /* */- Difference: (target_flags->flags ^ set_flags->flags) identifies bit mismatches. /* */- Filtering: The result is masked by 'set_flags->mask' to restrict changes. /* */- Application: XORing back to 'target_flags->flags' flips only the mismatched bits to the desired state./* */This approach avoids multiple assignment steps and preserves all bits outside of the mask./* > >> Signed-off-by: James Zhu<James.Zhu@amd.com> >> --- >> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +- >> drivers/gpu/drm/amd/amdkfd/kfd_debug.c | 48 ++++++++++++------------ >> drivers/gpu/drm/amd/amdkfd/kfd_debug.h | 3 +- >> include/uapi/linux/kfd_ioctl.h | 3 +- >> 4 files changed, 30 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> index 79586abad7fd..fd43ef7c9076 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> @@ -3377,7 +3377,7 @@ static int kfd_ioctl_set_debug_trap(struct file *filep, >> struct kfd_process *p, v >> args->clear_node_address_watch.id); >> break; >> case KFD_IOC_DBG_TRAP_SET_FLAGS: >> - r = kfd_dbg_trap_set_flags(target, &args->set_flags.flags); >> + r = kfd_dbg_trap_set_flags(target, &args->set_flags); >> break; >> case KFD_IOC_DBG_TRAP_QUERY_DEBUG_EVENT: >> r = kfd_dbg_ev_query_debug_event(target, >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c >> b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c >> index a04875236647..279160ca71a9 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c >> @@ -512,38 +512,42 @@ static void >> kfd_dbg_clear_process_address_watch(struct kfd_process *target) >> kfd_dbg_trap_clear_dev_address_watch(target- >>> pdds[i], j); >> } >> >> -int kfd_dbg_trap_set_flags(struct kfd_process *target, uint32_t *flags) >> +int kfd_dbg_trap_set_flags(struct kfd_process *target, >> + struct kfd_ioctl_dbg_trap_set_flags_args *set_flags) >> { >> uint32_t prev_flags = target->dbg_flags; >> int i, r = 0, rewind_count = 0; >> >> + if (!((set_flags->flags ^ prev_flags) & set_flags->mask)) > Does this block old debuggers from setting debug mode if pad is 0? > [JZ] Yes, this logical can been moved down after target->dbg_flags gets updated > if (target->dbg_flags == prev_flags) return 0; >> + return 0; >> + >> for (i = 0; i < target->n_pdds; i++) { >> struct kfd_topology_device *topo_dev = >> kfd_topology_device_by_id(target->pdds[i]- >>> dev->id); >> uint32_t caps = topo_dev->node_props.capability; >> uint32_t caps2 = topo_dev->node_props.capability2; >> + struct amdgpu_device *adev = target->pdds[i]->dev->adev; >> >> - if (!(caps & >> HSA_CAP_TRAP_DEBUG_PRECISE_MEMORY_OPERATIONS_SUPPORTED) && >> - (*flags & KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP)) { >> - *flags = prev_flags; >> - return -EACCES; >> - } >> - >> - if (!(caps & >> HSA_CAP_TRAP_DEBUG_PRECISE_ALU_OPERATIONS_SUPPORTED) && >> - (*flags & KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP)) { >> - *flags = prev_flags; >> - return -EACCES; >> - } >> - >> - if (!(caps2 & >> HSA_CAP2_TRAP_DEBUG_LDS_OUT_OF_ADDR_RANGE_SUPPORTED) && >> - (*flags & >> KFD_DBG_TRAP_FLAG_LDS_OUT_OF_ADDR_RANGE)) { >> - *flags = prev_flags; >> + if (set_flags->mask == 0xFFFFFFFF && !set_flags->flags) >> + break; > Seems like this is only for a deactivation system call. > Probably don't want to let users abuse this with a magic number then. > Maybe breakout into new static call in deactivation or just device loop deactivate directly there since this logic is starting to get complicated. [JZ] Yes, only for deactivation system call. > >> + if ((!(caps & >> HSA_CAP_TRAP_DEBUG_PRECISE_MEMORY_OPERATIONS_SUPPORTED) && >> + (set_flags->mask & >> KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP)) || >> + (!(caps & >> HSA_CAP_TRAP_DEBUG_PRECISE_ALU_OPERATIONS_SUPPORTED) && >> + (set_flags->mask & KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP)) >> || >> + (!(caps2 & >> HSA_CAP2_TRAP_DEBUG_LDS_OUT_OF_ADDR_RANGE_SUPPORTED) && >> + (set_flags->mask & >> KFD_DBG_TRAP_FLAG_LDS_OUT_OF_ADDR_RANGE))) { > Please vertically indent align "caps" and "set_flags" underneath each other for legibility. [JZ] sure > >> + set_flags->flags = prev_flags; >> + dev_dbg(adev->dev, "Debug Trap set mask 0x%x caps >> 0x%x caps2 0x%x", >> + set_flags->mask, caps, caps2); >> return -EACCES; >> } >> } >> >> - target->dbg_flags = *flags; >> - *flags = prev_flags; >> + target->dbg_flags ^= (target->dbg_flags ^ set_flags->flags) & set_flags- >>> mask; > I assume we can only set/unset 1 flag at a time, which is why this works. > Otherwise, maybe I don't have enough background on this, but please do elaborate. [JZ] Here is not just for set/unset 1 bit. ^ is bitwise XOR. target->dbg_flags ^ set_flags->flags can find which bits are changed (set those bits to 1 if not equal), & set_flags-mask can filter out unrelated bits. target->dbg_flags ^= can change related bits accordingly. > mask > >> + pr_debug("Debug Trap previous flags 0x%x set flags 0x%x set mask 0x%x >> target flags 0x%x", >> + prev_flags, set_flags->flags, set_flags->mask, target- >>> dbg_flags); >> + >> + set_flags->flags = prev_flags; >> for (i = 0; i < target->n_pdds; i++) { >> struct kfd_process_device *pdd = target->pdds[i]; >> >> @@ -555,10 +559,8 @@ int kfd_dbg_trap_set_flags(struct kfd_process *target, >> uint32_t *flags) >> else >> r = kfd_dbg_set_mes_debug_mode(pdd, true); >> >> - if (r) { >> - target->dbg_flags = prev_flags; > Doesn't rewind require the previous value for something to rewind to? [JZ] Yes, since after break, it will rewind flags over there when check r value at the end. > >> + if (r) >> break; >> - } >> >> rewind_count++; >> } >> @@ -596,7 +598,7 @@ void kfd_dbg_trap_deactivate(struct kfd_process *target, >> bool unwind, int unwind >> int i; >> >> if (!unwind) { >> - uint32_t flags = 0; >> + struct kfd_ioctl_dbg_trap_set_flags_args set_flags = {0, >> 0xFFFFFFFF}; > Similar to 3 comments above. Just call a new static or loop directly to reset debug mode (i.e. all flags clear). > i.e. you don't have to rewind on preemption failure here because deactivation itself is a rewind we're pretty much out of luck at this point if we fail. [JZ] I will send out v2 soon, See if your concern gets addressed or not. > > Jon > >> int resume_count = resume_queues(target, 0, NULL); >> >> if (resume_count) >> @@ -606,7 +608,7 @@ void kfd_dbg_trap_deactivate(struct kfd_process *target, >> bool unwind, int unwind >> kfd_dbg_clear_process_address_watch(target); >> kfd_dbg_trap_set_wave_launch_mode(target, 0); >> >> - kfd_dbg_trap_set_flags(target, &flags); >> + kfd_dbg_trap_set_flags(target, &set_flags); >> } >> >> for (i = 0; i < target->n_pdds; i++) { >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.h >> b/drivers/gpu/drm/amd/amdkfd/kfd_debug.h >> index 894753818cba..34d27eb500a5 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.h >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.h >> @@ -62,7 +62,8 @@ int kfd_dbg_trap_set_dev_address_watch(struct >> kfd_process_device *pdd, >> uint32_t watch_address_mask, >> uint32_t *watch_id, >> uint32_t watch_mode); >> -int kfd_dbg_trap_set_flags(struct kfd_process *target, uint32_t *flags); >> +int kfd_dbg_trap_set_flags(struct kfd_process *target, >> + struct kfd_ioctl_dbg_trap_set_flags_args *set_flags); >> int kfd_dbg_trap_query_exception_info(struct kfd_process *target, >> uint32_t source_id, >> uint32_t exception_code, >> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h >> index e9b756ca228c..0522fe7344e4 100644 >> --- a/include/uapi/linux/kfd_ioctl.h >> +++ b/include/uapi/linux/kfd_ioctl.h >> @@ -1515,6 +1515,7 @@ struct >> kfd_ioctl_dbg_trap_clear_node_address_watch_args { >> * Sets flags for wave behaviour. >> * >> * @flags (IN/OUT) - IN = flags to enable, OUT = flags previously enabled >> + * @mask (IN) - IN = specified debug trap operation bits on flag >> * >> * Generic errors apply (see kfd_dbg_trap_operations). >> * Return - 0 on SUCCESS. >> @@ -1522,7 +1523,7 @@ struct >> kfd_ioctl_dbg_trap_clear_node_address_watch_args { >> */ >> struct kfd_ioctl_dbg_trap_set_flags_args { >> __u32 flags; >> - __u32 pad; >> + __u32 mask; >> }; >> >> /** >> -- >> 2.34.1 [-- Attachment #2: Type: text/html, Size: 16331 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] drm/amdkfd: add mask for debug trap flag setting 2026-01-19 17:15 [PATCH] drm/amdkfd: add mask for debug trap flag setting James Zhu 2026-01-19 19:28 ` Kim, Jonathan @ 2026-01-20 10:34 ` Six, Lancelot 2026-01-20 14:39 ` James Zhu 1 sibling, 1 reply; 6+ messages in thread From: Six, Lancelot @ 2026-01-20 10:34 UTC (permalink / raw) To: Zhu, James, amd-gfx@lists.freedesktop.org; +Cc: Kim, Jonathan [AMD Official Use Only - AMD Internal Distribution Only] Hi, > -----Original Message----- > From: Zhu, James <James.Zhu@amd.com> > Sent: 19 January 2026 17:15 > To: amd-gfx@lists.freedesktop.org > Cc: Six, Lancelot <Lancelot.Six@amd.com>; Kim, Jonathan > <Jonathan.Kim@amd.com>; Zhu, James <James.Zhu@amd.com> > Subject: [PATCH] drm/amdkfd: add mask for debug trap flag setting > > to specify which bits are valid setting on flags. > > Signed-off-by: James Zhu <James.Zhu@amd.com> > --- > drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +- > drivers/gpu/drm/amd/amdkfd/kfd_debug.c | 48 ++++++++++++------------ > drivers/gpu/drm/amd/amdkfd/kfd_debug.h | 3 +- > include/uapi/linux/kfd_ioctl.h | 3 +- > 4 files changed, 30 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > index 79586abad7fd..fd43ef7c9076 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > @@ -3377,7 +3377,7 @@ static int kfd_ioctl_set_debug_trap(struct file *filep, > struct kfd_process *p, v > args->clear_node_address_watch.id); > break; > case KFD_IOC_DBG_TRAP_SET_FLAGS: > - r = kfd_dbg_trap_set_flags(target, &args->set_flags.flags); > + r = kfd_dbg_trap_set_flags(target, &args->set_flags); > break; > case KFD_IOC_DBG_TRAP_QUERY_DEBUG_EVENT: > r = kfd_dbg_ev_query_debug_event(target, > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c > b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c > index a04875236647..279160ca71a9 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c > @@ -512,38 +512,42 @@ static void > kfd_dbg_clear_process_address_watch(struct kfd_process *target) > kfd_dbg_trap_clear_dev_address_watch(target- > >pdds[i], j); > } > > -int kfd_dbg_trap_set_flags(struct kfd_process *target, uint32_t *flags) > +int kfd_dbg_trap_set_flags(struct kfd_process *target, > + struct kfd_ioctl_dbg_trap_set_flags_args *set_flags) > { > uint32_t prev_flags = target->dbg_flags; > int i, r = 0, rewind_count = 0; > > + if (!((set_flags->flags ^ prev_flags) & set_flags->mask)) > + return 0; > + > for (i = 0; i < target->n_pdds; i++) { > struct kfd_topology_device *topo_dev = > kfd_topology_device_by_id(target->pdds[i]- > >dev->id); > uint32_t caps = topo_dev->node_props.capability; > uint32_t caps2 = topo_dev->node_props.capability2; > + struct amdgpu_device *adev = target->pdds[i]->dev->adev; > > - if (!(caps & > HSA_CAP_TRAP_DEBUG_PRECISE_MEMORY_OPERATIONS_SUPPORTED) && > - (*flags & KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP)) { > - *flags = prev_flags; > - return -EACCES; > - } > - > - if (!(caps & > HSA_CAP_TRAP_DEBUG_PRECISE_ALU_OPERATIONS_SUPPORTED) && > - (*flags & KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP)) { > - *flags = prev_flags; > - return -EACCES; > - } > - > - if (!(caps2 & > HSA_CAP2_TRAP_DEBUG_LDS_OUT_OF_ADDR_RANGE_SUPPORTED) && > - (*flags & > KFD_DBG_TRAP_FLAG_LDS_OUT_OF_ADDR_RANGE)) { > - *flags = prev_flags; > + if (set_flags->mask == 0xFFFFFFFF && !set_flags->flags) > + break; > + if ((!(caps & > HSA_CAP_TRAP_DEBUG_PRECISE_MEMORY_OPERATIONS_SUPPORTED) && > + (set_flags->mask & > KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP)) || > + (!(caps & > HSA_CAP_TRAP_DEBUG_PRECISE_ALU_OPERATIONS_SUPPORTED) && > + (set_flags->mask & KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP)) > || > + (!(caps2 & > HSA_CAP2_TRAP_DEBUG_LDS_OUT_OF_ADDR_RANGE_SUPPORTED) && > + (set_flags->mask & > KFD_DBG_TRAP_FLAG_LDS_OUT_OF_ADDR_RANGE))) { > + set_flags->flags = prev_flags; > + dev_dbg(adev->dev, "Debug Trap set mask 0x%x caps > 0x%x caps2 0x%x", > + set_flags->mask, caps, caps2); The logic here seems odd. If "set_flags->mask" is the set of flags a caller wants to modify (I don't think it should, see below), a call which asks for PRECISE_MEMORY to be disabled would return EACCESS if PRECISE_MEMORY is not supported. Before this patch, it would have been perfectly valid set (or confirm) the PRECISE_MEMORY flag to 0. Also, consider the following scenario where a user wants to set PRECISE_ALU=0 and PRECISE_MEMORY=1: set_flags->flags = KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP; set_flags->mask = KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP | KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP; If the platform only supports PRECISE_MEMORY (like gfx11 for example), the call would erroneously fail with EACCESS. This was possible before this patch. > return -EACCES; > } > } > > - target->dbg_flags = *flags; > - *flags = prev_flags; > + target->dbg_flags ^= (target->dbg_flags ^ set_flags->flags) & set_flags- > >mask; > + pr_debug("Debug Trap previous flags 0x%x set flags 0x%x set mask > 0x%x target flags 0x%x", > + prev_flags, set_flags->flags, set_flags->mask, target->dbg_flags); > + > + set_flags->flags = prev_flags; > for (i = 0; i < target->n_pdds; i++) { > struct kfd_process_device *pdd = target->pdds[i]; > > @@ -555,10 +559,8 @@ int kfd_dbg_trap_set_flags(struct kfd_process *target, > uint32_t *flags) > else > r = kfd_dbg_set_mes_debug_mode(pdd, true); > > - if (r) { > - target->dbg_flags = prev_flags; > + if (r) > break; > - } > > rewind_count++; > } > @@ -596,7 +598,7 @@ void kfd_dbg_trap_deactivate(struct kfd_process > *target, bool unwind, int unwind > int i; > > if (!unwind) { > - uint32_t flags = 0; > + struct kfd_ioctl_dbg_trap_set_flags_args set_flags = {0, > 0xFFFFFFFF}; > int resume_count = resume_queues(target, 0, NULL); > > if (resume_count) > @@ -606,7 +608,7 @@ void kfd_dbg_trap_deactivate(struct kfd_process > *target, bool unwind, int unwind > kfd_dbg_clear_process_address_watch(target); > kfd_dbg_trap_set_wave_launch_mode(target, 0); > > - kfd_dbg_trap_set_flags(target, &flags); > + kfd_dbg_trap_set_flags(target, &set_flags); > } > > for (i = 0; i < target->n_pdds; i++) { > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.h > b/drivers/gpu/drm/amd/amdkfd/kfd_debug.h > index 894753818cba..34d27eb500a5 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.h > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.h > @@ -62,7 +62,8 @@ int kfd_dbg_trap_set_dev_address_watch(struct > kfd_process_device *pdd, > uint32_t watch_address_mask, > uint32_t *watch_id, > uint32_t watch_mode); > -int kfd_dbg_trap_set_flags(struct kfd_process *target, uint32_t *flags); > +int kfd_dbg_trap_set_flags(struct kfd_process *target, > + struct kfd_ioctl_dbg_trap_set_flags_args *set_flags); > int kfd_dbg_trap_query_exception_info(struct kfd_process *target, > uint32_t source_id, > uint32_t exception_code, > diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h > index e9b756ca228c..0522fe7344e4 100644 > --- a/include/uapi/linux/kfd_ioctl.h > +++ b/include/uapi/linux/kfd_ioctl.h > @@ -1515,6 +1515,7 @@ struct > kfd_ioctl_dbg_trap_clear_node_address_watch_args { > * Sets flags for wave behaviour. > * > * @flags (IN/OUT) - IN = flags to enable, OUT = flags previously enabled > + * @mask (IN) - IN = specified debug trap operation bits on flag I am not sure I fully understand this comment. Is it meant to say which bits from FLAGS will be set? Also, this seems to break backwards compatibility. Before this patch, a caller would call this ioctl with "flags" set, and the PAD field would be 0. The semantics of this call is to set all bits from flags. The updated semantics of MASK would make it so no flags are set instead of alle. The updated ioctl must preserve this existing pattern, so instead of mask telling which bits must be changed, it should specify which bits should be preserved (or ignored from FLAGS). To make it clearer, I'd probably call this "preserve_mask" @preserve_mask (IN) - IN = Specify flags which must not be modified. Best, Lancelot. > * > * Generic errors apply (see kfd_dbg_trap_operations). > * Return - 0 on SUCCESS. > @@ -1522,7 +1523,7 @@ struct > kfd_ioctl_dbg_trap_clear_node_address_watch_args { > */ > struct kfd_ioctl_dbg_trap_set_flags_args { > __u32 flags; > - __u32 pad; > + __u32 mask; > }; > > /** > -- > 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amdkfd: add mask for debug trap flag setting 2026-01-20 10:34 ` Six, Lancelot @ 2026-01-20 14:39 ` James Zhu 2026-01-20 14:52 ` Lancelot SIX 0 siblings, 1 reply; 6+ messages in thread From: James Zhu @ 2026-01-20 14:39 UTC (permalink / raw) To: Six, Lancelot, Zhu, James, amd-gfx@lists.freedesktop.org; +Cc: Kim, Jonathan [-- Attachment #1: Type: text/plain, Size: 10217 bytes --] On 2026-01-20 05:34, Six, Lancelot wrote: > [AMD Official Use Only - AMD Internal Distribution Only] > > Hi, > > >> -----Original Message----- >> From: Zhu, James<James.Zhu@amd.com> >> Sent: 19 January 2026 17:15 >> To:amd-gfx@lists.freedesktop.org >> Cc: Six, Lancelot<Lancelot.Six@amd.com>; Kim, Jonathan >> <Jonathan.Kim@amd.com>; Zhu, James<James.Zhu@amd.com> >> Subject: [PATCH] drm/amdkfd: add mask for debug trap flag setting >> >> to specify which bits are valid setting on flags. >> >> Signed-off-by: James Zhu<James.Zhu@amd.com> >> --- >> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +- >> drivers/gpu/drm/amd/amdkfd/kfd_debug.c | 48 ++++++++++++------------ >> drivers/gpu/drm/amd/amdkfd/kfd_debug.h | 3 +- >> include/uapi/linux/kfd_ioctl.h | 3 +- >> 4 files changed, 30 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> index 79586abad7fd..fd43ef7c9076 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> @@ -3377,7 +3377,7 @@ static int kfd_ioctl_set_debug_trap(struct file *filep, >> struct kfd_process *p, v >> args->clear_node_address_watch.id); >> break; >> case KFD_IOC_DBG_TRAP_SET_FLAGS: >> - r = kfd_dbg_trap_set_flags(target, &args->set_flags.flags); >> + r = kfd_dbg_trap_set_flags(target, &args->set_flags); >> break; >> case KFD_IOC_DBG_TRAP_QUERY_DEBUG_EVENT: >> r = kfd_dbg_ev_query_debug_event(target, >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c >> b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c >> index a04875236647..279160ca71a9 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c >> @@ -512,38 +512,42 @@ static void >> kfd_dbg_clear_process_address_watch(struct kfd_process *target) >> kfd_dbg_trap_clear_dev_address_watch(target- >>> pdds[i], j); >> } >> >> -int kfd_dbg_trap_set_flags(struct kfd_process *target, uint32_t *flags) >> +int kfd_dbg_trap_set_flags(struct kfd_process *target, >> + struct kfd_ioctl_dbg_trap_set_flags_args *set_flags) >> { >> uint32_t prev_flags = target->dbg_flags; >> int i, r = 0, rewind_count = 0; >> >> + if (!((set_flags->flags ^ prev_flags) & set_flags->mask)) >> + return 0; >> + >> for (i = 0; i < target->n_pdds; i++) { >> struct kfd_topology_device *topo_dev = >> kfd_topology_device_by_id(target->pdds[i]- >>> dev->id); >> uint32_t caps = topo_dev->node_props.capability; >> uint32_t caps2 = topo_dev->node_props.capability2; >> + struct amdgpu_device *adev = target->pdds[i]->dev->adev; >> >> - if (!(caps & >> HSA_CAP_TRAP_DEBUG_PRECISE_MEMORY_OPERATIONS_SUPPORTED) && >> - (*flags & KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP)) { >> - *flags = prev_flags; >> - return -EACCES; >> - } >> - >> - if (!(caps & >> HSA_CAP_TRAP_DEBUG_PRECISE_ALU_OPERATIONS_SUPPORTED) && >> - (*flags & KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP)) { >> - *flags = prev_flags; >> - return -EACCES; >> - } >> - >> - if (!(caps2 & >> HSA_CAP2_TRAP_DEBUG_LDS_OUT_OF_ADDR_RANGE_SUPPORTED) && >> - (*flags & >> KFD_DBG_TRAP_FLAG_LDS_OUT_OF_ADDR_RANGE)) { >> - *flags = prev_flags; >> + if (set_flags->mask == 0xFFFFFFFF && !set_flags->flags) >> + break; >> + if ((!(caps & >> HSA_CAP_TRAP_DEBUG_PRECISE_MEMORY_OPERATIONS_SUPPORTED) && >> + (set_flags->mask & >> KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP)) || >> + (!(caps & >> HSA_CAP_TRAP_DEBUG_PRECISE_ALU_OPERATIONS_SUPPORTED) && >> + (set_flags->mask & KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP)) >> || >> + (!(caps2 & >> HSA_CAP2_TRAP_DEBUG_LDS_OUT_OF_ADDR_RANGE_SUPPORTED) && >> + (set_flags->mask & >> KFD_DBG_TRAP_FLAG_LDS_OUT_OF_ADDR_RANGE))) { >> + set_flags->flags = prev_flags; >> + dev_dbg(adev->dev, "Debug Trap set mask 0x%x caps >> 0x%x caps2 0x%x", >> + set_flags->mask, caps, caps2); > The logic here seems odd. If "set_flags->mask" is the set of flags a caller wants to modify (I don't think it should, see below), a call which asks for PRECISE_MEMORY to be disabled would return EACCESS if PRECISE_MEMORY is not supported. Before this patch, it would have been perfectly valid set (or confirm) the PRECISE_MEMORY flag to 0. > [JZ] the logical here is checking input setting, if caps does not support such feature, mask trys to set this feature bit, then exit with -EACCESS. > Also, consider the following scenario where a user wants to set PRECISE_ALU=0 and PRECISE_MEMORY=1: > > set_flags->flags = KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP; > set_flags->mask = KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP | KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP; > > If the platform only supports PRECISE_MEMORY (like gfx11 for example), the call would erroneously fail with EACCESS. This was possible before this patch. [JZ] then mask should not include KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP which tells user want to clear KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP on this ASIC, but it actually not been supported. err tells user that the operation clearing PRECISE_ALU is invalid. > >> return -EACCES; >> } >> } >> >> - target->dbg_flags = *flags; >> - *flags = prev_flags; >> + target->dbg_flags ^= (target->dbg_flags ^ set_flags->flags) & set_flags- >>> mask; >> + pr_debug("Debug Trap previous flags 0x%x set flags 0x%x set mask >> 0x%x target flags 0x%x", >> + prev_flags, set_flags->flags, set_flags->mask, target->dbg_flags); >> + >> + set_flags->flags = prev_flags; >> for (i = 0; i < target->n_pdds; i++) { >> struct kfd_process_device *pdd = target->pdds[i]; >> >> @@ -555,10 +559,8 @@ int kfd_dbg_trap_set_flags(struct kfd_process *target, >> uint32_t *flags) >> else >> r = kfd_dbg_set_mes_debug_mode(pdd, true); >> >> - if (r) { >> - target->dbg_flags = prev_flags; >> + if (r) >> break; >> - } >> >> rewind_count++; >> } >> @@ -596,7 +598,7 @@ void kfd_dbg_trap_deactivate(struct kfd_process >> *target, bool unwind, int unwind >> int i; >> >> if (!unwind) { >> - uint32_t flags = 0; >> + struct kfd_ioctl_dbg_trap_set_flags_args set_flags = {0, >> 0xFFFFFFFF}; >> int resume_count = resume_queues(target, 0, NULL); >> >> if (resume_count) >> @@ -606,7 +608,7 @@ void kfd_dbg_trap_deactivate(struct kfd_process >> *target, bool unwind, int unwind >> kfd_dbg_clear_process_address_watch(target); >> kfd_dbg_trap_set_wave_launch_mode(target, 0); >> >> - kfd_dbg_trap_set_flags(target, &flags); >> + kfd_dbg_trap_set_flags(target, &set_flags); >> } >> >> for (i = 0; i < target->n_pdds; i++) { >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.h >> b/drivers/gpu/drm/amd/amdkfd/kfd_debug.h >> index 894753818cba..34d27eb500a5 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.h >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.h >> @@ -62,7 +62,8 @@ int kfd_dbg_trap_set_dev_address_watch(struct >> kfd_process_device *pdd, >> uint32_t watch_address_mask, >> uint32_t *watch_id, >> uint32_t watch_mode); >> -int kfd_dbg_trap_set_flags(struct kfd_process *target, uint32_t *flags); >> +int kfd_dbg_trap_set_flags(struct kfd_process *target, >> + struct kfd_ioctl_dbg_trap_set_flags_args *set_flags); >> int kfd_dbg_trap_query_exception_info(struct kfd_process *target, >> uint32_t source_id, >> uint32_t exception_code, >> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h >> index e9b756ca228c..0522fe7344e4 100644 >> --- a/include/uapi/linux/kfd_ioctl.h >> +++ b/include/uapi/linux/kfd_ioctl.h >> @@ -1515,6 +1515,7 @@ struct >> kfd_ioctl_dbg_trap_clear_node_address_watch_args { >> * Sets flags for wave behaviour. >> * >> * @flags (IN/OUT) - IN = flags to enable, OUT = flags previously enabled >> + * @mask (IN) - IN = specified debug trap operation bits on flag > I am not sure I fully understand this comment. Is it meant to say which bits from FLAGS will be set? > > Also, this seems to break backwards compatibility. Before this patch, a caller would call this ioctl with "flags" set, and the PAD field would be 0. The semantics of this call is to set all bits from flags. The updated semantics of MASK would make it so no flags are set instead of alle. [JZ] Jon has pointed out this. I have a new version to fix it. I will add below code fallback to old version. if (!set_flags->mask) set_flags->mask = set_flags->flags; > > The updated ioctl must preserve this existing pattern, so instead of mask telling which bits must be changed, it should specify which bits should be preserved (or ignored from FLAGS). > > To make it clearer, I'd probably call this "preserve_mask" [JZ] to use mask to tell which bits in flag will be modified seem more straightforward. > > @preserve_mask (IN) - IN = Specify flags which must not be modified. > > Best, > Lancelot. > >> * >> * Generic errors apply (see kfd_dbg_trap_operations). >> * Return - 0 on SUCCESS. >> @@ -1522,7 +1523,7 @@ struct >> kfd_ioctl_dbg_trap_clear_node_address_watch_args { >> */ >> struct kfd_ioctl_dbg_trap_set_flags_args { >> __u32 flags; >> - __u32 pad; >> + __u32 mask; >> }; >> >> /** >> -- >> 2.34.1 [-- Attachment #2: Type: text/html, Size: 12482 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amdkfd: add mask for debug trap flag setting 2026-01-20 14:39 ` James Zhu @ 2026-01-20 14:52 ` Lancelot SIX 0 siblings, 0 replies; 6+ messages in thread From: Lancelot SIX @ 2026-01-20 14:52 UTC (permalink / raw) To: James Zhu, Zhu, James, amd-gfx@lists.freedesktop.org; +Cc: Kim, Jonathan On 20/01/2026 14:39, James Zhu wrote: > > On 2026-01-20 05:34, Six, Lancelot wrote: >> [AMD Official Use Only - AMD Internal Distribution Only] >> >> Hi, >> >> >>> -----Original Message----- >>> From: Zhu, James<James.Zhu@amd.com> >>> Sent: 19 January 2026 17:15 >>> To:amd-gfx@lists.freedesktop.org >>> Cc: Six, Lancelot<Lancelot.Six@amd.com>; Kim, Jonathan >>> <Jonathan.Kim@amd.com>; Zhu, James<James.Zhu@amd.com> >>> Subject: [PATCH] drm/amdkfd: add mask for debug trap flag setting >>> >>> to specify which bits are valid setting on flags. >>> >>> Signed-off-by: James Zhu<James.Zhu@amd.com> >>> --- >>> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +- >>> drivers/gpu/drm/amd/amdkfd/kfd_debug.c | 48 ++++++++++++------------ >>> drivers/gpu/drm/amd/amdkfd/kfd_debug.h | 3 +- >>> include/uapi/linux/kfd_ioctl.h | 3 +- >>> 4 files changed, 30 insertions(+), 26 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>> index 79586abad7fd..fd43ef7c9076 100644 >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>> @@ -3377,7 +3377,7 @@ static int kfd_ioctl_set_debug_trap(struct file *filep, >>> struct kfd_process *p, v >>> args->clear_node_address_watch.id); >>> break; >>> case KFD_IOC_DBG_TRAP_SET_FLAGS: >>> - r = kfd_dbg_trap_set_flags(target, &args->set_flags.flags); >>> + r = kfd_dbg_trap_set_flags(target, &args->set_flags); >>> break; >>> case KFD_IOC_DBG_TRAP_QUERY_DEBUG_EVENT: >>> r = kfd_dbg_ev_query_debug_event(target, >>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c >>> b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c >>> index a04875236647..279160ca71a9 100644 >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c >>> @@ -512,38 +512,42 @@ static void >>> kfd_dbg_clear_process_address_watch(struct kfd_process *target) >>> kfd_dbg_trap_clear_dev_address_watch(target- >>>> pdds[i], j); >>> } >>> >>> -int kfd_dbg_trap_set_flags(struct kfd_process *target, uint32_t *flags) >>> +int kfd_dbg_trap_set_flags(struct kfd_process *target, >>> + struct kfd_ioctl_dbg_trap_set_flags_args *set_flags) >>> { >>> uint32_t prev_flags = target->dbg_flags; >>> int i, r = 0, rewind_count = 0; >>> >>> + if (!((set_flags->flags ^ prev_flags) & set_flags->mask)) >>> + return 0; >>> + >>> for (i = 0; i < target->n_pdds; i++) { >>> struct kfd_topology_device *topo_dev = >>> kfd_topology_device_by_id(target->pdds[i]- >>>> dev->id); >>> uint32_t caps = topo_dev->node_props.capability; >>> uint32_t caps2 = topo_dev->node_props.capability2; >>> + struct amdgpu_device *adev = target->pdds[i]->dev->adev; >>> >>> - if (!(caps & >>> HSA_CAP_TRAP_DEBUG_PRECISE_MEMORY_OPERATIONS_SUPPORTED) && >>> - (*flags & KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP)) { >>> - *flags = prev_flags; >>> - return -EACCES; >>> - } >>> - >>> - if (!(caps & >>> HSA_CAP_TRAP_DEBUG_PRECISE_ALU_OPERATIONS_SUPPORTED) && >>> - (*flags & KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP)) { >>> - *flags = prev_flags; >>> - return -EACCES; >>> - } >>> - >>> - if (!(caps2 & >>> HSA_CAP2_TRAP_DEBUG_LDS_OUT_OF_ADDR_RANGE_SUPPORTED) && >>> - (*flags & >>> KFD_DBG_TRAP_FLAG_LDS_OUT_OF_ADDR_RANGE)) { >>> - *flags = prev_flags; >>> + if (set_flags->mask == 0xFFFFFFFF && !set_flags->flags) >>> + break; >>> + if ((!(caps & >>> HSA_CAP_TRAP_DEBUG_PRECISE_MEMORY_OPERATIONS_SUPPORTED) && >>> + (set_flags->mask & >>> KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP)) || >>> + (!(caps & >>> HSA_CAP_TRAP_DEBUG_PRECISE_ALU_OPERATIONS_SUPPORTED) && >>> + (set_flags->mask & KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP)) >>> || >>> + (!(caps2 & >>> HSA_CAP2_TRAP_DEBUG_LDS_OUT_OF_ADDR_RANGE_SUPPORTED) && >>> + (set_flags->mask & >>> KFD_DBG_TRAP_FLAG_LDS_OUT_OF_ADDR_RANGE))) { >>> + set_flags->flags = prev_flags; >>> + dev_dbg(adev->dev, "Debug Trap set mask 0x%x caps >>> 0x%x caps2 0x%x", >>> + set_flags->mask, caps, caps2); >> The logic here seems odd. If "set_flags->mask" is the set of flags a caller wants to modify (I don't think it should, see below), a call which asks for PRECISE_MEMORY to be disabled would return EACCESS if PRECISE_MEMORY is not supported. Before this patch, it would have been perfectly valid set (or confirm) the PRECISE_MEMORY flag to 0. >> [JZ] the logical here is checking input setting, if caps does not support such feature, mask trys to set this feature bit, then exit with -EACCESS. >> Also, consider the following scenario where a user wants to set PRECISE_ALU=0 and PRECISE_MEMORY=1: >> >> set_flags->flags = KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP; >> set_flags->mask = KFD_DBG_TRAP_FLAG_SINGLE_MEM_OP | KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP; >> >> If the platform only supports PRECISE_MEMORY (like gfx11 for example), the call would erroneously fail with EACCESS. This was possible before this patch. > > [JZ] then mask should not include KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP which > tells user want to clear KFD_DBG_TRAP_FLAG_SINGLE_ALU_OP on this > > ASIC, but it actually not been supported. err tells user that the > operation clearing PRECISE_ALU is invalid. See below. > >>> return -EACCES; >>> } >>> } >>> >>> - target->dbg_flags = *flags; >>> - *flags = prev_flags; >>> + target->dbg_flags ^= (target->dbg_flags ^ set_flags->flags) & set_flags- >>>> mask; >>> + pr_debug("Debug Trap previous flags 0x%x set flags 0x%x set mask >>> 0x%x target flags 0x%x", >>> + prev_flags, set_flags->flags, set_flags->mask, target->dbg_flags); >>> + >>> + set_flags->flags = prev_flags; >>> for (i = 0; i < target->n_pdds; i++) { >>> struct kfd_process_device *pdd = target->pdds[i]; >>> >>> @@ -555,10 +559,8 @@ int kfd_dbg_trap_set_flags(struct kfd_process *target, >>> uint32_t *flags) >>> else >>> r = kfd_dbg_set_mes_debug_mode(pdd, true); >>> >>> - if (r) { >>> - target->dbg_flags = prev_flags; >>> + if (r) >>> break; >>> - } >>> >>> rewind_count++; >>> } >>> @@ -596,7 +598,7 @@ void kfd_dbg_trap_deactivate(struct kfd_process >>> *target, bool unwind, int unwind >>> int i; >>> >>> if (!unwind) { >>> - uint32_t flags = 0; >>> + struct kfd_ioctl_dbg_trap_set_flags_args set_flags = {0, >>> 0xFFFFFFFF}; >>> int resume_count = resume_queues(target, 0, NULL); >>> >>> if (resume_count) >>> @@ -606,7 +608,7 @@ void kfd_dbg_trap_deactivate(struct kfd_process >>> *target, bool unwind, int unwind >>> kfd_dbg_clear_process_address_watch(target); >>> kfd_dbg_trap_set_wave_launch_mode(target, 0); >>> >>> - kfd_dbg_trap_set_flags(target, &flags); >>> + kfd_dbg_trap_set_flags(target, &set_flags); >>> } >>> >>> for (i = 0; i < target->n_pdds; i++) { >>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.h >>> b/drivers/gpu/drm/amd/amdkfd/kfd_debug.h >>> index 894753818cba..34d27eb500a5 100644 >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.h >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.h >>> @@ -62,7 +62,8 @@ int kfd_dbg_trap_set_dev_address_watch(struct >>> kfd_process_device *pdd, >>> uint32_t watch_address_mask, >>> uint32_t *watch_id, >>> uint32_t watch_mode); >>> -int kfd_dbg_trap_set_flags(struct kfd_process *target, uint32_t *flags); >>> +int kfd_dbg_trap_set_flags(struct kfd_process *target, >>> + struct kfd_ioctl_dbg_trap_set_flags_args *set_flags); >>> int kfd_dbg_trap_query_exception_info(struct kfd_process *target, >>> uint32_t source_id, >>> uint32_t exception_code, >>> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h >>> index e9b756ca228c..0522fe7344e4 100644 >>> --- a/include/uapi/linux/kfd_ioctl.h >>> +++ b/include/uapi/linux/kfd_ioctl.h >>> @@ -1515,6 +1515,7 @@ struct >>> kfd_ioctl_dbg_trap_clear_node_address_watch_args { >>> * Sets flags for wave behaviour. >>> * >>> * @flags (IN/OUT) - IN = flags to enable, OUT = flags previously enabled >>> + * @mask (IN) - IN = specified debug trap operation bits on flag >> I am not sure I fully understand this comment. Is it meant to say which bits from FLAGS will be set? >> >> Also, this seems to break backwards compatibility. Before this patch, a caller would call this ioctl with "flags" set, and the PAD field would be 0. The semantics of this call is to set all bits from flags. The updated semantics of MASK would make it so no flags are set instead of alle. > [JZ] Jon has pointed out this. I have a new version to fix it. I will > add below code fallback to old version. > if (!set_flags->mask) > set_flags->mask = set_flags->flags; > >> The updated ioctl must preserve this existing pattern, so instead of mask telling which bits must be changed, it should specify which bits should be preserved (or ignored from FLAGS). >> >> To make it clearer, I'd probably call this "preserve_mask" > [JZ] to use mask to tell which bits in flag will be modified seem more > straightforward. I agree, but I don't think this is relevant. There are clients today using this ioctl, which 0-initialize the bits used to hold MASK. If you change the meaning of those bits, this breaks backwards compatibility, and this is a no-go. I see 2 options: - add the MASK field after the existing PAD (effectively extend the struct). - Re-use PAD's bit to hold the mask, in which case we need to ensure that old patterns contiune to work as they do today: setting 0s means to replace all flags, and it is valid to ask clear flags which are not supported for the current platform. I don't have a strong opinion either way, just need to make sure old clients continue to work once this patch lands. Best, Lancelot. >> @preserve_mask (IN) - IN = Specify flags which must not be modified. >> >> Best, >> Lancelot. >> >>> * >>> * Generic errors apply (see kfd_dbg_trap_operations). >>> * Return - 0 on SUCCESS. >>> @@ -1522,7 +1523,7 @@ struct >>> kfd_ioctl_dbg_trap_clear_node_address_watch_args { >>> */ >>> struct kfd_ioctl_dbg_trap_set_flags_args { >>> __u32 flags; >>> - __u32 pad; >>> + __u32 mask; >>> }; >>> >>> /** >>> -- >>> 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-01-20 14:52 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-01-19 17:15 [PATCH] drm/amdkfd: add mask for debug trap flag setting James Zhu 2026-01-19 19:28 ` Kim, Jonathan 2026-01-19 20:57 ` James Zhu 2026-01-20 10:34 ` Six, Lancelot 2026-01-20 14:39 ` James Zhu 2026-01-20 14:52 ` Lancelot SIX
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox