Re: [PATCH v2 02/23] drm/xe: Modify xe_force_wake_put to handle _get returned mask

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 12.09.2024 21:15, Himal Prasad Ghimiray wrote:
> Instead of calling xe_force_wake_put on all domains that were input to
> xe_force_wake_get, call _put only on the domains whose reference counts
> were successfully incremented by the _get call. Since the return value
> of _get can be a mask that does not match any specific value in the enum
> xe_force_wake_domains, change the input parameter of _put to int.
> 
> Add kernel-doc for the xe_force_wake_put()
> 
> Cc: Badal Nilawar <badal.nilawar@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Nirmoy Das <nirmoy.das@intel.com>
> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_force_wake.c | 24 ++++++++++++++++++++++--
>  drivers/gpu/drm/xe/xe_force_wake.h |  2 +-
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
> index fa42d652d23f..e91bbcbddb8b 100644
> --- a/drivers/gpu/drm/xe/xe_force_wake.c
> +++ b/drivers/gpu/drm/xe/xe_force_wake.c
> @@ -198,8 +198,21 @@ int xe_force_wake_get(struct xe_force_wake *fw,
>  	return ret;
>  }
>  
> +/**
> + * xe_force_wake_put - Decrement the refcount and put domain to sleep if refcount becomes 0
> + * @fw: Pointer to the force wake structure
> + * @domains_mask: forcewake domain mask to put reference
> + *
> + * This function reduces the reference counts for specified domain mask. If
> + * refcount for any of the specified domain reaches 0, it puts the domain to sleep
> + * and waits for acknowledgment for domain to sleep within specified timeout.
> + * Ensure this function is always called with return of xe_force_wake_get() as
> + * @domains_mask.
> + *
> + * Returns 0 in case of success or non-zero in case of timeout of ack

what's the expectation for the caller when this function returns an
error? if just WARN() then maybe this can be done inside this function
and we can make it void ?

> + */
>  int xe_force_wake_put(struct xe_force_wake *fw,
> -		      enum xe_force_wake_domains domains)
> +		      int domains_mask)
>  {
>  	struct xe_gt *gt = fw->gt;
>  	struct xe_force_wake_domain *domain;
> @@ -207,8 +220,15 @@ int xe_force_wake_put(struct xe_force_wake *fw,
>  	unsigned long flags;
>  	int ret = 0;
>  
> +	/*
> +	 * Avoid unnecessary lock and unlock when the function is called
> +	 * in error path of individual domains.
> +	 */
> +	if (!domains_mask)
> +		return 0;
> +
>  	spin_lock_irqsave(&fw->lock, flags);
> -	for_each_fw_domain_masked(domain, domains, fw, tmp) {
> +	for_each_fw_domain_masked(domain, domains_mask, fw, tmp) {
>  		if (!--domain->ref) {
>  			sleep |= BIT(domain->id);
>  			domain_sleep(gt, domain);
> diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/xe/xe_force_wake.h
> index a2577672f4e3..e2bb6b03e495 100644
> --- a/drivers/gpu/drm/xe/xe_force_wake.h
> +++ b/drivers/gpu/drm/xe/xe_force_wake.h
> @@ -18,7 +18,7 @@ void xe_force_wake_init_engines(struct xe_gt *gt,
>  int xe_force_wake_get(struct xe_force_wake *fw,
>  		      enum xe_force_wake_domains domains);
>  int xe_force_wake_put(struct xe_force_wake *fw,
> -		      enum xe_force_wake_domains domains);
> +		      int domains_mask);
>  
>  static inline int
>  xe_force_wake_ref(struct xe_force_wake *fw,