| Message ID | 1449262730.2880.24.camel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 4 Dec 2015 22:58:50 +0200 Imre Deak <imre.deak@intel.com> wrote: > On Fri, 2015-12-04 at 10:37 -0800, Bob Paauwe wrote: > > On Fri, 4 Dec 2015 17:22:11 +0200 > > Imre Deak <imre.deak@intel.com> wrote: > > [...] > > > > 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. > > Ok, so for both of the above observations: If you write the min-freq > value via sysfs, it will result in cur-freq raised and "getting stuck" > at min-freq if cur-freq was below the new min value. This is a bit > strange since as we already established normally cur-freq should always > drop to idle-freq if the GPU is idle regardless of min-freq. AFAIR the > sysfs entry works in this way, since setting the frequency also changes > the interrupt mask and what we really need is this latter thing. To get > back to the normal drop-to-idle-freq policy again you need to submit > some GPU work after setting the sysfs entry. So we want the policy to be that we'll only drop below min to idle when the GPU transitions from not idle to idle. Without that transition, we'll stay at the user specified min. > It's done already in most > of the places in pm_rps, except for few. The following should fix up > those: > > diff --git a/tests/pm_rps.c b/tests/pm_rps.c > index 74f08f4..ad06ef0 100644 > --- a/tests/pm_rps.c > +++ b/tests/pm_rps.c > @@ -388,10 +388,14 @@ static void min_max_config(void (*check)(void), > bool load_gpu) > > igt_debug("\nIncrease min to midpoint...\n"); > writeval(stuff[MIN].filp, fmid); > + if (load_gpu) > + do_load_gpu(); > check(); > > igt_debug("\nIncrease min to RP0...\n"); > writeval(stuff[MIN].filp, origfreqs[RP0]); > + if (load_gpu) > + do_load_gpu(); > check(); > > igt_debug("\nIncrease min above RP0 (invalid)...\n"); > @@ -416,6 +420,8 @@ static void min_max_config(void (*check)(void), > bool load_gpu) > > igt_debug("\nDecrease min below RPn (invalid)...\n"); > writeval_inval(stuff[MIN].filp, 0); > + if (load_gpu) > + do_load_gpu(); > check(); > > igt_debug("\nDecrease max to midpoint...\n"); Yes, this does make the test pass. However, what is the expected behavior in the following: echo 500 > gt_min_freq_mhz echo 200 > gt_min_freq_mhz With no GPU load? Right now, the current frequency will stay stuck at 500 until there is GPU load or until I double set the min freq to 200. For example: bxt-test:drm root $ echo 500 > card0/gt_min_freq_mhz bxt-test:drm root $ cat card0/gt_cur_freq_mhz 500 bxt-test:drm root $ echo 200 > card0/gt_min_freq_mhz bxt-test:drm root $ cat card0/gt_cur_freq_mhz 500 bxt-test:drm root $ echo 200 > card0/gt_min_freq_mhz bxt-test:drm root $ cat card0/gt_cur_freq_mhz 200 If I add a delay before the posting read in gen6_set_rps() then the frequency will drop after the first "echo 200 >". It's like the gen6_set_rps() is behind. A slightly more interesting result is: echo 500 > min --> cur = 500 echo 200 > min --> cur = 500 echo 400 > min --> cur = 200 Something doesn't seem right.
On pe, 2015-12-04 at 14:41 -0800, Bob Paauwe wrote: > On Fri, 4 Dec 2015 22:58:50 +0200 > Imre Deak <imre.deak@intel.com> wrote: > [...] > So we want the policy to be that we'll only drop below min to idle > when > the GPU transitions from not idle to idle. Without that transition, > we'll stay at the user specified min. I would put it, that after an idle->not-idle transition we require that the frequency drops to the idle frequency. What happens right after writing a valid value to the min-freq sysfs entry is more loose, it can end up anywhere between the idle-freq..max(new-min-freq,old-cur-freq) range. > > It's done already in most > > of the places in pm_rps, except for few. The following should fix > > up > > those: > > > > diff --git a/tests/pm_rps.c b/tests/pm_rps.c > > index 74f08f4..ad06ef0 100644 > > --- a/tests/pm_rps.c > > +++ b/tests/pm_rps.c > > @@ -388,10 +388,14 @@ static void min_max_config(void > > (*check)(void), > > bool load_gpu) > > > > igt_debug("\nIncrease min to midpoint...\n"); > > writeval(stuff[MIN].filp, fmid); > > + if (load_gpu) > > + do_load_gpu(); > > check(); > > > > igt_debug("\nIncrease min to RP0...\n"); > > writeval(stuff[MIN].filp, origfreqs[RP0]); > > + if (load_gpu) > > + do_load_gpu(); > > check(); > > > > igt_debug("\nIncrease min above RP0 (invalid)...\n"); > > @@ -416,6 +420,8 @@ static void min_max_config(void (*check)(void), > > bool load_gpu) > > > > igt_debug("\nDecrease min below RPn (invalid)...\n"); > > writeval_inval(stuff[MIN].filp, 0); > > + if (load_gpu) > > + do_load_gpu(); > > check(); > > > > igt_debug("\nDecrease max to midpoint...\n"); > > Yes, this does make the test pass. Ok, this is according to expectations then. We don't actually need the last chunk, since for invalid settings there should be no change to cur-freq. So could you follow up with a cleaned up patch for this? > However, what is the expected behavior in the following: > > echo 500 > gt_min_freq_mhz > echo 200 > gt_min_freq_mhz > > With no GPU load? > > Right now, the current frequency will stay stuck at 500 until there > is > GPU load or until I double set the min freq to 200. For example: > > bxt-test:drm root $ echo 500 > card0/gt_min_freq_mhz > bxt-test:drm root $ cat card0/gt_cur_freq_mhz > 500 > bxt-test:drm root $ echo 200 > > card0/gt_min_freq_mhz > bxt-test:drm root $ cat card0/gt_cur_freq_mhz > 500 > bxt-test:drm root $ echo 200 > > card0/gt_min_freq_mhz > bxt-test:drm root $ cat card0/gt_cur_freq_mhz > 200 > > If I add a delay before the posting read in gen6_set_rps() then the > frequency will drop after the first "echo 200 >". It's like the > gen6_set_rps() is behind. A slightly more interesting result > is: > > echo 500 > min --> cur = 500 > echo 200 > min --> cur = 500 > echo 400 > min --> cur = 200 > > Something doesn't seem right. All the above can be explained by two things: 1. gt_min_freq_mhz_store() will not change cur-freq if cur-freq is already within the new min-freq..max-freq change. Otherwise it will raise cur-freq to the new min-freq value. 2. gt_min_freq_mhz_store() will access the HW whenever there is a valid min-freq passed in (so even if it just needs to reset the old cur-freq based on point 1.), so that the RPS interrupt mask is reprogrammed according to the new min-freq value. This HW access in turn can generate RPS down interrupts (even though the GPU is already idle) which can lower cur-freq as low as the min-freq value gen6_pm_rps_work() currently sees. All-in-all what we care about here is that cur-freq drops to idle-freq after an idle->not-idle transition and we can ignore the cur-freq value right after we wrote to the sysfs entry. Based on your tests the patch above would ensure that. --Imre
On Mon, 7 Dec 2015 17:00:20 +0200 Imre Deak <imre.deak@intel.com> wrote: > On pe, 2015-12-04 at 14:41 -0800, Bob Paauwe wrote: > > On Fri, 4 Dec 2015 22:58:50 +0200 > > Imre Deak <imre.deak@intel.com> wrote: > > [...] > > So we want the policy to be that we'll only drop below min to idle > > when > > the GPU transitions from not idle to idle. Without that transition, > > we'll stay at the user specified min. > > I would put it, that after an idle->not-idle transition we require that > the frequency drops to the idle frequency. What happens right after > writing a valid value to the min-freq sysfs entry is more loose, it can > end up anywhere between the idle-freq..max(new-min-freq,old-cur-freq) > range. > > > > It's done already in most > > > of the places in pm_rps, except for few. The following should fix > > > up > > > those: > > > > > > diff --git a/tests/pm_rps.c b/tests/pm_rps.c > > > index 74f08f4..ad06ef0 100644 > > > --- a/tests/pm_rps.c > > > +++ b/tests/pm_rps.c > > > @@ -388,10 +388,14 @@ static void min_max_config(void > > > (*check)(void), > > > bool load_gpu) > > > > > > igt_debug("\nIncrease min to midpoint...\n"); > > > writeval(stuff[MIN].filp, fmid); > > > + if (load_gpu) > > > + do_load_gpu(); > > > check(); > > > > > > igt_debug("\nIncrease min to RP0...\n"); > > > writeval(stuff[MIN].filp, origfreqs[RP0]); > > > + if (load_gpu) > > > + do_load_gpu(); > > > check(); > > > > > > igt_debug("\nIncrease min above RP0 (invalid)...\n"); > > > @@ -416,6 +420,8 @@ static void min_max_config(void (*check)(void), > > > bool load_gpu) > > > > > > igt_debug("\nDecrease min below RPn (invalid)...\n"); > > > writeval_inval(stuff[MIN].filp, 0); > > > + if (load_gpu) > > > + do_load_gpu(); > > > check(); > > > > > > igt_debug("\nDecrease max to midpoint...\n"); > > > > Yes, this does make the test pass. > > Ok, this is according to expectations then. We don't actually need the > last chunk, since for invalid settings there should be no change to > cur-freq. So could you follow up with a cleaned up patch for this? > > > However, what is the expected behavior in the following: > > > > echo 500 > gt_min_freq_mhz > > echo 200 > gt_min_freq_mhz > > > > With no GPU load? > > > > Right now, the current frequency will stay stuck at 500 until there > > is > > GPU load or until I double set the min freq to 200. For example: > > > > bxt-test:drm root $ echo 500 > card0/gt_min_freq_mhz > > bxt-test:drm root $ cat card0/gt_cur_freq_mhz > > 500 > > bxt-test:drm root $ echo 200 > > > card0/gt_min_freq_mhz > > bxt-test:drm root $ cat card0/gt_cur_freq_mhz > > 500 > > bxt-test:drm root $ echo 200 > > > card0/gt_min_freq_mhz > > bxt-test:drm root $ cat card0/gt_cur_freq_mhz > > 200 > > > > If I add a delay before the posting read in gen6_set_rps() then the > > frequency will drop after the first "echo 200 >". It's like the > > gen6_set_rps() is behind. A slightly more interesting result > > is: > > > > echo 500 > min --> cur = 500 > > echo 200 > min --> cur = 500 > > echo 400 > min --> cur = 200 > > > > Something doesn't seem right. > > All the above can be explained by two things: > 1. gt_min_freq_mhz_store() will not change cur-freq if cur-freq is > already within the new min-freq..max-freq change. Otherwise it will > raise cur-freq to the new min-freq value. > 2. gt_min_freq_mhz_store() will access the HW whenever there is a valid > min-freq passed in (so even if it just needs to reset the old cur-freq > based on point 1.), so that the RPS interrupt mask is reprogrammed > according to the new min-freq value. This HW access in turn can > generate RPS down interrupts (even though the GPU is already idle) > which can lower cur-freq as low as the min-freq value > gen6_pm_rps_work() currently sees. > > All-in-all what we care about here is that cur-freq drops to idle-freq > after an idle->not-idle transition and we can ignore the cur-freq value > right after we wrote to the sysfs entry. Based on your tests the patch > above would ensure that. > > --Imre OK, that makes sense. Thanks for taking the time to explain all this. I'll send out the patch to pm_rps.c shortly. Bob
diff --git a/tests/pm_rps.c b/tests/pm_rps.c index 74f08f4..ad06ef0 100644 --- a/tests/pm_rps.c +++ b/tests/pm_rps.c @@ -388,10 +388,14 @@ static void min_max_config(void (*check)(void), bool load_gpu) igt_debug("\nIncrease min to midpoint...\n"); writeval(stuff[MIN].filp, fmid); + if (load_gpu) + do_load_gpu(); check(); igt_debug("\nIncrease min to RP0...\n"); writeval(stuff[MIN].filp, origfreqs[RP0]); + if (load_gpu) + do_load_gpu(); check();