[Intel-gfx] [PATCH 2/2] igt/pm_rps: Add checks for freq = idle (RPn) in specific cases.

Fri Dec 4 10:37:28 PST 2015

On Fri, 4 Dec 2015 17:22:11 +0200
Imre Deak <imre.deak at intel.com> wrote:

> On to, 2015-12-03 at 16:43 -0800, Bob Paauwe wrote:
> > On Tue, 1 Dec 2015 19:43:05 +0200
> > Imre Deak <imre.deak at intel.com> wrote:
> > 
> > > On ti, 2015-12-01 at 09:22 -0800, Bob Paauwe wrote:
> > > > On Tue, 1 Dec 2015 15:56:55 +0200
> > > > Imre Deak <imre.deak at intel.com> wrote:
> > > > 
> > > > > On ma, 2015-11-30 at 16:23 -0800, Bob Paauwe wrote:
> > > > > > Now that the frequency can drop below the user specified
> > > > > > minimum
> > > > > > when
> > > > > > the gpu is idle, add some checking to verify that it really
> > > > > > does
> > > > > > drop
> > > > > > down to the RPn frequency in specific cases.
> > > > > > 
> > > > > > To do this, modify the main test flow to take two 'check'
> > > > > > routines
> > > > > > instead of one. When running the config-min-max-idle subtest
> > > > > > make
> > > > > > use of the second check routine to verify that the frequency
> > > > > > drops
> > > > > > to RPn instead of simply <= user min frequency.  For all
> > > > > > other
> > > > > > subtests, use the original check routines for both checks.
> > > > > > 
> > > > > > Signed-off-by: Bob Paauwe <bob.j.paauwe at intel.com>
> > > > > 
> > > > > I don't see the point. The frequency should always drop to the
> > > > > idle
> > > > > frequency if the GPU is idle, it's not enough that it's below
> > > > > MIN-
> > > > > freq. 
> > > > > So why do we need separate CUR-freq<=MIN-freq and CUR-
> > > > > freq==idle-
> > > > > freq
> > > > > checks?
> > > > 
> > > > I started from the premise that it should always drop to idle but
> > > > that's just not the case.
> > > 
> > > It is the correct premise and if it doesn't hold then there is a
> > > real
> > > bug either in the testcase or the kernel which needs to be
> > > addressed
> > > differently. I haven't found anything concrete but one suspect is
> > > the
> > > logic that waits for the GPU idle condition, maybe the timeout
> > > value idle_check() or the hard-coded duration in do_load_gpu() is
> > > incorrect. In any case let's not paper over this issue, the very
> > > reason
> > > we have test cases is to uncover such bugs.
> > > 
> > > > The min_max_config() function is used for
> > > > both the idle and loaded subtests.  If you try to check for
> > > > freq=idle
> > > > when doing the loaded subtest it will fail since it never goes
> > > > idle.
> > > > Even in the idle subtest there are cases where it doesn't drop
> > > > down
> > > > to
> > > > idle. 
> > > 
> > > The only place we should check for freq==idle is in idle_check()
> > > and
> > > that one is not called during the loaded subtest, it wouldn't even
> > > make
> > > sense to do so. So I'm not sure how this subtest fails for you. 
> > > 
> > > > I suppose I could duplicate min_max_config and have a
> > > > min_max_config_idle() and min_max_conifg_not_idle() for use by
> > > > the
> > > > different subtests. 
> > > 
> > > The point of the "check" argument of min_max_config() is to
> > > distinguish
> > > between the idle and loaded cases. The check functions passed in
> > > know
> > > already if they can expect the frequency to reach the idle
> > > frequency
> > > or not.
> > > 
> > > > The real problem is that the test was not designed to handle the
> > > > case
> > > > where the freq could drop below min and probably needs to be
> > > > re-designed.  I've been trying to find a quick fix vs. a complete
> > > > overhaul as this isn't really a priority for me.
> > > 
> > > I think we only need your first patch and if there is any failure
> > > after
> > > that we have to root cause that and fix it properly.
> > > 
> > > --Imre
> > 
> > You're right.  I'm working with BXT and it seems like it's got some
> > unique issues with RPS.  I just sent a new patch based on the first
> > one
> > but with the changes you suggested to check for ==idle instead of
> > <=min.
> > 
> > Maybe you have some insights into what I'm seeing with BXT?  The first
> > problem I had was that I would see threshold up interrupts but not any
> > threshold down interrupts.  The missing down interrupts was related to
> > the BIOS setting.  I had runtime PM disabled so it seems strange that I
> > was getting the up interrupts. 
> 
> Yes, I saw this too. I wonder if we could check this somehow and not
> enable RPS if BIOS hasn't set things up properly. Sagar has a patch
> that checks the RC6 setup [1]; that's not exactly RPS, but since they
> are setup at the same place I think we could use that for now for RPS
> too.

I'll take a look at that patch and see if I can do something like that
for RPS.  Thanks.

> 
> > With the BIOS setting corrected, the driver started disabling the down
> > interrupts once the freq dropped to min or just below because of the min
> > vs. idle difference.  I have a patch for this that I'll send shortly.
> 
> Hm, that's not necessarily a problem. To reach the idle frequency we
> don't depend on the up/down interrupts. The idle frequency gets set
> explicitly from intel_mark_idle(), so if you don't reach the idle freq
> then this function doesn't get called for some reason. Or as I said
> earlier we just don't wait enough in do_load_gpu() or idle_check() in
> the test, so we read out cur-freq before intel_mark_idle() would get
> called.

Maybe because I'm just testing from a console environment, but if I
change the min freq. via sysfs I don't get any type of call to drop
back to idle.  Possibly because I'm not generating any GPU load so it
never re-triggers an idle gpu?  I've waited quite a while and have
bumped up the time in idle_check().  Waiting for minutes doesn't seem
reasonable.

Should intel_mark_idle() get called periodically, even if the rings are
empty?

> 
> > The remaining issue seems to be some type of timing issue. I've had
> > to
> > add a 35000us sleep between updating the interrupt enable register
> > (0xA168) and the posting read of freq. register (0xA008), otherwise
> > it
> > acts like the change to the interrupt enable register never happened.
> > None of the documentation I have indicates that this is needed.
> 
> What happens exactly? The frequency gets stuck above min-freq and you
> never get a down interrupt? Not sure how this could happen, but there
> is an interesting comment in intel_rps_limits() about this scenario.

Basically if I set the min freq. through sysfs, it flows through all
the set threshold, create the interrupt mask code but the system
behaves as if the new threshold limits or interrupts were not changed.

So say RPn = 100, min = 100, current = 100
 - echo 383 > /sys/class/drm/card0/gt_min_freq_mhz  (midpoint freq)

gt_cur_freq_mhz will report 383 now and I don't see any interrupts.

If I add the delay, then right after I set the min to 383, I'll start
seeing the down threshold interrupts and it will lower the freq back
down to RPn (100) after two or three interrupts occur.

So I don't think this has anything to with RC6 and missing
interrupts that the comment refers to.

It took me a while to track this down because I was using a bunch of
printk's to trace the flow and values.  With the printk's it was
working but when I removed them, it stopped working. So it was about 3
or 4 printk's worth of delay that was needed.  But I really wonder if
there's something else that should be checked vs. just delaying. 

Bob

> 
> --Imre
> 
> [1] http://lists.freedesktop.org/archives/intel-gfx/2015-November/07938
> 6.html
> 
> 
> > 
> > Bob
> > 
> > 
> > > 

> > 
> > 
> > 

-- 
--
Bob Paauwe                  
Bob.J.Paauwe at intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    