Re: [RFC 1/9] drm/xe: Error handling in xe_force_wake_get()

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

On 30-08-2024 12:07, Jani Nikula wrote:
> On Fri, 30 Aug 2024, Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com> wrote:
>> If an acknowledgment timeout occurs for a domain awake request, put to
>> sleep all domains awakened by the caller and decrease the reference
>> count for all requested domains. This prevents xe_force_wake_get() from
>> leaving an unhandled reference count in case of failure.
>> While at it, add simple kernel-doc for xe_force_wake_get() and
>> xe_force_wake_put() functions.
>>
>> 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 | 52 +++++++++++++++++++++++++++---
>>   1 file changed, 47 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
>> index b263fff15273..8aa8d9b41052 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake.c
>> +++ b/drivers/gpu/drm/xe/xe_force_wake.c
>> @@ -150,31 +150,73 @@ static int domain_sleep_wait(struct xe_gt *gt,
>>   					 (ffs(tmp__) - 1))) && \
>>   					 domain__->reg_ctl.addr)
>>   
>> +/**
>> + * xe_force_wake_get : Increase the domain refcount; if it was 0 initially, wake the domain
>> + * @fw: struct xe_force_wake
>> + * @domains: forcewake domains to get refcount on
>> + *
>> + * Increment refcount for the force-wake domain. If the domain is
>> + * asleep, awaken it and wait for acknowledgment within the specified
>> + * timeout. If a timeout occurs, decrement the refcount and put the
>> + * caller awaken domains to sleep.
>> + *
>> + * Return: 0 on success or 1 on ack timeout from domains.
> 
> Please stick to conventions on error returns, i.e. please use defined
> negative error values instead of magic 1.


My bad. Thanks for pointing this. Will address it in next version.

> 
>> + */
>>   int xe_force_wake_get(struct xe_force_wake *fw,
>>   		      enum xe_force_wake_domains domains)
>>   {
>>   	struct xe_gt *gt = fw->gt;
>>   	struct xe_force_wake_domain *domain;
>> -	enum xe_force_wake_domains tmp, woken = 0;
>> +	enum xe_force_wake_domains tmp, awake_rqst = 0, awake_ack = 0;
>>   	unsigned long flags;
>>   	int ret = 0;
>>   
>>   	spin_lock_irqsave(&fw->lock, flags);
>>   	for_each_fw_domain_masked(domain, domains, fw, tmp) {
>>   		if (!domain->ref++) {
>> -			woken |= BIT(domain->id);
>> +			awake_rqst |= BIT(domain->id);
>>   			domain_wake(gt, domain);
>>   		}
>>   	}
>> -	for_each_fw_domain_masked(domain, woken, fw, tmp) {
>> -		ret |= domain_wake_wait(gt, domain);
>> +	for_each_fw_domain_masked(domain, awake_rqst, fw, tmp) {
>> +		if (domain_wake_wait(gt, domain) == 0)
>> +			awake_ack |= BIT(domain->id);
>> +	}
>> +
>> +	ret = (awake_ack == awake_rqst) ? 0 : 1;
>> +
>> +	/*
>> +	 * If @domains is XE_FORCEWAKE_ALL and an acknowledgment times out
>> +	 * for any domain, decrease the reference count and put the awake
>> +	 * domains to sleep. For individual domains, just decrement the
>> +	 * reference count.
>> +	 */
>> +	if (ret) {
>> +		for_each_fw_domain_masked(domain, awake_rqst, fw, tmp) {
>> +			if (!--domain->ref && (awake_ack & BIT(domain->id)))
>> +				domain_sleep(gt, domain);
>> +		}
>> +		awake_ack = 0;
>>   	}
>> -	fw->awake_domains |= woken;
>> +
>> +	fw->awake_domains |= awake_ack;
>>   	spin_unlock_irqrestore(&fw->lock, flags);
>>   
>>   	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: forcewake domains to put reference
>> + *
>> + * This function reduces the reference counts for specified domains. 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 called only in case of successful xe_force_wake_get().
>> + *
>> + * Returns 0 in case of success or non-zero in case of timeout of ack
>> + */
>>   int xe_force_wake_put(struct xe_force_wake *fw,
>>   		      enum xe_force_wake_domains domains)
>>   {
>