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

[Imre Deak]

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 at 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