[PATCH v6 04/25] drm/xe: Modify xe_force_wake_put to handle _get returned mask

[Michal Wajdeczko]

On 30.09.2024 22:13, Michal Wajdeczko wrote:
> 
> 
> On 30.09.2024 07:31, 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 unsigned int.
> 
> at this point in the series xe_force_wake_get() still returns errno

oops, scratch that, somehow missed patch 03/25

> 
> maybe rephrase this to something like:
> 
> "Prepare xe_force_wake_put() to accept force_wake reference with
> refcounted domains"
> 
> and also maybe it is a good time to change this function to void (or
> maybe even do it before this path?
> 
>>
>> v3
>> - Move WARN to this patch (Badal)
>> - use xe_gt_WARN instead of XE_WARN (Michal)
>> - Stop using xe_force_wake_domains for non enum values.
>> - Remove kernel-doc from this patch (Badal)
>>
>> -v5
>> - Fix global awake_domain
>>
>> -v6
>> - put all initialized domains in case of FORCEWAKE_ALL.
>> - Modify ret variable name (Michal)
>> - Modify input var name (Michal)
>> - Modify commit message and warn (Badal)
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Badal Nilawar <badal.nilawar at intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> Cc: Nirmoy Das <nirmoy.das at intel.com>
>> Reviewed-by: Badal Nilawar <badal.nilawar at intel.com>
> 
> probably this applies to some earlier #rev, so please mark it as such
> 
>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_force_wake.c | 28 +++++++++++++++++++++-------
>>  drivers/gpu/drm/xe/xe_force_wake.h |  2 +-
>>  2 files changed, 22 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
>> index 7f358e42c5d4..372ea43b0d06 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake.c
>> +++ b/drivers/gpu/drm/xe/xe_force_wake.c
>> @@ -211,26 +211,40 @@ unsigned int xe_force_wake_get(struct xe_force_wake *fw,
>>  }
>>  
>>  int xe_force_wake_put(struct xe_force_wake *fw,
>> -		      enum xe_force_wake_domains domains)
>> +		      unsigned int fw_ref)
>>  {
>>  	struct xe_gt *gt = fw->gt;
>>  	struct xe_force_wake_domain *domain;
>> -	enum xe_force_wake_domains tmp, sleep = 0;
>> +	unsigned int tmp, sleep = 0;
>>  	unsigned long flags;
>> -	int ret = 0;
>> +	int ack_fail = 0;
>> +
>> +	/*
>> +	 * Avoid unnecessary lock and unlock when the function is called
>> +	 * in error path of individual domains.
>> +	 */
>> +	if (!fw_ref)
>> +		return 0;
>> +
>> +	if (fw_ref == XE_FORCEWAKE_ALL)
>> +		fw_ref = fw->initialized_domains;
>>  
>>  	spin_lock_irqsave(&fw->lock, flags);
>> -	for_each_fw_domain_masked(domain, domains, fw, tmp) {
>> +	for_each_fw_domain_masked(domain, fw_ref, fw, tmp) {
>>  		if (!--domain->ref) {
>>  			sleep |= BIT(domain->id);
>>  			domain_sleep(gt, domain);
>>  		}
>>  	}
>>  	for_each_fw_domain_masked(domain, sleep, fw, tmp) {
>> -		ret |= domain_sleep_wait(gt, domain);
>> +		if (domain_sleep_wait(gt, domain) == 0)
>> +			fw->awake_domains &= ~BIT(domain->id);
>> +		else
>> +			ack_fail |= BIT(domain->id);
>>  	}
>> -	fw->awake_domains &= ~sleep;
>>  	spin_unlock_irqrestore(&fw->lock, flags);
>>  
>> -	return ret;
>> +	xe_gt_WARN(gt, ack_fail, "Forcewake domain%s %#x failed to acknowledge sleep request\n",
>> +		   str_plural(hweight_long(ack_fail)), ack_fail);
>> +	return ack_fail;
>>  }
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/xe/xe_force_wake.h
>> index eb638128952d..b5a75544d86a 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,
>>  unsigned 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);
>> +		      unsigned int fw_ref);
>>  
>>  static inline int
>>  xe_force_wake_ref(struct xe_force_wake *fw,
>