Instead of finding bugs where we may or may not release force_wake, I've
decided to be inspired by the spinlock guards, and use the same ones to
do xe_force_wake handling.

Examples are added as documentation in xe_force_wake.c

Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/xe/xe_force_wake.c | 51 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_force_wake.h | 15 +++++++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
index 4f6784e5abf88..805c19f6de9e7 100644
--- a/drivers/gpu/drm/xe/xe_force_wake.c
+++ b/drivers/gpu/drm/xe/xe_force_wake.c
@@ -16,6 +16,57 @@
 
 #define XE_FORCE_WAKE_ACK_TIMEOUT_MS	50
 
+/**
+ * DOC: Force wake handling
+ *
+ * Traditionally, the force wake handling has been done using the error prone
+ * set of calls:
+ *
+ * int func(struct xe_force_wake *fw)
+ * {
+ * 	unsigned int fw_ref = xe_force_wake_get(fw, XE_FORCEWAKE_ALL);
+ * 	if (!fw_ref)
+ * 		return -ETIMEDOUT;
+ *
+ * 	err = do_something();
+ *
+ * 	xe_force_wake_put(fw, fw_ref);
+ * 	return err;
+ * }
+ *
+ * A new, failure-safe approach is by using the scoped helpers,
+ * which changes the function to this:
+ *
+ * int func(struct xe_force_wake *fw)
+ * {
+ * 	scoped_cond_guard(xe_force_wake_get, return -ETIMEDOUT, fw, XE_FORCEWAKE_ALL) {
+ * 		return do_something();
+ * 	}
+ * }
+ *
+ * For completeness, the following options also work:
+ * void func(struct xe_force_wake *fw)
+ * {
+ * 	scoped_guard(xe_force_wake_get, fw, XE_FORCEWAKE_ALL) {
+ * 		do_something_only_if_fw_acquired();
+ * 	}
+ * }
+ *
+ * You can use xe_force_wake instead of force_wake_get, if the code
+ * must run but errors acquiring ignored:
+ * void func(struct xe_force_wake *fw)
+ * {
+ * 	scoped_guard(xe_force_wake, fw, XE_FORCEWAKE_ALL) {
+ * 		always_do_something_maybe_fw();
+ * 	}
+ *
+ * 	do_something_no_fw();
+ *
+ * 	guard(xe_force_wake)(fw, XE_FORCEWAKE_ALL);
+ * 	always_do_something_maybe_fw();
+ * }
+ */
+
 static const char *str_wake_sleep(bool wake)
 {
 	return wake ? "wake" : "sleep";
diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/xe/xe_force_wake.h
index 0e3e84bfa51c3..0fb1baae0a3a3 100644
--- a/drivers/gpu/drm/xe/xe_force_wake.h
+++ b/drivers/gpu/drm/xe/xe_force_wake.h
@@ -9,6 +9,8 @@
 #include "xe_assert.h"
 #include "xe_force_wake_types.h"
 
+#include <linux/cleanup.h>
+
 struct xe_gt;
 
 void xe_force_wake_init_gt(struct xe_gt *gt,
@@ -61,4 +63,17 @@ xe_force_wake_ref_has_domain(unsigned int fw_ref, enum xe_force_wake_domains dom
 	return fw_ref & domain;
 }
 
+DEFINE_LOCK_GUARD_1(xe_force_wake, struct xe_force_wake,
+		    _T->fw_ref = xe_force_wake_get(_T->lock, domain),
+		    xe_force_wake_put(_T->lock, _T->fw_ref),
+		    unsigned int fw_ref, enum xe_force_wake_domains domain);
+
+DEFINE_LOCK_GUARD_1_COND(xe_force_wake, _get,
+			 _T->fw_ref = xe_force_wake_get_all(_T->lock, domain),
+			 enum xe_force_wake_domains domain);
+
+/* Only useful for guard xe_force_wake, guard xe_force_wake_get gets all or nothing */
+#define xe_force_wake_scope_has_domain(domain) \
+	(xe_force_wake_ref_has_domain(scope.fw_ref, domain))
+
 #endif
-- 
2.47.1

Hi Maarten,

On 04.02.2025 14:22, Maarten Lankhorst wrote:
> Instead of finding bugs where we may or may not release force_wake, I've
> decided to be inspired by the spinlock guards, and use the same ones to
> do xe_force_wake handling.

You may want to take a look at [1], which was based on [2], that
introduce fw guard class (and it was already acked and reviewed).
Merging was postponed only due to a request to prepare larger series
that would convert all existing usages to the new model.

And similar guard approach for our RPM was proposed in [3]

Michal

[1] https://patchwork.freedesktop.org/series/141516/
[2] https://patchwork.freedesktop.org/series/134958/
[3] https://patchwork.freedesktop.org/series/134955/

> 
> Examples are added as documentation in xe_force_wake.c
> 
> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> ---
>  drivers/gpu/drm/xe/xe_force_wake.c | 51 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_force_wake.h | 15 +++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
> index 4f6784e5abf88..805c19f6de9e7 100644
> --- a/drivers/gpu/drm/xe/xe_force_wake.c
> +++ b/drivers/gpu/drm/xe/xe_force_wake.c
> @@ -16,6 +16,57 @@
>  
>  #define XE_FORCE_WAKE_ACK_TIMEOUT_MS	50
>  
> +/**
> + * DOC: Force wake handling
> + *
> + * Traditionally, the force wake handling has been done using the error prone
> + * set of calls:
> + *
> + * int func(struct xe_force_wake *fw)
> + * {
> + * 	unsigned int fw_ref = xe_force_wake_get(fw, XE_FORCEWAKE_ALL);
> + * 	if (!fw_ref)
> + * 		return -ETIMEDOUT;
> + *
> + * 	err = do_something();
> + *
> + * 	xe_force_wake_put(fw, fw_ref);
> + * 	return err;
> + * }
> + *
> + * A new, failure-safe approach is by using the scoped helpers,
> + * which changes the function to this:
> + *
> + * int func(struct xe_force_wake *fw)
> + * {
> + * 	scoped_cond_guard(xe_force_wake_get, return -ETIMEDOUT, fw, XE_FORCEWAKE_ALL) {
> + * 		return do_something();
> + * 	}
> + * }
> + *
> + * For completeness, the following options also work:
> + * void func(struct xe_force_wake *fw)
> + * {
> + * 	scoped_guard(xe_force_wake_get, fw, XE_FORCEWAKE_ALL) {
> + * 		do_something_only_if_fw_acquired();
> + * 	}
> + * }
> + *
> + * You can use xe_force_wake instead of force_wake_get, if the code
> + * must run but errors acquiring ignored:
> + * void func(struct xe_force_wake *fw)
> + * {
> + * 	scoped_guard(xe_force_wake, fw, XE_FORCEWAKE_ALL) {
> + * 		always_do_something_maybe_fw();
> + * 	}
> + *
> + * 	do_something_no_fw();
> + *
> + * 	guard(xe_force_wake)(fw, XE_FORCEWAKE_ALL);
> + * 	always_do_something_maybe_fw();
> + * }
> + */
> +
>  static const char *str_wake_sleep(bool wake)
>  {
>  	return wake ? "wake" : "sleep";
> diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/xe/xe_force_wake.h
> index 0e3e84bfa51c3..0fb1baae0a3a3 100644
> --- a/drivers/gpu/drm/xe/xe_force_wake.h
> +++ b/drivers/gpu/drm/xe/xe_force_wake.h
> @@ -9,6 +9,8 @@
>  #include "xe_assert.h"
>  #include "xe_force_wake_types.h"
>  
> +#include <linux/cleanup.h>
> +
>  struct xe_gt;
>  
>  void xe_force_wake_init_gt(struct xe_gt *gt,
> @@ -61,4 +63,17 @@ xe_force_wake_ref_has_domain(unsigned int fw_ref, enum xe_force_wake_domains dom
>  	return fw_ref & domain;
>  }
>  
> +DEFINE_LOCK_GUARD_1(xe_force_wake, struct xe_force_wake,
> +		    _T->fw_ref = xe_force_wake_get(_T->lock, domain),
> +		    xe_force_wake_put(_T->lock, _T->fw_ref),
> +		    unsigned int fw_ref, enum xe_force_wake_domains domain);
> +
> +DEFINE_LOCK_GUARD_1_COND(xe_force_wake, _get,
> +			 _T->fw_ref = xe_force_wake_get_all(_T->lock, domain),
> +			 enum xe_force_wake_domains domain);
> +
> +/* Only useful for guard xe_force_wake, guard xe_force_wake_get gets all or nothing */
> +#define xe_force_wake_scope_has_domain(domain) \
> +	(xe_force_wake_ref_has_domain(scope.fw_ref, domain))
> +
>  #endif

Hey,


On 2025-02-04 17:30, Michal Wajdeczko wrote:
> Hi Maarten,
> 
> On 04.02.2025 14:22, Maarten Lankhorst wrote:
>> Instead of finding bugs where we may or may not release force_wake, I've
>> decided to be inspired by the spinlock guards, and use the same ones to
>> do xe_force_wake handling.
> 
> You may want to take a look at [1], which was based on [2], that
> introduce fw guard class (and it was already acked and reviewed).
> Merging was postponed only due to a request to prepare larger series
> that would convert all existing usages to the new model.
> 
> And similar guard approach for our RPM was proposed in [3]
> 
> Michal
> 
> [1] https://patchwork.freedesktop.org/series/141516/
> [2] https://patchwork.freedesktop.org/series/134958/
> [3] https://patchwork.freedesktop.org/series/134955/

Excellent. I'm glad we're in agreement that doing forcewake handling in 
guard handlers is a good thing. :-)

I have taken a look at the patch series. I think the approach I've taken 
is a refinement of your series. Yours is already nearly there, but it 
still keeps the rough edges of the original API.

To smooth them, I have created 2 constructors, xe_force_wake, and 
xe_force_wake_get. The former is used if you want to run code regardless 
whether it succeeds, the latter is when you do.

This allows code like:
scoped_cond_guard(xe_force_wake_get, return -ETIMEDOUT, fw, 
XE_FORCE_WAKE_ALL) {}
to work flawlessly as intended, without having to check 
xe_force_wake_ref_has_domain(XE_FORCE_WAKE_ALL);

I think this cleanup removes a nasty source of errors.

When you don't care about failure:
scoped_guard(xe_force_wake, fw, XE_FORCE_WAKE_ALL) {
	if (!xe_force_wake_scope_has_domain(XE_FORCE_WAKE_ALL))
		printk("Oh noez, anyway..\n");

	/* Continue and pretend nothing happened */
}

And for optional code, same as scoped_cond_guard, but as scoped_guard:

scoped_guard(xe_force_wake_get, fw, XE_FORCE_WAKE_ALL) {
	/* Only runs this block if acquire completely succeeded, otherwise use 
xe_force_wake */
}

All in all, I'm open for bikesheds, but I think this has the potential 
to improve xe_force_wake handling even further!

I wasn't aware of your previous attempt when I wrote this and fought 
linux/cleanup.h, otherwise I would have taken that as a base and credit 
your work.

Cheers,
~Maarten

On Tue, Feb 04, 2025 at 11:28:03PM +0100, Maarten Lankhorst wrote:
> Hey,
> 
> 
> On 2025-02-04 17:30, Michal Wajdeczko wrote:
> > Hi Maarten,
> > 
> > On 04.02.2025 14:22, Maarten Lankhorst wrote:
> > > Instead of finding bugs where we may or may not release force_wake, I've
> > > decided to be inspired by the spinlock guards, and use the same ones to
> > > do xe_force_wake handling.
> > 
> > You may want to take a look at [1], which was based on [2], that
> > introduce fw guard class (and it was already acked and reviewed).
> > Merging was postponed only due to a request to prepare larger series
> > that would convert all existing usages to the new model.
> > 
> > And similar guard approach for our RPM was proposed in [3]
> > 
> > Michal
> > 
> > [1] https://patchwork.freedesktop.org/series/141516/
> > [2] https://patchwork.freedesktop.org/series/134958/
> > [3] https://patchwork.freedesktop.org/series/134955/
> 
> Excellent. I'm glad we're in agreement that doing forcewake handling in
> guard handlers is a good thing. :-)

Just for the record. I had a similar feeling back there and
also now with the new series: I believe the code itself keeps harder
to read and follow.

But if that's really a big advantage on the protection like you are
all advocating for, let's go ahead.

> 
> I have taken a look at the patch series. I think the approach I've taken is
> a refinement of your series. Yours is already nearly there, but it still
> keeps the rough edges of the original API.
> 
> To smooth them, I have created 2 constructors, xe_force_wake, and
> xe_force_wake_get. The former is used if you want to run code regardless
> whether it succeeds, the latter is when you do.
> 
> This allows code like:
> scoped_cond_guard(xe_force_wake_get, return -ETIMEDOUT, fw,
> XE_FORCE_WAKE_ALL) {}
> to work flawlessly as intended, without having to check
> xe_force_wake_ref_has_domain(XE_FORCE_WAKE_ALL);
> 
> I think this cleanup removes a nasty source of errors.
> 
> When you don't care about failure:
> scoped_guard(xe_force_wake, fw, XE_FORCE_WAKE_ALL) {
> 	if (!xe_force_wake_scope_has_domain(XE_FORCE_WAKE_ALL))
> 		printk("Oh noez, anyway..\n");
> 
> 	/* Continue and pretend nothing happened */
> }
> 
> And for optional code, same as scoped_cond_guard, but as scoped_guard:
> 
> scoped_guard(xe_force_wake_get, fw, XE_FORCE_WAKE_ALL) {
> 	/* Only runs this block if acquire completely succeeded, otherwise use
> xe_force_wake */
> }
> 
> All in all, I'm open for bikesheds, but I think this has the potential to
> improve xe_force_wake handling even further!
> 
> I wasn't aware of your previous attempt when I wrote this and fought
> linux/cleanup.h, otherwise I would have taken that as a base and credit your
> work.
> 
> Cheers,
> ~Maarten
>

On Tue, Feb 04, 2025 at 02:22:32PM +0100, Maarten Lankhorst wrote:
>Instead of finding bugs where we may or may not release force_wake, I've
>decided to be inspired by the spinlock guards, and use the same ones to
>do xe_force_wake handling.
>
>Examples are added as documentation in xe_force_wake.c
>
>Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
>---
> drivers/gpu/drm/xe/xe_force_wake.c | 51 ++++++++++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_force_wake.h | 15 +++++++++
> 2 files changed, 66 insertions(+)
>
>diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
>index 4f6784e5abf88..805c19f6de9e7 100644
>--- a/drivers/gpu/drm/xe/xe_force_wake.c
>+++ b/drivers/gpu/drm/xe/xe_force_wake.c
>@@ -16,6 +16,57 @@
>
> #define XE_FORCE_WAKE_ACK_TIMEOUT_MS	50
>
>+/**
>+ * DOC: Force wake handling
>+ *

doc here should start explaining what is force wake, what it does, the
flags to wake specific parts of the gpu.

>+ * Traditionally, the force wake handling has been done using the error prone
>+ * set of calls:

I'd start with the new/recommended way rather than to digress on
non-recommended ways - this style doesn't age well in a few years.

>+ *
>+ * int func(struct xe_force_wake *fw)
>+ * {
>+ * 	unsigned int fw_ref = xe_force_wake_get(fw, XE_FORCEWAKE_ALL);
>+ * 	if (!fw_ref)
>+ * 		return -ETIMEDOUT;
>+ *
>+ * 	err = do_something();
>+ *
>+ * 	xe_force_wake_put(fw, fw_ref);
>+ * 	return err;
>+ * }
>+ *
>+ * A new, failure-safe approach is by using the scoped helpers,
>+ * which changes the function to this:
>+ *
>+ * int func(struct xe_force_wake *fw)
>+ * {
>+ * 	scoped_cond_guard(xe_force_wake_get, return -ETIMEDOUT, fw, XE_FORCEWAKE_ALL) {
>+ * 		return do_something();
>+ * 	}
>+ * }
>+ *
>+ * For completeness, the following options also work:
>+ * void func(struct xe_force_wake *fw)
>+ * {
>+ * 	scoped_guard(xe_force_wake_get, fw, XE_FORCEWAKE_ALL) {
>+ * 		do_something_only_if_fw_acquired();
>+ * 	}
>+ * }
>+ *
>+ * You can use xe_force_wake instead of force_wake_get, if the code
>+ * must run but errors acquiring ignored:
>+ * void func(struct xe_force_wake *fw)
>+ * {
>+ * 	scoped_guard(xe_force_wake, fw, XE_FORCEWAKE_ALL) {
>+ * 		always_do_something_maybe_fw();
>+ * 	}
>+ *
>+ * 	do_something_no_fw();
>+ *
>+ * 	guard(xe_force_wake)(fw, XE_FORCEWAKE_ALL);
>+ * 	always_do_something_maybe_fw();
>+ * }
>+ */
>+
> static const char *str_wake_sleep(bool wake)
> {
> 	return wake ? "wake" : "sleep";
>diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/xe/xe_force_wake.h
>index 0e3e84bfa51c3..0fb1baae0a3a3 100644
>--- a/drivers/gpu/drm/xe/xe_force_wake.h
>+++ b/drivers/gpu/drm/xe/xe_force_wake.h
>@@ -9,6 +9,8 @@
> #include "xe_assert.h"
> #include "xe_force_wake_types.h"
>
>+#include <linux/cleanup.h>
>+
> struct xe_gt;
>
> void xe_force_wake_init_gt(struct xe_gt *gt,
>@@ -61,4 +63,17 @@ xe_force_wake_ref_has_domain(unsigned int fw_ref, enum xe_force_wake_domains dom
> 	return fw_ref & domain;
> }
>
>+DEFINE_LOCK_GUARD_1(xe_force_wake, struct xe_force_wake,
>+		    _T->fw_ref = xe_force_wake_get(_T->lock, domain),
>+		    xe_force_wake_put(_T->lock, _T->fw_ref),
>+		    unsigned int fw_ref, enum xe_force_wake_domains domain);
>+
>+DEFINE_LOCK_GUARD_1_COND(xe_force_wake, _get,
>+			 _T->fw_ref = xe_force_wake_get_all(_T->lock, domain),
>+			 enum xe_force_wake_domains domain);
>+
>+/* Only useful for guard xe_force_wake, guard xe_force_wake_get gets all or nothing */

please add an usage example here is it's where people trying to use the
interface will look at rather than the kernel-doc. But this seems not
used

>+#define xe_force_wake_scope_has_domain(domain) \
>+	(xe_force_wake_ref_has_domain(scope.fw_ref, domain))

extra paranthesis here?

Lucas De Marchi

>+
> #endif
>-- 
>2.47.1
>