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

[Lucas De Marchi]

On Tue, Jun 04, 2024 at 04:21:25PM GMT, Nirmoy Das wrote:
>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 ?

there's not much can be done here. I even don't think there's a reason
to wait on ack for force_wake put... in most of the cases.
+Rodrigo, what exactly are we protecting against there?

If that xe_mmio_wait32() fails, driver is toast. The reason we check (or
should check) the return on _get() is because we want to bail out
whatever we are starting to do rather than attempting it and handling
bogus values of an IP that is not awaken.

So, maybe what could be done is

a) XE_WARN_ON(xe_force_wake_put()) -> xe_force_wake_put()

	We cann probably move the WARN_ON() inside xe_force_wake_put()
	if we want to maintain the behavior in most places

	Then make xe_force_wake_put() void as suggested here.

b) I'm not sure about the benefit of the weirdness of
xe_force_wake_get() incrementing the ref even if we failed to wait for
ack.  The only thing the caller is going to do is to call
xe_force_wake_put() and bail out. Why are we not unwinding then?


Lucas De Marchi

>
>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));
>>>  }
>>>  /**