Re: [igt PATCH] igt/pm_rps: Remove remaining assert on CUR <= MAX

Arkadiusz Hiler
2017-06-21T09:55:49Z

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?