[PATCH 3/9] drm/xe/xe_reg_sr: Always call xe_force_wake_put/get in pairs

[Nirmoy Das]

Hi Michal,

On 6/4/2024 3:12 PM, Michal Wajdeczko wrote:
>
> On 04.06.2024 13:02, Nirmoy Das wrote:
>> xe_force_wake_get() increments the domain ref regardless of success
>> or failure so call xe_force_wake_put() even on failure to keep ref
>> count accurate.
>>
>> Signed-off-by: Nirmoy Das<nirmoy.das at intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_reg_sr.c | 20 ++++++++------------
>>   1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_reg_sr.c b/drivers/gpu/drm/xe/xe_reg_sr.c
>> index 440ac572f6e5..8fcc08658d89 100644
>> --- a/drivers/gpu/drm/xe/xe_reg_sr.c
>> +++ b/drivers/gpu/drm/xe/xe_reg_sr.c
>> @@ -201,13 +201,11 @@ void xe_reg_sr_apply_mmio(struct xe_reg_sr *sr, struct xe_gt *gt)
>>   	xa_for_each(&sr->xa, reg, entry)
>>   		apply_one_mmio(gt, entry);
>>   
>> -	err = xe_force_wake_put(&gt->mmio.fw, XE_FORCEWAKE_ALL);
>> -	XE_WARN_ON(err);
>> -
>> -	return;
>> -
>>   err_force_wake:
>> -	xe_gt_err(gt, "Failed to apply, err=%d\n", err);
>> +	if (err)
>> +		xe_gt_err(gt, "Failed to whitelist %s registers, err=%d\n",
>> +			  sr->name, err);
>> +	XE_WARN_ON(xe_force_wake_put(&gt->mmio.fw, XE_FORCEWAKE_ALL));
> what's the rule whether to WARN about the failed xe_force_wake_put() or
> not ? should we also WARN when xe_force_wake_get() failed ? does it make
> sense to WARN about put() if get() already failed ?
We don't have one yet.
>
> maybe simpler solution would be make function xe_force_wake_put() void
> as it almost nothing that caller can do and move WARN there if needed ?

We have code that  does "return xe_force_wake_put()" So question is what 
shall we do

if xe_force_wake_get() worked but subsequent xe_force_wake_put() fails ?

Regards,

Nirmoy

>
> what about making the flow more intuitive like this:
>
> bool xe_force_wake_get(fw, d);
> void xe_force_wake_put(fw, d);
>
> 	if (xe_force_wake_get(fw, d)) {
> 		...
> 		xe_force_wake_put(fw, d);
> 	}
>
>>   }
>>   
>>   void xe_reg_sr_apply_whitelist(struct xe_hw_engine *hwe)
>> @@ -253,13 +251,11 @@ void xe_reg_sr_apply_whitelist(struct xe_hw_engine *hwe)
>>   		xe_mmio_write32(gt, RING_FORCE_TO_NONPRIV(mmio_base, slot), addr);
>>   	}
>>   
>> -	err = xe_force_wake_put(&gt->mmio.fw, XE_FORCEWAKE_ALL);
>> -	XE_WARN_ON(err);
>> -
>> -	return;
>> -
>>   err_force_wake:
>> -	drm_err(&xe->drm, "Failed to apply, err=%d\n", err);
>> +	if (err)
>> +		xe_gt_err(gt, "Failed to whitelist %s registers, err=%d\n",
>> +			  sr->name, err);
>> +	XE_WARN_ON(xe_force_wake_put(&gt->mmio.fw, XE_FORCEWAKE_ALL));
>>   }
>>   
>>   /**
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-xe/attachments/20240604/ddc05384/attachment-0001.htm>