Re: [PATCH v2 06/30] drm/xe/gt: Use scope-based cleanup

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

Quoting Matt Roper (2025-11-10 20:20:24-03:00)
>Using scope-based cleanup for forcewake and runtime PM allows us to
>reduce or eliminate some of the goto-based error handling and simplify
>several functions.
>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

I provided some suggestions below.  With or without them,

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

>---
> drivers/gpu/drm/xe/xe_gt.c | 151 ++++++++++++-------------------------
> 1 file changed, 48 insertions(+), 103 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>index 6d479948bf21..e81674c40e57 100644
>--- a/drivers/gpu/drm/xe/xe_gt.c
>+++ b/drivers/gpu/drm/xe/xe_gt.c
>@@ -103,14 +103,13 @@ void xe_gt_sanitize(struct xe_gt *gt)
> 
> static void xe_gt_enable_host_l2_vram(struct xe_gt *gt)
> {
>-        unsigned int fw_ref;
>         u32 reg;
> 
>         if (!XE_GT_WA(gt, 16023588340))
>                 return;
> 
>-        fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
>-        if (!fw_ref)
>+        CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FW_GT);
>+        if (!fw_ref.domains)
>                 return;
> 
>         if (xe_gt_is_main_type(gt)) {
>@@ -120,12 +119,10 @@ static void xe_gt_enable_host_l2_vram(struct xe_gt *gt)
>         }
> 
>         xe_gt_mcr_multicast_write(gt, XEHPC_L3CLOS_MASK(3), 0xF);
>-        xe_force_wake_put(gt_to_fw(gt), fw_ref);
> }
> 
> static void xe_gt_disable_host_l2_vram(struct xe_gt *gt)
> {
>-        unsigned int fw_ref;
>         u32 reg;
> 
>         if (!XE_GT_WA(gt, 16023588340))
>@@ -134,15 +131,13 @@ static void xe_gt_disable_host_l2_vram(struct xe_gt *gt)
>         if (xe_gt_is_media_type(gt))
>                 return;
> 
>-        fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
>-        if (!fw_ref)
>+        CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FW_GT);
>+        if (!fw_ref.domains)
>                 return;
> 
>         reg = xe_gt_mcr_unicast_read_any(gt, XE2_GAMREQSTRM_CTRL);
>         reg &= ~CG_DIS_CNTLBUS;
>         xe_gt_mcr_multicast_write(gt, XE2_GAMREQSTRM_CTRL, reg);
>-
>-        xe_force_wake_put(gt_to_fw(gt), fw_ref);
> }
> 
> static void gt_reset_worker(struct work_struct *w);
>@@ -389,7 +384,6 @@ int xe_gt_record_default_lrcs(struct xe_gt *gt)
> 
> int xe_gt_init_early(struct xe_gt *gt)
> {
>-        unsigned int fw_ref;
>         int err;
> 
>         if (IS_SRIOV_PF(gt_to_xe(gt))) {
>@@ -436,13 +430,12 @@ int xe_gt_init_early(struct xe_gt *gt)
>         if (err)
>                 return err;
> 
>-        fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
>-        if (!fw_ref)
>+        CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FW_GT);
>+        if (!fw_ref.domains)
>                 return -ETIMEDOUT;
> 
>         xe_gt_mcr_init_early(gt);
>         xe_pat_init(gt);
>-        xe_force_wake_put(gt_to_fw(gt), fw_ref);
> 
>         return 0;
> }
>@@ -460,16 +453,15 @@ static void dump_pat_on_error(struct xe_gt *gt)
> 
> static int gt_init_with_gt_forcewake(struct xe_gt *gt)
> {
>-        unsigned int fw_ref;
>         int err;
> 
>-        fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
>-        if (!fw_ref)
>+        CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FW_GT);
>+        if (!fw_ref.domains)
>                 return -ETIMEDOUT;
> 
>         err = xe_uc_init(&gt->uc);
>         if (err)
>-                goto err_force_wake;
>+                return err;
> 
>         xe_gt_topology_init(gt);
>         xe_gt_mcr_init(gt);
>@@ -478,7 +470,7 @@ static int gt_init_with_gt_forcewake(struct xe_gt *gt)
>         if (xe_gt_is_main_type(gt)) {
>                 err = xe_ggtt_init(gt_to_tile(gt)->mem.ggtt);
>                 if (err)
>-                        goto err_force_wake;
>+                        return err;
>                 if (IS_SRIOV_PF(gt_to_xe(gt)))
>                         xe_lmtt_init(&gt_to_tile(gt)->sriov.pf.lmtt);
>         }
>@@ -492,17 +484,17 @@ static int gt_init_with_gt_forcewake(struct xe_gt *gt)
>         err = xe_hw_engines_init_early(gt);
>         if (err) {
>                 dump_pat_on_error(gt);
>-                goto err_force_wake;
>+                return err;
>         }
> 
>         err = xe_hw_engine_class_sysfs_init(gt);
>         if (err)
>-                goto err_force_wake;
>+                return err;
> 
>         /* Initialize CCS mode sysfs after early initialization of HW engines */
>         err = xe_gt_ccs_mode_sysfs_init(gt);
>         if (err)
>-                goto err_force_wake;
>+                return err;
> 
>         /*
>          * Stash hardware-reported version.  Since this register does not exist
>@@ -510,25 +502,16 @@ static int gt_init_with_gt_forcewake(struct xe_gt *gt)
>          */
>         gt->info.gmdid = xe_mmio_read32(&gt->mmio, GMD_ID);
> 
>-        xe_force_wake_put(gt_to_fw(gt), fw_ref);
>         return 0;
>-
>-err_force_wake:
>-        xe_force_wake_put(gt_to_fw(gt), fw_ref);
>-
>-        return err;
> }
> 
> static int gt_init_with_all_forcewake(struct xe_gt *gt)
> {
>-        unsigned int fw_ref;
>         int err;
> 
>-        fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>-        if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL)) {
>-                err = -ETIMEDOUT;
>-                goto err_force_wake;
>-        }
>+        CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>+        if (!xe_force_wake_ref_has_domain(fw_ref.domains, XE_FORCEWAKE_ALL))
>+                return -ETIMEDOUT;
> 
>         xe_gt_mcr_set_implicit_defaults(gt);
>         xe_wa_process_gt(gt);
>@@ -537,20 +520,20 @@ static int gt_init_with_all_forcewake(struct xe_gt *gt)
> 
>         err = xe_gt_clock_init(gt);
>         if (err)
>-                goto err_force_wake;
>+                return err;
> 
>         xe_mocs_init(gt);
>         err = xe_execlist_init(gt);
>         if (err)
>-                goto err_force_wake;
>+                return err;
> 
>         err = xe_hw_engines_init(gt);
>         if (err)
>-                goto err_force_wake;
>+                return err;
> 
>         err = xe_uc_init_post_hwconfig(&gt->uc);
>         if (err)
>-                goto err_force_wake;
>+                return err;
> 
>         if (xe_gt_is_main_type(gt)) {
>                 /*
>@@ -561,10 +544,8 @@ static int gt_init_with_all_forcewake(struct xe_gt *gt)
> 
>                         gt->usm.bb_pool = xe_sa_bo_manager_init(gt_to_tile(gt),
>                                                                 IS_DGFX(xe) ? SZ_1M : SZ_512K, 16);
>-                        if (IS_ERR(gt->usm.bb_pool)) {
>-                                err = PTR_ERR(gt->usm.bb_pool);
>-                                goto err_force_wake;
>-                        }
>+                        if (IS_ERR(gt->usm.bb_pool))
>+                                return PTR_ERR(gt->usm.bb_pool);
>                 }
>         }
> 
>@@ -573,12 +554,12 @@ static int gt_init_with_all_forcewake(struct xe_gt *gt)
> 
>                 err = xe_migrate_init(tile->migrate);
>                 if (err)
>-                        goto err_force_wake;
>+                        return err;
>         }
> 
>         err = xe_uc_load_hw(&gt->uc);
>         if (err)
>-                goto err_force_wake;
>+                return err;
> 
>         /* Configure default CCS mode of 1 engine with all resources */
>         if (xe_gt_ccs_mode_enabled(gt)) {
>@@ -592,14 +573,7 @@ static int gt_init_with_all_forcewake(struct xe_gt *gt)
>         if (IS_SRIOV_PF(gt_to_xe(gt)))
>                 xe_gt_sriov_pf_init_hw(gt);
> 
>-        xe_force_wake_put(gt_to_fw(gt), fw_ref);
>-
>         return 0;
>-
>-err_force_wake:
>-        xe_force_wake_put(gt_to_fw(gt), fw_ref);
>-
>-        return err;
> }
> 
> static void xe_gt_fini(void *arg)
>@@ -819,15 +793,17 @@ static int do_gt_restart(struct xe_gt *gt)
> static void gt_reset_worker(struct work_struct *w)
> {
>         struct xe_gt *gt = container_of(w, typeof(*gt), reset.worker);
>-        unsigned int fw_ref;
>         int err;
> 
>+        /* Drop the existing runtime PM reference when exiting this function */

I believe the comment above is kind of already implied. I think I would
prefer to keep the previous comment, i.e.:

    /* Pair with get while enqueueing the work in xe_gt_reset_async() */

, which adds info about where the ref came from.  Hmm, maybe that's
implied as well? Not sure...

>+        guard(xe_pm_runtime_release_only)(gt_to_xe(gt));
>+
>         if (xe_device_wedged(gt_to_xe(gt)))
>-                goto err_pm_put;
>+                return;
> 
>         /* We only support GT resets with GuC submission */
>         if (!xe_device_uc_enabled(gt_to_xe(gt)))
>-                goto err_pm_put;
>+                return;
> 
>         xe_gt_info(gt, "reset started\n");
> 
>@@ -838,8 +814,8 @@ static void gt_reset_worker(struct work_struct *w)
> 
>         xe_gt_sanitize(gt);
> 
>-        fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>-        if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL)) {
>+        CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>+        if (!xe_force_wake_ref_has_domain(fw_ref.domains, XE_FORCEWAKE_ALL)) {
>                 err = -ETIMEDOUT;
>                 goto err_out;
>         }
>@@ -863,25 +839,16 @@ static void gt_reset_worker(struct work_struct *w)
>         if (err)
>                 goto err_out;
> 
>-        xe_force_wake_put(gt_to_fw(gt), fw_ref);
>-
>-        /* Pair with get while enqueueing the work in xe_gt_reset_async() */
>-        xe_pm_runtime_put(gt_to_xe(gt));
>-
>         xe_gt_info(gt, "reset done\n");
> 
>         return;
> 
> err_out:

Aren't we are violating one of the expectations of cleanup.h here? Quoting
below:

     * Lastly, given that the benefit of cleanup helpers is removal of
     * "goto", and that the "goto" statement can jump between scopes, the
     * expectation is that usage of "goto" and cleanup helpers is never
     * mixed in the same function. I.e. for a given routine, convert all
     * resources that need a "goto" cleanup to scope-based cleanup, or
     * convert none of them.

That said, I do believe this is a bit too restrictive. In simple cases,
like this one, I believe the mix is okay.

>-        xe_force_wake_put(gt_to_fw(gt), fw_ref);
>         XE_WARN_ON(xe_uc_start(&gt->uc));
> 
> err_fail:
>         xe_gt_err(gt, "reset failed (%pe)\n", ERR_PTR(err));
>         xe_device_declare_wedged(gt_to_xe(gt));
>-
>-err_pm_put:
>-        xe_pm_runtime_put(gt_to_xe(gt));
> }
> 
> void xe_gt_reset_async(struct xe_gt *gt)
>@@ -902,56 +869,42 @@ void xe_gt_reset_async(struct xe_gt *gt)
> 
> void xe_gt_suspend_prepare(struct xe_gt *gt)
> {
>-        unsigned int fw_ref;
>-
>-        fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>-
>+        CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>         xe_uc_suspend_prepare(&gt->uc);
>-
>-        xe_force_wake_put(gt_to_fw(gt), fw_ref);
> }
> 
> int xe_gt_suspend(struct xe_gt *gt)
> {
>-        unsigned int fw_ref;
>         int err;
> 
>         xe_gt_dbg(gt, "suspending\n");
>         xe_gt_sanitize(gt);
> 
>-        fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>-        if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL))
>-                goto err_msg;
>+        CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>+        if (!xe_force_wake_ref_has_domain(fw_ref.domains, XE_FORCEWAKE_ALL)) {
>+                xe_gt_err(gt, "suspend failed (%pe)\n", ERR_PTR(-ETIMEDOUT));

Since we are touching this part, we could add a more specific message
here. Something like "suspend failed due to force wake error: (%pe)\n".

The same suggestion also applies to xe_gt_resume().

>+                return -ETIMEDOUT;
>+        }
> 
>         err = xe_uc_suspend(&gt->uc);
>-        if (err)
>-                goto err_force_wake;
>+        if (err) {
>+                xe_gt_err(gt, "suspend failed (%pe)\n", ERR_PTR(err));
>+                return err;
>+        }
> 
>         xe_gt_idle_disable_pg(gt);
> 
>         xe_gt_disable_host_l2_vram(gt);
> 
>-        xe_force_wake_put(gt_to_fw(gt), fw_ref);
>         xe_gt_dbg(gt, "suspended\n");
> 
>         return 0;
>-
>-err_msg:
>-        err = -ETIMEDOUT;
>-err_force_wake:
>-        xe_force_wake_put(gt_to_fw(gt), fw_ref);
>-        xe_gt_err(gt, "suspend failed (%pe)\n", ERR_PTR(err));
>-
>-        return err;
> }
> 
> void xe_gt_shutdown(struct xe_gt *gt)
> {
>-        unsigned int fw_ref;
>-
>-        fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>+        CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>         do_gt_reset(gt);
>-        xe_force_wake_put(gt_to_fw(gt), fw_ref);
> }
> 
> /**
>@@ -976,32 +929,24 @@ int xe_gt_sanitize_freq(struct xe_gt *gt)
> 
> int xe_gt_resume(struct xe_gt *gt)
> {
>-        unsigned int fw_ref;
>         int err;
> 
>         xe_gt_dbg(gt, "resuming\n");
>-        fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>-        if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL))
>-                goto err_msg;
>+        CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>+        if (!xe_force_wake_ref_has_domain(fw_ref.domains, XE_FORCEWAKE_ALL)) {
>+                xe_gt_err(gt, "resume failed (%pe)\n", ERR_PTR(-ETIMEDOUT));
>+                return -ETIMEDOUT;
>+        }
> 
>         err = do_gt_restart(gt);
>         if (err)
>-                goto err_force_wake;
>+                return err;
> 
>         xe_gt_idle_enable_pg(gt);
> 
>-        xe_force_wake_put(gt_to_fw(gt), fw_ref);
>         xe_gt_dbg(gt, "resumed\n");
> 
>         return 0;
>-
>-err_msg:
>-        err = -ETIMEDOUT;
>-err_force_wake:
>-        xe_force_wake_put(gt_to_fw(gt), fw_ref);
>-        xe_gt_err(gt, "resume failed (%pe)\n", ERR_PTR(err));
>-
>-        return err;
> }
> 
> struct xe_hw_engine *xe_gt_hw_engine(struct xe_gt *gt,
>-- 
>2.51.1
>