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 <[email protected]> > --- > 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
