[PATCH v2] drm/amdkfd: Ignored various return code
Martin, Andrew
Andrew.Martin at amd.com
Wed Feb 12 17:58:35 UTC 2025
[Public]
Greetings, sending peace.
Still seeking a review on this. These changes are simple and should not take much of your time, so please send me a "Reviewed By:" or your comments.
One love!
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Martin,
> Andrew
> Sent: Monday, December 30, 2024 2:10 PM
> To: Russell, Kent <Kent.Russell at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Kuehling, Felix <Felix.Kuehling at amd.com>
> Subject: RE: [PATCH v2] drm/amdkfd: Ignored various return code
>
> [Public]
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> [Public]
>
> @Russell, Kent
>
> Thanks, I will send out the next version soon!
>
> > -----Original Message-----
> > From: Russell, Kent <Kent.Russell at amd.com>
> > Sent: Monday, December 30, 2024 1:37 PM
> > To: Martin, Andrew <Andrew.Martin at amd.com>; amd-
> > gfx at lists.freedesktop.org
> > Cc: Kuehling, Felix <Felix.Kuehling at amd.com>
> > Subject: RE: [PATCH v2] drm/amdkfd: Ignored various return code
> >
> > [Public]
> >
> > > -----Original Message-----
> > > From: Martin, Andrew <Andrew.Martin at amd.com>
> > > Sent: Monday, December 30, 2024 1:15 PM
> > > To: Russell, Kent <Kent.Russell at amd.com>;
> > > amd-gfx at lists.freedesktop.org
> > > Cc: Kuehling, Felix <Felix.Kuehling at amd.com>
> > > Subject: RE: [PATCH v2] drm/amdkfd: Ignored various return code
> > >
> > > [Public]
> > >
> > > > -----Original Message-----
> > > > From: Russell, Kent <Kent.Russell at amd.com>
> > > > Sent: Monday, December 30, 2024 12:44 PM
> > > > To: Martin, Andrew <Andrew.Martin at amd.com>; amd-
> > > > gfx at lists.freedesktop.org
> > > > Cc: Kuehling, Felix <Felix.Kuehling at amd.com>; Martin, Andrew
> > > > <Andrew.Martin at amd.com>
> > > > Subject: RE: [PATCH v2] drm/amdkfd: Ignored various return code
> > > >
> > > > [Public]
> > > >
> > > > Might be worth changing the commit headline to what the patch
> > > > does, something like Don't ignore return codes Or Various return
> > > > code cleanup
> > > >
> > > > Otherwise it sounds like your patch is intentionally ignoring
> > > > return codes, which is the opposite of what you're doing. Also 2
> > > > comments
> > below.
> > >
> > > Thanks, will do!, Please see response to your 2 comments also!
> > >
> > > >
> > > > > -----Original Message-----
> > > > > From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf
> > > > > Of Andrew Martin
> > > > > Sent: Monday, December 30, 2024 12:02 PM
> > > > > To: amd-gfx at lists.freedesktop.org
> > > > > Cc: Kuehling, Felix <Felix.Kuehling at amd.com>; Russell, Kent
> > > > > <Kent.Russell at amd.com>; Martin, Andrew
> > <Andrew.Martin at amd.com>
> > > > > Subject: [PATCH v2] drm/amdkfd: Ignored various return code
> > > > >
> > > > > This patch checks and handles the return code of the called function.
> > > > >
> > > > > Signed-off-by: Andrew Martin <Andrew.Martin at amd.com>
> > > > > ---
> > > > > drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +-
> > > > > drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 2 ++
> > > > > drivers/gpu/drm/amd/amdkfd/kfd_debug.c | 19 ++++++++++++++---
> > --
> > > > > .../drm/amd/amdkfd/kfd_device_queue_manager.c | 11 ++++++-----
> > > > > 4 files changed, 23 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > > > > b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > > > > index 065d87841459..52a11dc01422 100644
> > > > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > > > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > > > > @@ -2864,7 +2864,7 @@ static int runtime_disable(struct
> > > > > kfd_process *p)
> > > > >
> > > > > pdd->dev->vm_info.last_vmid_kfd);
> > > > >
> > > > > if (!pdd->dev->kfd->shared_resources.enable_mes)
> > > > > - debug_refresh_runlist(pdd->dev->dqm);
> > > > > +
> > > > > + (void)debug_refresh_runlist(pdd->dev->dqm);
> > > > > else
> > > > > kfd_dbg_set_mes_debug_mode(pdd,
> > > > >
> > > > > !kfd_dbg_has_cwsr_workaround(pdd->dev));
> > > > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > > > > b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > > > > index 693469c18c60..f335ed9e0b3a 100644
> > > > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > > > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > > > > @@ -2351,6 +2351,8 @@ static int kfd_create_vcrat_image_gpu(void
> > > > *pcrat_image,
> > > > > if (kdev->kfd->hive_id) {
> > > > > for (nid = 0; nid < proximity_domain; ++nid) {
> > > > > peer_dev =
> > > > > kfd_topology_device_by_proximity_domain_no_lock(nid);
> > > > > + if (!peer_dev)
> > > > > + return -ENODEV;
> > > > > if (!peer_dev->gpu)
> > > > > continue;
> > > > > if (peer_dev->gpu->kfd->hive_id !=
> > > > > kdev->kfd->hive_id) diff --git
> > > > > a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
> > > > > b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
> > > > > index a8abc3091801..b4db409d90a1 100644
> > > > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
> > > > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
> > > > > @@ -518,6 +518,9 @@ int kfd_dbg_trap_set_flags(struct
> > > > > kfd_process *target, uint32_t *flags)
> > > > > for (i = 0; i < target->n_pdds; i++) {
> > > > > struct kfd_topology_device *topo_dev =
> > > > >
> > > > > kfd_topology_device_by_id(target->pdds[i]->dev->id);
> > > > > + if (!topo_dev)
> > > > > + return -EINVAL;
> > > > > +
> > > > > uint32_t caps = topo_dev->node_props.capability;
> > > > >
> > > > > if (!(caps &
> > > > >
> > > HSA_CAP_TRAP_DEBUG_PRECISE_MEMORY_OPERATIONS_SUPPORTED)
> > > > > &&
> > > > > @@ -565,9 +568,9 @@ int kfd_dbg_trap_set_flags(struct
> > > > > kfd_process *target, uint32_t *flags)
> > > > > continue;
> > > > >
> > > > > if (!pdd->dev->kfd->shared_resources.enable_mes)
> > > > > - debug_refresh_runlist(pdd->dev->dqm);
> > > > > +
> > > > > + (void)debug_refresh_runlist(pdd->dev->dqm);
> > > > > else
> > > > > - kfd_dbg_set_mes_debug_mode(pdd, true);
> > > > > +
> > > > > + (void)kfd_dbg_set_mes_debug_mode(pdd,
> > > > > + true);
> > > > > }
> > > > > }
> > > > >
> > > > > @@ -584,8 +587,8 @@ int kfd_dbg_trap_set_flags(struct
> > > > > kfd_process *target, uint32_t *flags)
> > > > > */
> > > > > void kfd_dbg_trap_deactivate(struct kfd_process *target, bool
> > > > > unwind, int
> > > > > unwind_count)
> > > > > {
> > > > > - int i;
> > > > >
> > > > > + int i, r = 0;
> > > > > if (!unwind) {
> > > > > uint32_t flags = 0;
> > > > > int resume_count = resume_queues(target, 0, NULL);
> > > > > @@
> > > > > -627,9 +630,11 @@ void kfd_dbg_trap_deactivate(struct
> > > > > kfd_process *target, bool unwind, int unwind
> > > > > pr_err("Failed to release debug vmid on
> > > > > [%i]\n",
> > > > > pdd->dev-
> > > > > >id);
> > > > >
> > > > > if (!pdd->dev->kfd->shared_resources.enable_mes)
> > > > > - debug_refresh_runlist(pdd->dev->dqm);
> > > > > + r = debug_refresh_runlist(pdd->dev->dqm);
> > > > > else
> > > > > - kfd_dbg_set_mes_debug_mode(pdd,
> > > > > !kfd_dbg_has_cwsr_workaround(pdd->dev));
> > > > > + r = kfd_dbg_set_mes_debug_mode(pdd,
> > > > > !kfd_dbg_has_cwsr_workaround(pdd->dev));
> > > > > + if (r)
> > > > > + break;
> > > > > }
> > > > >
> > > > > kfd_dbg_set_workaround(target, false); @@ -1071,6 +1076,10
> > > > > @@ int kfd_dbg_trap_device_snapshot(struct kfd_process *target,
> > > > > for (i = 0; i < tmp_num_devices; i++) {
> > > > > struct kfd_process_device *pdd = target->pdds[i];
> > > > > struct kfd_topology_device *topo_dev =
> > > > > kfd_topology_device_by_id(pdd->dev->id);
> > > > > + if (!topo_dev) {
> > > > > + r = EINVAL;
> > > > > + break;
> > > > > + }
> > > > >
> > > > > device_info.gpu_id = pdd->dev->id;
> > > > > device_info.exception_status =
> > > > > pdd->exception_status; diff --git
> > > > > a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > > > > b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > > > > index 1405e8affd48..250aa43a39c8 100644
> > > > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > > > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > > > > @@ -1907,16 +1907,18 @@ static int start_cpsch(struct
> > > > > device_queue_manager
> > > > > *dqm)
> > > > >
> > > > > static int stop_cpsch(struct device_queue_manager *dqm) {
> > > > > + int ret = 0;
> > > > > dqm_lock(dqm);
> > > > > if (!dqm->sched_running) {
> > > > > dqm_unlock(dqm);
> > > > > - return 0;
> > > > > + return ret;
> > >
> > > I can agree with this one, since it minimizes the changes, but it
> > > does flow
> > the same.
> > >
> > > >
> > > > I think it's cleaner to just leave the "return 0" . e.g. If the
> > > > code gets refactored and new stuff gets added above, then this
> > > > might flow
> > differently.
> > > >
> > > >
> > > > > }
> > > > >
> > > > > if (!dqm->dev->kfd->shared_resources.enable_mes)
> > > > > - unmap_queues_cpsch(dqm,
> > > > > KFD_UNMAP_QUEUES_FILTER_ALL_QUEUES, 0,
> > > > USE_DEFAULT_GRACE_PERIOD,
> > > > > false);
> > > > > + ret = unmap_queues_cpsch(dqm,
> > > > > KFD_UNMAP_QUEUES_FILTER_ALL_QUEUES,
> > > > > + 0,
> > > > > + USE_DEFAULT_GRACE_PERIOD, false);
> > > > > else
> > > > > - remove_all_kfd_queues_mes(dqm);
> > > > > + ret = remove_all_kfd_queues_mes(dqm);
> > > > >
> > > > > dqm->sched_running = false;
> > > > >
> > > > > @@ -1930,7 +1932,7 @@ static int stop_cpsch(struct
> > > > > device_queue_manager
> > > > > *dqm)
> > > > > dqm->detect_hang_info = NULL;
> > > > > dqm_unlock(dqm);
> > > > >
> > > > > - return 0;
> > > > > + return ret;
> > > >
> > > > Same here
> > > >
> > >
> > > If I change this back to "return 0 ;" then I would have lost the
> > > return value from "unmap_queues_cpsch()" or
> > > "remove_all_kfd_queues_mes()" which was the whole point of touching
> > > this function. But since we in the "stop_cpch()" perhaps it is a
> > > more optimal solution is to ignore the return code with ...
> > > (void)unmap_queues_cpsch()
> > > else
> > > (void)remove_all_kfd_queues_mes()
> > > ...
> > >
> > > Which would be the only change here. Otherwise, I would need to
> > > introduce a 3rd return statement. So do let me know which path
> > complements the design more.
> > >
> > Good point, I missed that it would drop the one from above. That's
> > fine to leave in then.
> >
> > Kent
> >
> > > > Kent
> > > >
> > > > > }
> > > > >
> > > > > static int create_kernel_queue_cpsch(struct
> > > > > device_queue_manager *dqm, @@ -3439,7 +3441,6 @@ int
> > > > > suspend_queues(struct kfd_process
> > *p,
> > > > > else
> > > > > per_device_suspended++;
> > > > > } else if (err != -EBUSY) {
> > > > > - r = err;
> > > > > queue_ids[q_idx] |=
> > > > > KFD_DBG_QUEUE_ERROR_MASK;
> > > > > break;
> > > > > }
> > > > > --
> > > > > 2.43.0
> > > >
> > >
> >
More information about the amd-gfx
mailing list