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

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

On 01-10-2024 01:45, Michal Wajdeczko wrote:
> 
> 
> 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?

Changing this to void here or before this patch will break the
compilation, since the cleanup is in subsequent patches. Therefore,
patch 25/25 addresses changing it to void after all the cleanups.

>>
>>>
>>> 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@intel.com>
>>> 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>
>>> Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>
>>
>> probably this applies to some earlier #rev, so please mark it as such

Sure. Miss at my end. Will do in this patch and others.

>>
>>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@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,
>>
>