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

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

On Mon, Jan 27, 2014 at 05:50:04PM +0100, Daniel Vetter wrote:
> 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@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.
> 
> 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).
> 
Ah yes, expanding the subtest to the point that it uncovers an old problem
will appear as a regression from recent driver updates, which it is not. I
didn't consider this. I agree with your approach to have the current subtest
cover only basic min/max parameter validation. But I would recommend taking
patches 1-3 of this set - they work within that scope and will not cause
failures. Can we just drop patch 4? I will re-introduce it as part of adding
the new subtests.
-Jeff

> 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx