From: "Dixit, Ashutosh" <ashutosh.dixit@intel.com> To: "Belgaumkar, Vinay" <vinay.belgaumkar@intel.com> Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org> Subject: Re: [PATCH 2/3] drm/i915/guc/slpc: Add waitboost functionality for SLPC Date: Mon, 01 Nov 2021 13:28:06 -0700 [thread overview] Message-ID: <87pmrj39k9.wl-ashutosh.dixit@intel.com> (raw) In-Reply-To: <20211101043937.35747-3-vinay.belgaumkar@intel.com> 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. > +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.
WARNING: multiple messages have this Message-ID (diff)
From: "Dixit, Ashutosh" <ashutosh.dixit@intel.com> To: "Belgaumkar, Vinay" <vinay.belgaumkar@intel.com> Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org> Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915/guc/slpc: Add waitboost functionality for SLPC Date: Mon, 01 Nov 2021 13:28:06 -0700 [thread overview] Message-ID: <87pmrj39k9.wl-ashutosh.dixit@intel.com> (raw) In-Reply-To: <20211101043937.35747-3-vinay.belgaumkar@intel.com> 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. > +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.
next prev parent reply other threads:[~2021-11-01 20:37 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-11-01 4:39 [PATCH v2 0/3] drm/i915/guc/slpc: Implement waitboost for SLPC Vinay Belgaumkar 2021-11-01 4:39 ` [Intel-gfx] " Vinay Belgaumkar 2021-11-01 4:39 ` [PATCH 1/3] drm/i915/guc/slpc: Define and initialize boost frequency Vinay Belgaumkar 2021-11-01 4:39 ` [Intel-gfx] " Vinay Belgaumkar 2021-11-01 20:26 ` Dixit, Ashutosh 2021-11-01 20:26 ` [Intel-gfx] " Dixit, Ashutosh 2021-11-02 0:20 ` Belgaumkar, Vinay 2021-11-02 0:20 ` [Intel-gfx] " Belgaumkar, Vinay 2021-11-01 4:39 ` [PATCH 2/3] drm/i915/guc/slpc: Add waitboost functionality for SLPC Vinay Belgaumkar 2021-11-01 4:39 ` [Intel-gfx] " Vinay Belgaumkar 2021-11-01 20:28 ` Dixit, Ashutosh [this message] 2021-11-01 20:28 ` Dixit, Ashutosh 2021-11-02 0:19 ` Belgaumkar, Vinay 2021-11-02 0:19 ` [Intel-gfx] " Belgaumkar, Vinay 2021-11-01 4:39 ` [PATCH 3/3] drm/i915/guc/slpc: Update boost sysfs hooks " Vinay Belgaumkar 2021-11-01 4:39 ` [Intel-gfx] " Vinay Belgaumkar 2021-11-01 20:28 ` Dixit, Ashutosh 2021-11-01 20:28 ` [Intel-gfx] " Dixit, Ashutosh 2021-11-04 0:39 ` Dixit, Ashutosh 2021-11-04 0:39 ` [Intel-gfx] " Dixit, Ashutosh 2021-11-01 5:05 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/guc/slpc: Implement waitboost for SLPC (rev2) Patchwork 2021-11-01 5:37 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 2021-11-01 6:53 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork 2021-11-01 20:24 ` [PATCH v2 0/3] drm/i915/guc/slpc: Implement waitboost for SLPC Dixit, Ashutosh 2021-11-01 20:24 ` [Intel-gfx] " Dixit, Ashutosh 2021-11-02 0:18 ` Belgaumkar, Vinay 2021-11-02 0:18 ` [Intel-gfx] " Belgaumkar, Vinay -- strict thread matches above, loose matches on Subject: below -- 2021-11-02 1:26 [PATCH v3 " Vinay Belgaumkar 2021-11-02 1:26 ` [PATCH 2/3] drm/i915/guc/slpc: Add waitboost functionality " Vinay Belgaumkar 2021-11-04 0:23 ` Dixit, Ashutosh 2021-10-20 19:52 [Intel-gfx] [PATCH 0/3] drm/i915/guc/slpc: Implement waitboost " Vinay Belgaumkar 2021-10-20 19:52 ` [PATCH 2/3] drm/i915/guc/slpc: Add waitboost functionality " Vinay Belgaumkar
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87pmrj39k9.wl-ashutosh.dixit@intel.com \ --to=ashutosh.dixit@intel.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=vinay.belgaumkar@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.