| Message ID | 20170614194401.25047-1-jeff.mcgee@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2017-06-14 at 12:44 -0700, jeff.mcgee@intel.com wrote: > From: Jeff McGee <jeff.mcgee@intel.com> > > This completes the change started by: > > commit 39cccab83b7c515a2b57abe679a8cb304c8933ef > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Fri May 19 09:41:40 2017 +0100 > > igt/pm_rps: Allow CUR to be greater than MAX (overclocking) > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com> Reviewed-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
On Tue, Jun 20, 2017 at 01:54:54PM +0000, Szwichtenberg, Radoslaw wrote: > On Wed, 2017-06-14 at 12:44 -0700, jeff.mcgee@intel.com wrote: > > From: Jeff McGee <jeff.mcgee@intel.com> > > > > This completes the change started by: > > > > commit 39cccab83b7c515a2b57abe679a8cb304c8933ef > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > Date: Fri May 19 09:41:40 2017 +0100 > > > > igt/pm_rps: Allow CUR to be greater than MAX (overclocking) > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com> > Reviewed-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> Pushed. Thanks for the patch and the review. Excerpt from CONTRIBUTING: ----------------------------------------------------------------------------- Please use --subject-prefix="PATCH i-g-t" so that i-g-t patches are easily identified in the massive amount mails on intel-gfx. To ensure this is always done, autogen.sh will run: git config format.subjectprefix "PATCH i-g-t" on its first invocation. ----------------------------------------------------------------------------- Lack of proper prefix breaks filtering / patchwork and makes changes harder to track, and effectilvely it takes longer to get the patch in. The autogen.sh thing is a recent addition.
On Tue, Jun 20, 2017 at 06:15:52PM +0300, Arkadiusz Hiler wrote: > On Tue, Jun 20, 2017 at 01:54:54PM +0000, Szwichtenberg, Radoslaw wrote: > > On Wed, 2017-06-14 at 12:44 -0700, jeff.mcgee@intel.com wrote: > > > From: Jeff McGee <jeff.mcgee@intel.com> > > > > > > This completes the change started by: > > > > > > commit 39cccab83b7c515a2b57abe679a8cb304c8933ef > > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > > Date: Fri May 19 09:41:40 2017 +0100 > > > > > > igt/pm_rps: Allow CUR to be greater than MAX (overclocking) > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com> > > Reviewed-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> > > Pushed. Thanks for the patch and the review. > > Excerpt from CONTRIBUTING: > ----------------------------------------------------------------------------- > Please use --subject-prefix="PATCH i-g-t" so that i-g-t patches are easily > identified in the massive amount mails on intel-gfx. To ensure this is always > done, autogen.sh will run: > > git config format.subjectprefix "PATCH i-g-t" > > on its first invocation. > ----------------------------------------------------------------------------- > > Lack of proper prefix breaks filtering / patchwork and makes changes > harder to track, and effectilvely it takes longer to get the patch in. > > The autogen.sh thing is a recent addition. Hey Jeff, The change has been reverted. At first the it looked like a sensible folloup to previous changes and it even got r-bed, but as pointed out by Michal, this constraint still should hold, as it's checked in a non-boost scenario. It hides bug introduced into the kernel by the commit: "drm/i915: Define a separate variable and control for RPS waitboost frequency" for GuC submission path, as the check below always holds true as we have waiters all the time: if (client_boost || any_waiters) max = hw_max; What was the point of this change? Were you aware of the fail in the guc_submission scenario?
On Wed, Jun 21, 2017 at 12:55:42PM +0300, Arkadiusz Hiler wrote: > On Tue, Jun 20, 2017 at 06:15:52PM +0300, Arkadiusz Hiler wrote: > > On Tue, Jun 20, 2017 at 01:54:54PM +0000, Szwichtenberg, Radoslaw wrote: > > > On Wed, 2017-06-14 at 12:44 -0700, jeff.mcgee@intel.com wrote: > > > > From: Jeff McGee <jeff.mcgee@intel.com> > > > > > > > > This completes the change started by: > > > > > > > > commit 39cccab83b7c515a2b57abe679a8cb304c8933ef > > > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > > > Date: Fri May 19 09:41:40 2017 +0100 > > > > > > > > igt/pm_rps: Allow CUR to be greater than MAX (overclocking) > > > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com> > > > Reviewed-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> > > > > Pushed. Thanks for the patch and the review. > > > > Excerpt from CONTRIBUTING: > > ----------------------------------------------------------------------------- > > Please use --subject-prefix="PATCH i-g-t" so that i-g-t patches are easily > > identified in the massive amount mails on intel-gfx. To ensure this is always > > done, autogen.sh will run: > > > > git config format.subjectprefix "PATCH i-g-t" > > > > on its first invocation. > > ----------------------------------------------------------------------------- > > > > Lack of proper prefix breaks filtering / patchwork and makes changes > > harder to track, and effectilvely it takes longer to get the patch in. > > > > The autogen.sh thing is a recent addition. > > Hey Jeff, > > The change has been reverted. > > At first the it looked like a sensible folloup to previous changes and > it even got r-bed, but as pointed out by Michal, this constraint still > should hold, as it's checked in a non-boost scenario. > > It hides bug introduced into the kernel by the commit: > "drm/i915: Define a separate variable and control for RPS waitboost frequency" > > for GuC submission path, as the check below always holds true as we have > waiters all the time: > if (client_boost || any_waiters) max = hw_max; > > What was the point of this change? > Were you aware of the fail in the guc_submission scenario? > > -- > Cheers, > Arek Hi Arek. Yes, this was prompted by a regression of this subtest in Yocto which uses GuC submission. I root caused it to this same i915 commit from Chris but incorrectly thought that the new "policy" was that sysfs max freq could be ignored in pretty much any situation. I understand now that it should be ignored in only boost and real user wait situations (not those triggered by GuC). So I do agree with this revert. Is there an i915 fix coming soon? Chris? Jeff
On Wed, Jun 21, 2017 at 08:57:21AM -0700, Jeff McGee wrote: > On Wed, Jun 21, 2017 at 12:55:42PM +0300, Arkadiusz Hiler wrote: > > On Tue, Jun 20, 2017 at 06:15:52PM +0300, Arkadiusz Hiler wrote: > > > On Tue, Jun 20, 2017 at 01:54:54PM +0000, Szwichtenberg, Radoslaw wrote: > > > > On Wed, 2017-06-14 at 12:44 -0700, jeff.mcgee@intel.com wrote: > > > > > From: Jeff McGee <jeff.mcgee@intel.com> > > > > > > > > > > This completes the change started by: > > > > > > > > > > commit 39cccab83b7c515a2b57abe679a8cb304c8933ef > > > > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > > > > Date: Fri May 19 09:41:40 2017 +0100 > > > > > > > > > > igt/pm_rps: Allow CUR to be greater than MAX (overclocking) > > > > > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com> > > > > Reviewed-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> > > > > > > Pushed. Thanks for the patch and the review. > > > > > > Excerpt from CONTRIBUTING: > > > ----------------------------------------------------------------------------- > > > Please use --subject-prefix="PATCH i-g-t" so that i-g-t patches are easily > > > identified in the massive amount mails on intel-gfx. To ensure this is always > > > done, autogen.sh will run: > > > > > > git config format.subjectprefix "PATCH i-g-t" > > > > > > on its first invocation. > > > ----------------------------------------------------------------------------- > > > > > > Lack of proper prefix breaks filtering / patchwork and makes changes > > > harder to track, and effectilvely it takes longer to get the patch in. > > > > > > The autogen.sh thing is a recent addition. > > > > Hey Jeff, > > > > The change has been reverted. > > > > At first the it looked like a sensible folloup to previous changes and > > it even got r-bed, but as pointed out by Michal, this constraint still > > should hold, as it's checked in a non-boost scenario. > > > > It hides bug introduced into the kernel by the commit: > > "drm/i915: Define a separate variable and control for RPS waitboost frequency" > > > > for GuC submission path, as the check below always holds true as we have > > waiters all the time: > > if (client_boost || any_waiters) max = hw_max; > > > > What was the point of this change? > > Were you aware of the fail in the guc_submission scenario? > > > > -- > > Cheers, > > Arek > > Hi Arek. Yes, this was prompted by a regression of this subtest in Yocto > which uses GuC submission. I root caused it to this same i915 commit > from Chris but incorrectly thought that the new "policy" was that sysfs > max freq could be ignored in pretty much any situation. I understand now > that it should be ignored in only boost and real user wait situations > (not those triggered by GuC). So I do agree with this revert. Hey, Thank you for the explanation. I see where to confusion issued from - the change seemed to make sense on it's own and to be just a simple follow up to the previous patch. > Is there an i915 fix coming soon? Chris? I think it's under the review on the ML already: https://patchwork.freedesktop.org/patch/163146/
diff --git a/tests/pm_rps.c b/tests/pm_rps.c index f0455e78..1a322459 100644 --- a/tests/pm_rps.c +++ b/tests/pm_rps.c @@ -145,7 +145,6 @@ static int do_writeval(FILE *filp, int val, int lerrno, bool readback_check) static void checkit(const int *freqs) { igt_assert_lte(freqs[MIN], freqs[MAX]); - igt_assert_lte(freqs[CUR], freqs[MAX]); igt_assert_lte(freqs[RPn], freqs[CUR]); igt_assert_lte(freqs[RPn], freqs[MIN]); igt_assert_lte(freqs[MAX], freqs[RP0]);