On 11/1/2021 1:28 PM, Dixit, Ashutosh wrote:
On Sun, 31 Oct 2021 21:39:36 -0700, Belgaumkar, Vinay wrote:@@ -945,6 +960,17 @@ void intel_rps_boost(struct i915_request *rq) if (!test_and_set_bit(I915_FENCE_FLAG_BOOST, &rq->fence.flags)) { struct intel_rps *rps = &READ_ONCE(rq->engine)->gt->rps; + if (rps_uses_slpc(rps)) { + slpc = rps_to_slpc(rps); + + /* Return if old value is non zero */ + if (atomic_fetch_inc(&slpc->num_waiters)) + return; + + if (intel_rps_get_requested_frequency(rps) < slpc->boost_freq)I think this check is not needed because: a. The waitboost code only changes min_freq. i915 code should not depend on how GuC changes requested_freq in response to change in min_freq. b. What is more worrisome is that when we "de-boost" we set min_freq to min_freq_softlimit. If GuC e.g. has a delay in bringing requested_freq down and intel_rps_boost() gets called meanwhile we will miss the one opportunity we have to boost the freq (when num_waiters goes from 0 to 1. Asking GuC to boost when actual_freq is already boost_freq is harmless in comparison). So to avoid this risk of missing the chance to boost I think we should delete this check and replace the code above with something like: if (rps_uses_slpc(rps)) { struct intel_guc_slpc *slpc = rps_to_slpc(rps); if (slpc->boost_freq <= slpc->min_freq_softlimit) return; if (!atomic_fetch_inc(&slpc->num_waiters)) schedule_work(&slpc->boost_work); return; } Note that this check: if (slpc->boost_freq <= slpc->min_freq_softlimit) return; (which is basically a degenerate case in which we don't have to do anything), can be probably be implemented when boost_freq is set in sysfs, or may already be encompassed in "val < slpc->min_freq" in intel_guc_slpc_set_boost_freq() in which case this check can also be skipped from this function.
We already have that check in set_boost_freq function. So, just adding the atomic_fetch_inc check.
+void intel_guc_slpc_dec_waiters(struct intel_guc_slpc *slpc) +{ + /* Return min back to the softlimit. + * This is called during request retire, + * so we don't need to fail that if the + * set_param fails. + */nit: maybe follow kernel multi-line comment format.
Ok. Thanks, Vinay.
