| Message ID | 20171017124330.13273-1-radoslaw.szwichtenberg@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 17, 2017 at 02:43:30PM +0200, Radoslaw Szwichtenberg wrote: > Moving code out of the boost function will allow its usage > in other/new test scenarios. > > Signed-off-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> https://lists.freedesktop.org/mailman/listinfo/intel-gfx Reviewed-by: Katarzyna Dec <katarzyna.dec@intel.com> Cheers, Kasia
On 10/17/2017 6:13 PM, Radoslaw Szwichtenberg wrote: > Moving code out of the boost function will allow its usage > in other/new test scenarios. > > Signed-off-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > tests/pm_rps.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/tests/pm_rps.c b/tests/pm_rps.c > index 89f3e31c..dad87646 100644 > --- a/tests/pm_rps.c > +++ b/tests/pm_rps.c > @@ -583,11 +583,6 @@ static void boost_freq(int fd, int *boost_freqs) > int64_t timeout = 1; > igt_spin_t *load; > unsigned int engine; > - int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; > - > - fmid = get_hw_rounded_freq(fmid); > - /* Set max freq to less then boost freq */ > - writeval(sysfs_files[MAX].filp, fmid); > > /* Put boost on the same engine as low load */ > engine = I915_EXEC_RENDER; > @@ -604,9 +599,6 @@ static void boost_freq(int fd, int *boost_freqs) > igt_spin_batch_end(load); > gem_sync(fd, load->handle); > igt_spin_batch_free(fd, load); > - > - /* Set max freq to original softmax */ > - writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > } > > static void waitboost(int fd, bool reset) > @@ -615,6 +607,9 @@ static void waitboost(int fd, bool reset) > int boost_freqs[NUMFREQ]; > int post_freqs[NUMFREQ]; > > + int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; > + fmid = get_hw_rounded_freq(fmid); > + How about function for getting mid? We can reuse in min_max_config too. > load_helper_run(LOW); > > igt_debug("Apply low load...\n"); > @@ -627,11 +622,17 @@ static void waitboost(int fd, bool reset) > sleep(1); > } > > + /* Set max freq to less than boost freq */ > + writeval(sysfs_files[MAX].filp, fmid); > + > /* When we wait upon the GPU, we want to temporarily boost it > * to maximum. > */ > boost_freq(fd, boost_freqs); > > + /* Set max freq to original softmax */ > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > + > igt_debug("Apply low load again...\n"); > sleep(1); > stabilize_check(post_freqs); > @@ -643,7 +644,6 @@ static void waitboost(int fd, bool reset) > igt_assert_lt(pre_freqs[CUR], pre_freqs[MAX]); > igt_assert_eq(boost_freqs[CUR], boost_freqs[BOOST]); > igt_assert_lt(post_freqs[CUR], post_freqs[MAX]); > - > } > > static void pm_rps_exit_handler(int sig) > @@ -656,6 +656,7 @@ static void pm_rps_exit_handler(int sig) > writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > } > > + writeval(sysfs_files[BOOST].filp, origfreqs[BOOST]); We are not changing boost_freq in current patch. Is this planned in new testcase? Please defer this change until then. > load_helper_deinit(); > close(drm_fd); > }
On Wed, 2017-10-18 at 11:10 +0530, Sagar Arun Kamble wrote: > > On 10/17/2017 6:13 PM, Radoslaw Szwichtenberg wrote: > > Moving code out of the boost function will allow its usage > > in other/new test scenarios. > > > > Signed-off-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> > > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > --- > > tests/pm_rps.c | 19 ++++++++++--------- > > 1 file changed, 10 insertions(+), 9 deletions(-) > > > > diff --git a/tests/pm_rps.c b/tests/pm_rps.c > > index 89f3e31c..dad87646 100644 > > --- a/tests/pm_rps.c > > +++ b/tests/pm_rps.c > > @@ -583,11 +583,6 @@ static void boost_freq(int fd, int *boost_freqs) > > int64_t timeout = 1; > > igt_spin_t *load; > > unsigned int engine; > > - int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; > > - > > - fmid = get_hw_rounded_freq(fmid); > > - /* Set max freq to less then boost freq */ > > - writeval(sysfs_files[MAX].filp, fmid); > > > > /* Put boost on the same engine as low load */ > > engine = I915_EXEC_RENDER; > > @@ -604,9 +599,6 @@ static void boost_freq(int fd, int *boost_freqs) > > igt_spin_batch_end(load); > > gem_sync(fd, load->handle); > > igt_spin_batch_free(fd, load); > > - > > - /* Set max freq to original softmax */ > > - writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > > } > > > > static void waitboost(int fd, bool reset) > > @@ -615,6 +607,9 @@ static void waitboost(int fd, bool reset) > > int boost_freqs[NUMFREQ]; > > int post_freqs[NUMFREQ]; > > > > + int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; > > + fmid = get_hw_rounded_freq(fmid); > > + > > How about function for getting mid? We can reuse in min_max_config too. You think about something like: static int get_fmid(void) { int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; return get_hw_rounded_freq(fmid); } Even though it wasn't scope of this change I think I can add it - do you think it will be beneficial? We just use it two places so at this moment the benefit will be minimal. > > load_helper_run(LOW); > > > > igt_debug("Apply low load...\n"); > > @@ -627,11 +622,17 @@ static void waitboost(int fd, bool reset) > > sleep(1); > > } > > > > + /* Set max freq to less than boost freq */ > > + writeval(sysfs_files[MAX].filp, fmid); > > + > > /* When we wait upon the GPU, we want to temporarily boost it > > * to maximum. > > */ > > boost_freq(fd, boost_freqs); > > > > + /* Set max freq to original softmax */ > > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > > + > > igt_debug("Apply low load again...\n"); > > sleep(1); > > stabilize_check(post_freqs); > > @@ -643,7 +644,6 @@ static void waitboost(int fd, bool reset) > > igt_assert_lt(pre_freqs[CUR], pre_freqs[MAX]); > > igt_assert_eq(boost_freqs[CUR], boost_freqs[BOOST]); > > igt_assert_lt(post_freqs[CUR], post_freqs[MAX]); > > - > > } > > > > static void pm_rps_exit_handler(int sig) > > @@ -656,6 +656,7 @@ static void pm_rps_exit_handler(int sig) > > writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > > } > > > > + writeval(sysfs_files[BOOST].filp, origfreqs[BOOST]); > > We are not changing boost_freq in current patch. Is this planned in new > testcase? > Please defer this change until then. Will remove! Thanks for review! Radek > > load_helper_deinit(); > > close(drm_fd); > > } > >
On 10/18/2017 4:33 PM, Szwichtenberg, Radoslaw wrote: > On Wed, 2017-10-18 at 11:10 +0530, Sagar Arun Kamble wrote: >> On 10/17/2017 6:13 PM, Radoslaw Szwichtenberg wrote: >>> Moving code out of the boost function will allow its usage >>> in other/new test scenarios. >>> >>> Signed-off-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> >>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>> --- >>> tests/pm_rps.c | 19 ++++++++++--------- >>> 1 file changed, 10 insertions(+), 9 deletions(-) >>> >>> diff --git a/tests/pm_rps.c b/tests/pm_rps.c >>> index 89f3e31c..dad87646 100644 >>> --- a/tests/pm_rps.c >>> +++ b/tests/pm_rps.c >>> @@ -583,11 +583,6 @@ static void boost_freq(int fd, int *boost_freqs) >>> int64_t timeout = 1; >>> igt_spin_t *load; >>> unsigned int engine; >>> - int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; >>> - >>> - fmid = get_hw_rounded_freq(fmid); >>> - /* Set max freq to less then boost freq */ >>> - writeval(sysfs_files[MAX].filp, fmid); >>> >>> /* Put boost on the same engine as low load */ >>> engine = I915_EXEC_RENDER; >>> @@ -604,9 +599,6 @@ static void boost_freq(int fd, int *boost_freqs) >>> igt_spin_batch_end(load); >>> gem_sync(fd, load->handle); >>> igt_spin_batch_free(fd, load); >>> - >>> - /* Set max freq to original softmax */ >>> - writeval(sysfs_files[MAX].filp, origfreqs[MAX]); >>> } >>> >>> static void waitboost(int fd, bool reset) >>> @@ -615,6 +607,9 @@ static void waitboost(int fd, bool reset) >>> int boost_freqs[NUMFREQ]; >>> int post_freqs[NUMFREQ]; >>> >>> + int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; >>> + fmid = get_hw_rounded_freq(fmid); >>> + >> How about function for getting mid? We can reuse in min_max_config too. > You think about something like: > > static int get_fmid(void) > { > int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; > return get_hw_rounded_freq(fmid); > } > > Even though it wasn't scope of this change I think I can add it - do you think > it will be beneficial? We just use it two places so at this moment the benefit > will be minimal. Ok. Can skip for now. >>> load_helper_run(LOW); >>> >>> igt_debug("Apply low load...\n"); >>> @@ -627,11 +622,17 @@ static void waitboost(int fd, bool reset) >>> sleep(1); >>> } >>> >>> + /* Set max freq to less than boost freq */ >>> + writeval(sysfs_files[MAX].filp, fmid); >>> + >>> /* When we wait upon the GPU, we want to temporarily boost it >>> * to maximum. >>> */ >>> boost_freq(fd, boost_freqs); >>> >>> + /* Set max freq to original softmax */ >>> + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); >>> + >>> igt_debug("Apply low load again...\n"); >>> sleep(1); >>> stabilize_check(post_freqs); >>> @@ -643,7 +644,6 @@ static void waitboost(int fd, bool reset) >>> igt_assert_lt(pre_freqs[CUR], pre_freqs[MAX]); >>> igt_assert_eq(boost_freqs[CUR], boost_freqs[BOOST]); >>> igt_assert_lt(post_freqs[CUR], post_freqs[MAX]); >>> - >>> } >>> >>> static void pm_rps_exit_handler(int sig) >>> @@ -656,6 +656,7 @@ static void pm_rps_exit_handler(int sig) >>> writeval(sysfs_files[MAX].filp, origfreqs[MAX]); >>> } >>> >>> + writeval(sysfs_files[BOOST].filp, origfreqs[BOOST]); >> We are not changing boost_freq in current patch. Is this planned in new >> testcase? >> Please defer this change until then. > Will remove! > > Thanks for review! > Radek >>> load_helper_deinit(); >>> close(drm_fd); >>> }
diff --git a/tests/pm_rps.c b/tests/pm_rps.c index 89f3e31c..dad87646 100644 --- a/tests/pm_rps.c +++ b/tests/pm_rps.c @@ -583,11 +583,6 @@ static void boost_freq(int fd, int *boost_freqs) int64_t timeout = 1; igt_spin_t *load; unsigned int engine; - int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; - - fmid = get_hw_rounded_freq(fmid); - /* Set max freq to less then boost freq */ - writeval(sysfs_files[MAX].filp, fmid); /* Put boost on the same engine as low load */ engine = I915_EXEC_RENDER; @@ -604,9 +599,6 @@ static void boost_freq(int fd, int *boost_freqs) igt_spin_batch_end(load); gem_sync(fd, load->handle); igt_spin_batch_free(fd, load); - - /* Set max freq to original softmax */ - writeval(sysfs_files[MAX].filp, origfreqs[MAX]); } static void waitboost(int fd, bool reset) @@ -615,6 +607,9 @@ static void waitboost(int fd, bool reset) int boost_freqs[NUMFREQ]; int post_freqs[NUMFREQ]; + int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; + fmid = get_hw_rounded_freq(fmid); + load_helper_run(LOW); igt_debug("Apply low load...\n"); @@ -627,11 +622,17 @@ static void waitboost(int fd, bool reset) sleep(1); } + /* Set max freq to less than boost freq */ + writeval(sysfs_files[MAX].filp, fmid); + /* When we wait upon the GPU, we want to temporarily boost it * to maximum. */ boost_freq(fd, boost_freqs); + /* Set max freq to original softmax */ + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); + igt_debug("Apply low load again...\n"); sleep(1); stabilize_check(post_freqs); @@ -643,7 +644,6 @@ static void waitboost(int fd, bool reset) igt_assert_lt(pre_freqs[CUR], pre_freqs[MAX]); igt_assert_eq(boost_freqs[CUR], boost_freqs[BOOST]); igt_assert_lt(post_freqs[CUR], post_freqs[MAX]); - } static void pm_rps_exit_handler(int sig) @@ -656,6 +656,7 @@ static void pm_rps_exit_handler(int sig) writeval(sysfs_files[MAX].filp, origfreqs[MAX]); } + writeval(sysfs_files[BOOST].filp, origfreqs[BOOST]); load_helper_deinit(); close(drm_fd); }
Moving code out of the boost function will allow its usage in other/new test scenarios. Signed-off-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- tests/pm_rps.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)