Re: [PATCH-resent-to-correct-ml 3/8] drm/xe: Add scoped guards for xe_force_wake

[Michal Wajdeczko]

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@xxxxxxxxxxxx>
> ---
>  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