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

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

On 13-09-2024 03:04, Michal Wajdeczko wrote:
> 
> 
> 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 ?

Completely agreed here. Currently the failure is returned as error 
leading to abort/further escalation in few cases. IMO, xe_force_wake_put 
failure shouldn't be leading to abort/furtherescalation therefore have 
raised a patch for the same and is part of this series:

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


> 
>> + */
>>   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,