Re: [PATCH v2 15/30] drm/xe/devcoredump: Use scope-based cleanup

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Matt Roper (2025-11-10 20:20:33-03:00)
>Use scope-based cleanup for forcewake and runtime PM in the devcoredump
>code.  This eliminates some goto-based error handling and slightly
>simplifies other functions.
>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>---
> drivers/gpu/drm/xe/xe_devcoredump.c | 26 ++++++++++----------------
> 1 file changed, 10 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
>index 203e3038cc81..599c886c865b 100644
>--- a/drivers/gpu/drm/xe/xe_devcoredump.c
>+++ b/drivers/gpu/drm/xe/xe_devcoredump.c
>@@ -276,7 +276,6 @@ static void xe_devcoredump_deferred_snap_work(struct work_struct *work)
>         struct xe_devcoredump_snapshot *ss = container_of(work, typeof(*ss), work);
>         struct xe_devcoredump *coredump = container_of(ss, typeof(*coredump), snapshot);
>         struct xe_device *xe = coredump_to_xe(coredump);
>-        unsigned int fw_ref;
> 
>         /*
>          * NB: Despite passing a GFP_ flags parameter here, more allocations are done
>@@ -287,15 +286,15 @@ static void xe_devcoredump_deferred_snap_work(struct work_struct *work)
>                               xe_devcoredump_read, xe_devcoredump_free,
>                               XE_COREDUMP_TIMEOUT_JIFFIES);
> 
>-        xe_pm_runtime_get(xe);
>+        guard(xe_pm_runtime)(xe);
> 
>         /* keep going if fw fails as we still want to save the memory and SW data */
>-        fw_ref = xe_force_wake_get(gt_to_fw(ss->gt), XE_FORCEWAKE_ALL);
>-        if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL))
>-                xe_gt_info(ss->gt, "failed to get forcewake for coredump capture\n");
>-        xe_vm_snapshot_capture_delayed(ss->vm);
>-        xe_guc_exec_queue_snapshot_capture_delayed(ss->ge);
>-        xe_force_wake_put(gt_to_fw(ss->gt), fw_ref);
>+        xe_with_force_wake(fw_ref, gt_to_fw(ss->gt), XE_FORCEWAKE_ALL) {
>+                if (!xe_force_wake_ref_has_domain(fw_ref.domains, XE_FORCEWAKE_ALL))
>+                        xe_gt_info(ss->gt, "failed to get forcewake for coredump capture\n");
>+                xe_vm_snapshot_capture_delayed(ss->vm);
>+                xe_guc_exec_queue_snapshot_capture_delayed(ss->ge);
>+        }
> 
>         ss->read.chunk_position = 0;
> 
>@@ -306,7 +305,7 @@ static void xe_devcoredump_deferred_snap_work(struct work_struct *work)
>                 ss->read.buffer = kvmalloc(XE_DEVCOREDUMP_CHUNK_MAX,
>                                            GFP_USER);
>                 if (!ss->read.buffer)
>-                        goto put_pm;
>+                        return;
> 
>                 __xe_devcoredump_read(ss->read.buffer,
>                                       XE_DEVCOREDUMP_CHUNK_MAX,
>@@ -314,15 +313,12 @@ static void xe_devcoredump_deferred_snap_work(struct work_struct *work)
>         } else {
>                 ss->read.buffer = kvmalloc(ss->read.size, GFP_USER);
>                 if (!ss->read.buffer)
>-                        goto put_pm;
>+                        return;
> 
>                 __xe_devcoredump_read(ss->read.buffer, ss->read.size, 0,
>                                       coredump);
>                 xe_devcoredump_snapshot_free(ss);
>         }
>-
>-put_pm:
>-        xe_pm_runtime_put(xe);
> }
> 
> static void devcoredump_snapshot(struct xe_devcoredump *coredump,
>@@ -332,7 +328,6 @@ static void devcoredump_snapshot(struct xe_devcoredump *coredump,
>         struct xe_devcoredump_snapshot *ss = &coredump->snapshot;
>         struct xe_guc *guc = exec_queue_to_guc(q);
>         const char *process_name = "no process";
>-        unsigned int fw_ref;
>         bool cookie;
> 
>         ss->snapshot_time = ktime_get_real();
>@@ -351,7 +346,7 @@ static void devcoredump_snapshot(struct xe_devcoredump *coredump,
>         cookie = dma_fence_begin_signalling();
> 
>         /* keep going if fw fails as we still want to save the memory and SW data */
>-        fw_ref = xe_force_wake_get(gt_to_fw(q->gt), XE_FORCEWAKE_ALL);
>+        CLASS(xe_force_wake, fw_ref)(gt_to_fw(q->gt), XE_FORCEWAKE_ALL);

Should we move this to happen before dma_fence_begin_signalling(), just
so we keep the LIFO cleanup order at the end of the function?

--
Gustavo Sousa

> 
>         ss->guc.log = xe_guc_log_snapshot_capture(&guc->log, true);
>         ss->guc.ct = xe_guc_ct_snapshot_capture(&guc->ct);
>@@ -364,7 +359,6 @@ static void devcoredump_snapshot(struct xe_devcoredump *coredump,
> 
>         queue_work(system_unbound_wq, &ss->work);
> 
>-        xe_force_wake_put(gt_to_fw(q->gt), fw_ref);
>         dma_fence_end_signalling(cookie);
> }
> 
>-- 
>2.51.1
>