| Message ID | 20170907121514.29861-1-katarzyna.dec@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2017-09-07 at 14:15 +0200, Katarzyna Dec wrote: > Added comments in tricky places for better feature understanding. > Added IGT_TEST_DESCRIPTION and short description for non-obvious > subtests. > Changed name of 'magic' checkit() function to something meaningfull. > Changed junk struct and stuff array names. > Made some minor coding style changes. > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > Cc: Petri Latvala <petri.latvala@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off: Katarzyna Dec <katarzyna.dec@intel.com> Acked-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
On Thu, Sep 07, 2017 at 02:15:14PM +0200, Katarzyna Dec wrote: > Added comments in tricky places for better feature understanding. > Added IGT_TEST_DESCRIPTION and short description for non-obvious > subtests. > Changed name of 'magic' checkit() function to something meaningfull. > Changed junk struct and stuff array names. > Made some minor coding style changes. > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > Cc: Petri Latvala <petri.latvala@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off: Katarzyna Dec <katarzyna.dec@intel.com> > --- > tests/pm_rps.c | 108 ++++++++++++++++++++++++++++++++++----------------------- > 1 file changed, 64 insertions(+), 44 deletions(-) > > diff --git a/tests/pm_rps.c b/tests/pm_rps.c > index e79f0ea7..1eeb6c6a 100644 > --- a/tests/pm_rps.c > +++ b/tests/pm_rps.c > @@ -40,6 +40,8 @@ > > #include "intel_bufmgr.h" > > +IGT_TEST_DESCRIPTION("Render P-States tests - verify GPU frequency changes"); > + > static int drm_fd; > > static const char sysfs_base_path[] = "/sys/class/drm/card%d/gt_%s_freq_mhz"; > @@ -56,12 +58,19 @@ enum { > > static int origfreqs[NUMFREQ]; > > -struct junk { > +struct sysfs_file { > const char *name; > const char *mode; > FILE *filp; > -} stuff[] = { > - { "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL }, { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { "boost", "rb+", NULL }, { NULL, NULL, NULL } > +} sysfs_files[] = { > + { "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) > @@ -81,7 +90,7 @@ static void read_freqs(int *freqs) > int i; > > for (i = 0; i < NUMFREQ; i++) > - freqs[i] = readval(stuff[i].filp); > + freqs[i] = readval(sysfs_files[i].filp); > } > > static void nsleep(unsigned long ns) > @@ -143,7 +152,7 @@ static int do_writeval(FILE *filp, int val, int lerrno, bool readback_check) > #define writeval_inval(filp, val) do_writeval(filp, val, EINVAL, true) > #define writeval_nocheck(filp, val) do_writeval(filp, val, 0, false) > > -static void checkit(const int *freqs) > +static void check_freq_constraints(const int *freqs) > { > igt_assert_lte(freqs[MIN], freqs[MAX]); > igt_assert_lte(freqs[CUR], freqs[MAX]); > @@ -162,7 +171,7 @@ static void dump(const int *freqs) > > igt_debug("gt freq (MHz):"); > for (i = 0; i < NUMFREQ; i++) > - igt_debug(" %s=%d", stuff[i].name, freqs[i]); > + igt_debug(" %s=%d", sysfs_files[i].name, freqs[i]); > > igt_debug("\n"); > } > @@ -387,14 +396,18 @@ static int get_hw_rounded_freq(int target) > idx = MAX; > > old_freq = freqs[idx]; > - writeval_nocheck(stuff[idx].filp, target); > + writeval_nocheck(sysfs_files[idx].filp, target); > read_freqs(freqs); > ret = freqs[idx]; > - writeval_nocheck(stuff[idx].filp, old_freq); > + writeval_nocheck(sysfs_files[idx].filp, old_freq); > > return ret; > } > > +/* > + * Modify softlimit MIN and MAX freqs to valid and invalid levels. Depending > + * on subtest run different check after each modification. > + */ > static void min_max_config(void (*check)(void), bool load_gpu) > { > int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; > @@ -411,78 +424,78 @@ static void min_max_config(void (*check)(void), bool load_gpu) > check(); > > igt_debug("\nSet min=RPn and max=RP0...\n"); > - writeval(stuff[MIN].filp, origfreqs[RPn]); > - writeval(stuff[MAX].filp, origfreqs[RP0]); > + writeval(sysfs_files[MIN].filp, origfreqs[RPn]); > + writeval(sysfs_files[MAX].filp, origfreqs[RP0]); > if (load_gpu) > do_load_gpu(); > check(); > > igt_debug("\nIncrease min to midpoint...\n"); > - writeval(stuff[MIN].filp, fmid); > + writeval(sysfs_files[MIN].filp, fmid); > if (load_gpu) > do_load_gpu(); > check(); > > igt_debug("\nIncrease min to RP0...\n"); > - writeval(stuff[MIN].filp, origfreqs[RP0]); > + writeval(sysfs_files[MIN].filp, origfreqs[RP0]); > if (load_gpu) > do_load_gpu(); > check(); > > igt_debug("\nIncrease min above RP0 (invalid)...\n"); > - writeval_inval(stuff[MIN].filp, origfreqs[RP0] + 1000); > + writeval_inval(sysfs_files[MIN].filp, origfreqs[RP0] + 1000); > check(); > > igt_debug("\nDecrease max to RPn (invalid)...\n"); > - writeval_inval(stuff[MAX].filp, origfreqs[RPn]); > + writeval_inval(sysfs_files[MAX].filp, origfreqs[RPn]); > check(); > > igt_debug("\nDecrease min to midpoint...\n"); > - writeval(stuff[MIN].filp, fmid); > + writeval(sysfs_files[MIN].filp, fmid); > if (load_gpu) > do_load_gpu(); > check(); > > igt_debug("\nDecrease min to RPn...\n"); > - writeval(stuff[MIN].filp, origfreqs[RPn]); > + writeval(sysfs_files[MIN].filp, origfreqs[RPn]); > if (load_gpu) > do_load_gpu(); > check(); > > igt_debug("\nDecrease min below RPn (invalid)...\n"); > - writeval_inval(stuff[MIN].filp, 0); > + writeval_inval(sysfs_files[MIN].filp, 0); > check(); > > igt_debug("\nDecrease max to midpoint...\n"); > - writeval(stuff[MAX].filp, fmid); > + writeval(sysfs_files[MAX].filp, fmid); > check(); > > igt_debug("\nDecrease max to RPn...\n"); > - writeval(stuff[MAX].filp, origfreqs[RPn]); > + writeval(sysfs_files[MAX].filp, origfreqs[RPn]); > check(); > > igt_debug("\nDecrease max below RPn (invalid)...\n"); > - writeval_inval(stuff[MAX].filp, 0); > + writeval_inval(sysfs_files[MAX].filp, 0); > check(); > > igt_debug("\nIncrease min to RP0 (invalid)...\n"); > - writeval_inval(stuff[MIN].filp, origfreqs[RP0]); > + writeval_inval(sysfs_files[MIN].filp, origfreqs[RP0]); > check(); > > igt_debug("\nIncrease max to midpoint...\n"); > - writeval(stuff[MAX].filp, fmid); > + writeval(sysfs_files[MAX].filp, fmid); > check(); > > igt_debug("\nIncrease max to RP0...\n"); > - writeval(stuff[MAX].filp, origfreqs[RP0]); > + writeval(sysfs_files[MAX].filp, origfreqs[RP0]); > check(); > > igt_debug("\nIncrease max above RP0 (invalid)...\n"); > - writeval_inval(stuff[MAX].filp, origfreqs[RP0] + 1000); > + writeval_inval(sysfs_files[MAX].filp, origfreqs[RP0] + 1000); > check(); > > - writeval(stuff[MIN].filp, origfreqs[MIN]); > - writeval(stuff[MAX].filp, origfreqs[MAX]); > + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > } > > static void basic_check(void) > @@ -491,7 +504,7 @@ static void basic_check(void) > > read_freqs(freqs); > dump(freqs); > - checkit(freqs); > + check_freq_constraints(freqs); > } > > #define IDLE_WAIT_TIMESTEP_MSEC 250 > @@ -506,7 +519,7 @@ static void idle_check(void) > do { > read_freqs(freqs); > dump(freqs); > - checkit(freqs); > + check_freq_constraints(freqs); > if (freqs[CUR] == freqs[RPn]) > break; > usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC); > @@ -529,7 +542,7 @@ static void loaded_check(void) > do { > read_freqs(freqs); > dump(freqs); > - checkit(freqs); > + check_freq_constraints(freqs); > if (freqs[CUR] >= freqs[MAX]) > break; > usleep(1000 * LOADED_WAIT_TIMESTEP_MSEC); > @@ -547,6 +560,8 @@ static void stabilize_check(int *out) > int freqs[NUMFREQ]; > int wait = 0; > > + /* Monitor frequencies until HW will stabilize cur frequency. > + * It should happen within allotted time */ > read_freqs(freqs); > dump(freqs); > usleep(1000 * STABILIZE_WAIT_TIMESTEP_MSEC); > @@ -573,7 +588,7 @@ static void boost_freq(int fd, int *boost_freqs) > > fmid = get_hw_rounded_freq(fmid); > /* Set max freq to less then boost freq */ > - writeval(stuff[MAX].filp, fmid); > + writeval(sysfs_files[MAX].filp, fmid); > > /* Put boost on the same engine as low load */ > engine = I915_EXEC_RENDER; > @@ -592,7 +607,7 @@ static void boost_freq(int fd, int *boost_freqs) > igt_spin_batch_free(fd, load); > > /* Set max freq to original softmax */ > - writeval(stuff[MAX].filp, origfreqs[MAX]); > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > } > > static void waitboost(int fd, bool reset) > @@ -634,12 +649,12 @@ static void waitboost(int fd, bool reset) > > static void pm_rps_exit_handler(int sig) > { > - if (origfreqs[MIN] > readval(stuff[MAX].filp)) { > - writeval(stuff[MAX].filp, origfreqs[MAX]); > - writeval(stuff[MIN].filp, origfreqs[MIN]); > + if (origfreqs[MIN] > readval(sysfs_files[MAX].filp)) { > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); > } else { > - writeval(stuff[MIN].filp, origfreqs[MIN]); > - writeval(stuff[MAX].filp, origfreqs[MAX]); > + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > } > > load_helper_deinit(); > @@ -652,7 +667,7 @@ igt_main > > igt_fixture { > const int device = drm_get_card(); > - struct junk *junk = stuff; > + struct sysfs_file *sysfs_file = sysfs_files; > int ret; > > /* Use drm_open_driver to verify device existence */ > @@ -663,16 +678,17 @@ igt_main > do { > int val = -1; > char *path; > - ret = asprintf(&path, sysfs_base_path, device, junk->name); > + > + ret = asprintf(&path, sysfs_base_path, device, sysfs_file->name); > igt_assert(ret != -1); > - junk->filp = fopen(path, junk->mode); > - igt_require(junk->filp); > - setbuf(junk->filp, NULL); > + sysfs_file->filp = fopen(path, sysfs_file->mode); > + igt_require(sysfs_file->filp); > + setbuf(sysfs_file->filp, NULL); > > - val = readval(junk->filp); > + val = readval(sysfs_file->filp); > igt_assert(val >= 0); > - junk++; > - } while (junk->name != NULL); > + sysfs_file++; > + } while (sysfs_file->name != NULL); > > read_freqs(origfreqs); > > @@ -684,18 +700,22 @@ igt_main > igt_subtest("basic-api") > min_max_config(basic_check, false); > > + /* Verify the constraints, check if we can reach idle */ Nitpick: either end the sentence with a comma or don't start it with an uppercase letter. Other than that - looks good to me. It's everything I had in mind when discussing with Vinay. You've commented on the surprising parts (e.g. waitboost does not wait for boost but causes it using gem_wait, therefore the name). You've renamed the oddities (checkit, junk) to something more meaningful instead of bloating them with comments. And you have not overdid it, withholding on adding comments in places where they would be just redundant. IMHO pm_rps can serve as a good example of how things should look like. Acked-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> But let's wait and see what others (at least those CCed) have to say before popping a champagne on this.
On 9/7/2017 5:15 AM, Katarzyna Dec wrote: > Added comments in tricky places for better feature understanding. > Added IGT_TEST_DESCRIPTION and short description for non-obvious > subtests. > Changed name of 'magic' checkit() function to something meaningfull. > Changed junk struct and stuff array names. > Made some minor coding style changes. > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > Cc: Petri Latvala <petri.latvala@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off: Katarzyna Dec <katarzyna.dec@intel.com> > --- > tests/pm_rps.c | 108 ++++++++++++++++++++++++++++++++++----------------------- > 1 file changed, 64 insertions(+), 44 deletions(-) > > diff --git a/tests/pm_rps.c b/tests/pm_rps.c > index e79f0ea7..1eeb6c6a 100644 > --- a/tests/pm_rps.c > +++ b/tests/pm_rps.c > @@ -40,6 +40,8 @@ > > #include "intel_bufmgr.h" > > +IGT_TEST_DESCRIPTION("Render P-States tests - verify GPU frequency changes"); > + > static int drm_fd; > > static const char sysfs_base_path[] = "/sys/class/drm/card%d/gt_%s_freq_mhz"; > @@ -56,12 +58,19 @@ enum { > > static int origfreqs[NUMFREQ]; > > -struct junk { > +struct sysfs_file { > const char *name; > const char *mode; > FILE *filp; > -} stuff[] = { > - { "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL }, { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { "boost", "rb+", NULL }, { NULL, NULL, NULL } > +} sysfs_files[] = { > + { "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) > @@ -81,7 +90,7 @@ static void read_freqs(int *freqs) > int i; > > for (i = 0; i < NUMFREQ; i++) > - freqs[i] = readval(stuff[i].filp); > + freqs[i] = readval(sysfs_files[i].filp); > } > > static void nsleep(unsigned long ns) > @@ -143,7 +152,7 @@ static int do_writeval(FILE *filp, int val, int lerrno, bool readback_check) > #define writeval_inval(filp, val) do_writeval(filp, val, EINVAL, true) > #define writeval_nocheck(filp, val) do_writeval(filp, val, 0, false) > > -static void checkit(const int *freqs) > +static void check_freq_constraints(const int *freqs) > { > igt_assert_lte(freqs[MIN], freqs[MAX]); > igt_assert_lte(freqs[CUR], freqs[MAX]); > @@ -162,7 +171,7 @@ static void dump(const int *freqs) > > igt_debug("gt freq (MHz):"); > for (i = 0; i < NUMFREQ; i++) > - igt_debug(" %s=%d", stuff[i].name, freqs[i]); > + igt_debug(" %s=%d", sysfs_files[i].name, freqs[i]); > > igt_debug("\n"); > } > @@ -387,14 +396,18 @@ static int get_hw_rounded_freq(int target) > idx = MAX; > > old_freq = freqs[idx]; > - writeval_nocheck(stuff[idx].filp, target); > + writeval_nocheck(sysfs_files[idx].filp, target); > read_freqs(freqs); > ret = freqs[idx]; > - writeval_nocheck(stuff[idx].filp, old_freq); > + writeval_nocheck(sysfs_files[idx].filp, old_freq); > > return ret; > } > > +/* > + * Modify softlimit MIN and MAX freqs to valid and invalid levels. Depending > + * on subtest run different check after each modification. > + */ > static void min_max_config(void (*check)(void), bool load_gpu) > { > int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; > @@ -411,78 +424,78 @@ static void min_max_config(void (*check)(void), bool load_gpu) > check(); > > igt_debug("\nSet min=RPn and max=RP0...\n"); > - writeval(stuff[MIN].filp, origfreqs[RPn]); > - writeval(stuff[MAX].filp, origfreqs[RP0]); > + writeval(sysfs_files[MIN].filp, origfreqs[RPn]); > + writeval(sysfs_files[MAX].filp, origfreqs[RP0]); > if (load_gpu) > do_load_gpu(); > check(); > > igt_debug("\nIncrease min to midpoint...\n"); > - writeval(stuff[MIN].filp, fmid); > + writeval(sysfs_files[MIN].filp, fmid); > if (load_gpu) > do_load_gpu(); > check(); > > igt_debug("\nIncrease min to RP0...\n"); > - writeval(stuff[MIN].filp, origfreqs[RP0]); > + writeval(sysfs_files[MIN].filp, origfreqs[RP0]); > if (load_gpu) > do_load_gpu(); > check(); > > igt_debug("\nIncrease min above RP0 (invalid)...\n"); > - writeval_inval(stuff[MIN].filp, origfreqs[RP0] + 1000); > + writeval_inval(sysfs_files[MIN].filp, origfreqs[RP0] + 1000); > check(); > > igt_debug("\nDecrease max to RPn (invalid)...\n"); > - writeval_inval(stuff[MAX].filp, origfreqs[RPn]); > + writeval_inval(sysfs_files[MAX].filp, origfreqs[RPn]); > check(); > > igt_debug("\nDecrease min to midpoint...\n"); > - writeval(stuff[MIN].filp, fmid); > + writeval(sysfs_files[MIN].filp, fmid); > if (load_gpu) > do_load_gpu(); > check(); > > igt_debug("\nDecrease min to RPn...\n"); > - writeval(stuff[MIN].filp, origfreqs[RPn]); > + writeval(sysfs_files[MIN].filp, origfreqs[RPn]); > if (load_gpu) > do_load_gpu(); > check(); > > igt_debug("\nDecrease min below RPn (invalid)...\n"); > - writeval_inval(stuff[MIN].filp, 0); > + writeval_inval(sysfs_files[MIN].filp, 0); > check(); > > igt_debug("\nDecrease max to midpoint...\n"); > - writeval(stuff[MAX].filp, fmid); > + writeval(sysfs_files[MAX].filp, fmid); > check(); > > igt_debug("\nDecrease max to RPn...\n"); > - writeval(stuff[MAX].filp, origfreqs[RPn]); > + writeval(sysfs_files[MAX].filp, origfreqs[RPn]); > check(); > > igt_debug("\nDecrease max below RPn (invalid)...\n"); > - writeval_inval(stuff[MAX].filp, 0); > + writeval_inval(sysfs_files[MAX].filp, 0); > check(); > > igt_debug("\nIncrease min to RP0 (invalid)...\n"); > - writeval_inval(stuff[MIN].filp, origfreqs[RP0]); > + writeval_inval(sysfs_files[MIN].filp, origfreqs[RP0]); > check(); > > igt_debug("\nIncrease max to midpoint...\n"); > - writeval(stuff[MAX].filp, fmid); > + writeval(sysfs_files[MAX].filp, fmid); > check(); > > igt_debug("\nIncrease max to RP0...\n"); > - writeval(stuff[MAX].filp, origfreqs[RP0]); > + writeval(sysfs_files[MAX].filp, origfreqs[RP0]); > check(); > > igt_debug("\nIncrease max above RP0 (invalid)...\n"); > - writeval_inval(stuff[MAX].filp, origfreqs[RP0] + 1000); > + writeval_inval(sysfs_files[MAX].filp, origfreqs[RP0] + 1000); > check(); > > - writeval(stuff[MIN].filp, origfreqs[MIN]); > - writeval(stuff[MAX].filp, origfreqs[MAX]); > + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > } > > static void basic_check(void) > @@ -491,7 +504,7 @@ static void basic_check(void) > > read_freqs(freqs); > dump(freqs); Almost feel like we should create a new function read_and_dump_freqs() instead of calling 2 functions multiple times. We should keep the old read_freqs() since it is called in a loop on one occasion. Something to think about maybe? > - checkit(freqs); > + check_freq_constraints(freqs); > } > > #define IDLE_WAIT_TIMESTEP_MSEC 250 > @@ -506,7 +519,7 @@ static void idle_check(void) > do { > read_freqs(freqs); > dump(freqs); > - checkit(freqs); > + check_freq_constraints(freqs); > if (freqs[CUR] == freqs[RPn]) > break; > usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC); > @@ -529,7 +542,7 @@ static void loaded_check(void) > do { > read_freqs(freqs); > dump(freqs); > - checkit(freqs); > + check_freq_constraints(freqs); > if (freqs[CUR] >= freqs[MAX]) > break; > usleep(1000 * LOADED_WAIT_TIMESTEP_MSEC); > @@ -547,6 +560,8 @@ static void stabilize_check(int *out) > int freqs[NUMFREQ]; > int wait = 0; > > + /* Monitor frequencies until HW will stabilize cur frequency. > + * It should happen within allotted time */ > read_freqs(freqs); > dump(freqs); > usleep(1000 * STABILIZE_WAIT_TIMESTEP_MSEC); > @@ -573,7 +588,7 @@ static void boost_freq(int fd, int *boost_freqs) > > fmid = get_hw_rounded_freq(fmid); > /* Set max freq to less then boost freq */ > - writeval(stuff[MAX].filp, fmid); > + writeval(sysfs_files[MAX].filp, fmid); > > /* Put boost on the same engine as low load */ > engine = I915_EXEC_RENDER; > @@ -592,7 +607,7 @@ static void boost_freq(int fd, int *boost_freqs) > igt_spin_batch_free(fd, load); > > /* Set max freq to original softmax */ > - writeval(stuff[MAX].filp, origfreqs[MAX]); > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > } > > static void waitboost(int fd, bool reset) > @@ -634,12 +649,12 @@ static void waitboost(int fd, bool reset) > > static void pm_rps_exit_handler(int sig) > { > - if (origfreqs[MIN] > readval(stuff[MAX].filp)) { > - writeval(stuff[MAX].filp, origfreqs[MAX]); > - writeval(stuff[MIN].filp, origfreqs[MIN]); > + if (origfreqs[MIN] > readval(sysfs_files[MAX].filp)) { > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); > } else { > - writeval(stuff[MIN].filp, origfreqs[MIN]); > - writeval(stuff[MAX].filp, origfreqs[MAX]); > + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > } > > load_helper_deinit(); > @@ -652,7 +667,7 @@ igt_main > > igt_fixture { > const int device = drm_get_card(); > - struct junk *junk = stuff; > + struct sysfs_file *sysfs_file = sysfs_files; > int ret; > > /* Use drm_open_driver to verify device existence */ > @@ -663,16 +678,17 @@ igt_main > do { > int val = -1; > char *path; > - ret = asprintf(&path, sysfs_base_path, device, junk->name); > + > + ret = asprintf(&path, sysfs_base_path, device, sysfs_file->name); > igt_assert(ret != -1); > - junk->filp = fopen(path, junk->mode); > - igt_require(junk->filp); > - setbuf(junk->filp, NULL); > + sysfs_file->filp = fopen(path, sysfs_file->mode); > + igt_require(sysfs_file->filp); > + setbuf(sysfs_file->filp, NULL); > > - val = readval(junk->filp); > + val = readval(sysfs_file->filp); > igt_assert(val >= 0); > - junk++; > - } while (junk->name != NULL); > + sysfs_file++; > + } while (sysfs_file->name != NULL); > > read_freqs(origfreqs); > > @@ -684,18 +700,22 @@ igt_main > igt_subtest("basic-api") > min_max_config(basic_check, false); > > + /* Verify the constraints, check if we can reach idle */ > igt_subtest("min-max-config-idle") > min_max_config(idle_check, true); > > + /* Verify the constraints with high load, check if we can reach max */ > igt_subtest("min-max-config-loaded") { > load_helper_run(HIGH); > min_max_config(loaded_check, false); > load_helper_stop(); > } > > + /* Checks if we achieve boost using gem_wait */ We should mention this is doing gem_wait on a spinning batch, hence the boost. Other than this and the above comment, LGTM. Acked-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > igt_subtest("waitboost") > waitboost(drm_fd, false); > > + /* Test boost frequency after GPU reset */ > igt_subtest("reset") { > igt_hang_t hang = igt_allow_hang(drm_fd, 0, 0); > waitboost(drm_fd, true); >
On Thu, Sep 07, 2017 at 11:28:38AM -0700, Belgaumkar, Vinay wrote: > > > On 9/7/2017 5:15 AM, Katarzyna Dec wrote: > > Added comments in tricky places for better feature understanding. > > Added IGT_TEST_DESCRIPTION and short description for non-obvious > > subtests. > > Changed name of 'magic' checkit() function to something meaningfull. > > Changed junk struct and stuff array names. > > Made some minor coding style changes. > > > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > Cc: Petri Latvala <petri.latvala@intel.com> > > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > Signed-off: Katarzyna Dec <katarzyna.dec@intel.com> > > --- > > tests/pm_rps.c | 108 ++++++++++++++++++++++++++++++++++----------------------- > > 1 file changed, 64 insertions(+), 44 deletions(-) > > > > diff --git a/tests/pm_rps.c b/tests/pm_rps.c > > index e79f0ea7..1eeb6c6a 100644 > > --- a/tests/pm_rps.c > > +++ b/tests/pm_rps.c > > @@ -40,6 +40,8 @@ > > > > #include "intel_bufmgr.h" > > > > +IGT_TEST_DESCRIPTION("Render P-States tests - verify GPU frequency changes"); > > + > > static int drm_fd; > > > > static const char sysfs_base_path[] = "/sys/class/drm/card%d/gt_%s_freq_mhz"; > > @@ -56,12 +58,19 @@ enum { > > > > static int origfreqs[NUMFREQ]; > > > > -struct junk { > > +struct sysfs_file { > > const char *name; > > const char *mode; > > FILE *filp; > > -} stuff[] = { > > - { "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL }, { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { "boost", "rb+", NULL }, { NULL, NULL, NULL } > > +} sysfs_files[] = { > > + { "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) > > @@ -81,7 +90,7 @@ static void read_freqs(int *freqs) > > int i; > > > > for (i = 0; i < NUMFREQ; i++) > > - freqs[i] = readval(stuff[i].filp); > > + freqs[i] = readval(sysfs_files[i].filp); > > } > > > > static void nsleep(unsigned long ns) > > @@ -143,7 +152,7 @@ static int do_writeval(FILE *filp, int val, int lerrno, bool readback_check) > > #define writeval_inval(filp, val) do_writeval(filp, val, EINVAL, true) > > #define writeval_nocheck(filp, val) do_writeval(filp, val, 0, false) > > > > -static void checkit(const int *freqs) > > +static void check_freq_constraints(const int *freqs) > > { > > igt_assert_lte(freqs[MIN], freqs[MAX]); > > igt_assert_lte(freqs[CUR], freqs[MAX]); > > @@ -162,7 +171,7 @@ static void dump(const int *freqs) > > > > igt_debug("gt freq (MHz):"); > > for (i = 0; i < NUMFREQ; i++) > > - igt_debug(" %s=%d", stuff[i].name, freqs[i]); > > + igt_debug(" %s=%d", sysfs_files[i].name, freqs[i]); > > > > igt_debug("\n"); > > } > > @@ -387,14 +396,18 @@ static int get_hw_rounded_freq(int target) > > idx = MAX; > > > > old_freq = freqs[idx]; > > - writeval_nocheck(stuff[idx].filp, target); > > + writeval_nocheck(sysfs_files[idx].filp, target); > > read_freqs(freqs); > > ret = freqs[idx]; > > - writeval_nocheck(stuff[idx].filp, old_freq); > > + writeval_nocheck(sysfs_files[idx].filp, old_freq); > > > > return ret; > > } > > > > +/* > > + * Modify softlimit MIN and MAX freqs to valid and invalid levels. Depending > > + * on subtest run different check after each modification. > > + */ > > static void min_max_config(void (*check)(void), bool load_gpu) > > { > > int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; > > @@ -411,78 +424,78 @@ static void min_max_config(void (*check)(void), bool load_gpu) > > check(); > > > > igt_debug("\nSet min=RPn and max=RP0...\n"); > > - writeval(stuff[MIN].filp, origfreqs[RPn]); > > - writeval(stuff[MAX].filp, origfreqs[RP0]); > > + writeval(sysfs_files[MIN].filp, origfreqs[RPn]); > > + writeval(sysfs_files[MAX].filp, origfreqs[RP0]); > > if (load_gpu) > > do_load_gpu(); > > check(); > > > > igt_debug("\nIncrease min to midpoint...\n"); > > - writeval(stuff[MIN].filp, fmid); > > + writeval(sysfs_files[MIN].filp, fmid); > > if (load_gpu) > > do_load_gpu(); > > check(); > > > > igt_debug("\nIncrease min to RP0...\n"); > > - writeval(stuff[MIN].filp, origfreqs[RP0]); > > + writeval(sysfs_files[MIN].filp, origfreqs[RP0]); > > if (load_gpu) > > do_load_gpu(); > > check(); > > > > igt_debug("\nIncrease min above RP0 (invalid)...\n"); > > - writeval_inval(stuff[MIN].filp, origfreqs[RP0] + 1000); > > + writeval_inval(sysfs_files[MIN].filp, origfreqs[RP0] + 1000); > > check(); > > > > igt_debug("\nDecrease max to RPn (invalid)...\n"); > > - writeval_inval(stuff[MAX].filp, origfreqs[RPn]); > > + writeval_inval(sysfs_files[MAX].filp, origfreqs[RPn]); > > check(); > > > > igt_debug("\nDecrease min to midpoint...\n"); > > - writeval(stuff[MIN].filp, fmid); > > + writeval(sysfs_files[MIN].filp, fmid); > > if (load_gpu) > > do_load_gpu(); > > check(); > > > > igt_debug("\nDecrease min to RPn...\n"); > > - writeval(stuff[MIN].filp, origfreqs[RPn]); > > + writeval(sysfs_files[MIN].filp, origfreqs[RPn]); > > if (load_gpu) > > do_load_gpu(); > > check(); > > > > igt_debug("\nDecrease min below RPn (invalid)...\n"); > > - writeval_inval(stuff[MIN].filp, 0); > > + writeval_inval(sysfs_files[MIN].filp, 0); > > check(); > > > > igt_debug("\nDecrease max to midpoint...\n"); > > - writeval(stuff[MAX].filp, fmid); > > + writeval(sysfs_files[MAX].filp, fmid); > > check(); > > > > igt_debug("\nDecrease max to RPn...\n"); > > - writeval(stuff[MAX].filp, origfreqs[RPn]); > > + writeval(sysfs_files[MAX].filp, origfreqs[RPn]); > > check(); > > > > igt_debug("\nDecrease max below RPn (invalid)...\n"); > > - writeval_inval(stuff[MAX].filp, 0); > > + writeval_inval(sysfs_files[MAX].filp, 0); > > check(); > > > > igt_debug("\nIncrease min to RP0 (invalid)...\n"); > > - writeval_inval(stuff[MIN].filp, origfreqs[RP0]); > > + writeval_inval(sysfs_files[MIN].filp, origfreqs[RP0]); > > check(); > > > > igt_debug("\nIncrease max to midpoint...\n"); > > - writeval(stuff[MAX].filp, fmid); > > + writeval(sysfs_files[MAX].filp, fmid); > > check(); > > > > igt_debug("\nIncrease max to RP0...\n"); > > - writeval(stuff[MAX].filp, origfreqs[RP0]); > > + writeval(sysfs_files[MAX].filp, origfreqs[RP0]); > > check(); > > > > igt_debug("\nIncrease max above RP0 (invalid)...\n"); > > - writeval_inval(stuff[MAX].filp, origfreqs[RP0] + 1000); > > + writeval_inval(sysfs_files[MAX].filp, origfreqs[RP0] + 1000); > > check(); > > > > - writeval(stuff[MIN].filp, origfreqs[MIN]); > > - writeval(stuff[MAX].filp, origfreqs[MAX]); > > + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); > > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > > } > > > > static void basic_check(void) > > @@ -491,7 +504,7 @@ static void basic_check(void) > > > > read_freqs(freqs); > > dump(freqs); > > Almost feel like we should create a new function read_and_dump_freqs() > instead of calling 2 functions multiple times. We should keep the old > read_freqs() since it is called in a loop on one occasion. Something to > think about maybe? If anybody reviewing this patch feel that it would be better to change basic_check function name, I will do it :) Also, as far as I know this function is used only in this file, so there is no reason to add duplicate it. > > > - checkit(freqs); > > + check_freq_constraints(freqs); > > } > > > > #define IDLE_WAIT_TIMESTEP_MSEC 250 > > @@ -506,7 +519,7 @@ static void idle_check(void) > > do { > > read_freqs(freqs); > > dump(freqs); > > - checkit(freqs); > > + check_freq_constraints(freqs); > > if (freqs[CUR] == freqs[RPn]) > > break; > > usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC); > > @@ -529,7 +542,7 @@ static void loaded_check(void) > > do { > > read_freqs(freqs); > > dump(freqs); > > - checkit(freqs); > > + check_freq_constraints(freqs); > > if (freqs[CUR] >= freqs[MAX]) > > break; > > usleep(1000 * LOADED_WAIT_TIMESTEP_MSEC); > > @@ -547,6 +560,8 @@ static void stabilize_check(int *out) > > int freqs[NUMFREQ]; > > int wait = 0; > > > > + /* Monitor frequencies until HW will stabilize cur frequency. > > + * It should happen within allotted time */ > > read_freqs(freqs); > > dump(freqs); > > usleep(1000 * STABILIZE_WAIT_TIMESTEP_MSEC); > > @@ -573,7 +588,7 @@ static void boost_freq(int fd, int *boost_freqs) > > > > fmid = get_hw_rounded_freq(fmid); > > /* Set max freq to less then boost freq */ > > - writeval(stuff[MAX].filp, fmid); > > + writeval(sysfs_files[MAX].filp, fmid); > > > > /* Put boost on the same engine as low load */ > > engine = I915_EXEC_RENDER; > > @@ -592,7 +607,7 @@ static void boost_freq(int fd, int *boost_freqs) > > igt_spin_batch_free(fd, load); > > > > /* Set max freq to original softmax */ > > - writeval(stuff[MAX].filp, origfreqs[MAX]); > > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > > } > > > > static void waitboost(int fd, bool reset) > > @@ -634,12 +649,12 @@ static void waitboost(int fd, bool reset) > > > > static void pm_rps_exit_handler(int sig) > > { > > - if (origfreqs[MIN] > readval(stuff[MAX].filp)) { > > - writeval(stuff[MAX].filp, origfreqs[MAX]); > > - writeval(stuff[MIN].filp, origfreqs[MIN]); > > + if (origfreqs[MIN] > readval(sysfs_files[MAX].filp)) { > > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > > + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); > > } else { > > - writeval(stuff[MIN].filp, origfreqs[MIN]); > > - writeval(stuff[MAX].filp, origfreqs[MAX]); > > + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); > > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > > } > > > > load_helper_deinit(); > > @@ -652,7 +667,7 @@ igt_main > > > > igt_fixture { > > const int device = drm_get_card(); > > - struct junk *junk = stuff; > > + struct sysfs_file *sysfs_file = sysfs_files; > > int ret; > > > > /* Use drm_open_driver to verify device existence */ > > @@ -663,16 +678,17 @@ igt_main > > do { > > int val = -1; > > char *path; > > - ret = asprintf(&path, sysfs_base_path, device, junk->name); > > + > > + ret = asprintf(&path, sysfs_base_path, device, sysfs_file->name); > > igt_assert(ret != -1); > > - junk->filp = fopen(path, junk->mode); > > - igt_require(junk->filp); > > - setbuf(junk->filp, NULL); > > + sysfs_file->filp = fopen(path, sysfs_file->mode); > > + igt_require(sysfs_file->filp); > > + setbuf(sysfs_file->filp, NULL); > > > > - val = readval(junk->filp); > > + val = readval(sysfs_file->filp); > > igt_assert(val >= 0); > > - junk++; > > - } while (junk->name != NULL); > > + sysfs_file++; > > + } while (sysfs_file->name != NULL); > > > > read_freqs(origfreqs); > > > > @@ -684,18 +700,22 @@ igt_main > > igt_subtest("basic-api") > > min_max_config(basic_check, false); > > > > + /* Verify the constraints, check if we can reach idle */ > > igt_subtest("min-max-config-idle") > > min_max_config(idle_check, true); > > > > + /* Verify the constraints with high load, check if we can reach max */ > > igt_subtest("min-max-config-loaded") { > > load_helper_run(HIGH); > > min_max_config(loaded_check, false); > > load_helper_stop(); > > } > > > > + /* Checks if we achieve boost using gem_wait */ > > We should mention this is doing gem_wait on a spinning batch, hence the > boost. I think that additional information is not needed here. I got 2 reasons: - we assume that subtest comment will be oneliner - do we need such details here? Thanks, Kasia > > Other than this and the above comment, LGTM. > > Acked-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > > > igt_subtest("waitboost") > > waitboost(drm_fd, false); > > > > + /* Test boost frequency after GPU reset */ > > igt_subtest("reset") { > > igt_hang_t hang = igt_allow_hang(drm_fd, 0, 0); > > waitboost(drm_fd, true); > >
On Thu, 2017-09-07 at 11:28 -0700, Belgaumkar, Vinay wrote: > > On 9/7/2017 5:15 AM, Katarzyna Dec wrote: > > Added comments in tricky places for better feature understanding. > > Added IGT_TEST_DESCRIPTION and short description for non-obvious > > subtests. > > Changed name of 'magic' checkit() function to something meaningfull. > > Changed junk struct and stuff array names. > > Made some minor coding style changes. > > > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > Cc: Petri Latvala <petri.latvala@intel.com> > > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > Signed-off: Katarzyna Dec <katarzyna.dec@intel.com> > > --- > > tests/pm_rps.c | 108 ++++++++++++++++++++++++++++++++++------------------ > > ----- > > 1 file changed, 64 insertions(+), 44 deletions(-) > > > > diff --git a/tests/pm_rps.c b/tests/pm_rps.c > > index e79f0ea7..1eeb6c6a 100644 > > --- a/tests/pm_rps.c > > +++ b/tests/pm_rps.c > > @@ -40,6 +40,8 @@ > > > > #include "intel_bufmgr.h" > > > > +IGT_TEST_DESCRIPTION("Render P-States tests - verify GPU frequency > > changes"); > > + > > static int drm_fd; > > > > static const char sysfs_base_path[] = > > "/sys/class/drm/card%d/gt_%s_freq_mhz"; > > @@ -56,12 +58,19 @@ enum { > > > > static int origfreqs[NUMFREQ]; > > > > -struct junk { > > +struct sysfs_file { > > const char *name; > > const char *mode; > > FILE *filp; > > -} stuff[] = { > > - { "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL > > }, { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { > > "boost", "rb+", NULL }, { NULL, NULL, NULL } > > +} sysfs_files[] = { > > + { "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) > > @@ -81,7 +90,7 @@ static void read_freqs(int *freqs) > > int i; > > > > for (i = 0; i < NUMFREQ; i++) > > - freqs[i] = readval(stuff[i].filp); > > + freqs[i] = readval(sysfs_files[i].filp); > > } > > > > static void nsleep(unsigned long ns) > > @@ -143,7 +152,7 @@ static int do_writeval(FILE *filp, int val, int lerrno, > > bool readback_check) > > #define writeval_inval(filp, val) do_writeval(filp, val, EINVAL, true) > > #define writeval_nocheck(filp, val) do_writeval(filp, val, 0, false) > > > > -static void checkit(const int *freqs) > > +static void check_freq_constraints(const int *freqs) > > { > > igt_assert_lte(freqs[MIN], freqs[MAX]); > > igt_assert_lte(freqs[CUR], freqs[MAX]); > > @@ -162,7 +171,7 @@ static void dump(const int *freqs) > > > > igt_debug("gt freq (MHz):"); > > for (i = 0; i < NUMFREQ; i++) > > - igt_debug(" %s=%d", stuff[i].name, freqs[i]); > > + igt_debug(" %s=%d", sysfs_files[i].name, freqs[i]); > > > > igt_debug("\n"); > > } > > @@ -387,14 +396,18 @@ static int get_hw_rounded_freq(int target) > > idx = MAX; > > > > old_freq = freqs[idx]; > > - writeval_nocheck(stuff[idx].filp, target); > > + writeval_nocheck(sysfs_files[idx].filp, target); > > read_freqs(freqs); > > ret = freqs[idx]; > > - writeval_nocheck(stuff[idx].filp, old_freq); > > + writeval_nocheck(sysfs_files[idx].filp, old_freq); > > > > return ret; > > } > > > > +/* > > + * Modify softlimit MIN and MAX freqs to valid and invalid levels. > > Depending > > + * on subtest run different check after each modification. > > + */ > > static void min_max_config(void (*check)(void), bool load_gpu) > > { > > int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; > > @@ -411,78 +424,78 @@ static void min_max_config(void (*check)(void), bool > > load_gpu) > > check(); > > > > igt_debug("\nSet min=RPn and max=RP0...\n"); > > - writeval(stuff[MIN].filp, origfreqs[RPn]); > > - writeval(stuff[MAX].filp, origfreqs[RP0]); > > + writeval(sysfs_files[MIN].filp, origfreqs[RPn]); > > + writeval(sysfs_files[MAX].filp, origfreqs[RP0]); > > if (load_gpu) > > do_load_gpu(); > > check(); > > > > igt_debug("\nIncrease min to midpoint...\n"); > > - writeval(stuff[MIN].filp, fmid); > > + writeval(sysfs_files[MIN].filp, fmid); > > if (load_gpu) > > do_load_gpu(); > > check(); > > > > igt_debug("\nIncrease min to RP0...\n"); > > - writeval(stuff[MIN].filp, origfreqs[RP0]); > > + writeval(sysfs_files[MIN].filp, origfreqs[RP0]); > > if (load_gpu) > > do_load_gpu(); > > check(); > > > > igt_debug("\nIncrease min above RP0 (invalid)...\n"); > > - writeval_inval(stuff[MIN].filp, origfreqs[RP0] + 1000); > > + writeval_inval(sysfs_files[MIN].filp, origfreqs[RP0] + 1000); > > check(); > > > > igt_debug("\nDecrease max to RPn (invalid)...\n"); > > - writeval_inval(stuff[MAX].filp, origfreqs[RPn]); > > + writeval_inval(sysfs_files[MAX].filp, origfreqs[RPn]); > > check(); > > > > igt_debug("\nDecrease min to midpoint...\n"); > > - writeval(stuff[MIN].filp, fmid); > > + writeval(sysfs_files[MIN].filp, fmid); > > if (load_gpu) > > do_load_gpu(); > > check(); > > > > igt_debug("\nDecrease min to RPn...\n"); > > - writeval(stuff[MIN].filp, origfreqs[RPn]); > > + writeval(sysfs_files[MIN].filp, origfreqs[RPn]); > > if (load_gpu) > > do_load_gpu(); > > check(); > > > > igt_debug("\nDecrease min below RPn (invalid)...\n"); > > - writeval_inval(stuff[MIN].filp, 0); > > + writeval_inval(sysfs_files[MIN].filp, 0); > > check(); > > > > igt_debug("\nDecrease max to midpoint...\n"); > > - writeval(stuff[MAX].filp, fmid); > > + writeval(sysfs_files[MAX].filp, fmid); > > check(); > > > > igt_debug("\nDecrease max to RPn...\n"); > > - writeval(stuff[MAX].filp, origfreqs[RPn]); > > + writeval(sysfs_files[MAX].filp, origfreqs[RPn]); > > check(); > > > > igt_debug("\nDecrease max below RPn (invalid)...\n"); > > - writeval_inval(stuff[MAX].filp, 0); > > + writeval_inval(sysfs_files[MAX].filp, 0); > > check(); > > > > igt_debug("\nIncrease min to RP0 (invalid)...\n"); > > - writeval_inval(stuff[MIN].filp, origfreqs[RP0]); > > + writeval_inval(sysfs_files[MIN].filp, origfreqs[RP0]); > > check(); > > > > igt_debug("\nIncrease max to midpoint...\n"); > > - writeval(stuff[MAX].filp, fmid); > > + writeval(sysfs_files[MAX].filp, fmid); > > check(); > > > > igt_debug("\nIncrease max to RP0...\n"); > > - writeval(stuff[MAX].filp, origfreqs[RP0]); > > + writeval(sysfs_files[MAX].filp, origfreqs[RP0]); > > check(); > > > > igt_debug("\nIncrease max above RP0 (invalid)...\n"); > > - writeval_inval(stuff[MAX].filp, origfreqs[RP0] + 1000); > > + writeval_inval(sysfs_files[MAX].filp, origfreqs[RP0] + 1000); > > check(); > > > > - writeval(stuff[MIN].filp, origfreqs[MIN]); > > - writeval(stuff[MAX].filp, origfreqs[MAX]); > > + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); > > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > > } > > > > static void basic_check(void) > > @@ -491,7 +504,7 @@ static void basic_check(void) > > > > read_freqs(freqs); > > dump(freqs); > > Almost feel like we should create a new function read_and_dump_freqs() > instead of calling 2 functions multiple times. We should keep the old > read_freqs() since it is called in a loop on one occasion. Something to > think about maybe? I disagree. Those are perfectly fine two functions that have very descriptive names. I think that it is a good practice to have one function to do only one thing (if this makes sense) because it does not limit its reuse and is easier to understand by the reader. -Radek > > > - checkit(freqs); > > + check_freq_constraints(freqs); > > } > > > > #define IDLE_WAIT_TIMESTEP_MSEC 250 > > @@ -506,7 +519,7 @@ static void idle_check(void) > > do { > > read_freqs(freqs); > > dump(freqs); > > - checkit(freqs); > > + check_freq_constraints(freqs); > > if (freqs[CUR] == freqs[RPn]) > > break; > > usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC); > > @@ -529,7 +542,7 @@ static void loaded_check(void) > > do { > > read_freqs(freqs); > > dump(freqs); > > - checkit(freqs); > > + check_freq_constraints(freqs); > > if (freqs[CUR] >= freqs[MAX]) > > break; > > usleep(1000 * LOADED_WAIT_TIMESTEP_MSEC); > > @@ -547,6 +560,8 @@ static void stabilize_check(int *out) > > int freqs[NUMFREQ]; > > int wait = 0; > > > > + /* Monitor frequencies until HW will stabilize cur frequency. > > + * It should happen within allotted time */ > > read_freqs(freqs); > > dump(freqs); > > usleep(1000 * STABILIZE_WAIT_TIMESTEP_MSEC); > > @@ -573,7 +588,7 @@ static void boost_freq(int fd, int *boost_freqs) > > > > fmid = get_hw_rounded_freq(fmid); > > /* Set max freq to less then boost freq */ > > - writeval(stuff[MAX].filp, fmid); > > + writeval(sysfs_files[MAX].filp, fmid); > > > > /* Put boost on the same engine as low load */ > > engine = I915_EXEC_RENDER; > > @@ -592,7 +607,7 @@ static void boost_freq(int fd, int *boost_freqs) > > igt_spin_batch_free(fd, load); > > > > /* Set max freq to original softmax */ > > - writeval(stuff[MAX].filp, origfreqs[MAX]); > > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > > } > > > > static void waitboost(int fd, bool reset) > > @@ -634,12 +649,12 @@ static void waitboost(int fd, bool reset) > > > > static void pm_rps_exit_handler(int sig) > > { > > - if (origfreqs[MIN] > readval(stuff[MAX].filp)) { > > - writeval(stuff[MAX].filp, origfreqs[MAX]); > > - writeval(stuff[MIN].filp, origfreqs[MIN]); > > + if (origfreqs[MIN] > readval(sysfs_files[MAX].filp)) { > > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > > + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); > > } else { > > - writeval(stuff[MIN].filp, origfreqs[MIN]); > > - writeval(stuff[MAX].filp, origfreqs[MAX]); > > + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); > > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > > } > > > > load_helper_deinit(); > > @@ -652,7 +667,7 @@ igt_main > > > > igt_fixture { > > const int device = drm_get_card(); > > - struct junk *junk = stuff; > > + struct sysfs_file *sysfs_file = sysfs_files; > > int ret; > > > > /* Use drm_open_driver to verify device existence */ > > @@ -663,16 +678,17 @@ igt_main > > do { > > int val = -1; > > char *path; > > - ret = asprintf(&path, sysfs_base_path, device, > > junk->name); > > + > > + ret = asprintf(&path, sysfs_base_path, device, > > sysfs_file->name); > > igt_assert(ret != -1); > > - junk->filp = fopen(path, junk->mode); > > - igt_require(junk->filp); > > - setbuf(junk->filp, NULL); > > + sysfs_file->filp = fopen(path, sysfs_file->mode); > > + igt_require(sysfs_file->filp); > > + setbuf(sysfs_file->filp, NULL); > > > > - val = readval(junk->filp); > > + val = readval(sysfs_file->filp); > > igt_assert(val >= 0); > > - junk++; > > - } while (junk->name != NULL); > > + sysfs_file++; > > + } while (sysfs_file->name != NULL); > > > > read_freqs(origfreqs); > > > > @@ -684,18 +700,22 @@ igt_main > > igt_subtest("basic-api") > > min_max_config(basic_check, false); > > > > + /* Verify the constraints, check if we can reach idle */ > > igt_subtest("min-max-config-idle") > > min_max_config(idle_check, true); > > > > + /* Verify the constraints with high load, check if we can reach max > > */ > > igt_subtest("min-max-config-loaded") { > > load_helper_run(HIGH); > > min_max_config(loaded_check, false); > > load_helper_stop(); > > } > > > > + /* Checks if we achieve boost using gem_wait */ > > We should mention this is doing gem_wait on a spinning batch, hence the > boost. > > Other than this and the above comment, LGTM. > > Acked-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > > > igt_subtest("waitboost") > > waitboost(drm_fd, false); > > > > + /* Test boost frequency after GPU reset */ > > igt_subtest("reset") { > > igt_hang_t hang = igt_allow_hang(drm_fd, 0, 0); > > waitboost(drm_fd, true); > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 9/8/2017 12:19 AM, Katarzyna Dec wrote: > On Thu, Sep 07, 2017 at 11:28:38AM -0700, Belgaumkar, Vinay wrote: >> >> >> On 9/7/2017 5:15 AM, Katarzyna Dec wrote: >>> Added comments in tricky places for better feature understanding. >>> Added IGT_TEST_DESCRIPTION and short description for non-obvious >>> subtests. >>> Changed name of 'magic' checkit() function to something meaningfull. >>> Changed junk struct and stuff array names. >>> Made some minor coding style changes. >>> >>> Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> >>> Cc: Petri Latvala <petri.latvala@intel.com> >>> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> >>> Signed-off: Katarzyna Dec <katarzyna.dec@intel.com> >>> --- >>> tests/pm_rps.c | 108 ++++++++++++++++++++++++++++++++++----------------------- >>> 1 file changed, 64 insertions(+), 44 deletions(-) >>> >>> diff --git a/tests/pm_rps.c b/tests/pm_rps.c >>> index e79f0ea7..1eeb6c6a 100644 >>> --- a/tests/pm_rps.c >>> +++ b/tests/pm_rps.c >>> @@ -40,6 +40,8 @@ >>> >>> #include "intel_bufmgr.h" >>> >>> +IGT_TEST_DESCRIPTION("Render P-States tests - verify GPU frequency changes"); >>> + >>> static int drm_fd; >>> >>> static const char sysfs_base_path[] = "/sys/class/drm/card%d/gt_%s_freq_mhz"; >>> @@ -56,12 +58,19 @@ enum { >>> >>> static int origfreqs[NUMFREQ]; >>> >>> -struct junk { >>> +struct sysfs_file { >>> const char *name; >>> const char *mode; >>> FILE *filp; >>> -} stuff[] = { >>> - { "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL }, { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { "boost", "rb+", NULL }, { NULL, NULL, NULL } >>> +} sysfs_files[] = { >>> + { "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) >>> @@ -81,7 +90,7 @@ static void read_freqs(int *freqs) >>> int i; >>> >>> for (i = 0; i < NUMFREQ; i++) >>> - freqs[i] = readval(stuff[i].filp); >>> + freqs[i] = readval(sysfs_files[i].filp); >>> } >>> >>> static void nsleep(unsigned long ns) >>> @@ -143,7 +152,7 @@ static int do_writeval(FILE *filp, int val, int lerrno, bool readback_check) >>> #define writeval_inval(filp, val) do_writeval(filp, val, EINVAL, true) >>> #define writeval_nocheck(filp, val) do_writeval(filp, val, 0, false) >>> >>> -static void checkit(const int *freqs) >>> +static void check_freq_constraints(const int *freqs) >>> { >>> igt_assert_lte(freqs[MIN], freqs[MAX]); >>> igt_assert_lte(freqs[CUR], freqs[MAX]); >>> @@ -162,7 +171,7 @@ static void dump(const int *freqs) >>> >>> igt_debug("gt freq (MHz):"); >>> for (i = 0; i < NUMFREQ; i++) >>> - igt_debug(" %s=%d", stuff[i].name, freqs[i]); >>> + igt_debug(" %s=%d", sysfs_files[i].name, freqs[i]); >>> >>> igt_debug("\n"); >>> } >>> @@ -387,14 +396,18 @@ static int get_hw_rounded_freq(int target) >>> idx = MAX; >>> >>> old_freq = freqs[idx]; >>> - writeval_nocheck(stuff[idx].filp, target); >>> + writeval_nocheck(sysfs_files[idx].filp, target); >>> read_freqs(freqs); >>> ret = freqs[idx]; >>> - writeval_nocheck(stuff[idx].filp, old_freq); >>> + writeval_nocheck(sysfs_files[idx].filp, old_freq); >>> >>> return ret; >>> } >>> >>> +/* >>> + * Modify softlimit MIN and MAX freqs to valid and invalid levels. Depending >>> + * on subtest run different check after each modification. >>> + */ >>> static void min_max_config(void (*check)(void), bool load_gpu) >>> { >>> int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; >>> @@ -411,78 +424,78 @@ static void min_max_config(void (*check)(void), bool load_gpu) >>> check(); >>> >>> igt_debug("\nSet min=RPn and max=RP0...\n"); >>> - writeval(stuff[MIN].filp, origfreqs[RPn]); >>> - writeval(stuff[MAX].filp, origfreqs[RP0]); >>> + writeval(sysfs_files[MIN].filp, origfreqs[RPn]); >>> + writeval(sysfs_files[MAX].filp, origfreqs[RP0]); >>> if (load_gpu) >>> do_load_gpu(); >>> check(); >>> >>> igt_debug("\nIncrease min to midpoint...\n"); >>> - writeval(stuff[MIN].filp, fmid); >>> + writeval(sysfs_files[MIN].filp, fmid); >>> if (load_gpu) >>> do_load_gpu(); >>> check(); >>> >>> igt_debug("\nIncrease min to RP0...\n"); >>> - writeval(stuff[MIN].filp, origfreqs[RP0]); >>> + writeval(sysfs_files[MIN].filp, origfreqs[RP0]); >>> if (load_gpu) >>> do_load_gpu(); >>> check(); >>> >>> igt_debug("\nIncrease min above RP0 (invalid)...\n"); >>> - writeval_inval(stuff[MIN].filp, origfreqs[RP0] + 1000); >>> + writeval_inval(sysfs_files[MIN].filp, origfreqs[RP0] + 1000); >>> check(); >>> >>> igt_debug("\nDecrease max to RPn (invalid)...\n"); >>> - writeval_inval(stuff[MAX].filp, origfreqs[RPn]); >>> + writeval_inval(sysfs_files[MAX].filp, origfreqs[RPn]); >>> check(); >>> >>> igt_debug("\nDecrease min to midpoint...\n"); >>> - writeval(stuff[MIN].filp, fmid); >>> + writeval(sysfs_files[MIN].filp, fmid); >>> if (load_gpu) >>> do_load_gpu(); >>> check(); >>> >>> igt_debug("\nDecrease min to RPn...\n"); >>> - writeval(stuff[MIN].filp, origfreqs[RPn]); >>> + writeval(sysfs_files[MIN].filp, origfreqs[RPn]); >>> if (load_gpu) >>> do_load_gpu(); >>> check(); >>> >>> igt_debug("\nDecrease min below RPn (invalid)...\n"); >>> - writeval_inval(stuff[MIN].filp, 0); >>> + writeval_inval(sysfs_files[MIN].filp, 0); >>> check(); >>> >>> igt_debug("\nDecrease max to midpoint...\n"); >>> - writeval(stuff[MAX].filp, fmid); >>> + writeval(sysfs_files[MAX].filp, fmid); >>> check(); >>> >>> igt_debug("\nDecrease max to RPn...\n"); >>> - writeval(stuff[MAX].filp, origfreqs[RPn]); >>> + writeval(sysfs_files[MAX].filp, origfreqs[RPn]); >>> check(); >>> >>> igt_debug("\nDecrease max below RPn (invalid)...\n"); >>> - writeval_inval(stuff[MAX].filp, 0); >>> + writeval_inval(sysfs_files[MAX].filp, 0); >>> check(); >>> >>> igt_debug("\nIncrease min to RP0 (invalid)...\n"); >>> - writeval_inval(stuff[MIN].filp, origfreqs[RP0]); >>> + writeval_inval(sysfs_files[MIN].filp, origfreqs[RP0]); >>> check(); >>> >>> igt_debug("\nIncrease max to midpoint...\n"); >>> - writeval(stuff[MAX].filp, fmid); >>> + writeval(sysfs_files[MAX].filp, fmid); >>> check(); >>> >>> igt_debug("\nIncrease max to RP0...\n"); >>> - writeval(stuff[MAX].filp, origfreqs[RP0]); >>> + writeval(sysfs_files[MAX].filp, origfreqs[RP0]); >>> check(); >>> >>> igt_debug("\nIncrease max above RP0 (invalid)...\n"); >>> - writeval_inval(stuff[MAX].filp, origfreqs[RP0] + 1000); >>> + writeval_inval(sysfs_files[MAX].filp, origfreqs[RP0] + 1000); >>> check(); >>> >>> - writeval(stuff[MIN].filp, origfreqs[MIN]); >>> - writeval(stuff[MAX].filp, origfreqs[MAX]); >>> + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); >>> + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); >>> } >>> >>> static void basic_check(void) >>> @@ -491,7 +504,7 @@ static void basic_check(void) >>> >>> read_freqs(freqs); >>> dump(freqs); >> >> Almost feel like we should create a new function read_and_dump_freqs() >> instead of calling 2 functions multiple times. We should keep the old >> read_freqs() since it is called in a loop on one occasion. Something to >> think about maybe? > If anybody reviewing this patch feel that it would be better to change > basic_check function name, I will do it :) > Also, as far as I know this function is used only in this file, so there > is no reason to add duplicate it. >> >>> - checkit(freqs); >>> + check_freq_constraints(freqs); >>> } >>> >>> #define IDLE_WAIT_TIMESTEP_MSEC 250 >>> @@ -506,7 +519,7 @@ static void idle_check(void) >>> do { >>> read_freqs(freqs); >>> dump(freqs); >>> - checkit(freqs); >>> + check_freq_constraints(freqs); >>> if (freqs[CUR] == freqs[RPn]) >>> break; >>> usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC); >>> @@ -529,7 +542,7 @@ static void loaded_check(void) >>> do { >>> read_freqs(freqs); >>> dump(freqs); >>> - checkit(freqs); >>> + check_freq_constraints(freqs); >>> if (freqs[CUR] >= freqs[MAX]) >>> break; >>> usleep(1000 * LOADED_WAIT_TIMESTEP_MSEC); >>> @@ -547,6 +560,8 @@ static void stabilize_check(int *out) >>> int freqs[NUMFREQ]; >>> int wait = 0; >>> >>> + /* Monitor frequencies until HW will stabilize cur frequency. >>> + * It should happen within allotted time */ >>> read_freqs(freqs); >>> dump(freqs); >>> usleep(1000 * STABILIZE_WAIT_TIMESTEP_MSEC); >>> @@ -573,7 +588,7 @@ static void boost_freq(int fd, int *boost_freqs) >>> >>> fmid = get_hw_rounded_freq(fmid); >>> /* Set max freq to less then boost freq */ >>> - writeval(stuff[MAX].filp, fmid); >>> + writeval(sysfs_files[MAX].filp, fmid); >>> >>> /* Put boost on the same engine as low load */ >>> engine = I915_EXEC_RENDER; >>> @@ -592,7 +607,7 @@ static void boost_freq(int fd, int *boost_freqs) >>> igt_spin_batch_free(fd, load); >>> >>> /* Set max freq to original softmax */ >>> - writeval(stuff[MAX].filp, origfreqs[MAX]); >>> + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); >>> } >>> >>> static void waitboost(int fd, bool reset) >>> @@ -634,12 +649,12 @@ static void waitboost(int fd, bool reset) >>> >>> static void pm_rps_exit_handler(int sig) >>> { >>> - if (origfreqs[MIN] > readval(stuff[MAX].filp)) { >>> - writeval(stuff[MAX].filp, origfreqs[MAX]); >>> - writeval(stuff[MIN].filp, origfreqs[MIN]); >>> + if (origfreqs[MIN] > readval(sysfs_files[MAX].filp)) { >>> + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); >>> + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); >>> } else { >>> - writeval(stuff[MIN].filp, origfreqs[MIN]); >>> - writeval(stuff[MAX].filp, origfreqs[MAX]); >>> + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); >>> + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); >>> } >>> >>> load_helper_deinit(); >>> @@ -652,7 +667,7 @@ igt_main >>> >>> igt_fixture { >>> const int device = drm_get_card(); >>> - struct junk *junk = stuff; >>> + struct sysfs_file *sysfs_file = sysfs_files; >>> int ret; >>> >>> /* Use drm_open_driver to verify device existence */ >>> @@ -663,16 +678,17 @@ igt_main >>> do { >>> int val = -1; >>> char *path; >>> - ret = asprintf(&path, sysfs_base_path, device, junk->name); >>> + >>> + ret = asprintf(&path, sysfs_base_path, device, sysfs_file->name); >>> igt_assert(ret != -1); >>> - junk->filp = fopen(path, junk->mode); >>> - igt_require(junk->filp); >>> - setbuf(junk->filp, NULL); >>> + sysfs_file->filp = fopen(path, sysfs_file->mode); >>> + igt_require(sysfs_file->filp); >>> + setbuf(sysfs_file->filp, NULL); >>> >>> - val = readval(junk->filp); >>> + val = readval(sysfs_file->filp); >>> igt_assert(val >= 0); >>> - junk++; >>> - } while (junk->name != NULL); >>> + sysfs_file++; >>> + } while (sysfs_file->name != NULL); >>> >>> read_freqs(origfreqs); >>> >>> @@ -684,18 +700,22 @@ igt_main >>> igt_subtest("basic-api") >>> min_max_config(basic_check, false); >>> >>> + /* Verify the constraints, check if we can reach idle */ >>> igt_subtest("min-max-config-idle") >>> min_max_config(idle_check, true); >>> >>> + /* Verify the constraints with high load, check if we can reach max */ >>> igt_subtest("min-max-config-loaded") { >>> load_helper_run(HIGH); >>> min_max_config(loaded_check, false); >>> load_helper_stop(); >>> } >>> >>> + /* Checks if we achieve boost using gem_wait */ >> >> We should mention this is doing gem_wait on a spinning batch, hence the >> boost. > I think that additional information is not needed here. I got 2 reasons: > - we assume that subtest comment will be oneliner > - do we need such details here? This current form gives the idea that we can get turbo boost by simply doing a gem_wait, which is incorrect. We either say "Checks if we can achieve boost" or then give the complete info - "Checks if we can achieve boost using gem_wait on a spinning batch". Either ways, it is still single line. > > Thanks, > Kasia >> >> Other than this and the above comment, LGTM. >> >> Acked-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> >> >> >>> igt_subtest("waitboost") >>> waitboost(drm_fd, false); >>> >>> + /* Test boost frequency after GPU reset */ >>> igt_subtest("reset") { >>> igt_hang_t hang = igt_allow_hang(drm_fd, 0, 0); >>> waitboost(drm_fd, true); >>>
On Fri, Sep 08, 2017 at 09:48:17AM -0700, Belgaumkar, Vinay wrote: [SNIP] > > > > > > > > + /* Checks if we achieve boost using gem_wait */ > > > > > > We should mention this is doing gem_wait on a spinning batch, hence the > > > boost. > > I think that additional information is not needed here. I got 2 reasons: > > - we assume that subtest comment will be oneliner > > - do we need such details here? > > This current form gives the idea that we can get turbo boost by simply doing > a gem_wait, which is incorrect. We either say "Checks if we can achieve > boost" or then give the complete info - "Checks if we can achieve boost > using gem_wait on a spinning batch". Either ways, it is still single line. Yes, the request needs to be incomplete to grant boost. No, stating that we're granting boost on spinning batches is confusing because of terminology overload (spinbatch is a specific type of GPU load that we're using). -Michał > > > > > Thanks, > > Kasia > > > > > > Other than this and the above comment, LGTM. > > > > > > Acked-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > > > > > > > > > igt_subtest("waitboost") > > > > waitboost(drm_fd, false); > > > > > > > > + /* Test boost frequency after GPU reset */ > > > > igt_subtest("reset") { > > > > igt_hang_t hang = igt_allow_hang(drm_fd, 0, 0); > > > > waitboost(drm_fd, true); > > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Sep 07, 2017 at 02:15:14PM +0200, Katarzyna Dec wrote: > Added comments in tricky places for better feature understanding. > Added IGT_TEST_DESCRIPTION and short description for non-obvious > subtests. > Changed name of 'magic' checkit() function to something meaningfull. > Changed junk struct and stuff array names. > Made some minor coding style changes. > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > Cc: Petri Latvala <petri.latvala@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off: Katarzyna Dec <katarzyna.dec@intel.com> LGTM. I especially like changing the identifiers for more self-explanatory forms. Acked-by: Petri Latvala <petri.latvala@intel.com> > --- > tests/pm_rps.c | 108 ++++++++++++++++++++++++++++++++++----------------------- > 1 file changed, 64 insertions(+), 44 deletions(-) > > diff --git a/tests/pm_rps.c b/tests/pm_rps.c > index e79f0ea7..1eeb6c6a 100644 > --- a/tests/pm_rps.c > +++ b/tests/pm_rps.c > @@ -40,6 +40,8 @@ > > #include "intel_bufmgr.h" > > +IGT_TEST_DESCRIPTION("Render P-States tests - verify GPU frequency changes"); > + > static int drm_fd; > > static const char sysfs_base_path[] = "/sys/class/drm/card%d/gt_%s_freq_mhz"; > @@ -56,12 +58,19 @@ enum { > > static int origfreqs[NUMFREQ]; > > -struct junk { > +struct sysfs_file { > const char *name; > const char *mode; > FILE *filp; > -} stuff[] = { > - { "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL }, { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { "boost", "rb+", NULL }, { NULL, NULL, NULL } > +} sysfs_files[] = { > + { "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) > @@ -81,7 +90,7 @@ static void read_freqs(int *freqs) > int i; > > for (i = 0; i < NUMFREQ; i++) > - freqs[i] = readval(stuff[i].filp); > + freqs[i] = readval(sysfs_files[i].filp); > } > > static void nsleep(unsigned long ns) > @@ -143,7 +152,7 @@ static int do_writeval(FILE *filp, int val, int lerrno, bool readback_check) > #define writeval_inval(filp, val) do_writeval(filp, val, EINVAL, true) > #define writeval_nocheck(filp, val) do_writeval(filp, val, 0, false) > > -static void checkit(const int *freqs) > +static void check_freq_constraints(const int *freqs) > { > igt_assert_lte(freqs[MIN], freqs[MAX]); > igt_assert_lte(freqs[CUR], freqs[MAX]); > @@ -162,7 +171,7 @@ static void dump(const int *freqs) > > igt_debug("gt freq (MHz):"); > for (i = 0; i < NUMFREQ; i++) > - igt_debug(" %s=%d", stuff[i].name, freqs[i]); > + igt_debug(" %s=%d", sysfs_files[i].name, freqs[i]); > > igt_debug("\n"); > } > @@ -387,14 +396,18 @@ static int get_hw_rounded_freq(int target) > idx = MAX; > > old_freq = freqs[idx]; > - writeval_nocheck(stuff[idx].filp, target); > + writeval_nocheck(sysfs_files[idx].filp, target); > read_freqs(freqs); > ret = freqs[idx]; > - writeval_nocheck(stuff[idx].filp, old_freq); > + writeval_nocheck(sysfs_files[idx].filp, old_freq); > > return ret; > } > > +/* > + * Modify softlimit MIN and MAX freqs to valid and invalid levels. Depending > + * on subtest run different check after each modification. > + */ > static void min_max_config(void (*check)(void), bool load_gpu) > { > int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; > @@ -411,78 +424,78 @@ static void min_max_config(void (*check)(void), bool load_gpu) > check(); > > igt_debug("\nSet min=RPn and max=RP0...\n"); > - writeval(stuff[MIN].filp, origfreqs[RPn]); > - writeval(stuff[MAX].filp, origfreqs[RP0]); > + writeval(sysfs_files[MIN].filp, origfreqs[RPn]); > + writeval(sysfs_files[MAX].filp, origfreqs[RP0]); > if (load_gpu) > do_load_gpu(); > check(); > > igt_debug("\nIncrease min to midpoint...\n"); > - writeval(stuff[MIN].filp, fmid); > + writeval(sysfs_files[MIN].filp, fmid); > if (load_gpu) > do_load_gpu(); > check(); > > igt_debug("\nIncrease min to RP0...\n"); > - writeval(stuff[MIN].filp, origfreqs[RP0]); > + writeval(sysfs_files[MIN].filp, origfreqs[RP0]); > if (load_gpu) > do_load_gpu(); > check(); > > igt_debug("\nIncrease min above RP0 (invalid)...\n"); > - writeval_inval(stuff[MIN].filp, origfreqs[RP0] + 1000); > + writeval_inval(sysfs_files[MIN].filp, origfreqs[RP0] + 1000); > check(); > > igt_debug("\nDecrease max to RPn (invalid)...\n"); > - writeval_inval(stuff[MAX].filp, origfreqs[RPn]); > + writeval_inval(sysfs_files[MAX].filp, origfreqs[RPn]); > check(); > > igt_debug("\nDecrease min to midpoint...\n"); > - writeval(stuff[MIN].filp, fmid); > + writeval(sysfs_files[MIN].filp, fmid); > if (load_gpu) > do_load_gpu(); > check(); > > igt_debug("\nDecrease min to RPn...\n"); > - writeval(stuff[MIN].filp, origfreqs[RPn]); > + writeval(sysfs_files[MIN].filp, origfreqs[RPn]); > if (load_gpu) > do_load_gpu(); > check(); > > igt_debug("\nDecrease min below RPn (invalid)...\n"); > - writeval_inval(stuff[MIN].filp, 0); > + writeval_inval(sysfs_files[MIN].filp, 0); > check(); > > igt_debug("\nDecrease max to midpoint...\n"); > - writeval(stuff[MAX].filp, fmid); > + writeval(sysfs_files[MAX].filp, fmid); > check(); > > igt_debug("\nDecrease max to RPn...\n"); > - writeval(stuff[MAX].filp, origfreqs[RPn]); > + writeval(sysfs_files[MAX].filp, origfreqs[RPn]); > check(); > > igt_debug("\nDecrease max below RPn (invalid)...\n"); > - writeval_inval(stuff[MAX].filp, 0); > + writeval_inval(sysfs_files[MAX].filp, 0); > check(); > > igt_debug("\nIncrease min to RP0 (invalid)...\n"); > - writeval_inval(stuff[MIN].filp, origfreqs[RP0]); > + writeval_inval(sysfs_files[MIN].filp, origfreqs[RP0]); > check(); > > igt_debug("\nIncrease max to midpoint...\n"); > - writeval(stuff[MAX].filp, fmid); > + writeval(sysfs_files[MAX].filp, fmid); > check(); > > igt_debug("\nIncrease max to RP0...\n"); > - writeval(stuff[MAX].filp, origfreqs[RP0]); > + writeval(sysfs_files[MAX].filp, origfreqs[RP0]); > check(); > > igt_debug("\nIncrease max above RP0 (invalid)...\n"); > - writeval_inval(stuff[MAX].filp, origfreqs[RP0] + 1000); > + writeval_inval(sysfs_files[MAX].filp, origfreqs[RP0] + 1000); > check(); > > - writeval(stuff[MIN].filp, origfreqs[MIN]); > - writeval(stuff[MAX].filp, origfreqs[MAX]); > + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > } > > static void basic_check(void) > @@ -491,7 +504,7 @@ static void basic_check(void) > > read_freqs(freqs); > dump(freqs); > - checkit(freqs); > + check_freq_constraints(freqs); > } > > #define IDLE_WAIT_TIMESTEP_MSEC 250 > @@ -506,7 +519,7 @@ static void idle_check(void) > do { > read_freqs(freqs); > dump(freqs); > - checkit(freqs); > + check_freq_constraints(freqs); > if (freqs[CUR] == freqs[RPn]) > break; > usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC); > @@ -529,7 +542,7 @@ static void loaded_check(void) > do { > read_freqs(freqs); > dump(freqs); > - checkit(freqs); > + check_freq_constraints(freqs); > if (freqs[CUR] >= freqs[MAX]) > break; > usleep(1000 * LOADED_WAIT_TIMESTEP_MSEC); > @@ -547,6 +560,8 @@ static void stabilize_check(int *out) > int freqs[NUMFREQ]; > int wait = 0; > > + /* Monitor frequencies until HW will stabilize cur frequency. > + * It should happen within allotted time */ > read_freqs(freqs); > dump(freqs); > usleep(1000 * STABILIZE_WAIT_TIMESTEP_MSEC); > @@ -573,7 +588,7 @@ static void boost_freq(int fd, int *boost_freqs) > > fmid = get_hw_rounded_freq(fmid); > /* Set max freq to less then boost freq */ > - writeval(stuff[MAX].filp, fmid); > + writeval(sysfs_files[MAX].filp, fmid); > > /* Put boost on the same engine as low load */ > engine = I915_EXEC_RENDER; > @@ -592,7 +607,7 @@ static void boost_freq(int fd, int *boost_freqs) > igt_spin_batch_free(fd, load); > > /* Set max freq to original softmax */ > - writeval(stuff[MAX].filp, origfreqs[MAX]); > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > } > > static void waitboost(int fd, bool reset) > @@ -634,12 +649,12 @@ static void waitboost(int fd, bool reset) > > static void pm_rps_exit_handler(int sig) > { > - if (origfreqs[MIN] > readval(stuff[MAX].filp)) { > - writeval(stuff[MAX].filp, origfreqs[MAX]); > - writeval(stuff[MIN].filp, origfreqs[MIN]); > + if (origfreqs[MIN] > readval(sysfs_files[MAX].filp)) { > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); > } else { > - writeval(stuff[MIN].filp, origfreqs[MIN]); > - writeval(stuff[MAX].filp, origfreqs[MAX]); > + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); > + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); > } > > load_helper_deinit(); > @@ -652,7 +667,7 @@ igt_main > > igt_fixture { > const int device = drm_get_card(); > - struct junk *junk = stuff; > + struct sysfs_file *sysfs_file = sysfs_files; > int ret; > > /* Use drm_open_driver to verify device existence */ > @@ -663,16 +678,17 @@ igt_main > do { > int val = -1; > char *path; > - ret = asprintf(&path, sysfs_base_path, device, junk->name); > + > + ret = asprintf(&path, sysfs_base_path, device, sysfs_file->name); > igt_assert(ret != -1); > - junk->filp = fopen(path, junk->mode); > - igt_require(junk->filp); > - setbuf(junk->filp, NULL); > + sysfs_file->filp = fopen(path, sysfs_file->mode); > + igt_require(sysfs_file->filp); > + setbuf(sysfs_file->filp, NULL); > > - val = readval(junk->filp); > + val = readval(sysfs_file->filp); > igt_assert(val >= 0); > - junk++; > - } while (junk->name != NULL); > + sysfs_file++; > + } while (sysfs_file->name != NULL); > > read_freqs(origfreqs); > > @@ -684,18 +700,22 @@ igt_main > igt_subtest("basic-api") > min_max_config(basic_check, false); > > + /* Verify the constraints, check if we can reach idle */ > igt_subtest("min-max-config-idle") > min_max_config(idle_check, true); > > + /* Verify the constraints with high load, check if we can reach max */ > igt_subtest("min-max-config-loaded") { > load_helper_run(HIGH); > min_max_config(loaded_check, false); > load_helper_stop(); > } > > + /* Checks if we achieve boost using gem_wait */ > igt_subtest("waitboost") > waitboost(drm_fd, false); > > + /* Test boost frequency after GPU reset */ > igt_subtest("reset") { > igt_hang_t hang = igt_allow_hang(drm_fd, 0, 0); > waitboost(drm_fd, true); > -- > 2.13.4 >
On Thu, Sep 07, 2017 at 02:15:14PM +0200, Katarzyna Dec wrote: > Added comments in tricky places for better feature understanding. > Added IGT_TEST_DESCRIPTION and short description for non-obvious > subtests. > Changed name of 'magic' checkit() function to something meaningfull. > Changed junk struct and stuff array names. > Made some minor coding style changes. > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > Cc: Petri Latvala <petri.latvala@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off: Katarzyna Dec <katarzyna.dec@intel.com> Gathered all Acks here and pushed, as there is no use in further bikesheeding the wording ;-) It can be fine-tuned in the following patches if it proves necessary anyway. Even if it's not perfect it makes pm_rps a really good, real-world example how things should look like. Thanks for the patch and the reviews!
diff --git a/tests/pm_rps.c b/tests/pm_rps.c index e79f0ea7..1eeb6c6a 100644 --- a/tests/pm_rps.c +++ b/tests/pm_rps.c @@ -40,6 +40,8 @@ #include "intel_bufmgr.h" +IGT_TEST_DESCRIPTION("Render P-States tests - verify GPU frequency changes"); + static int drm_fd; static const char sysfs_base_path[] = "/sys/class/drm/card%d/gt_%s_freq_mhz"; @@ -56,12 +58,19 @@ enum { static int origfreqs[NUMFREQ]; -struct junk { +struct sysfs_file { const char *name; const char *mode; FILE *filp; -} stuff[] = { - { "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL }, { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { "boost", "rb+", NULL }, { NULL, NULL, NULL } +} sysfs_files[] = { + { "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) @@ -81,7 +90,7 @@ static void read_freqs(int *freqs) int i; for (i = 0; i < NUMFREQ; i++) - freqs[i] = readval(stuff[i].filp); + freqs[i] = readval(sysfs_files[i].filp); } static void nsleep(unsigned long ns) @@ -143,7 +152,7 @@ static int do_writeval(FILE *filp, int val, int lerrno, bool readback_check) #define writeval_inval(filp, val) do_writeval(filp, val, EINVAL, true) #define writeval_nocheck(filp, val) do_writeval(filp, val, 0, false) -static void checkit(const int *freqs) +static void check_freq_constraints(const int *freqs) { igt_assert_lte(freqs[MIN], freqs[MAX]); igt_assert_lte(freqs[CUR], freqs[MAX]); @@ -162,7 +171,7 @@ static void dump(const int *freqs) igt_debug("gt freq (MHz):"); for (i = 0; i < NUMFREQ; i++) - igt_debug(" %s=%d", stuff[i].name, freqs[i]); + igt_debug(" %s=%d", sysfs_files[i].name, freqs[i]); igt_debug("\n"); } @@ -387,14 +396,18 @@ static int get_hw_rounded_freq(int target) idx = MAX; old_freq = freqs[idx]; - writeval_nocheck(stuff[idx].filp, target); + writeval_nocheck(sysfs_files[idx].filp, target); read_freqs(freqs); ret = freqs[idx]; - writeval_nocheck(stuff[idx].filp, old_freq); + writeval_nocheck(sysfs_files[idx].filp, old_freq); return ret; } +/* + * Modify softlimit MIN and MAX freqs to valid and invalid levels. Depending + * on subtest run different check after each modification. + */ static void min_max_config(void (*check)(void), bool load_gpu) { int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2; @@ -411,78 +424,78 @@ static void min_max_config(void (*check)(void), bool load_gpu) check(); igt_debug("\nSet min=RPn and max=RP0...\n"); - writeval(stuff[MIN].filp, origfreqs[RPn]); - writeval(stuff[MAX].filp, origfreqs[RP0]); + writeval(sysfs_files[MIN].filp, origfreqs[RPn]); + writeval(sysfs_files[MAX].filp, origfreqs[RP0]); if (load_gpu) do_load_gpu(); check(); igt_debug("\nIncrease min to midpoint...\n"); - writeval(stuff[MIN].filp, fmid); + writeval(sysfs_files[MIN].filp, fmid); if (load_gpu) do_load_gpu(); check(); igt_debug("\nIncrease min to RP0...\n"); - writeval(stuff[MIN].filp, origfreqs[RP0]); + writeval(sysfs_files[MIN].filp, origfreqs[RP0]); if (load_gpu) do_load_gpu(); check(); igt_debug("\nIncrease min above RP0 (invalid)...\n"); - writeval_inval(stuff[MIN].filp, origfreqs[RP0] + 1000); + writeval_inval(sysfs_files[MIN].filp, origfreqs[RP0] + 1000); check(); igt_debug("\nDecrease max to RPn (invalid)...\n"); - writeval_inval(stuff[MAX].filp, origfreqs[RPn]); + writeval_inval(sysfs_files[MAX].filp, origfreqs[RPn]); check(); igt_debug("\nDecrease min to midpoint...\n"); - writeval(stuff[MIN].filp, fmid); + writeval(sysfs_files[MIN].filp, fmid); if (load_gpu) do_load_gpu(); check(); igt_debug("\nDecrease min to RPn...\n"); - writeval(stuff[MIN].filp, origfreqs[RPn]); + writeval(sysfs_files[MIN].filp, origfreqs[RPn]); if (load_gpu) do_load_gpu(); check(); igt_debug("\nDecrease min below RPn (invalid)...\n"); - writeval_inval(stuff[MIN].filp, 0); + writeval_inval(sysfs_files[MIN].filp, 0); check(); igt_debug("\nDecrease max to midpoint...\n"); - writeval(stuff[MAX].filp, fmid); + writeval(sysfs_files[MAX].filp, fmid); check(); igt_debug("\nDecrease max to RPn...\n"); - writeval(stuff[MAX].filp, origfreqs[RPn]); + writeval(sysfs_files[MAX].filp, origfreqs[RPn]); check(); igt_debug("\nDecrease max below RPn (invalid)...\n"); - writeval_inval(stuff[MAX].filp, 0); + writeval_inval(sysfs_files[MAX].filp, 0); check(); igt_debug("\nIncrease min to RP0 (invalid)...\n"); - writeval_inval(stuff[MIN].filp, origfreqs[RP0]); + writeval_inval(sysfs_files[MIN].filp, origfreqs[RP0]); check(); igt_debug("\nIncrease max to midpoint...\n"); - writeval(stuff[MAX].filp, fmid); + writeval(sysfs_files[MAX].filp, fmid); check(); igt_debug("\nIncrease max to RP0...\n"); - writeval(stuff[MAX].filp, origfreqs[RP0]); + writeval(sysfs_files[MAX].filp, origfreqs[RP0]); check(); igt_debug("\nIncrease max above RP0 (invalid)...\n"); - writeval_inval(stuff[MAX].filp, origfreqs[RP0] + 1000); + writeval_inval(sysfs_files[MAX].filp, origfreqs[RP0] + 1000); check(); - writeval(stuff[MIN].filp, origfreqs[MIN]); - writeval(stuff[MAX].filp, origfreqs[MAX]); + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); } static void basic_check(void) @@ -491,7 +504,7 @@ static void basic_check(void) read_freqs(freqs); dump(freqs); - checkit(freqs); + check_freq_constraints(freqs); } #define IDLE_WAIT_TIMESTEP_MSEC 250 @@ -506,7 +519,7 @@ static void idle_check(void) do { read_freqs(freqs); dump(freqs); - checkit(freqs); + check_freq_constraints(freqs); if (freqs[CUR] == freqs[RPn]) break; usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC); @@ -529,7 +542,7 @@ static void loaded_check(void) do { read_freqs(freqs); dump(freqs); - checkit(freqs); + check_freq_constraints(freqs); if (freqs[CUR] >= freqs[MAX]) break; usleep(1000 * LOADED_WAIT_TIMESTEP_MSEC); @@ -547,6 +560,8 @@ static void stabilize_check(int *out) int freqs[NUMFREQ]; int wait = 0; + /* Monitor frequencies until HW will stabilize cur frequency. + * It should happen within allotted time */ read_freqs(freqs); dump(freqs); usleep(1000 * STABILIZE_WAIT_TIMESTEP_MSEC); @@ -573,7 +588,7 @@ static void boost_freq(int fd, int *boost_freqs) fmid = get_hw_rounded_freq(fmid); /* Set max freq to less then boost freq */ - writeval(stuff[MAX].filp, fmid); + writeval(sysfs_files[MAX].filp, fmid); /* Put boost on the same engine as low load */ engine = I915_EXEC_RENDER; @@ -592,7 +607,7 @@ static void boost_freq(int fd, int *boost_freqs) igt_spin_batch_free(fd, load); /* Set max freq to original softmax */ - writeval(stuff[MAX].filp, origfreqs[MAX]); + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); } static void waitboost(int fd, bool reset) @@ -634,12 +649,12 @@ static void waitboost(int fd, bool reset) static void pm_rps_exit_handler(int sig) { - if (origfreqs[MIN] > readval(stuff[MAX].filp)) { - writeval(stuff[MAX].filp, origfreqs[MAX]); - writeval(stuff[MIN].filp, origfreqs[MIN]); + if (origfreqs[MIN] > readval(sysfs_files[MAX].filp)) { + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); } else { - writeval(stuff[MIN].filp, origfreqs[MIN]); - writeval(stuff[MAX].filp, origfreqs[MAX]); + writeval(sysfs_files[MIN].filp, origfreqs[MIN]); + writeval(sysfs_files[MAX].filp, origfreqs[MAX]); } load_helper_deinit(); @@ -652,7 +667,7 @@ igt_main igt_fixture { const int device = drm_get_card(); - struct junk *junk = stuff; + struct sysfs_file *sysfs_file = sysfs_files; int ret; /* Use drm_open_driver to verify device existence */ @@ -663,16 +678,17 @@ igt_main do { int val = -1; char *path; - ret = asprintf(&path, sysfs_base_path, device, junk->name); + + ret = asprintf(&path, sysfs_base_path, device, sysfs_file->name); igt_assert(ret != -1); - junk->filp = fopen(path, junk->mode); - igt_require(junk->filp); - setbuf(junk->filp, NULL); + sysfs_file->filp = fopen(path, sysfs_file->mode); + igt_require(sysfs_file->filp); + setbuf(sysfs_file->filp, NULL); - val = readval(junk->filp); + val = readval(sysfs_file->filp); igt_assert(val >= 0); - junk++; - } while (junk->name != NULL); + sysfs_file++; + } while (sysfs_file->name != NULL); read_freqs(origfreqs); @@ -684,18 +700,22 @@ igt_main igt_subtest("basic-api") min_max_config(basic_check, false); + /* Verify the constraints, check if we can reach idle */ igt_subtest("min-max-config-idle") min_max_config(idle_check, true); + /* Verify the constraints with high load, check if we can reach max */ igt_subtest("min-max-config-loaded") { load_helper_run(HIGH); min_max_config(loaded_check, false); load_helper_stop(); } + /* Checks if we achieve boost using gem_wait */ igt_subtest("waitboost") waitboost(drm_fd, false); + /* Test boost frequency after GPU reset */ igt_subtest("reset") { igt_hang_t hang = igt_allow_hang(drm_fd, 0, 0); waitboost(drm_fd, true);