Re: [PATCH v2 04/30] drm/xe/forcewake: Add scope-based cleanup for forcewake

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

Quoting Matt Roper (2025-11-10 20:20:22-03:00)
>Since forcewake uses a reference counting get/put model, there are many
>places where we need to be careful to drop the forcewake reference when
>bailing out of a function early on an error path.  Add scope-based
>cleanup options that can be used in place of explicit get/put to help
>prevent mistakes in this area.
>
>Examples:
>
>   CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FW_GT);
>
>       Obtain forcewake on the XE_FW_GT domain and hold it until the
>       end of the current block.  The wakeref will be dropped
>       automatically when the current scope is exited by any means
>       (return, break, reaching the end of the block, etc.).

Looking further down in the patches, I see that fw_ref is only being
used to check for success.  I wish there was a way for us to do
something like:

    if (xe_force_wake_require(gt_to_fw(gt), XE_FW_GT))
        return; /* ... or whatever that handles the force wake error. */

, but I can't think of a way define such a macro, since we need a
variable in the scope where xe_force_wake_require() would be called for
the cleanup to work properly.

>
>   xe_with_force_wake(fw_ref, gt_to_fw(ss->gt), XE_FORCEWAKE_ALL) {

In display code, we usually have with_intel_something().  I wonder if we
should follow suit and use with_xe_force_wake().

>        ...
>   }
>
>       Hold all forcewake domains for the following block.  As with the
>       CLASS usage, forcewake will be dropped automatically when the
>       block is exited by any means.
>
>Use of these cleanup helpers should allow us to remove some ugly
>goto-based error handling and help avoid mistakes in functions with lots
>of early error exits.
>
>v2:
> - Create a separate constructor that just wraps xe_force_wake_get for
>   use in the class.  This eliminates the need to update the signature
>   of xe_force_wake_get().  (Michal)
>
>Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>---
> drivers/gpu/drm/xe/xe_force_wake.h | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
>diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/xe/xe_force_wake.h
>index 0e3e84bfa51c..0204503b53a0 100644
>--- a/drivers/gpu/drm/xe/xe_force_wake.h
>+++ b/drivers/gpu/drm/xe/xe_force_wake.h
>@@ -61,4 +61,32 @@ xe_force_wake_ref_has_domain(unsigned int fw_ref, enum xe_force_wake_domains dom
>         return fw_ref & domain;
> }
> 
>+struct xe_force_wake_ref {
>+        struct xe_force_wake *fw;
>+        unsigned int domains;
>+};
>+
>+static struct xe_force_wake_ref
>+xe_force_wake_constructor(struct xe_force_wake *fw, unsigned int domains)
>+{
>+        struct xe_force_wake_ref fw_ref = { .fw = fw };
>+
>+        fw_ref.domains = xe_force_wake_get(fw, domains);
>+
>+        return fw_ref;
>+}
>+
>+DEFINE_CLASS(xe_force_wake, struct xe_force_wake_ref,
>+             xe_force_wake_put(_T.fw, _T.domains),
>+             xe_force_wake_constructor(fw, domains),
>+             struct xe_force_wake *fw, unsigned int domains);
>+
>+/*
>+ * Scoped helper for the forcewake class, using the same trick as scoped_guard()
>+ * to bind the lifetime to the next statement/block.
>+ */
>+#define xe_with_force_wake(ref, fw, domains) \
>+        for (CLASS(xe_force_wake, ref)(fw, domains), *done = NULL; \
>+             !done; done = (void *)1)

I think it would be good to use __UNIQUE_ID() for "done" here, to avoid
shadowing any existing variable from the outer scope.  We could do it
with something like:

	#define xe_with_force_wake(ref, fw, domains, done) \
		for (CLASS(xe_force_wake, ref)(fw, domains), *done = NULL; \
		     !done; done = (void *)1)

	#define xe_with_force_wake(ref, fw, domains) \
		__xe_with_force_wake(ref, fw, domains, __UNIQUE_ID(done))

--
Gustavo Sousa

>+
> #endif
>-- 
>2.51.1
>