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

From: Bob Paauwe <bob.j.paauwe@intel.com>
To: Imre Deak <imre.deak@intel.com>
Cc: intel-gfx <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 2/2] igt/pm_rps: Add checks for freq = idle (RPn) in specific cases.
Date: Mon, 7 Dec 2015 13:56:18 -0800	[thread overview]
Message-ID: <20151207135618.5bfde35f@bpaauwe-desk.fm.intel.com> (raw)
In-Reply-To: <1449500420.32540.23.camel@intel.com>

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

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx