Re: [PATCH v5 23/23] drm/xe: Change return type to void for xe_force_wake_put

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 25-09-2024 19:37, Michal Wajdeczko wrote:
> 
> 
> On 24.09.2024 14:16, Himal Prasad Ghimiray wrote:
>> There is no need to return an error from xe_force_wake_put(), as a
>> failure implicitly indicates that the domain failed to sleep.
>>
>> v3
>> - Move kernel-doc to this patch (Badal)
>>
>> v5
>> - change parameter to unsigned int in 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 | 20 ++++++++++++++++----
>>   drivers/gpu/drm/xe/xe_force_wake.h |  4 ++--
>>   2 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
>> index d2eb367da7c1..c2c55d3de36f 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake.c
>> +++ b/drivers/gpu/drm/xe/xe_force_wake.c
>> @@ -11,6 +11,7 @@
>>   #include "regs/xe_reg_defs.h"
>>   #include "xe_gt.h"
>>   #include "xe_gt_printk.h"
>> +#include "xe_macros.h"
> 
> this include is likely not needed

will recheck. Probably a leftover from XE_WARN_ON. Thanks for pointing 
this out.

> 
>>   #include "xe_mmio.h"
>>   #include "xe_sriov.h"
>>   
>> @@ -196,8 +197,20 @@ unsigned int __must_check xe_force_wake_get(struct xe_force_wake *fw,
>>   	return ret;
>>   }
>>   
>> -int xe_force_wake_put(struct xe_force_wake *fw,
>> -		      unsigned int domains_mask)
>> +/**
>> + * 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.
> 
> nit: if this timeout is hardcoded maybe mention or refer it?

Sure.

> 
>> + * Ensure this function is always called with return of xe_force_wake_get() as
>> + * @domains_mask.
>> + * Warns in case of timeout of ack from domain.
>> + */
>> +void xe_force_wake_put(struct xe_force_wake *fw,
>> +		       unsigned int domains_mask)
> 
> nit: no need to wrap, your kernel-doc is much wider

Sure.

> 
>>   {
>>   	struct xe_gt *gt = fw->gt;
>>   	struct xe_force_wake_domain *domain;
>> @@ -210,7 +223,7 @@ int xe_force_wake_put(struct xe_force_wake *fw,
>>   	 * in error path of individual domains.
>>   	 */
>>   	if (!domains_mask)
>> -		return 0;
>> +		return;
>>   
>>   	spin_lock_irqsave(&fw->lock, flags);
>>   	for_each_fw_domain_masked(domain, domains_mask, fw, tmp) {
>> @@ -229,5 +242,4 @@ int xe_force_wake_put(struct xe_force_wake *fw,
>>   
>>   	xe_gt_WARN(gt, ret, "domain%s %#x failed to acknowledgment sleep\n",
>>   		   str_plural(hweight_long(ret)), ret);
>> -	return ret;
>>   }
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/xe/xe_force_wake.h
>> index 0b676462c97b..a79c98ed6d2f 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake.h
>> +++ b/drivers/gpu/drm/xe/xe_force_wake.h
>> @@ -17,8 +17,8 @@ void xe_force_wake_init_engines(struct xe_gt *gt,
>>   				struct xe_force_wake *fw);
>>   unsigned int __must_check 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,
>> -		      unsigned int domains_mask);
>> +void xe_force_wake_put(struct xe_force_wake *fw,
>> +		       unsigned int domains_mask);
> 
> no need to wrap the line

Sure

> 
>>   
>>   static inline int
>>   xe_force_wake_ref(struct xe_force_wake *fw,
>