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

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

Quoting Gustavo Sousa (2025-11-12 17:00:15-03:00)
>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);

Another thing: I think it would be nice to have a note in
xe_force_wake_get()'s documentation about using CLASS(xe_force_wake,
ref)(fw, domains) and xe_with_force_wake() when it makes sense.

--
Gustavo Sousa

>>+
>>+/*
>>+ * 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
>>