| Message ID | 20250204132238.162608-4-dev@lankhorst.se (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/xe: Convert xe_force_wake calls to guard helpers. | expand |
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 >
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 >
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
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(+)