From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7E0A06F05E for ; Wed, 23 Jan 2019 22:41:39 +0000 (UTC) From: "Souza, Jose" Date: Wed, 23 Jan 2019 22:41:37 +0000 Message-ID: <2200fded99e754801872f22ece22ebe8a832643c.camel@intel.com> References: <20190123010950.25867-1-jose.souza@intel.com> <20190123010950.25867-2-jose.souza@intel.com> In-Reply-To: Content-Language: en-US MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH i-g-t v3 2/2] test: Add PSR2 selective update tests List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============1802565446==" Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: "igt-dev@lists.freedesktop.org" , "Pandiyan, Dhinakaran" Cc: "Vivi, Rodrigo" List-ID: --===============1802565446== Content-Language: en-US Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-I3w0o4ISh4WbOH6kOXWy" --=-I3w0o4ISh4WbOH6kOXWy Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2019-01-22 at 21:30 -0800, Dhinakaran Pandiyan wrote: > On Tue, 2019-01-22 at 17:09 -0800, Jos=C3=A9 Roberto de Souza wrote: > > This tests checks if hardware is able to do selective update when > > screen changes. > > PSR2 don't trigger interruptions and the 'PSR2 SU status' register > > is not kept loaded all the times, so it is necessary keep polling > > PSR status debugfs until those values are loaded. > >=20 > > Also from DEEP_SLEEP state HW will not do a seletive update, as > > most of the memory/context is lost in deep sleep state hardware > > will > > need to exit PSR mode then wait a configured number of frames to > > activate PSR again to then start doing seletive updates, that is > > why > > just one screen change is not enough to pass this tests. >=20 > How do you ensure the hardware hasn't gone to deep sleep? Can we make > the test fail if the test configuration allowed DEEP_SLEEP?=20 The test will fail if it goes to DEEP_SLEEP after the MAX_SCREEN_CHANGES but this is unlikely to happen as the minimum number of frames to enter deep sleep is 6. > > When a selective update happens and the values are loaded and read > > from debugfs it is compared with the expected value of seletive > > update blocks, if matches the polling is stopped and the test > > passed > > otherwise it will wait until it reachs a maximum number o screen > > changes to fail the test. > >=20 > > v2: Using new SU blocks debugfs output > >=20 > > v3: > > - removed the timerfd to fail the test, now failing based in a > > maximum number of screen changes > > - removing thread to read debugfs, read from main thread is enough > > - improved commit message > >=20 > > Cc: Rodrigo Vivi > > Cc: Dhinakaran Pandiyan > > Signed-off-by: Jos=C3=A9 Roberto de Souza > > --- > > lib/igt_psr.c | 29 ++++ > > lib/igt_psr.h | 1 + > > tests/Makefile.sources | 1 + > > tests/kms_psr2_su.c | 302 > > +++++++++++++++++++++++++++++++++++++++++ > > tests/meson.build | 1 + > > 5 files changed, 334 insertions(+) > > create mode 100644 tests/kms_psr2_su.c > >=20 > > diff --git a/lib/igt_psr.c b/lib/igt_psr.c > > index d726fad5..8c0f05e8 100644 > > --- a/lib/igt_psr.c > > +++ b/lib/igt_psr.c > > @@ -178,3 +178,32 @@ bool psr_sink_support(int debugfs_fd, enum > > psr_mode mode) > > */ > > return strstr(buf, "Sink support: yes [0x03]"); > > } > > + > > +#define PSR2_SU_BLOCK_STR_LOOKUP "PSR2 SU blocks:\n0\t" > > + > > +static bool > > +psr2_read_last_num_su_blocks_val(int debugfs_fd, uint16_t > > *num_su_blocks) > > +{ > > + char buf[PSR_STATUS_MAX_LEN]; > > + char *str; > > + int ret; > > + > > + ret =3D igt_debugfs_simple_read(debugfs_fd, > > "i915_edp_psr_status", buf, > > + sizeof(buf)); > > + if (ret < 0) > > + return false; > > + > > + str =3D strstr(buf, PSR2_SU_BLOCK_STR_LOOKUP); > > + if (!str) > > + return false; > > + > > + str =3D &str[strlen(PSR2_SU_BLOCK_STR_LOOKUP)]; > > + *num_su_blocks =3D (uint16_t)strtol(str, NULL, 10); > > + > > + return true; > > +} > > + > > +bool psr2_wait_su(int debugfs_fd, uint16_t *num_su_blocks) > > +{ > > + return igt_wait(psr2_read_last_num_su_blocks_val(debugfs_fd, > > num_su_blocks), 40, 1); > > +} > > diff --git a/lib/igt_psr.h b/lib/igt_psr.h > > index 7e7017bf..49599cf8 100644 > > --- a/lib/igt_psr.h > > +++ b/lib/igt_psr.h > > @@ -40,5 +40,6 @@ bool psr_wait_update(int debugfs_fd, enum > > psr_mode > > mode); > > bool psr_enable(int debugfs_fd, enum psr_mode); > > bool psr_disable(int debugfs_fd); > > bool psr_sink_support(int debugfs_fd, enum psr_mode); > > +bool psr2_wait_su(int debugfs_fd, uint16_t *num_su_blocks); > > =20 > > #endif > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > > index 519eac79..9174aecc 100644 > > --- a/tests/Makefile.sources > > +++ b/tests/Makefile.sources > > @@ -80,6 +80,7 @@ TESTS_progs =3D \ > > kms_plane_scaling \ > > kms_properties \ > > kms_psr \ > > + kms_psr2_su \ > > kms_pwrite_crc \ > > kms_rmfb \ > > kms_rotation_crc \ > > diff --git a/tests/kms_psr2_su.c b/tests/kms_psr2_su.c > > new file mode 100644 > > index 00000000..f6e85a2f > > --- /dev/null > > +++ b/tests/kms_psr2_su.c > > @@ -0,0 +1,302 @@ > > +/* > > + * Copyright =C2=A9 2019 Intel Corporation > > + * > > + * Permission is hereby granted, free of charge, to any person > > obtaining a > > + * copy of this software and associated documentation files (the > > "Software"), > > + * to deal in the Software without restriction, including without > > limitation > > + * the rights to use, copy, modify, merge, publish, distribute, > > sublicense, > > + * and/or sell copies of the Software, and to permit persons to > > whom > > the > > + * Software is furnished to do so, subject to the following > > conditions: > > + * > > + * The above copyright notice and this permission notice > > (including > > the next > > + * paragraph) shall be included in all copies or substantial > > portions of the > > + * Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > > EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > > MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO > > EVENT SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, > > DAMAGES > > OR OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > > ARISING > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR > > OTHER DEALINGS > > + * IN THE SOFTWARE. > > + * > > + */ > > + > > +#include "igt.h" > > +#include "igt_sysfs.h" > > +#include "igt_psr.h" > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include "intel_bufmgr.h" > > + > > +IGT_TEST_DESCRIPTION("Test PSR2 selective update"); > > + > > +#define SQUARE_SIZE 100 > > +/* each selective update block is 4 lines tall */ > > +#define EXPECTED_NUM_SU_BLOCKS ((SQUARE_SIZE / 4) + (SQUARE_SIZE % > > 4 > > ? 1 : 0)) > > + > > +/* > > + * Minimum is 15 as the number of frames to active PSR2 could be > > configured > > + * to 15 frames plus a few more in case we miss a selective update > > between > > + * debugfs reads. > > + */ > > +#define MAX_SCREEN_CHANGES 20 > > + > > +enum operations { > > + PAGE_FLIP, > > + FRONTBUFFER, > > + LAST > > +}; > > + > > +static const char *op_str(enum operations op) > > +{ > > + static const char * const name[] =3D { > > + [PAGE_FLIP] =3D "page_flip", > > + [FRONTBUFFER] =3D "frontbuffer" > > + }; > > + > > + return name[op]; > > +} > > + > > +typedef struct { > > + int drm_fd; > > + int debugfs_fd; > > + igt_display_t display; > > + drm_intel_bufmgr *bufmgr; > > + drmModeModeInfo *mode; > > + igt_output_t *output; > > + struct igt_fb fb[2]; > > + struct pollfd pollfds[1]; > > + enum operations op; > > + int change_screen_timerfd; > > + uint32_t screen_changes; > > + bool success; > > + > > +} data_t; > > + > > +static void setup_output(data_t *data) > > +{ > > + igt_display_t *display =3D &data->display; > > + igt_output_t *output; > > + enum pipe pipe; > > + > > + for_each_pipe_with_valid_output(display, pipe, output) { > > + drmModeConnectorPtr c =3D output->config.connector; > > + > > + if (c->connector_type !=3D DRM_MODE_CONNECTOR_eDP) > > + continue; > > + > > + igt_output_set_pipe(output, pipe); > > + data->output =3D output; > > + data->mode =3D igt_output_get_mode(output); > > + > > + return; > > + } > > +} > > + > > +static void display_init(data_t *data) > > +{ > > + igt_display_require(&data->display, data->drm_fd); > > + setup_output(data); > > +} > > + > > +static void display_fini(data_t *data) > > +{ > > + igt_display_fini(&data->display); > > +} > > + > > +static void prepare(data_t *data) > > +{ > > + igt_plane_t *primary; > > + > > + /* all green frame */ > > + igt_create_color_fb(data->drm_fd, > > + data->mode->hdisplay, data->mode->vdisplay, > > + DRM_FORMAT_XRGB8888, > > + LOCAL_DRM_FORMAT_MOD_NONE, > > + 0.0, 1.0, 0.0, > > + &data->fb[0]); > > + > > + if (data->op =3D=3D PAGE_FLIP) { > > + cairo_t *cr; > > + > > + igt_create_color_fb(data->drm_fd, > > + data->mode->hdisplay, data->mode- > > > vdisplay, > > + DRM_FORMAT_XRGB8888, > > + LOCAL_DRM_FORMAT_MOD_NONE, > > + 0.0, 1.0, 0.0, > > + &data->fb[1]); > > + > > + cr =3D igt_get_cairo_ctx(data->drm_fd, &data->fb[1]); > > + /* paint a white square */ > > + igt_paint_color_alpha(cr, 0, 0, SQUARE_SIZE, > > SQUARE_SIZE, > > + 1.0, 1.0, 1.0, 1.0); > > + igt_put_cairo_ctx(data->drm_fd, &data->fb[1], cr); > > + } > > + > > + primary =3D igt_output_get_plane_type(data->output, > > + DRM_PLANE_TYPE_PRIMARY); > > + igt_plane_set_fb(primary, NULL); > > + > > + igt_display_commit(&data->display); > > + igt_plane_set_fb(primary, &data->fb[0]); > > + igt_display_commit(&data->display); > > + > > + igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2)); > > + > > + data->success =3D false; > > + data->screen_changes =3D 0; > > +} > > + > > +static void update_screen_and_test(data_t *data) > > +{ > > + uint16_t su_blocks; > > + > > + data->screen_changes++; > > + > > + switch (data->op) { > > + case PAGE_FLIP: { > > + igt_plane_t *primary; > > + > > + primary =3D igt_output_get_plane_type(data->output, > > + DRM_PLANE_TYPE_PRIM > > ARY); > > + > > + igt_plane_set_fb(primary, &data->fb[data- > > > screen_changes & 1]); > > + igt_display_commit(&data->display); > > + break; > > + } > > + case FRONTBUFFER: { > > + drmModeClip clip; > > + cairo_t *cr; > > + int r; > > + > > + clip.x1 =3D clip.y1 =3D 0; > > + clip.x2 =3D clip.y2 =3D SQUARE_SIZE; > > + > > + cr =3D igt_get_cairo_ctx(data->drm_fd, &data->fb[0]); > I'm not familiar with cairo usage, looks like igt_put_cairo_ctx() is > missing. And do you need a get and put for each update? I assume you > could do a get at the beginning of the test. Thanks for catching it, I forgot to call igt_put_cairo_ctx() for frontbuffer and yes the context could be acquired when preparing for test. >=20 > > + > > + if (data->screen_changes & 1) { > > + /* go back to all green frame with with square > > */ > > + igt_paint_color_alpha(cr, 0, 0, SQUARE_SIZE, > > + SQUARE_SIZE, 1.0, 1.0, > > 1.0, 1.0); > > + } else { > > + /* go back to all green frame */ > > + igt_paint_color_alpha(cr, 0, 0, SQUARE_SIZE, > > + SQUARE_SIZE, 0, 1.0, 0, > > 1.0); > > + } > > + > > + r =3D drmModeDirtyFB(data->drm_fd, data->fb[0].fb_id, > > &clip, 1); > > + igt_assert(r =3D=3D 0 || r =3D=3D -ENOSYS); > > + break; > > + } > > + default: > > + igt_assert_f(data->op, "Operation not handled\n"); > > + } > > + > > + if (psr2_wait_su(data->debugfs_fd, &su_blocks)) > > + data->success =3D su_blocks =3D=3D EXPECTED_NUM_SU_BLOCKS; >=20 > Looks good overall, I haven't reviewed the details yet. Now that > there's only one thread, return bool and kill data->success? done >=20 > > +} > > + > > +static void run(data_t *data) > > +{ > > + while (data->screen_changes < MAX_SCREEN_CHANGES && !data- > > > success) { > > + uint64_t exp; > > + int r; > > + > > + r =3D poll(data->pollfds, > > + sizeof(data->pollfds) / sizeof(data- > > > pollfds[0]), -1); > > + if (r < 0) > > + break; > > + > > + if (data->pollfds[0].revents & POLLIN) { > > + r =3D read(data->pollfds[0].fd, &exp, > > sizeof(exp)); > > + > > + if (r !=3D sizeof(uint64_t)) { > > + igt_warn("read a not expected number of > > bytes from change_screen_timerfd: %i\n", r); > > + } else if (exp) > > + update_screen_and_test(data); > > + } > > + } > > + > > + igt_debug("Screen changes: %u\n", data->screen_changes); > > + igt_assert(data->success); > Consider using assert_f() to add some debug information when the test > fails. > > +} > > + > > +static void cleanup(data_t *data) > > +{ > > + igt_plane_t *primary; > > + > > + primary =3D igt_output_get_plane_type(data->output, > > + DRM_PLANE_TYPE_PRIMARY); > > + igt_plane_set_fb(primary, NULL); > > + igt_display_commit(&data->display); > > + > > + igt_remove_fb(data->drm_fd, &data->fb[0]); > > + if (data->op =3D=3D PAGE_FLIP) > > + igt_remove_fb(data->drm_fd, &data->fb[1]); > > +} > > + > > +int main(int argc, char *argv[]) > > +{ > > + data_t data =3D {}; > > + > > + igt_subtest_init_parse_opts(&argc, argv, "", NULL, > > + NULL, NULL, NULL); > > + igt_skip_on_simulation(); > > + > > + igt_fixture { > > + struct itimerspec interval; > > + int r; > > + > > + data.drm_fd =3D drm_open_driver_master(DRIVER_INTEL); > > + data.debugfs_fd =3D igt_debugfs_dir(data.drm_fd); > > + kmstest_set_vt_graphics_mode(); > > + > > + igt_require_f(psr_sink_support(data.debugfs_fd, > > PSR_MODE_2), > > + "Sink does not support PSR2\n"); > > + > > + data.bufmgr =3D drm_intel_bufmgr_gem_init(data.drm_fd, > > 4096); > > + igt_assert(data.bufmgr); > > + drm_intel_bufmgr_gem_enable_reuse(data.bufmgr); > > + > > + display_init(&data); > > + > > + igt_require(psr_enable(data.debugfs_fd, PSR_MODE_2)); > > + igt_require(psr_wait_entry(data.debugfs_fd, > > PSR_MODE_2)); > > + > > + data.change_screen_timerfd =3D > > timerfd_create(CLOCK_MONOTONIC, > > + TFD_NONBLOC > > K); > Does this need to be non-blocking, can't we allow read() to block > until > the timer expires and then do a screen update? Huum, this way we can remove the poll(). >=20 > > + igt_require(data.change_screen_timerfd !=3D -1); > > + /* Changing screen at 30hz to support 30hz panels */ > > + interval.it_value.tv_nsec =3D NSEC_PER_SEC / 30; > > + interval.it_value.tv_sec =3D 0; > > + interval.it_interval.tv_nsec =3D > > interval.it_value.tv_nsec; > > + interval.it_interval.tv_sec =3D interval.it_value.tv_sec; > > + r =3D timerfd_settime(data.change_screen_timerfd, 0, > > &interval, NULL); > > + igt_require_f(r !=3D -1, "Error setting timerfd\n"); > > + > > + data.pollfds[0].fd =3D data.change_screen_timerfd; > > + data.pollfds[0].events =3D POLLIN; > > + data.pollfds[0].revents =3D 0; > > + } > > + > > + for (data.op =3D PAGE_FLIP; data.op < LAST; data.op++) { > > + igt_subtest_f("%s", op_str(data.op)) { > > + prepare(&data); > > + run(&data); > > + cleanup(&data); > > + } > > + } > > + > > + igt_fixture { > > + close(data.debugfs_fd); > > + drm_intel_bufmgr_destroy(data.bufmgr); > > + display_fini(&data); > > + } > > + > > + igt_exit(); > > +} > > diff --git a/tests/meson.build b/tests/meson.build > > index e14ab2b4..682ca939 100644 > > --- a/tests/meson.build > > +++ b/tests/meson.build > > @@ -50,6 +50,7 @@ test_progs =3D [ > > 'kms_plane_scaling', > > 'kms_properties', > > 'kms_psr', > > + 'kms_psr2_su', > Please send a hack patch to include this in BAT so that we can see > the > results. Okay >=20 > > 'kms_pwrite_crc', > > 'kms_rmfb', > > 'kms_rotation_crc', --=-I3w0o4ISh4WbOH6kOXWy Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEVNG051EijGa0MiaQVenbO/mOWkkFAlxI7Z8ACgkQVenbO/mO WkmaRQgAosl2oWlnG2/3d8MXJiJ2uT43zVklyFjGPGymr9+l5IWjeLVs1/t/Oki9 MK1SSunY/iBaslIMJt8p74XzTII/WUKrL09Q+1rOletCfzLW1IiqvVUkc1KFCQq2 oI7x5VQ8uL1OqF5405F/ljoX3VTINSoO8y3TsjyPbKiMtkLMzGf/svrmjt9Z7TRw 0jv1T8+FK840d++9rOiN55OWMCCLWn16GYjJHCHXmPBfjvtvsxv6JC2A4MgAF/tf criondUB+YYtobIt1EqY4mNOvUVC1Cmw6GoHyKJdgwhaiU8YBKNxvsVT0R9eBhV0 E7GLC/kAW0YOpXKmmFKdVq59VQylaw== =nauO -----END PGP SIGNATURE----- --=-I3w0o4ISh4WbOH6kOXWy-- --===============1802565446== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KaWd0LWRldiBt YWlsaW5nIGxpc3QKaWd0LWRldkBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pZ3QtZGV2Cg== --===============1802565446==--