Re: [PATCH 2/3] drm/i915/guc/slpc: Add waitboost functionality for SLPC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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.



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux