| Message ID | 20180806202239.15522-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t] igt/kms_frontbuffer_tracking: Restore modparams around test | expand |
Em Seg, 2018-08-06 às 21:22 +0100, Chris Wilson escreveu: > Save the module parameters from setup and restore them on teardown, > so > that we leave the system in the same state as we found it. Currently kms_fbt uses igt_set_module_param_int() which uses igt_save_module_param(), which installs an exit handler, which is already supposed to accomplish what's described in the sentence above. What's missing from the current exit handler? Why is the new strategy better? Thanks, Paulo > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > tests/kms_frontbuffer_tracking.c | 43 +++++++++++++++++++++++++++++- > -- > 1 file changed, 39 insertions(+), 4 deletions(-) > > diff --git a/tests/kms_frontbuffer_tracking.c > b/tests/kms_frontbuffer_tracking.c > index 1dfd7c1ce..503a6fb6d 100644 > --- a/tests/kms_frontbuffer_tracking.c > +++ b/tests/kms_frontbuffer_tracking.c > @@ -155,6 +155,7 @@ struct rect { > struct { > int fd; > int debugfs; > + int modparams; > igt_display_t display; > > drm_intel_bufmgr *bufmgr; > @@ -939,10 +940,10 @@ static bool > drrs_wait_until_rr_switch_to_low(void) > return igt_wait(is_drrs_low(), 5000, 1); > } > > -#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 fbc_enable() igt_sysfs_set(drm.modparams, "enable_fbc", "1") > +#define fbc_disable() igt_sysfs_set(drm.modparams, "enable_fbc", > "0") > +#define psr_enable() igt_sysfs_set(drm.modparams, "enable_psr", "1") > +#define psr_disable() igt_sysfs_set(drm.modparams, "enable_psr", > "0") > #define drrs_enable() drrs_set(1) > #define drrs_disable() drrs_set(0) > > @@ -1286,6 +1287,7 @@ static void setup_drm(void) > { > drm.fd = drm_open_driver_master(DRIVER_INTEL); > drm.debugfs = igt_debugfs_dir(drm.fd); > + drm.modparams = igt_sysfs_open_parameters(drm.fd); > > kmstest_set_vt_graphics_mode(); > igt_display_init(&drm.display, drm.fd); > @@ -1299,6 +1301,7 @@ static void teardown_drm(void) > { > drm_intel_bufmgr_destroy(drm.bufmgr); > igt_display_fini(&drm.display); > + close(drm.modparams); > close(drm.fd); > } > > @@ -1467,11 +1470,40 @@ static void setup_drrs(void) > drrs.can_test = true; > } > > +static struct { > + const char *param; > + char buf[12]; > + int len; > +} modparams[] = { > + { "enable_psr" }, > + { "enable_fbc" }, > + { } > +}; > + > +static void save_modparams(void) > +{ > + for (typeof(*modparams) *p = modparams; p->param; p++) > + p->len = igt_sysfs_read(drm.modparams, > + p->param, p->buf, sizeof(p- > >buf)); > +} > + > +static void restore_modparams(void) > +{ > + for (typeof(*modparams) *p = modparams; p->param; p++) { > + if (p->len <= 0) > + continue; > + > + igt_sysfs_write(drm.modparams, p->param, p->buf, p- > >len); > + } > +} > + > static void setup_environment(void) > { > setup_drm(); > setup_modeset(); > > + save_modparams(); > + > setup_fbc(); > setup_psr(); > setup_drrs(); > @@ -1486,6 +1518,9 @@ static void teardown_environment(void) > teardown_crcs(); > teardown_psr(); > teardown_fbc(); > + > + restore_modparams(); > + > teardown_modeset(); > teardown_drm(); > }
Quoting Paulo Zanoni (2018-08-06 21:56:15) > Em Seg, 2018-08-06 às 21:22 +0100, Chris Wilson escreveu: > > Save the module parameters from setup and restore them on teardown, > > so > > that we leave the system in the same state as we found it. > > Currently kms_fbt uses igt_set_module_param_int() which uses > igt_save_module_param(), which installs an exit handler, which is > already supposed to accomplish what's described in the sentence above. > What's missing from the current exit handler? Why is the new strategy > better? Hmm, it's clearly not working then as fbc remains enabled after kms_frontbuffer_tracking: <7>[ 3305.252856] [IGT] kms_frontbuffer_tracking: exiting, ret=0 <5>[ 3305.252940] Setting dangerous option enable_psr - tainting kernel <5>[ 3305.253033] Setting dangerous option enable_fbc - tainting kernel <7>[ 3305.279279] [drm:intel_atomic_check [i915]] [CONNECTOR:67:HDMI-A-1] checking for sink bpp constrains <7>[ 3305.279317] [drm:intel_hdmi_compute_config [i915]] picking bpc to 12 for HDMI output <7>[ 3305.279346] [drm:intel_hdmi_compute_config [i915]] forcing pipe bpp to 36 for HDMI <7>[ 3305.279376] [drm:intel_atomic_check [i915]] hw max bpp: 36, pipe bpp: 36, dithering: 0 <7>[ 3305.279405] [drm:intel_dump_pipe_config [i915]] [CRTC:51:pipe B][modeset] <7>[ 3305.279434] [drm:intel_dump_pipe_config [i915]] output_types: HDMI (0x40) <7>[ 3305.279462] [drm:intel_dump_pipe_config [i915]] cpu_transcoder: B, pipe bpp: 36, dithering: 0 <7>[ 3305.279491] [drm:intel_dump_pipe_config [i915]] audio: 1, infoframes: 1 <7>[ 3305.279519] [drm:intel_dump_pipe_config [i915]] requested mode: <7>[ 3305.279525] [drm:drm_mode_debug_printmodeline] Modeline 0:"1920x1080" 60 148500 1920 2008 2052 2200 1080 1083 1088 1125 0x48 0x9 <7>[ 3305.279555] [drm:intel_dump_pipe_config [i915]] adjusted mode: <7>[ 3305.279561] [drm:drm_mode_debug_printmodeline] Modeline 0:"1920x1080" 60 148500 1920 2008 2052 2200 1080 1083 1088 1125 0x48 0x9 <7>[ 3305.279592] [drm:intel_dump_pipe_config [i915]] crtc timings: 148500 1920 2008 2052 2200 1080 1083 1088 1125, type: 0x48 flags: 0x9 <7>[ 3305.279621] [drm:intel_dump_pipe_config [i915]] port clock: 222750, pipe src size: 1920x1080, pixel rate 148500 <7>[ 3305.279651] [drm:intel_dump_pipe_config [i915]] pch pfit: pos: 0x00000000, size: 0x00000000, disabled <7>[ 3305.279680] [drm:intel_dump_pipe_config [i915]] ips: 0, double wide: 0 <7>[ 3305.279708] [drm:intel_dump_pipe_config [i915]] dpll_hw_state: wrpll: 0xb0210414 spll: 0x0 <7>[ 3305.279736] [drm:intel_dump_pipe_config [i915]] planes on this crtc <7>[ 3305.279764] [drm:intel_dump_pipe_config [i915]] [PLANE:40:primary B] disabled, scaler_id = 0 <7>[ 3305.279793] [drm:intel_dump_pipe_config [i915]] [PLANE:43:sprite B] disabled, scaler_id = 0 <7>[ 3305.279822] [drm:intel_dump_pipe_config [i915]] [PLANE:48:cursor B] disabled, scaler_id = 0 <7>[ 3305.280439] [drm:intel_find_shared_dpll [i915]] [CRTC:51:pipe B] allocated WRPLL 1 <7>[ 3305.280465] [drm:intel_reference_shared_dpll [i915]] using WRPLL 1 for pipe B <7>[ 3305.280519] [drm:intel_atomic_commit_tail [i915]] [ENCODER:65:CRT] <7>[ 3305.280544] [drm:intel_atomic_commit_tail [i915]] [ENCODER:66:DDI B] <7>[ 3305.280570] [drm:intel_atomic_commit_tail [i915]] [ENCODER:73:DDI C] <7>[ 3305.280595] [drm:intel_atomic_commit_tail [i915]] [ENCODER:76:DDI D] <7>[ 3305.280620] [drm:intel_atomic_commit_tail [i915]] [ENCODER:78:DP-MST A] <7>[ 3305.280645] [drm:intel_atomic_commit_tail [i915]] [ENCODER:79:DP-MST B] <7>[ 3305.280671] [drm:intel_atomic_commit_tail [i915]] [ENCODER:80:DP-MST C] <7>[ 3305.280696] [drm:verify_single_dpll_state.isra.88 [i915]] WRPLL 1 <7>[ 3305.280722] [drm:verify_single_dpll_state.isra.88 [i915]] WRPLL 2 <7>[ 3305.280747] [drm:verify_single_dpll_state.isra.88 [i915]] SPLL <7>[ 3305.280773] [drm:verify_single_dpll_state.isra.88 [i915]] LCPLL 810 <7>[ 3305.280798] [drm:verify_single_dpll_state.isra.88 [i915]] LCPLL 1350 <7>[ 3305.280823] [drm:verify_single_dpll_state.isra.88 [i915]] LCPLL 2700 <7>[ 3305.280857] [drm:intel_enable_shared_dpll [i915]] enable WRPLL 1 (active 2, on? 0) for crtc 51 <7>[ 3305.280883] [drm:intel_enable_shared_dpll [i915]] enabling WRPLL 1 <7>[ 3305.280992] [drm:intel_enable_pipe [i915]] enabling pipe B <7>[ 3305.281048] [drm:intel_audio_codec_enable [i915]] ELD on [CONNECTOR:67:HDMI-A-1], [ENCODER:66:DDI B] <7>[ 3305.281086] [drm:hsw_audio_codec_enable [i915]] Enable audio codec on pipe B, 32 bytes ELD <7>[ 3305.281120] [drm:audio_config_hdmi_pixel_clock [i915]] Configuring HDMI audio for pixel clock 148500 (0x00090000) <7>[ 3305.281146] [drm:hsw_audio_config_update [i915]] using automatic N <7>[ 3305.319297] [drm:__intel_fbc_disable [i915]] Disabling FBC on pipe A <7>[ 3305.319328] [drm:verify_connector_state.isra.87 [i915]] [CONNECTOR:67:HDMI-A-1] <7>[ 3305.319362] [drm:intel_atomic_commit_tail [i915]] [CRTC:51:pipe B] <7>[ 3305.319396] [drm:verify_single_dpll_state.isra.88 [i915]] WRPLL 1 <7>[ 3305.319794] [drm:intel_fbc_enable [i915]] reserved 11796480 bytes of contiguous stolen space for FBC, threshold: 1 <7>[ 3305.319818] [drm:intel_fbc_enable [i915]] Enabling FBC on pipe A And that I think makes the difference between rpm working and not. -Chris
Em Seg, 2018-08-06 às 22:07 +0100, Chris Wilson escreveu: > Quoting Paulo Zanoni (2018-08-06 21:56:15) > > Em Seg, 2018-08-06 às 21:22 +0100, Chris Wilson escreveu: > > > Save the module parameters from setup and restore them on > > > teardown, > > > so > > > that we leave the system in the same state as we found it. > > > > Currently kms_fbt uses igt_set_module_param_int() which uses > > igt_save_module_param(), which installs an exit handler, which is > > already supposed to accomplish what's described in the sentence > > above. > > What's missing from the current exit handler? Why is the new > > strategy > > better? > > Hmm, it's clearly not working then as fbc remains enabled after > kms_frontbuffer_tracking: > > <7>[ 3305.252856] [IGT] kms_frontbuffer_tracking: exiting, ret=0 > <5>[ 3305.252940] Setting dangerous option enable_psr - tainting > kernel > <5>[ 3305.253033] Setting dangerous option enable_fbc - tainting > kernel > <7>[ 3305.279279] [drm:intel_atomic_check [i915]] [CONNECTOR:67:HDMI- > A-1] checking for sink bpp constrains > <7>[ 3305.279317] [drm:intel_hdmi_compute_config [i915]] picking bpc > to 12 for HDMI output > <7>[ 3305.279346] [drm:intel_hdmi_compute_config [i915]] forcing pipe > bpp to 36 for HDMI > <7>[ 3305.279376] [drm:intel_atomic_check [i915]] hw max bpp: 36, > pipe bpp: 36, dithering: 0 > <7>[ 3305.279405] [drm:intel_dump_pipe_config [i915]] [CRTC:51:pipe > B][modeset] > <7>[ 3305.279434] [drm:intel_dump_pipe_config [i915]] output_types: > HDMI (0x40) > <7>[ 3305.279462] [drm:intel_dump_pipe_config [i915]] cpu_transcoder: > B, pipe bpp: 36, dithering: 0 > <7>[ 3305.279491] [drm:intel_dump_pipe_config [i915]] audio: 1, > infoframes: 1 > <7>[ 3305.279519] [drm:intel_dump_pipe_config [i915]] requested mode: > <7>[ 3305.279525] [drm:drm_mode_debug_printmodeline] Modeline > 0:"1920x1080" 60 148500 1920 2008 2052 2200 1080 1083 1088 1125 0x48 > 0x9 > <7>[ 3305.279555] [drm:intel_dump_pipe_config [i915]] adjusted mode: > <7>[ 3305.279561] [drm:drm_mode_debug_printmodeline] Modeline > 0:"1920x1080" 60 148500 1920 2008 2052 2200 1080 1083 1088 1125 0x48 > 0x9 > <7>[ 3305.279592] [drm:intel_dump_pipe_config [i915]] crtc timings: > 148500 1920 2008 2052 2200 1080 1083 1088 1125, type: 0x48 flags: 0x9 > <7>[ 3305.279621] [drm:intel_dump_pipe_config [i915]] port clock: > 222750, pipe src size: 1920x1080, pixel rate 148500 > <7>[ 3305.279651] [drm:intel_dump_pipe_config [i915]] pch pfit: pos: > 0x00000000, size: 0x00000000, disabled > <7>[ 3305.279680] [drm:intel_dump_pipe_config [i915]] ips: 0, double > wide: 0 > <7>[ 3305.279708] [drm:intel_dump_pipe_config [i915]] dpll_hw_state: > wrpll: 0xb0210414 spll: 0x0 > <7>[ 3305.279736] [drm:intel_dump_pipe_config [i915]] planes on this > crtc > <7>[ 3305.279764] [drm:intel_dump_pipe_config [i915]] > [PLANE:40:primary B] disabled, scaler_id = 0 > <7>[ 3305.279793] [drm:intel_dump_pipe_config [i915]] > [PLANE:43:sprite B] disabled, scaler_id = 0 > <7>[ 3305.279822] [drm:intel_dump_pipe_config [i915]] > [PLANE:48:cursor B] disabled, scaler_id = 0 > <7>[ 3305.280439] [drm:intel_find_shared_dpll [i915]] [CRTC:51:pipe > B] allocated WRPLL 1 > <7>[ 3305.280465] [drm:intel_reference_shared_dpll [i915]] using > WRPLL 1 for pipe B > <7>[ 3305.280519] [drm:intel_atomic_commit_tail [i915]] > [ENCODER:65:CRT] > <7>[ 3305.280544] [drm:intel_atomic_commit_tail [i915]] > [ENCODER:66:DDI B] > <7>[ 3305.280570] [drm:intel_atomic_commit_tail [i915]] > [ENCODER:73:DDI C] > <7>[ 3305.280595] [drm:intel_atomic_commit_tail [i915]] > [ENCODER:76:DDI D] > <7>[ 3305.280620] [drm:intel_atomic_commit_tail [i915]] > [ENCODER:78:DP-MST A] > <7>[ 3305.280645] [drm:intel_atomic_commit_tail [i915]] > [ENCODER:79:DP-MST B] > <7>[ 3305.280671] [drm:intel_atomic_commit_tail [i915]] > [ENCODER:80:DP-MST C] > <7>[ 3305.280696] [drm:verify_single_dpll_state.isra.88 [i915]] WRPLL > 1 > <7>[ 3305.280722] [drm:verify_single_dpll_state.isra.88 [i915]] WRPLL > 2 > <7>[ 3305.280747] [drm:verify_single_dpll_state.isra.88 [i915]] SPLL > <7>[ 3305.280773] [drm:verify_single_dpll_state.isra.88 [i915]] LCPLL > 810 > <7>[ 3305.280798] [drm:verify_single_dpll_state.isra.88 [i915]] LCPLL > 1350 > <7>[ 3305.280823] [drm:verify_single_dpll_state.isra.88 [i915]] LCPLL > 2700 > <7>[ 3305.280857] [drm:intel_enable_shared_dpll [i915]] enable WRPLL > 1 (active 2, on? 0) for crtc 51 > <7>[ 3305.280883] [drm:intel_enable_shared_dpll [i915]] enabling > WRPLL 1 > <7>[ 3305.280992] [drm:intel_enable_pipe [i915]] enabling pipe B > <7>[ 3305.281048] [drm:intel_audio_codec_enable [i915]] ELD on > [CONNECTOR:67:HDMI-A-1], [ENCODER:66:DDI B] > <7>[ 3305.281086] [drm:hsw_audio_codec_enable [i915]] Enable audio > codec on pipe B, 32 bytes ELD > <7>[ 3305.281120] [drm:audio_config_hdmi_pixel_clock [i915]] > Configuring HDMI audio for pixel clock 148500 (0x00090000) > <7>[ 3305.281146] [drm:hsw_audio_config_update [i915]] using > automatic N > <7>[ 3305.319297] [drm:__intel_fbc_disable [i915]] Disabling FBC on > pipe A > <7>[ 3305.319328] [drm:verify_connector_state.isra.87 [i915]] > [CONNECTOR:67:HDMI-A-1] > <7>[ 3305.319362] [drm:intel_atomic_commit_tail [i915]] [CRTC:51:pipe > B] > <7>[ 3305.319396] [drm:verify_single_dpll_state.isra.88 [i915]] WRPLL > 1 > <7>[ 3305.319794] [drm:intel_fbc_enable [i915]] reserved 11796480 > bytes of contiguous stolen space for FBC, threshold: 1 > <7>[ 3305.319818] [drm:intel_fbc_enable [i915]] Enabling FBC on pipe > A > > And that I think makes the difference between rpm working and not. Then I suppose it's better to debug the current solution instead of writing a new one, especially since the current one is part of lib/ and may be used by other tests. > -Chris > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c index 1dfd7c1ce..503a6fb6d 100644 --- a/tests/kms_frontbuffer_tracking.c +++ b/tests/kms_frontbuffer_tracking.c @@ -155,6 +155,7 @@ struct rect { struct { int fd; int debugfs; + int modparams; igt_display_t display; drm_intel_bufmgr *bufmgr; @@ -939,10 +940,10 @@ static bool drrs_wait_until_rr_switch_to_low(void) return igt_wait(is_drrs_low(), 5000, 1); } -#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 fbc_enable() igt_sysfs_set(drm.modparams, "enable_fbc", "1") +#define fbc_disable() igt_sysfs_set(drm.modparams, "enable_fbc", "0") +#define psr_enable() igt_sysfs_set(drm.modparams, "enable_psr", "1") +#define psr_disable() igt_sysfs_set(drm.modparams, "enable_psr", "0") #define drrs_enable() drrs_set(1) #define drrs_disable() drrs_set(0) @@ -1286,6 +1287,7 @@ static void setup_drm(void) { drm.fd = drm_open_driver_master(DRIVER_INTEL); drm.debugfs = igt_debugfs_dir(drm.fd); + drm.modparams = igt_sysfs_open_parameters(drm.fd); kmstest_set_vt_graphics_mode(); igt_display_init(&drm.display, drm.fd); @@ -1299,6 +1301,7 @@ static void teardown_drm(void) { drm_intel_bufmgr_destroy(drm.bufmgr); igt_display_fini(&drm.display); + close(drm.modparams); close(drm.fd); } @@ -1467,11 +1470,40 @@ static void setup_drrs(void) drrs.can_test = true; } +static struct { + const char *param; + char buf[12]; + int len; +} modparams[] = { + { "enable_psr" }, + { "enable_fbc" }, + { } +}; + +static void save_modparams(void) +{ + for (typeof(*modparams) *p = modparams; p->param; p++) + p->len = igt_sysfs_read(drm.modparams, + p->param, p->buf, sizeof(p->buf)); +} + +static void restore_modparams(void) +{ + for (typeof(*modparams) *p = modparams; p->param; p++) { + if (p->len <= 0) + continue; + + igt_sysfs_write(drm.modparams, p->param, p->buf, p->len); + } +} + static void setup_environment(void) { setup_drm(); setup_modeset(); + save_modparams(); + setup_fbc(); setup_psr(); setup_drrs(); @@ -1486,6 +1518,9 @@ static void teardown_environment(void) teardown_crcs(); teardown_psr(); teardown_fbc(); + + restore_modparams(); + teardown_modeset(); teardown_drm(); }
Save the module parameters from setup and restore them on teardown, so that we leave the system in the same state as we found it. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- tests/kms_frontbuffer_tracking.c | 43 +++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 4 deletions(-)