[Intel-gfx] [PATCH 4/4 v2] pm_rps: Require that cur reaches min at idle

[Daniel Vetter]

On Mon, Jan 27, 2014 at 10:24:53AM -0600, Jeff McGee wrote:
> 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 at intel.com wrote:
> > > From: Jeff McGee <jeff.mcgee at 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 at 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.

Oh, I didn't realize that the idle limits are currently broken and that
your first patches fixes that. I think I now also understand why do you
the idle-check at each step, so that this actually gets properly
exercised.

Problem with your approach here is that this will cause a spurious
regression report from our QA since the current min-max-config-at-idle
will suddenly starts failing (since it will test more and currently broken
things with your patches applied). And that might shadow other basic
issues (yeah, unlikely but still).

So I think the right approach here is to keep the current subtest as-is
(maybe rename it to "basic-api" since that's pretty much the only thing it
does), and then add a completely new set of subtests like you've laid out
here. So just leave the existing code as-is and copypaste the code for all
your new tests (where you make changes at least).

It's probably simplest if you do this shuffling to avoid a rebase mess
with your existing patches.

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

Excellent test plan imo and thanks a lot for unconfusing me.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch