From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4BBEF6E230 for ; Fri, 11 Jan 2019 20:56:32 +0000 (UTC) From: "Souza, Jose" Date: Fri, 11 Jan 2019 20:56:30 +0000 Message-ID: <0cece6f17031cd7754d5da9b6e74ae20fbdfbb7c.camel@intel.com> References: <20190103143641.20200-1-jose.souza@intel.com> <20190103143641.20200-7-jose.souza@intel.com> <1b8b803eb063c3754a086f0796d19d696363f7f6.camel@intel.com> <84e2114d118e294af5801df0882a3b7e762f63d5.camel@intel.com> <7ca39c5a45ce39b5b2bd81bb3f36d70ba1979d58.camel@intel.com> In-Reply-To: <7ca39c5a45ce39b5b2bd81bb3f36d70ba1979d58.camel@intel.com> Content-Language: en-US MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH i-g-t v2 7/9] tests/psr: Add the same test coverage that we have for PSR1 to PSR2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============0156962030==" Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: "igt-dev@lists.freedesktop.org" , "Pandiyan, Dhinakaran" Cc: "Vivi, Rodrigo" List-ID: --===============0156962030== Content-Language: en-US Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-b5ReSqgOOARJwj5b++2B" --=-b5ReSqgOOARJwj5b++2B Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2019-01-11 at 11:49 -0800, Dhinakaran Pandiyan wrote: > On Thu, 2019-01-10 at 17:12 -0800, Souza, Jose wrote: > > On Thu, 2019-01-10 at 15:52 -0800, Dhinakaran Pandiyan wrote: > > > On Thu, 2019-01-03 at 06:36 -0800, Jos=C3=A9 Roberto de Souza wrote: > > > > The main tests for PSR1 check if hardware tracking is detecting > > > > changes in planes when modifing it in different ways and now > > > > those tests will also run for PSR2 if supported by source and > > > > sink. > > > >=20 > > > > Cc: Rodrigo Vivi > > > > Cc: Dhinakaran Pandiyan > > > > Signed-off-by: Jos=C3=A9 Roberto de Souza > > > > --- > > > > tests/kms_psr.c | 112 > > > > +++++++++++++++++++++++++++++++++++++++++++++- > > > > -- > > > > 1 file changed, 105 insertions(+), 7 deletions(-) > > > >=20 > > > > diff --git a/tests/kms_psr.c b/tests/kms_psr.c > > > > index 855679b0..d4d38320 100644 > > > > --- a/tests/kms_psr.c > > > > +++ b/tests/kms_psr.c > > > > @@ -60,6 +60,7 @@ typedef struct { > > > > int drm_fd; > > > > int debugfs_fd; > > > > enum operations op; > > > > + bool op_psr2; > > > > uint32_t devid; > > > > uint32_t crtc_id; > > > > igt_display_t display; > > > > @@ -71,6 +72,7 @@ typedef struct { > > > > drmModeModeInfo *mode; > > > > igt_output_t *output; > > > > bool with_psr_disabled; > > > > + bool supports_psr2; > > > > } data_t; > > > > =20 > > > > static void create_cursor_fb(data_t *data) > > > > @@ -194,12 +196,22 @@ static bool sink_support(data_t *data) > > > > return data->with_psr_disabled || > > > > psr_sink_support(data- > > > > > debugfs_fd); > > > >=20 > > > > } > > > > =20 > > > > -static bool psr_wait_entry_if_enabled(data_t *data) > > > > +static bool sink_support_psr2(data_t *data) > > > > { > > > > - if (data->with_psr_disabled) > > > > - return true; > > > > + return data->with_psr_disabled || > > > > psr2_sink_support(data- > > > > > debugfs_fd); > > > >=20 > > > > +} > > > > =20 > > > > - return psr_wait_entry(data->debugfs_fd); > > > > +static bool psr_wait_entry_if_enabled(data_t *data) > > > > +{ > > > > + if (!data->op_psr2) { > > > > + if (data->with_psr_disabled) > > > > + return true; > > > > + return psr_wait_entry(data->debugfs_fd); > > > > + } else { > > > > + if (data->with_psr_disabled) > > > > + return true; > > > > + return psr2_wait_deep_sleep(data->debugfs_fd); > > > > + } > > > > } > > >=20 > > > Unless I am missing something, we can simplify all these changes > > > by > > > pushing the PSR mode down through the call chain. > > >=20 > > > =20 > > > -static bool psr_active(int debugfs_fd, bool check_active) > > > +static bool psr_active(int debugfs_fd, enum psr_mode mode, bool > > > check_active) > > > { > >=20 > > I added the PSR2 functions with other names to avoid any > > misinterpretation as PSR2 could be active in deep sleep state and > > doing > > selective update and in the states between and in the kms_psr2_su > > we > > need to check both states to properly test. >=20 > Selective update testing does not need modifying the behavior of any > of > these functions, does it? Both PSR1 and PSR2(non-SU) tests wait until > PSR is in an active state, do some operation and then verify we are > out > of the state. Yes but after perform some operation it can keep into a active state in PSR2 that is why psr_wait_exit() is not a good name. Same for psr_wait_entry() it is wait entry in deep sleep? or wait entry in PSR active? > Since they follow the same test steps, I believe it is > better to share the interface. We also want kms_fbt_fbcon and > kms_frontbuffer_tracking to setup PSR2, that is another reason > psr_wait_entry()/psr_wait_exit() in the library should handle PSR1 > v/s > PSR2 instead of the callers in kms_psr. >=20 >=20 > > =20 > > I'm okay in adding the PSR mode to enable(), disable(), > > sink_support() > > and reuse the internal functions too but in the other functions the > > code saved is not worth. What do you think DK? > >=20 > > > bool active; > > > char buf[PSR_STATUS_MAX_LEN]; > > > + char *s =3D mode =3D=3D PSR_MODE_2 ? "DEEP_SLEEP" ? "SRDENT"; > > > =20 > > > igt_debugfs_simple_read(debugfs_fd, > > > "i915_edp_psr_status", > > > buf, > > > sizeof(buf)); > > > active =3D strstr(buf, "HW Enabled & Active bit: yes\n") && > > > - (strstr(buf, "SRDENT") || strstr(buf, "SLEEP")); > > > + strstr(buf, s); > > > return check_active ? active : !active; > > > } > > > =20 > > > -bool psr_wait_entry(int debugfs_fd) > > > +bool psr_wait_entry(int debugfs_fd, int psr_mode) > > > { > > > - return igt_wait(psr_active(debugfs_fd, true), 500, 20); > > > + return igt_wait(psr_active(debugfs_fd, true, psr_mode), > > > 500, > > > 20); > > > } > > > =20 > > > -bool psr_wait_exit(int debugfs_fd) > > > +bool psr_wait_exit(int debugfs_fd, int psr_mode) > > > { > > > return igt_wait(psr_active(debugfs_fd, false), 40, 10); > > > } > > > diff --git a/tests/kms_psr.c b/tests/kms_psr.c > > > index 855679b0..bb90b994 100644 > > > --- a/tests/kms_psr.c > > > +++ b/tests/kms_psr.c > > > @@ -199,7 +199,7 @@ static bool psr_wait_entry_if_enabled(data_t > > > *data) > > > if (data->with_psr_disabled) > > > return true; > > > =20 > > > - return psr_wait_entry(data->debugfs_fd); > > > + return psr_wait_entry(data->debugfs_fd, data->psr_mode); > > > } > > > =20 > > > static inline void manual(const char *expected) > > > @@ -289,7 +289,7 @@ static void run_test(data_t *data) > > > expected =3D "screen GREEN"; > > > break; > > > } > > > - igt_assert(psr_wait_exit(data->debugfs_fd)); > > > + igt_assert(psr_wait_exit(data->debugfs_fd, data- > > > > psr_mode)); > > > manual(expected); > > > } > > >=20 > > >=20 > > >=20 > > > > =20 > > > > static inline void manual(const char *expected) > > > > @@ -289,11 +301,16 @@ static void run_test(data_t *data) > > > > expected =3D "screen GREEN"; > > > > break; > > > > } > > > > - igt_assert(psr_wait_exit(data->debugfs_fd)); > > > > + > > > > + if (!data->op_psr2) > > > > + igt_assert(psr_wait_exit(data->debugfs_fd)); > > > > + else > > > > + igt_assert(psr2_wait_update(data->debugfs_fd)); > > > > manual(expected); > > > > } > > > > =20 > > > > -static void test_cleanup(data_t *data) { > > > > +static void test_cleanup(data_t *data) > > > > +{ > > > > igt_plane_t *primary; > > > > =20 > > > > primary =3D igt_output_get_plane_type(data->output, > > > > @@ -304,6 +321,11 @@ static void test_cleanup(data_t *data) { > > > > =20 > > > > igt_remove_fb(data->drm_fd, &data->fb_green); > > > > igt_remove_fb(data->drm_fd, &data->fb_white); > > > > + > > > > + /* switch to PSR1 again */ > > > > + if (data->op_psr2 && !data->with_psr_disabled) > > > > + psr_enable(data->debugfs_fd); > > > > + data->op_psr2 =3D false; > > > > } > > > > =20 > > > > static void setup_test_plane(data_t *data, int test_plane) > > > > @@ -311,6 +333,11 @@ static void setup_test_plane(data_t *data, > > > > int > > > > test_plane) > > > > uint32_t white_h, white_v; > > > > igt_plane_t *primary, *sprite, *cursor; > > > > =20 > > > > + if (data->op_psr2 && !data->with_psr_disabled) { > > > > + igt_require(data->supports_psr2); > > > > + psr2_enable(data->debugfs_fd); > > > > + } > > > > + > > > > igt_create_color_fb(data->drm_fd, > > > > data->mode->hdisplay, data->mode- > > > > >vdisplay, > > > > DRM_FORMAT_XRGB8888, > > > > @@ -391,7 +418,7 @@ static int opt_handler(int opt, int > > > > opt_index, > > > > void *_data) > > > > int main(int argc, char *argv[]) > > > > { > > > > const char *help_str =3D > > > > - " --no-psr\tRun test without PSR."; > > > > + " --no-psr\tRun test without PSR/PSR2.\n"; > > > > static struct option long_options[] =3D { > > > > {"no-psr", 0, 0, 'n'}, > > > > { 0, 0, 0, 0 } > > > > @@ -414,6 +441,7 @@ int main(int argc, char *argv[]) > > > > =20 > > > > igt_require_f(sink_support(&data), > > > > "Sink does not support PSR\n"); > > > > + data.supports_psr2 =3D sink_support_psr2(&data); > > > > =20 > > > > data.bufmgr =3D > > > > drm_intel_bufmgr_gem_init(data.drm_fd, > > > > 4096); > > > > igt_assert(data.bufmgr); > > > > @@ -428,6 +456,13 @@ int main(int argc, char *argv[]) > > > > test_cleanup(&data); > > > > } > > > > =20 > > > > + igt_subtest("psr2_basic") { > > > > + data.op_psr2 =3D true; > > > > + setup_test_plane(&data, > > > > DRM_PLANE_TYPE_PRIMARY); > > > > + igt_assert(psr_wait_entry_if_enabled(&data)); > > > > + test_cleanup(&data); > > > > + } > > > > + > > > > igt_subtest("no_drrs") { > > > > setup_test_plane(&data, > > > > DRM_PLANE_TYPE_PRIMARY); > > > > igt_assert(psr_wait_entry_if_enabled(&data)); > > > > @@ -435,6 +470,14 @@ int main(int argc, char *argv[]) > > > > test_cleanup(&data); > > > > } > > > > =20 > > > > + igt_subtest("psr2_no_drrs") { > > > > + data.op_psr2 =3D true; > > > > + setup_test_plane(&data, > > > > DRM_PLANE_TYPE_PRIMARY); > > > > + igt_assert(psr_wait_entry_if_enabled(&data)); > > > > + igt_assert(drrs_disabled(&data)); > > > > + test_cleanup(&data); > > > > + } > > > > + > > > > for (op =3D PAGE_FLIP; op <=3D RENDER; op++) { > > > > igt_subtest_f("primary_%s", op_str(op)) { > > > > data.op =3D op; > > > > @@ -445,6 +488,17 @@ int main(int argc, char *argv[]) > > > > } > > > > } > > > > =20 > > > > + for (op =3D PAGE_FLIP; op <=3D RENDER; op++) { > > > > + igt_subtest_f("psr2_primary_%s", op_str(op)) { > > > > + data.op_psr2 =3D true; > > > > + data.op =3D op; > > > > + setup_test_plane(&data, > > > > DRM_PLANE_TYPE_PRIMARY); > > > > + igt_assert(psr_wait_entry_if_enabled(&d > > > > ata)); > > > > + run_test(&data); > > > > + test_cleanup(&data); > > > > + } > > > > + } > > > > + > > > > for (op =3D MMAP_GTT; op <=3D PLANE_ONOFF; op++) { > > > > igt_subtest_f("sprite_%s", op_str(op)) { > > > > data.op =3D op; > > > > @@ -455,6 +509,17 @@ int main(int argc, char *argv[]) > > > > } > > > > } > > > > =20 > > > > + for (op =3D MMAP_GTT; op <=3D PLANE_ONOFF; op++) { > > > > + igt_subtest_f("psr2_sprite_%s", op_str(op)) { > > > > + data.op_psr2 =3D true; > > > > + data.op =3D op; > > > > + setup_test_plane(&data, > > > > DRM_PLANE_TYPE_OVERLAY); > > > > + igt_assert(psr_wait_entry_if_enabled(&d > > > > ata)); > > > > + run_test(&data); > > > > + test_cleanup(&data); > > > > + } > > > > + } > > > > + > > > > for (op =3D MMAP_GTT; op <=3D PLANE_ONOFF; op++) { > > > > igt_subtest_f("cursor_%s", op_str(op)) { > > > > data.op =3D op; > > > > @@ -465,6 +530,17 @@ int main(int argc, char *argv[]) > > > > } > > > > } > > > > =20 > > > > + for (op =3D MMAP_GTT; op <=3D PLANE_ONOFF; op++) { > > > > + igt_subtest_f("psr2_cursor_%s", op_str(op)) { > > > > + data.op_psr2 =3D true; > > > > + data.op =3D op; > > > > + setup_test_plane(&data, > > > > DRM_PLANE_TYPE_CURSOR); > > > > + igt_assert(psr_wait_entry_if_enabled(&d > > > > ata)); > > > > + run_test(&data); > > > > + test_cleanup(&data); > > > > + } > > > > + } > > > > + > > > > igt_subtest_f("dpms") { > > > > data.op =3D RENDER; > > > > setup_test_plane(&data, > > > > DRM_PLANE_TYPE_PRIMARY); > > > > @@ -474,6 +550,16 @@ int main(int argc, char *argv[]) > > > > test_cleanup(&data); > > > > } > > > > =20 > > > > + igt_subtest_f("psr2_dpms") { > > > > + data.op_psr2 =3D true; > > > > + data.op =3D RENDER; > > > > + setup_test_plane(&data, > > > > DRM_PLANE_TYPE_PRIMARY); > > > > + igt_assert(psr_wait_entry_if_enabled(&data)); > > > > + dpms_off_on(&data); > > > > + run_test(&data); > > > > + test_cleanup(&data); > > > > + } > > > > + > > > > igt_subtest_f("suspend") { > > > > data.op =3D PLANE_ONOFF; > > > > setup_test_plane(&data, DRM_PLANE_TYPE_CURSOR); > > > > @@ -485,6 +571,18 @@ int main(int argc, char *argv[]) > > > > test_cleanup(&data); > > > > } > > > > =20 > > > > + igt_subtest_f("psr2_suspend") { > > > > + data.op_psr2 =3D true; > > > > + data.op =3D PLANE_ONOFF; > > > > + setup_test_plane(&data, DRM_PLANE_TYPE_CURSOR); > > > > + igt_assert(psr_wait_entry_if_enabled(&data)); > > > > + igt_system_suspend_autoresume(SUSPEND_STATE_MEM > > > > , > > > > + SUSPEND_TEST_NONE > > > > ); > > > > + igt_assert(psr_wait_entry_if_enabled(&data)); > > > > + run_test(&data); > > > > + test_cleanup(&data); > > > > + } > > > > + > > > > igt_fixture { > > > > if (!data.with_psr_disabled) > > > > psr_disable(data.debugfs_fd); --=-b5ReSqgOOARJwj5b++2B 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/mOWkkFAlw5AvwACgkQVenbO/mO WkndGggAoRPmLccmz2QSols7QLjwpDbaDAFaNzqIPsv1KmErI7IrKR5WhoSEMpg4 f2/4TP/052HolATRvxgU/lKp5EW3wSj2PCP1ja9O97ucYCO6e44Iv1AOyLCXGWfL owlEemOmmMg7+kmOGSvsUJ4TN7Qm/KWlFkDpsizPNr6k+MkIWcV+UVCtPiv0qO9d qMyr6ogUdHQjSY35x8aVZW+sGb5DsqPGgOeyPo8zKCXxArm9vnRnjXl/h8JamLTq P+bgnOxgATSW15dg2gYDqagaLJupLTdvqq/frfTI1xBdyv0hUeDJnGNWzHA9mQdU IyJnPIsbaHWkAgvGomflq4spZAw0bA== =jlBb -----END PGP SIGNATURE----- --=-b5ReSqgOOARJwj5b++2B-- --===============0156962030== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KaWd0LWRldiBt YWlsaW5nIGxpc3QKaWd0LWRldkBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pZ3QtZGV2Cg== --===============0156962030==--