Re: [PATCH v3 01/27] drm/xe/forcewake: Add scope-based cleanup for forcewake

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

Quoting Gustavo Sousa (2025-11-17 19:03:21-03:00)
>Quoting Matt Roper (2025-11-14 18:43:37-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.).
>>
>>   xe_with_force_wake(fw_ref, gt_to_fw(ss->gt), XE_FORCEWAKE_ALL) {
>>        ...
>>   }
>>
>>       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.
>>
>>An 'xe_force_wake_release_only' class is also added for cases where a
>>forcewake reference is passed in from another function and the current
>>function is responsible for releasing it in every flow and error path.
>>
>>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)
>>
>>v3:
>> - Wrap xe_with_force_wake's 'done' marker in __UNIQUE_ID.  (Gustavo)
>> - Add a note to xe_force_wake_get()'s kerneldoc explaining that
>>   scope-based cleanup is preferred when possible.  (Gustavo)
>> - Add an xe_force_wake_release_only class.  (Gustavo)
>>
>>Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>Cc: Gustavo Sousa <gustavo.sousa@intel.com>
>>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>
>Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
>
>>---
>> drivers/gpu/drm/xe/xe_force_wake.c |  7 ++++++
>> drivers/gpu/drm/xe/xe_force_wake.h | 40 ++++++++++++++++++++++++++++++
>> 2 files changed, 47 insertions(+)
>>
>>diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
>>index c59a9b330697..76e054f314ee 100644
>>--- a/drivers/gpu/drm/xe/xe_force_wake.c
>>+++ b/drivers/gpu/drm/xe/xe_force_wake.c
>>@@ -166,6 +166,13 @@ static int domain_sleep_wait(struct xe_gt *gt,
>>  * xe_force_wake_ref_has_domain() function. Caller must call
>>  * xe_force_wake_put() function to decrease incremented refcounts.
>>  *
>>+ * When possible, scope-based forcewake (through CLASS(xe_force_wake, ...) or
>>+ * xe_with_force_wake()) should be used instead of direct calls to this
>>+ * function.  Direct usage of get/put should only be used when the function
>>+ * has goto-based flows that can interfere with scope-based cleanup, or when
>>+ * the lifetime of the forcewake reference does not match a specific scope
>>+ * (e.g., forcewake obtained in one function and released in a different one).
>>+ *
>>  * Return: opaque reference to woken domains or zero if none of requested
>>  * domains were awake.
>>  */
>>diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/xe/xe_force_wake.h
>>index 0e3e84bfa51c..ffc4e103fe31 100644
>>--- a/drivers/gpu/drm/xe/xe_force_wake.h
>>+++ b/drivers/gpu/drm/xe/xe_force_wake.h
>>@@ -61,4 +61,44 @@ 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, 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))
>>+
>>+/*
>>+ * Used when xe_force_wake_constructor() has already been called by another
>>+ * function and the current function is responsible for releasing the forcewake
>>+ * reference in all possible cases and error paths.
>>+ */
>>+DEFINE_CLASS(xe_force_wake_release_only, struct xe_force_wake_ref,
>>+             xe_force_wake_put(_T.fw, _T.domains), fw_ref,

Hm... Should we have a NULL check on _T.fw here?

--
Gustavo Sousa

>>+             struct xe_force_wake_ref fw_ref);
>>+
>> #endif
>>-- 
>>2.51.1
>>