[Intel-gfx] [PATCH i-g-t] pm_rps: [RFC] RPS tests documentation update
Arkadiusz Hiler
arkadiusz.hiler at intel.com
Thu Sep 7 13:43:17 UTC 2017
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 at intel.com>
> Cc: Petri Latvala <petri.latvala at intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>
> Signed-off: Katarzyna Dec <katarzyna.dec at 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 at intel.com>
But let's wait and see what others (at least those CCed) have to say
before popping a champagne on this.
--
Cheers,
Arek
> 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
>
More information about the Intel-gfx
mailing list