All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: "Szwichtenberg, Radoslaw" <radoslaw.szwichtenberg@intel.com>
Cc: "Dec, Katarzyna" <katarzyna.dec@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH i-g-t v4] pm_rps: Changes in waitboost scenario
Date: Tue, 29 Aug 2017 10:30:43 +0200	[thread overview]
Message-ID: <20170829083043.nlnzb5nv5bco7vqx@phenom.ffwll.local> (raw)
In-Reply-To: <1503992597.17329.11.camel@intel.com>

On Tue, Aug 29, 2017 at 07:43:20AM +0000, Szwichtenberg, Radoslaw wrote:
> On Mon, 2017-08-28 at 10:50 +0200, Katarzyna Dec wrote:
> > CI is observing sporadical failures in pm_rps subtests.
> > There are a couple of reasons. One of them is the fact that
> > on gen6, gen7 and gen7.5, max frequency (as in the HW limit)
> > is not set to RP0, but the value obtaind from PCODE (which
> > may be different from RP0). Thus the test is operating under
> > wrong assumptions (SOFTMAX == RP0 == BOOST which is simply
> > not the case). Let's compare current frequency with BOOST
> > frequency rather than SOFTMAX to get the test behaviour under control.
> > In boost_freq function I set MAX freq to midium freqency, which ensures
> > that we for sure reach BOOST frequency. This could help with failures
> > with boost frequency failing to drop down.
> > GPU reset needs to be modified so we are not dependent on kernel's low
> > priority retire worker. Reset method was replaced by igt_force_gpu_reset()
> > and in reset testcase we make sure that we can recover from hang.
> > 
> > v2: Commit message, simplified waiting for boost to finish, drop
> > noisy whitespace cleanup.
> > 
> > v3: Removed reading from i915_rps_boost_info debugfs because it not
> > the same on every kernel. Removed function waiting for boost.
> > Instead of that I made sure we will reach in boost by setting MAX freq to
> > fmid.
> > 
> > v4: Moved proposal with making test drm master to other patch
> > 
> > v5: Used igt_force_gpu_reset() to reset GPU. Modified "reset" testcase.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Jeff Mcgee <jeff.mcgee@intel.com>
> > Cc: Petri Latvala <petri.latvala@intel.com>
> > Cc: Jani Saarinen <jani.saarinen@intel.com>
> > Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
> > Signed-off-by: Katarzyna Dec <katarzyna.dec@intel.com>
> > ---
> >  tests/pm_rps.c | 63 +++++++++++++++++++++++++++++++++++--------------------
> > ---
> >  1 file changed, 38 insertions(+), 25 deletions(-)
> > 
> > diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> > index f0455e78..a6c6f1eb 100644
> > --- a/tests/pm_rps.c
> > +++ b/tests/pm_rps.c
> > @@ -50,6 +50,7 @@ enum {
> >  	RP0,
> >  	RP1,
> >  	RPn,
> > +	BOOST,
> >  	NUMFREQ
> >  };
> >  
> > @@ -60,7 +61,7 @@ struct junk {
> >  	const char *mode;
> >  	FILE *filp;
> >  } stuff[] = {
> > -	{ "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL },
> > { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { NULL,
> > NULL, NULL }
> > +	{ "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL },
> > { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { "boost",
> > "rb+", NULL }, { NULL, NULL, NULL }
> >  };
> >  
> >  static int readval(FILE *filp)
> > @@ -167,7 +168,7 @@ static void dump(const int *freqs)
> >  }
> >  
> >  enum load {
> > -	LOW,
> > +	LOW = 0,
> >  	HIGH
> >  };
> >  
> > @@ -185,9 +186,10 @@ static struct load_helper {
> >  
> >  static void load_helper_signal_handler(int sig)
> >  {
> > -	if (sig == SIGUSR2)
> > -		lh.load = lh.load == LOW ? HIGH : LOW;
> > -	else
> > +	if (sig == SIGUSR2) {
> > +		lh.load = !lh.load;
> > +		igt_debug("Switching background load to %s\n", lh.load ?
> > "high" : "low");
> > +	} else
> >  		lh.exit = true;
> >  }
> >  
> > @@ -238,6 +240,7 @@ static void load_helper_run(enum load load)
> >  		return;
> >  	}
> >  
> > +	lh.exit = false;
> >  	lh.load = load;
> >  
> >  	igt_fork_helper(&lh.igt_proc) {
> > @@ -263,6 +266,8 @@ static void load_helper_run(enum load load)
> >  		if (intel_gen(lh.devid) >= 6)
> >  			execbuf.flags = I915_EXEC_BLT;
> >  
> > +		igt_debug("Applying %s load...\n", lh.load ? "high" : "low");
> > +
> >  		while (!lh.exit) {
> >  			memset(&object, 0, sizeof(object));
> >  			object.handle = fences[val%3];
> > @@ -296,6 +301,8 @@ static void load_helper_run(enum load load)
> >  		gem_close(drm_fd, fences[0]);
> >  		gem_close(drm_fd, fences[1]);
> >  		gem_close(drm_fd, fences[2]);
> > +
> > +		igt_drop_caches_set(drm_fd, DROP_RETIRE);
> >  	}
> >  }
> >  
> > @@ -553,38 +560,43 @@ static void stabilize_check(int *out)
> >  	igt_debug("Waited %d msec to stabilize cur\n", wait);
> >  }
> >  
> > -static void reset_gpu(void)
> > -{
> > -	int fd = drm_open_driver(DRIVER_INTEL);
> > -	igt_post_hang_ring(fd, igt_hang_ring(fd, I915_EXEC_DEFAULT));
> > -	close(fd);
> > -}
> > -
> >  static void boost_freq(int fd, int *boost_freqs)
> >  {
> >  	int64_t timeout = 1;
> > -	int ring = -1;
> >  	igt_spin_t *load;
> > +	unsigned int engine;
> > +	int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2;
> >  
> > -	load = igt_spin_batch_new(fd, ring, 0);
> > +	fmid = get_hw_rounded_freq(fmid);
> > +	//set max freq to less then boost freq
> Looks good to me.
> Just one very minor comment - we do use two different comment styles, should we
> instead use one? Do you think we should add any simple test descriptions above
> the tests or these tests are easy to understand?

We try to follow the kernel's CodingStyle, see

https://01.org/linuxgraphics/gfx-docs/drm/process/coding-style.html

This means /* */ C style comments everywhere. Please fix.

> > +	writeval(stuff[MAX].filp, fmid);
> >  
> > +	/* put boost on the same engine as low load */
> > +	engine = I915_EXEC_RENDER;
> 
> Otherwise
> Reviewed-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com

When you resubmit, plus include Radek's r-b tag.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2017-08-29  8:30 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-18  7:33 [PATCH i-g-t] pm_rps: Changes in waitboost scenario Katarzyna Dec
2017-08-18  7:57 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-08-18 11:08 ` [PATCH i-g-t v2] " Katarzyna Dec
2017-08-18 13:45   ` Chris Wilson
2017-08-18 20:28     ` Daniel Vetter
2017-08-18 20:42       ` Chris Wilson
2017-08-21  8:29         ` Dec, Katarzyna
2017-08-21  8:53           ` Chris Wilson
2017-08-21 10:43             ` Dec, Katarzyna
2017-08-21 11:29               ` Chris Wilson
2017-08-18 13:47   ` Chris Wilson
2017-08-21 13:50   ` [PATCH i-g-t v3] " Katarzyna Dec
2017-08-22 12:40     ` Katarzyna Dec
2017-08-24  9:44       ` Chris Wilson
2017-08-28  8:50       ` [PATCH i-g-t v4] " Katarzyna Dec
2017-08-29  7:43         ` Szwichtenberg, Radoslaw
2017-08-29  8:30           ` Daniel Vetter [this message]
2017-08-29  8:57         ` [PATCH i-g-t v5] " Katarzyna Dec
2017-08-30 13:05           ` [PATCH i-g-t v6] " Katarzyna Dec
2017-08-30 13:21             ` [PATCH i-g-t v7] " Katarzyna Dec
2017-08-31  7:40               ` [PATCH i-g-t v8] " Katarzyna Dec
2017-08-18 13:22 ` ✓ Fi.CI.BAT: success for pm_rps: Changes in waitboost scenario (rev2) Patchwork
2017-08-18 13:38 ` [PATCH i-g-t] pm_rps: Changes in waitboost scenario Chris Wilson
2017-08-21 14:22 ` ✓ Fi.CI.BAT: success for pm_rps: Changes in waitboost scenario (rev3) Patchwork
2017-08-22 13:00 ` ✓ Fi.CI.BAT: success for pm_rps: Changes in waitboost scenario (rev4) Patchwork
2017-08-28  9:09 ` ✓ Fi.CI.BAT: success for pm_rps: Changes in waitboost scenario (rev6) Patchwork
2017-08-28 10:20 ` ✗ Fi.CI.IGT: warning " Patchwork
2017-08-29  9:16 ` ✓ Fi.CI.BAT: success for pm_rps: Changes in waitboost scenario (rev7) Patchwork
2017-08-29 10:27 ` ✗ Fi.CI.IGT: warning " Patchwork
2017-08-30 13:28 ` ✓ Fi.CI.BAT: success for pm_rps: Changes in waitboost scenario (rev8) Patchwork
2017-08-30 13:45 ` ✓ Fi.CI.BAT: success for pm_rps: Changes in waitboost scenario (rev9) Patchwork
2017-08-30 14:53 ` ✗ Fi.CI.IGT: warning " Patchwork
2017-08-31 13:03 ` ✗ Fi.CI.BAT: failure for pm_rps: Changes in waitboost scenario (rev10) Patchwork
2017-08-31 14:31   ` Arkadiusz Hiler

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170829083043.nlnzb5nv5bco7vqx@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=katarzyna.dec@intel.com \
    --cc=radoslaw.szwichtenberg@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.