Re: [PATCH 4/4 v2] pm_rps: Require that cur reaches min at idle

[Jeff McGee <jeff.mcgee@intel.com>]

On Sat, Jan 25, 2014 at 08:46:45PM +0100, Daniel Vetter wrote:
> On Thu, Jan 23, 2014 at 03:54:50PM -0600, jeff.mcgee@intel.com wrote:
> > From: Jeff McGee <jeff.mcgee@intel.com>
> > 
> > The current frequency should reach the minimum frequency within a
> > reasonable time during idle.
> > 
> > v2: Not using forcewake for this particular subtest per Daniel's
> >     suggestion.
> > 
> > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
> 
> Hm, I guess I wasn't clear enough: I've thought about adding a new subtest
> which just does the check that the actual frequency reaches the lowest
> level on idle a new subtest, not extend the existing one.
> 
> Same somewhat holds for your first patch, atm the testcase is a very basic
> test of the kernel error checking.
> 
> But even ignoring that I'm not really sure what you're aiming at. Imo the
> current coverage is good enough since it makes sure that we have at least
> a bit of error checking in place. Any extensions to this test should imo
> only be done when we add new features or to exercise bugs (or classes of
> bugs) that actually happened. Iirc (and I didn't check olds mails, so this
> might be wrong) we have some corner-cases across suspend/resume and some
> with the in-kernel code to adjust the requested frequency under load. So
> imo that's what we should test for.
> 
> Reading through my test requirements write-up I've just noticed that I
> didn't emphasis that there's also too much testing possible imho. I'm not
> a big proponent of test driven developement and similar validate
> everything approaches. I guess I need to write up my thoughts about too
> much testing, too ;-)
> 
> Anyway I'm a bit confused about what's the overall goal here, so please
> elaborate.
> 
> Cheers, Daniel
> 
Hi Daniel. I guess it would have helped if I described my overall goals
with this test development in the beginning. I have two rps related
driver patches on hold from a few months ago because you felt the igt
testing wasn't adequate to accept these. They are:

"Update rps interrupt limits"
"Restore rps/rc6 on reset"

It took me some time to get around to this, but now I am intent on
expanding pm_rps with the proper subtests to expose the issues. An outline
of the subtests I have in mind:

min-max-config-at-idle
- Manipulate min and max through sysfs and make sure driver does the right
  thing. Right thing includes accepting valid values, rejecting invalid
  values, ensuring that cur remains between min and max, and (for idle)
  ensuring that cur reaches min after such changes. This last requirement
  is not being met in part because interrupt limits are not being updated
  properly when min and max are changed. That will be justification for
  patch "Update rps interrupt limits".
- I have been forming this subtest as an expansion of Ben's original test.
  His cases for min/max checking are a subset of this.

min-max-config-at-load
- Manipulate min and max as above, but do so during load. Check for the
  same basic requirements. When heavily loaded, cur should reach max. There
  is also potentially some problem with this scenario due to interrupt
  limits not being updated reliably. If cur is at max, and max is then
  increased, cur may stay at old max.
- This will be a new subtest that re-uses the min/max manipulation function
  but just do so under load.

restore-on-reset
- Trigger gpu reset, then test that user-set (non-default) min/max settings
  were retained and that turbo functions correctly when load is applied and
  removed. This scenario will fail today and is the point of "Restore rps/rc6
  on reset".
- This will be another new subtest not yet submitted,

So the subtests are focused on known issues. I'm open to any suggestions on
their design or implementation. Thanks.

Jeff