* [igt-dev] [PATCH i-g-t 1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3.
@ 2018-08-10 10:06 Maarten Lankhorst
2018-08-10 10:06 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_frontbuffer_tracking: Remove redundant modesets during subtest start, v3 Maarten Lankhorst
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Maarten Lankhorst @ 2018-08-10 10:06 UTC (permalink / raw)
To: igt-dev
It's harmful to write to enable_psr at runtime, and the patch that allows
us to change i915_edp_psr_debug with the panel running will require us
to abandon the module parameter. Hence the userspace change needs to be
put in IGT first before we can change it at kernel time.
Toggling it to debugfs will mean we can skip a modeset when changing our
feature set.
Changes since v1:
- Rebase with the previous patches dropped.
Changes since v2:
- Rebase on top of new api in i915_edp_psr_debug.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
tests/kms_frontbuffer_tracking.c | 44 ++++++++++++++++++++++++++++++--
1 file changed, 42 insertions(+), 2 deletions(-)
diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 1dfd7c1cee8d..06ad55709dc6 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -775,6 +775,46 @@ static void drrs_set(unsigned int val)
igt_assert_f(ret == (sizeof(buf) - 1), "debugfs_write failed");
}
+static void restore_psr_debugfs(int sig)
+{
+ debugfs_write("i915_edp_psr_debug", "0");
+}
+
+static bool psr_modparam_set(unsigned int val)
+{
+ static int oldval = -1;
+
+ igt_set_module_param_int("enable_psr", val);
+
+ if (val == oldval)
+ return false;
+
+ oldval = val;
+ return true;
+}
+
+static bool psr_set(bool enable)
+{
+ int ret;
+
+ /* Check if new PSR debugfs api is usable. */
+ ret = debugfs_write("i915_edp_psr_debug", "0xf");
+ if (ret == -ENODEV) {
+ /* PSR not enabled, should only be able to set or unset. */
+ igt_assert(!enable);
+ return false;
+ }
+
+ if (ret != -EINVAL)
+ return psr_modparam_set(enable);
+
+ ret = debugfs_write("i915_edp_psr_debug", enable ? "0x2" : "0x1");
+ igt_assert_lt(0, ret);
+
+ igt_install_exit_handler(restore_psr_debugfs);
+ return false;
+}
+
static bool is_drrs_high(void)
{
char buf[MAX_DRRS_STATUS_BUF_LEN];
@@ -941,8 +981,8 @@ static bool drrs_wait_until_rr_switch_to_low(void)
#define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
#define fbc_disable() igt_set_module_param_int("enable_fbc", 0)
-#define psr_enable() igt_set_module_param_int("enable_psr", 1)
-#define psr_disable() igt_set_module_param_int("enable_psr", 0)
+#define psr_enable() psr_set(1)
+#define psr_disable() psr_set(0)
#define drrs_enable() drrs_set(1)
#define drrs_disable() drrs_set(0)
--
2.18.0
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [igt-dev] [PATCH i-g-t 2/2] tests/kms_frontbuffer_tracking: Remove redundant modesets during subtest start, v3.
2018-08-10 10:06 [igt-dev] [PATCH i-g-t 1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3 Maarten Lankhorst
@ 2018-08-10 10:06 ` Maarten Lankhorst
2018-08-10 13:14 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3 Patchwork
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Maarten Lankhorst @ 2018-08-10 10:06 UTC (permalink / raw)
To: igt-dev
CRC capturing enables the display, then disables it again. With
igt_display we can use igt_display_reset to restore the original state,
without committing it to the hw.
All subtests first set their own state anyway, so we can save up on
the number of commits.
Changes since v1:
- Try to avoid modesets for PSR if the kernel supports it, but otherwise force
a modeset for the changes to take effect.
Changes since v2:
- Rebase on top of previous PSR changes.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
tests/kms_frontbuffer_tracking.c | 57 ++++++++++++++++++++++++--------
1 file changed, 44 insertions(+), 13 deletions(-)
diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 06ad55709dc6..35e774532813 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -1171,14 +1171,14 @@ static void unset_all_crtcs(void)
igt_display_commit(&drm.display);
}
-static void disable_features(const struct test_mode *t)
+static bool disable_features(const struct test_mode *t)
{
if (t->feature == FEATURE_DEFAULT)
- return;
+ return false;
fbc_disable();
- psr_disable();
drrs_disable();
+ return psr_disable();
}
static void *busy_thread_func(void *data)
@@ -1262,7 +1262,7 @@ static void init_blue_crc(enum pixel_format format)
print_crc("Blue CRC: ", &blue_crcs[format].crc);
- unset_all_crtcs();
+ igt_display_reset(&drm.display);
igt_remove_fb(drm.fd, &blue);
@@ -1314,7 +1314,7 @@ static void init_crcs(enum pixel_format format,
print_crc("", &pattern->crcs[format][r]);
}
- unset_all_crtcs();
+ igt_display_reset(&drm.display);
for (r = 0; r < pattern->n_rects; r++)
igt_remove_fb(drm.fd, &tmp_fbs[r]);
@@ -1737,6 +1737,22 @@ static void enable_scnd_screen_and_wait(const struct test_mode *t)
do_assertions(ASSERT_NO_ACTION_CHANGE);
}
+static void enable_both_screens_and_wait(const struct test_mode *t)
+{
+ fill_fb_region(&prim_mode_params.primary, COLOR_PRIM_BG);
+ fill_fb_region(&scnd_mode_params.primary, COLOR_SCND_BG);
+
+ __set_mode_for_params(&prim_mode_params);
+ __set_mode_for_params(&scnd_mode_params);
+
+ igt_display_commit2(&drm.display, drm.display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
+
+ wanted_crc = &blue_crcs[t->format].crc;
+ fbc_update_last_action();
+
+ do_assertions(ASSERT_NO_ACTION_CHANGE);
+}
+
static void set_region_for_test(const struct test_mode *t,
struct fb_region *reg)
{
@@ -1751,17 +1767,21 @@ static void set_region_for_test(const struct test_mode *t,
do_assertions(ASSERT_NO_ACTION_CHANGE);
}
-static void enable_features_for_test(const struct test_mode *t)
+static bool enable_features_for_test(const struct test_mode *t)
{
+ bool ret = false;
+
if (t->feature == FEATURE_DEFAULT)
- return;
+ return false;
if (t->feature & FEATURE_FBC)
fbc_enable();
if (t->feature & FEATURE_PSR)
- psr_enable();
+ ret = psr_enable();
if (t->feature & FEATURE_DRRS)
drrs_enable();
+
+ return ret;
}
static void check_test_requirements(const struct test_mode *t)
@@ -1843,28 +1863,40 @@ static void set_crtc_fbs(const struct test_mode *t)
static void prepare_subtest_data(const struct test_mode *t,
struct draw_pattern_info *pattern)
{
+ bool need_modeset;
+
check_test_requirements(t);
stop_busy_thread();
- disable_features(t);
+ need_modeset = disable_features(t);
set_crtc_fbs(t);
if (t->screen == SCREEN_OFFSCREEN)
fill_fb_region(&offscreen_fb, COLOR_OFFSCREEN_BG);
- unset_all_crtcs();
+ igt_display_reset(&drm.display);
+ if (need_modeset)
+ igt_display_commit(&drm.display);
init_blue_crc(t->format);
if (pattern)
init_crcs(t->format, pattern);
- enable_features_for_test(t);
+ igt_display_reset(&drm.display);
+
+ need_modeset = enable_features_for_test(t);
+ if (need_modeset)
+ igt_display_commit(&drm.display);
}
static void prepare_subtest_screens(const struct test_mode *t)
{
- enable_prim_screen_and_wait(t);
+ if (t->pipes == PIPE_DUAL)
+ enable_both_screens_and_wait(t);
+ else
+ enable_prim_screen_and_wait(t);
+
if (t->screen == SCREEN_PRIM) {
if (t->plane == PLANE_CUR)
set_region_for_test(t, &prim_mode_params.cursor);
@@ -1875,7 +1907,6 @@ static void prepare_subtest_screens(const struct test_mode *t)
if (t->pipes == PIPE_SINGLE)
return;
- enable_scnd_screen_and_wait(t);
if (t->screen == SCREEN_SCND) {
if (t->plane == PLANE_CUR)
set_region_for_test(t, &scnd_mode_params.cursor);
--
2.18.0
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3.
2018-08-10 10:06 [igt-dev] [PATCH i-g-t 1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3 Maarten Lankhorst
2018-08-10 10:06 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_frontbuffer_tracking: Remove redundant modesets during subtest start, v3 Maarten Lankhorst
@ 2018-08-10 13:14 ` Patchwork
2018-08-10 17:03 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
2018-08-11 1:50 ` [igt-dev] [PATCH i-g-t 1/2] " Dhinakaran Pandiyan
3 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-08-10 13:14 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: igt-dev
== Series Details ==
Series: series starting with [i-g-t,1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3.
URL : https://patchwork.freedesktop.org/series/47995/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4642 -> IGTPW_1702 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/47995/revisions/1/mbox/
== Known issues ==
Here are the changes found in IGTPW_1702 that come from known issues:
=== IGT changes ===
==== Issues hit ====
{igt@kms_psr@sprite_plane_onoff}:
{fi-bdw-samus}: NOTRUN -> FAIL (fdo#107360)
igt@prime_vgem@basic-fence-flip:
fi-ilk-650: PASS -> FAIL (fdo#104008)
==== Possible fixes ====
igt@drv_module_reload@basic-reload:
fi-glk-j4005: DMESG-WARN (fdo#106248, fdo#106725) -> PASS
igt@drv_selftest@live_workarounds:
{fi-bsw-kefka}: DMESG-FAIL (fdo#107292) -> PASS
fi-bdw-5557u: DMESG-FAIL (fdo#107292) -> PASS
igt@kms_flip@basic-flip-vs-wf_vblank:
fi-glk-j4005: FAIL (fdo#100368) -> PASS
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
fdo#107360 https://bugs.freedesktop.org/show_bug.cgi?id=107360
== Participating hosts (53 -> 49) ==
Additional (1): fi-bdw-samus
Missing (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u
== Build changes ==
* IGT: IGT_4591 -> IGTPW_1702
CI_DRM_4642: 0de4e9a02a422ebe523f59c3b462c949673746b6 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_1702: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1702/
IGT_4591: 6cb3d7dbe5831a7b2b5b7a4638d8a8b7ac624f5f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1702/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 9+ messages in thread
* [igt-dev] ✓ Fi.CI.IGT: success for series starting with [i-g-t,1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3.
2018-08-10 10:06 [igt-dev] [PATCH i-g-t 1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3 Maarten Lankhorst
2018-08-10 10:06 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_frontbuffer_tracking: Remove redundant modesets during subtest start, v3 Maarten Lankhorst
2018-08-10 13:14 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3 Patchwork
@ 2018-08-10 17:03 ` Patchwork
2018-08-11 1:50 ` [igt-dev] [PATCH i-g-t 1/2] " Dhinakaran Pandiyan
3 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-08-10 17:03 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: igt-dev
== Series Details ==
Series: series starting with [i-g-t,1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3.
URL : https://patchwork.freedesktop.org/series/47995/
State : success
== Summary ==
= CI Bug Log - changes from IGT_4591_full -> IGTPW_1702_full =
== Summary - WARNING ==
Minor unknown changes coming with IGTPW_1702_full need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in IGTPW_1702_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://patchwork.freedesktop.org/api/1.0/series/47995/revisions/1/mbox/
== Possible new issues ==
Here are the unknown changes that may have been introduced in IGTPW_1702_full:
=== IGT changes ===
==== Warnings ====
igt@pm_rc6_residency@rc6-accuracy:
shard-kbl: PASS -> SKIP
== Known issues ==
Here are the changes found in IGTPW_1702_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@drv_suspend@shrink:
shard-glk: PASS -> INCOMPLETE (fdo#106886, k.org#198133, fdo#103359)
igt@gem_exec_schedule@pi-ringfull-render:
shard-glk: NOTRUN -> FAIL (fdo#103158)
igt@gem_ppgtt@blt-vs-render-ctx0:
shard-kbl: PASS -> INCOMPLETE (fdo#103665, fdo#106023)
igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
shard-glk: PASS -> FAIL (fdo#105363)
igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-mmap-wc:
shard-glk: PASS -> FAIL (fdo#103167) +1
igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
shard-kbl: PASS -> INCOMPLETE (fdo#103665)
igt@kms_rotation_crc@sprite-rotation-180:
shard-snb: PASS -> FAIL (fdo#103925)
igt@kms_setmode@basic:
shard-apl: PASS -> FAIL (fdo#99912)
shard-glk: NOTRUN -> FAIL (fdo#99912)
igt@kms_universal_plane@cursor-fb-leak-pipe-a:
shard-glk: PASS -> FAIL (fdo#107241)
igt@perf@polling:
shard-hsw: PASS -> FAIL (fdo#102252)
igt@perf_pmu@rc6-runtime-pm-long:
shard-apl: PASS -> DMESG-WARN (fdo#103558, fdo#105602) +27
==== Possible fixes ====
igt@gem_exec_store@basic-blt:
shard-snb: INCOMPLETE (fdo#105411) -> PASS
igt@kms_available_modes_crc@available_mode_test_crc:
shard-snb: FAIL (fdo#106641) -> PASS
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
shard-kbl: INCOMPLETE (fdo#103665) -> PASS
igt@kms_rotation_crc@primary-rotation-180:
shard-snb: FAIL (fdo#103925) -> PASS
igt@pm_rpm@i2c:
shard-apl: FAIL (fdo#106539) -> PASS
shard-glk: FAIL (fdo#106539) -> PASS
shard-hsw: FAIL (fdo#106539) -> PASS
shard-kbl: FAIL (fdo#106539) -> PASS
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
fdo#106539 https://bugs.freedesktop.org/show_bug.cgi?id=106539
fdo#106641 https://bugs.freedesktop.org/show_bug.cgi?id=106641
fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
fdo#107241 https://bugs.freedesktop.org/show_bug.cgi?id=107241
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133
== Participating hosts (5 -> 5) ==
No changes in participating hosts
== Build changes ==
* IGT: IGT_4591 -> IGTPW_1702
* Linux: CI_DRM_4636 -> CI_DRM_4642
CI_DRM_4636: 084bb2fb549650b6da80976c9bc594779ce342b4 @ git://anongit.freedesktop.org/gfx-ci/linux
CI_DRM_4642: 0de4e9a02a422ebe523f59c3b462c949673746b6 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_1702: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1702/
IGT_4591: 6cb3d7dbe5831a7b2b5b7a4638d8a8b7ac624f5f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1702/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3.
2018-08-10 10:06 [igt-dev] [PATCH i-g-t 1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3 Maarten Lankhorst
` (2 preceding siblings ...)
2018-08-10 17:03 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
@ 2018-08-11 1:50 ` Dhinakaran Pandiyan
2018-08-13 7:11 ` Maarten Lankhorst
3 siblings, 1 reply; 9+ messages in thread
From: Dhinakaran Pandiyan @ 2018-08-11 1:50 UTC (permalink / raw)
To: igt-dev; +Cc: dhinakaran.pandiyan
On Friday, August 10, 2018 3:06:45 AM PDT Maarten Lankhorst wrote:
> It's harmful to write to enable_psr at runtime, and the patch that allows
> us to change i915_edp_psr_debug with the panel running will require us
> to abandon the module parameter. Hence the userspace change needs to be
> put in IGT first before we can change it at kernel time.
>
> Toggling it to debugfs will mean we can skip a modeset when changing our
> feature set.
>
> Changes since v1:
> - Rebase with the previous patches dropped.
> Changes since v2:
> - Rebase on top of new api in i915_edp_psr_debug.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> tests/kms_frontbuffer_tracking.c | 44 ++++++++++++++++++++++++++++++--
> 1 file changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c index 1dfd7c1cee8d..06ad55709dc6 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -775,6 +775,46 @@ static void drrs_set(unsigned int val)
> igt_assert_f(ret == (sizeof(buf) - 1), "debugfs_write failed");
> }
>
> +static void restore_psr_debugfs(int sig)
> +{
> + debugfs_write("i915_edp_psr_debug", "0");
> +}
> +
> +static bool psr_modparam_set(unsigned int val)
> +{
> + static int oldval = -1;
> +
> + igt_set_module_param_int("enable_psr", val);
> +
> + if (val == oldval)
> + return false;
> +
> + oldval = val;
> + return true;
> +}
> +
> +static bool psr_set(bool enable)
> +{
> + int ret;
> +
> + /* Check if new PSR debugfs api is usable. */
> + ret = debugfs_write("i915_edp_psr_debug", "0xf");
> + if (ret == -ENODEV) {
> + /* PSR not enabled, should only be able to set or unset. */
s/enabled/supported
-ENODEV is returned only when source or sink do not support PSR.
> + igt_assert(!enable);
> + return false;
> + }
> +
> + if (ret != -EINVAL)
I would add a comment to explain that you are tying -EINVAL return to mean the
driver supports the new API. This needs a comment in the driver too?
> + return psr_modparam_set(enable);
Don't see this return value being used.
> +
> + ret = debugfs_write("i915_edp_psr_debug", enable ? "0x2" : "0x1");
We should be using DEBUG_FORCE_PSR1 here. The only reason we were testing PSR2
is because the module parameter did not have an option to force PSR1.
> + igt_assert_lt(0, ret);
Even if the return was -ERESTARTSYS? Isn't one of the benefits of passing the
return value from modeset_lock and wait_for_completion_interruptible that
userspace can retry? Or does -ERESTARTSYS never reach this point?
> +
> + igt_install_exit_handler(restore_psr_debugfs);
> + return false;
> +}
It is better to move all of this to lib/igt_psr so that kms_psr and
kms_fbt_fbcon also use the same code.
> +
> static bool is_drrs_high(void)
> {
> char buf[MAX_DRRS_STATUS_BUF_LEN];
> @@ -941,8 +981,8 @@ static bool drrs_wait_until_rr_switch_to_low(void)
>
> #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
> #define fbc_disable() igt_set_module_param_int("enable_fbc", 0)
> -#define psr_enable() igt_set_module_param_int("enable_psr", 1)
> -#define psr_disable() igt_set_module_param_int("enable_psr", 0)
> +#define psr_enable() psr_set(1)
> +#define psr_disable() psr_set(0)
> #define drrs_enable() drrs_set(1)
> #define drrs_disable() drrs_set(0)
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3.
2018-08-11 1:50 ` [igt-dev] [PATCH i-g-t 1/2] " Dhinakaran Pandiyan
@ 2018-08-13 7:11 ` Maarten Lankhorst
2018-08-13 18:34 ` Dhinakaran Pandiyan
0 siblings, 1 reply; 9+ messages in thread
From: Maarten Lankhorst @ 2018-08-13 7:11 UTC (permalink / raw)
To: Dhinakaran Pandiyan, igt-dev; +Cc: dhinakaran.pandiyan
Op 11-08-18 om 03:50 schreef Dhinakaran Pandiyan:
> On Friday, August 10, 2018 3:06:45 AM PDT Maarten Lankhorst wrote:
>> It's harmful to write to enable_psr at runtime, and the patch that allows
>> us to change i915_edp_psr_debug with the panel running will require us
>> to abandon the module parameter. Hence the userspace change needs to be
>> put in IGT first before we can change it at kernel time.
>>
>> Toggling it to debugfs will mean we can skip a modeset when changing our
>> feature set.
>>
>> Changes since v1:
>> - Rebase with the previous patches dropped.
>> Changes since v2:
>> - Rebase on top of new api in i915_edp_psr_debug.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> tests/kms_frontbuffer_tracking.c | 44 ++++++++++++++++++++++++++++++--
>> 1 file changed, 42 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/kms_frontbuffer_tracking.c
>> b/tests/kms_frontbuffer_tracking.c index 1dfd7c1cee8d..06ad55709dc6 100644
>> --- a/tests/kms_frontbuffer_tracking.c
>> +++ b/tests/kms_frontbuffer_tracking.c
>> @@ -775,6 +775,46 @@ static void drrs_set(unsigned int val)
>> igt_assert_f(ret == (sizeof(buf) - 1), "debugfs_write failed");
>> }
>>
>> +static void restore_psr_debugfs(int sig)
>> +{
>> + debugfs_write("i915_edp_psr_debug", "0");
>> +}
>> +
>> +static bool psr_modparam_set(unsigned int val)
>> +{
>> + static int oldval = -1;
>> +
>> + igt_set_module_param_int("enable_psr", val);
>> +
>> + if (val == oldval)
>> + return false;
>> +
>> + oldval = val;
>> + return true;
>> +}
>> +
>> +static bool psr_set(bool enable)
>> +{
>> + int ret;
>> +
>> + /* Check if new PSR debugfs api is usable. */
>> + ret = debugfs_write("i915_edp_psr_debug", "0xf");
>> + if (ret == -ENODEV) {
>> + /* PSR not enabled, should only be able to set or unset. */
> s/enabled/supported
> -ENODEV is returned only when source or sink do not support PSR.
Indeed, typo. :)
>> + igt_assert(!enable);
>> + return false;
>> + }
>> +
>> + if (ret != -EINVAL)
> I would add a comment to explain that you are tying -EINVAL return to mean the
> driver supports the new API. This needs a comment in the driver too?
>
>> + return psr_modparam_set(enable);
> Don't see this return value being used.
It's used in 2/2.
>> +
>> + ret = debugfs_write("i915_edp_psr_debug", enable ? "0x2" : "0x1");
> We should be using DEBUG_FORCE_PSR1 here. The only reason we were testing PSR2
> is because the module parameter did not have an option to force PSR1.
Ok, I was under the impression we want to test the newest PSR possible,
because that's what the HW will use when enable_psr=1 is passed as modparam?
>> + igt_assert_lt(0, ret);
> Even if the return was -ERESTARTSYS? Isn't one of the benefits of passing the
> return value from modeset_lock and wait_for_completion_interruptible that
> userspace can retry? Or does -ERESTARTSYS never reach this point?
igt_sysfs_write handles signals with the static writeN() function.
>> +
>> + igt_install_exit_handler(restore_psr_debugfs);
>> + return false;
>> +}
> It is better to move all of this to lib/igt_psr so that kms_psr and
> kms_fbt_fbcon also use the same code.
Actually, it might very well be a good idea. :)
>> +
>> static bool is_drrs_high(void)
>> {
>> char buf[MAX_DRRS_STATUS_BUF_LEN];
>> @@ -941,8 +981,8 @@ static bool drrs_wait_until_rr_switch_to_low(void)
>>
>> #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
>> #define fbc_disable() igt_set_module_param_int("enable_fbc", 0)
>> -#define psr_enable() igt_set_module_param_int("enable_psr", 1)
>> -#define psr_disable() igt_set_module_param_int("enable_psr", 0)
>> +#define psr_enable() psr_set(1)
>> +#define psr_disable() psr_set(0)
>> #define drrs_enable() drrs_set(1)
>> #define drrs_disable() drrs_set(0)
>
>
>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3.
2018-08-13 7:11 ` Maarten Lankhorst
@ 2018-08-13 18:34 ` Dhinakaran Pandiyan
2018-08-14 11:08 ` Maarten Lankhorst
0 siblings, 1 reply; 9+ messages in thread
From: Dhinakaran Pandiyan @ 2018-08-13 18:34 UTC (permalink / raw)
To: Maarten Lankhorst, igt-dev
On Mon, 2018-08-13 at 09:11 +0200, Maarten Lankhorst wrote:
> Op 11-08-18 om 03:50 schreef Dhinakaran Pandiyan:
> > On Friday, August 10, 2018 3:06:45 AM PDT Maarten Lankhorst wrote:
> > > It's harmful to write to enable_psr at runtime, and the patch
> > > that allows
> > > us to change i915_edp_psr_debug with the panel running will
> > > require us
> > > to abandon the module parameter. Hence the userspace change needs
> > > to be
> > > put in IGT first before we can change it at kernel time.
> > >
> > > Toggling it to debugfs will mean we can skip a modeset when
> > > changing our
> > > feature set.
> > >
> > > Changes since v1:
> > > - Rebase with the previous patches dropped.
> > > Changes since v2:
> > > - Rebase on top of new api in i915_edp_psr_debug.
> > >
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
> > > om>
> > > ---
> > > tests/kms_frontbuffer_tracking.c | 44
> > > ++++++++++++++++++++++++++++++--
> > > 1 file changed, 42 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tests/kms_frontbuffer_tracking.c
> > > b/tests/kms_frontbuffer_tracking.c index
> > > 1dfd7c1cee8d..06ad55709dc6 100644
> > > --- a/tests/kms_frontbuffer_tracking.c
> > > +++ b/tests/kms_frontbuffer_tracking.c
> > > @@ -775,6 +775,46 @@ static void drrs_set(unsigned int val)
> > > igt_assert_f(ret == (sizeof(buf) - 1),
> > > "debugfs_write failed");
> > > }
> > >
> > > +static void restore_psr_debugfs(int sig)
> > > +{
> > > + debugfs_write("i915_edp_psr_debug", "0");
> > > +}
> > > +
> > > +static bool psr_modparam_set(unsigned int val)
> > > +{
> > > + static int oldval = -1;
> > > +
> > > + igt_set_module_param_int("enable_psr", val);
> > > +
> > > + if (val == oldval)
> > > + return false;
> > > +
> > > + oldval = val;
> > > + return true;
> > > +}
> > > +
> > > +static bool psr_set(bool enable)
> > > +{
> > > + int ret;
> > > +
> > > + /* Check if new PSR debugfs api is usable. */
> > > + ret = debugfs_write("i915_edp_psr_debug", "0xf");
> > > + if (ret == -ENODEV) {
> > > + /* PSR not enabled, should only be able to set
> > > or unset. */
> >
> > s/enabled/supported
> > -ENODEV is returned only when source or sink do not support PSR.
>
> Indeed, typo. :)
> > > + igt_assert(!enable);
> > > + return false;
> > > + }
> > > +
> > > + if (ret != -EINVAL)
> >
> > I would add a comment to explain that you are tying -EINVAL return
> > to mean the
> > driver supports the new API. This needs a comment in the driver
> > too?
> >
> > > + return psr_modparam_set(enable);
> >
> > Don't see this return value being used.
>
> It's used in 2/2.
> > > +
> > > + ret = debugfs_write("i915_edp_psr_debug", enable ? "0x2"
> > > : "0x1");
> >
> > We should be using DEBUG_FORCE_PSR1 here. The only reason we were
> > testing PSR2
> > is because the module parameter did not have an option to force
> > PSR1.
>
> Ok, I was under the impression we want to test the newest PSR
> possible,
No, PSR2 isn't ready for testing yet. We should be testing only PSR1
now.
> because that's what the HW will use when enable_psr=1 is passed as
> modparam?
> > > + igt_assert_lt(0, ret);
> >
> > Even if the return was -ERESTARTSYS? Isn't one of the benefits of
> > passing the
> > return value from modeset_lock and
> > wait_for_completion_interruptible that
> > userspace can retry? Or does -ERESTARTSYS never reach this point?
>
> igt_sysfs_write handles signals with the static writeN() function.
> > > +
> > > + igt_install_exit_handler(restore_psr_debugfs);
> > > + return false;
> > > +}
> >
> > It is better to move all of this to lib/igt_psr so that kms_psr
> > and
> > kms_fbt_fbcon also use the same code.
>
> Actually, it might very well be a good idea. :)
> > > +
> > > static bool is_drrs_high(void)
> > > {
> > > char buf[MAX_DRRS_STATUS_BUF_LEN];
> > > @@ -941,8 +981,8 @@ static bool
> > > drrs_wait_until_rr_switch_to_low(void)
> > >
> > > #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
> > > #define fbc_disable() igt_set_module_param_int("enable_fbc", 0)
> > > -#define psr_enable() igt_set_module_param_int("enable_psr", 1)
> > > -#define psr_disable() igt_set_module_param_int("enable_psr", 0)
> > > +#define psr_enable() psr_set(1)
> > > +#define psr_disable() psr_set(0)
> > > #define drrs_enable() drrs_set(1)
> > > #define drrs_disable() drrs_set(0)
> >
> >
> >
>
>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3.
2018-08-13 18:34 ` Dhinakaran Pandiyan
@ 2018-08-14 11:08 ` Maarten Lankhorst
2018-08-14 17:54 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 9+ messages in thread
From: Maarten Lankhorst @ 2018-08-14 11:08 UTC (permalink / raw)
To: dhinakaran.pandiyan, igt-dev
Op 13-08-18 om 20:34 schreef Dhinakaran Pandiyan:
> On Mon, 2018-08-13 at 09:11 +0200, Maarten Lankhorst wrote:
>> Op 11-08-18 om 03:50 schreef Dhinakaran Pandiyan:
>>> On Friday, August 10, 2018 3:06:45 AM PDT Maarten Lankhorst wrote:
>>>> It's harmful to write to enable_psr at runtime, and the patch
>>>> that allows
>>>> us to change i915_edp_psr_debug with the panel running will
>>>> require us
>>>> to abandon the module parameter. Hence the userspace change needs
>>>> to be
>>>> put in IGT first before we can change it at kernel time.
>>>>
>>>> Toggling it to debugfs will mean we can skip a modeset when
>>>> changing our
>>>> feature set.
>>>>
>>>> Changes since v1:
>>>> - Rebase with the previous patches dropped.
>>>> Changes since v2:
>>>> - Rebase on top of new api in i915_edp_psr_debug.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
>>>> om>
>>>> ---
>>>> tests/kms_frontbuffer_tracking.c | 44
>>>> ++++++++++++++++++++++++++++++--
>>>> 1 file changed, 42 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tests/kms_frontbuffer_tracking.c
>>>> b/tests/kms_frontbuffer_tracking.c index
>>>> 1dfd7c1cee8d..06ad55709dc6 100644
>>>> --- a/tests/kms_frontbuffer_tracking.c
>>>> +++ b/tests/kms_frontbuffer_tracking.c
>>>> @@ -775,6 +775,46 @@ static void drrs_set(unsigned int val)
>>>> igt_assert_f(ret == (sizeof(buf) - 1),
>>>> "debugfs_write failed");
>>>> }
>>>>
>>>> +static void restore_psr_debugfs(int sig)
>>>> +{
>>>> + debugfs_write("i915_edp_psr_debug", "0");
>>>> +}
>>>> +
>>>> +static bool psr_modparam_set(unsigned int val)
>>>> +{
>>>> + static int oldval = -1;
>>>> +
>>>> + igt_set_module_param_int("enable_psr", val);
>>>> +
>>>> + if (val == oldval)
>>>> + return false;
>>>> +
>>>> + oldval = val;
>>>> + return true;
>>>> +}
>>>> +
>>>> +static bool psr_set(bool enable)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + /* Check if new PSR debugfs api is usable. */
>>>> + ret = debugfs_write("i915_edp_psr_debug", "0xf");
>>>> + if (ret == -ENODEV) {
>>>> + /* PSR not enabled, should only be able to set
>>>> or unset. */
>>> s/enabled/supported
>>> -ENODEV is returned only when source or sink do not support PSR.
>> Indeed, typo. :)
>>>> + igt_assert(!enable);
>>>> + return false;
>>>> + }
>>>> +
>>>> + if (ret != -EINVAL)
>>> I would add a comment to explain that you are tying -EINVAL return
>>> to mean the
>>> driver supports the new API. This needs a comment in the driver
>>> too?
>>>
>>>> + return psr_modparam_set(enable);
>>> Don't see this return value being used.
>> It's used in 2/2.
>>>> +
>>>> + ret = debugfs_write("i915_edp_psr_debug", enable ? "0x2"
>>>> : "0x1");
>>> We should be using DEBUG_FORCE_PSR1 here. The only reason we were
>>> testing PSR2
>>> is because the module parameter did not have an option to force
>>> PSR1.
>> Ok, I was under the impression we want to test the newest PSR
>> possible,
> No, PSR2 isn't ready for testing yet. We should be testing only PSR1
> now.
Ok that's fine, should we also set the I915_PSR_DEBUG_IRQ flag?
~Maarten
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3.
2018-08-14 11:08 ` Maarten Lankhorst
@ 2018-08-14 17:54 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 9+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-08-14 17:54 UTC (permalink / raw)
To: igt-dev, maarten.lankhorst
On Tue, 2018-08-14 at 13:08 +0200, Maarten Lankhorst wrote:
> Op 13-08-18 om 20:34 schreef Dhinakaran Pandiyan:
> > On Mon, 2018-08-13 at 09:11 +0200, Maarten Lankhorst wrote:
> > > Op 11-08-18 om 03:50 schreef Dhinakaran Pandiyan:
> > > > On Friday, August 10, 2018 3:06:45 AM PDT Maarten Lankhorst
> > > > wrote:
> > > > > It's harmful to write to enable_psr at runtime, and the patch
> > > > > that allows
> > > > > us to change i915_edp_psr_debug with the panel running will
> > > > > require us
> > > > > to abandon the module parameter. Hence the userspace change
> > > > > needs
> > > > > to be
> > > > > put in IGT first before we can change it at kernel time.
> > > > >
> > > > > Toggling it to debugfs will mean we can skip a modeset when
> > > > > changing our
> > > > > feature set.
> > > > >
> > > > > Changes since v1:
> > > > > - Rebase with the previous patches dropped.
> > > > > Changes since v2:
> > > > > - Rebase on top of new api in i915_edp_psr_debug.
> > > > >
> > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.int
> > > > > el.c
> > > > > om>
> > > > > ---
> > > > > tests/kms_frontbuffer_tracking.c | 44
> > > > > ++++++++++++++++++++++++++++++--
> > > > > 1 file changed, 42 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/tests/kms_frontbuffer_tracking.c
> > > > > b/tests/kms_frontbuffer_tracking.c index
> > > > > 1dfd7c1cee8d..06ad55709dc6 100644
> > > > > --- a/tests/kms_frontbuffer_tracking.c
> > > > > +++ b/tests/kms_frontbuffer_tracking.c
> > > > > @@ -775,6 +775,46 @@ static void drrs_set(unsigned int val)
> > > > > igt_assert_f(ret == (sizeof(buf) - 1),
> > > > > "debugfs_write failed");
> > > > > }
> > > > >
> > > > > +static void restore_psr_debugfs(int sig)
> > > > > +{
> > > > > + debugfs_write("i915_edp_psr_debug", "0");
> > > > > +}
> > > > > +
> > > > > +static bool psr_modparam_set(unsigned int val)
> > > > > +{
> > > > > + static int oldval = -1;
> > > > > +
> > > > > + igt_set_module_param_int("enable_psr", val);
> > > > > +
> > > > > + if (val == oldval)
> > > > > + return false;
> > > > > +
> > > > > + oldval = val;
> > > > > + return true;
> > > > > +}
> > > > > +
> > > > > +static bool psr_set(bool enable)
> > > > > +{
> > > > > + int ret;
> > > > > +
> > > > > + /* Check if new PSR debugfs api is usable. */
> > > > > + ret = debugfs_write("i915_edp_psr_debug", "0xf");
> > > > > + if (ret == -ENODEV) {
> > > > > + /* PSR not enabled, should only be able to
> > > > > set
> > > > > or unset. */
> > > >
> > > > s/enabled/supported
> > > > -ENODEV is returned only when source or sink do not support
> > > > PSR.
> > >
> > > Indeed, typo. :)
> > > > > + igt_assert(!enable);
> > > > > + return false;
> > > > > + }
> > > > > +
> > > > > + if (ret != -EINVAL)
> > > >
> > > > I would add a comment to explain that you are tying -EINVAL
> > > > return
> > > > to mean the
> > > > driver supports the new API. This needs a comment in the driver
> > > > too?
> > > >
> > > > > + return psr_modparam_set(enable);
> > > >
> > > > Don't see this return value being used.
> > >
> > > It's used in 2/2.
> > > > > +
> > > > > + ret = debugfs_write("i915_edp_psr_debug", enable ?
> > > > > "0x2"
> > > > > : "0x1");
> > > >
> > > > We should be using DEBUG_FORCE_PSR1 here. The only reason we
> > > > were
> > > > testing PSR2
> > > > is because the module parameter did not have an option to force
> > > > PSR1.
> > >
> > > Ok, I was under the impression we want to test the newest PSR
> > > possible,
> >
> > No, PSR2 isn't ready for testing yet. We should be testing only
> > PSR1
> > now.
>
> Ok that's fine, should we also set the I915_PSR_DEBUG_IRQ flag?
Not yet. It is useful for interactive debugging but there were issues
when I tried to make IGT tests use the interrupts.
-DK
>
> ~Maarten
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-08-14 17:54 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-10 10:06 [igt-dev] [PATCH i-g-t 1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3 Maarten Lankhorst
2018-08-10 10:06 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_frontbuffer_tracking: Remove redundant modesets during subtest start, v3 Maarten Lankhorst
2018-08-10 13:14 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v3 Patchwork
2018-08-10 17:03 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
2018-08-11 1:50 ` [igt-dev] [PATCH i-g-t 1/2] " Dhinakaran Pandiyan
2018-08-13 7:11 ` Maarten Lankhorst
2018-08-13 18:34 ` Dhinakaran Pandiyan
2018-08-14 11:08 ` Maarten Lankhorst
2018-08-14 17:54 ` Pandiyan, Dhinakaran
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.