[igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Manasi Navare]

DSC enable gets configured during compute_config and needs
a full modeset to force DSC.
Sometimes in between the tests, if the initial output is same as the
mode being set for DSC then it will not do a full modeset.
So we disable the output before forcing a mode with DSC enable.

Fixes: db19bccc1c22 ("test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP")
Cc: Petri Latvala <petri.latvala@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 tests/kms_dp_dsc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/kms_dp_dsc.c b/tests/kms_dp_dsc.c
index da93cd74..f3a61029 100644
--- a/tests/kms_dp_dsc.c
+++ b/tests/kms_dp_dsc.c
@@ -174,6 +174,11 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
 				      &data->fb_test_pattern);
 		primary = igt_output_get_plane_type(data->output,
 						    DRM_PLANE_TYPE_PRIMARY);
+		/* Disable the output first */
+		igt_plane_set_fb(primary, NULL);
+		igt_output_set_pipe(data->output, PIPE_NONE);
+
+		/* Now set the output to the desired mode */
 		igt_plane_set_fb(primary, &data->fb_test_pattern);
 		igt_display_commit(&data->display);
 
@@ -187,7 +192,7 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
 		clear_dp_dsc_enable(data);
 
 		igt_assert_f(enabled,
-			     "Default DSC enable failed on Connector: %s Pipe: %s",
+			     "\nDefault DSC enable failed on Connector: %s Pipe: %s",
 			     data->conn_name,
 			     kmstest_pipe_name(data->pipe));
 	} else {
-- 
2.19.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.BAT: success for tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Patchwork]

== Series Details ==

Series: tests/kms_dp_dsc: Force a full modeset when we force dsc enable
URL   : https://patchwork.freedesktop.org/series/58887/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5856 -> IGTPW_2766
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/58887/revisions/1/mbox/

Known issues
------------

  Here are the changes found in IGTPW_2766 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_cs_nop@fork-compute0:
    - fi-icl-y:           NOTRUN -> SKIP [fdo#109315] +17

  * igt@amdgpu/amd_cs_nop@fork-gfx0:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109315] +17

  * igt@gem_exec_basic@basic-bsd2:
    - fi-kbl-7500u:       NOTRUN -> SKIP [fdo#109271] +9
    - fi-icl-y:           NOTRUN -> SKIP [fdo#109276] +7

  * igt@gem_exec_basic@readonly-bsd1:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109276] +7

  * igt@gem_exec_parse@basic-allowed:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109289] +1

  * igt@gem_exec_parse@basic-rejected:
    - fi-icl-y:           NOTRUN -> SKIP [fdo#109289] +1

  * igt@i915_selftest@live_contexts:
    - fi-icl-u2:          NOTRUN -> DMESG-FAIL [fdo#108569]
    - fi-icl-y:           NOTRUN -> DMESG-FAIL [fdo#108569]

  * igt@i915_selftest@live_execlists:
    - fi-apl-guc:         NOTRUN -> INCOMPLETE [fdo#103927] / [fdo#109720]

  * igt@kms_chamelium@dp-crc-fast:
    - fi-kbl-7500u:       NOTRUN -> DMESG-WARN [fdo#103841]
    - fi-icl-y:           NOTRUN -> SKIP [fdo#109284] +8

  * igt@kms_chamelium@dp-edid-read:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109316] +2

  * igt@kms_chamelium@vga-hpd-fast:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109309] +1

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-icl-y:           NOTRUN -> SKIP [fdo#109285] +3

  * igt@kms_force_connector_basic@prune-stale-modes:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109285] +3

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u2:          NOTRUN -> FAIL [fdo#103167]

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
    - fi-byt-clapper:     PASS -> FAIL [fdo#107362]

  * igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362]

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  * igt@kms_psr@primary_mmap_gtt:
    - fi-icl-y:           NOTRUN -> SKIP [fdo#110189] +3
    - fi-byt-clapper:     NOTRUN -> SKIP [fdo#109271] +23

  * igt@kms_psr@primary_page_flip:
    - fi-apl-guc:         NOTRUN -> SKIP [fdo#109271] +50

  * igt@prime_vgem@basic-fence-flip:
    - fi-icl-y:           NOTRUN -> SKIP [fdo#109294]

  * igt@runner@aborted:
    - fi-kbl-7500u:       NOTRUN -> FAIL [fdo#103841]
    - fi-apl-guc:         NOTRUN -> FAIL [fdo#108622] / [fdo#109720]

  
#### Possible fixes ####

  * igt@i915_selftest@live_contexts:
    - fi-bdw-gvtdvm:      DMESG-FAIL [fdo#110235 ] -> PASS
    - fi-skl-gvtdvm:      DMESG-FAIL [fdo#110235 ] -> PASS

  * igt@i915_selftest@live_uncore:
    - fi-skl-gvtdvm:      DMESG-FAIL [fdo#110210] -> PASS

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u3:          FAIL [fdo#103167] -> PASS
    - fi-byt-clapper:     FAIL [fdo#103167] -> PASS

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
    - fi-byt-clapper:     FAIL [fdo#107362] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-byt-clapper:     INCOMPLETE [fdo#102657] -> PASS

  
#### Warnings ####

  * igt@i915_selftest@live_contexts:
    - fi-icl-u3:          INCOMPLETE [fdo#108569] -> DMESG-FAIL [fdo#108569]

  
  [fdo#102657]: https://bugs.freedesktop.org/show_bug.cgi?id=102657
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103841]: https://bugs.freedesktop.org/show_bug.cgi?id=103841
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109294]: https://bugs.freedesktop.org/show_bug.cgi?id=109294
  [fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109316]: https://bugs.freedesktop.org/show_bug.cgi?id=109316
  [fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#110210]: https://bugs.freedesktop.org/show_bug.cgi?id=110210
  [fdo#110235 ]: https://bugs.freedesktop.org/show_bug.cgi?id=110235 


Participating hosts (43 -> 43)
------------------------------

  Additional (4): fi-icl-y fi-icl-u2 fi-apl-guc fi-kbl-7500u 
  Missing    (4): fi-ilk-m540 fi-byt-squawks fi-skl-6260u fi-bdw-samus 


Build changes
-------------

    * IGT: IGT_4922 -> IGTPW_2766

  CI_DRM_5856: 55074bd825098a71779cf65a69786547f0eccbe9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2766: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2766/
  IGT_4922: e941e4a29438c7130554492e4daf52afbc99ffdf @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2766/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.IGT: success for tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Patchwork]

== Series Details ==

Series: tests/kms_dp_dsc: Force a full modeset when we force dsc enable
URL   : https://patchwork.freedesktop.org/series/58887/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5856_full -> IGTPW_2766_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/58887/revisions/1/mbox/

Known issues
------------

  Here are the changes found in IGTPW_2766_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_store@cachelines-bsd2:
    - shard-glk:          NOTRUN -> SKIP [fdo#109271] +20

  * igt@i915_pm_rpm@system-suspend-modeset:
    - shard-kbl:          PASS -> INCOMPLETE [fdo#103665] / [fdo#107807]

  * igt@kms_atomic_transition@1x-modeset-transitions-nonblocking:
    - shard-apl:          PASS -> FAIL [fdo#109660]

  * igt@kms_atomic_transition@3x-modeset-transitions:
    - shard-hsw:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +3

  * igt@kms_atomic_transition@plane-all-modeset-transition-fencing:
    - shard-apl:          PASS -> INCOMPLETE [fdo#103927]

  * igt@kms_available_modes_crc@available_mode_test_crc:
    - shard-snb:          NOTRUN -> FAIL [fdo#106641]

  * igt@kms_busy@basic-modeset-e:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_busy@extended-modeset-hang-newfb-render-a:
    - shard-hsw:          PASS -> DMESG-WARN [fdo#110222] +2

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-c:
    - shard-kbl:          PASS -> DMESG-WARN [fdo#110222]

  * igt@kms_chamelium@common-hpd-after-suspend:
    - shard-hsw:          NOTRUN -> SKIP [fdo#109271] +21

  * igt@kms_cursor_crc@cursor-256x256-suspend:
    - shard-kbl:          PASS -> FAIL [fdo#103191] / [fdo#103232]
    - shard-apl:          PASS -> FAIL [fdo#103191] / [fdo#103232]

  * igt@kms_cursor_legacy@cursor-vs-flip-toggle:
    - shard-glk:          PASS -> INCOMPLETE [fdo#103359] / [k.org#198133]

  * igt@kms_flip@2x-wf_vblank-ts-check:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] +68

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-blt:
    - shard-apl:          PASS -> FAIL [fdo#103167]
    - shard-kbl:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-cur-indfb-draw-mmap-wc:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] +23

  * igt@kms_lease@atomic_implicit_crtc:
    - shard-kbl:          NOTRUN -> FAIL [fdo#110279]

  * igt@kms_plane_alpha_blend@pipe-a-alpha-7efc:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108145] / [fdo#108590]

  * igt@kms_plane_scaling@pipe-a-scaler-with-clipping-clamping:
    - shard-glk:          PASS -> SKIP [fdo#109271] / [fdo#109278] +1

  * igt@kms_plane_scaling@pipe-b-scaler-with-clipping-clamping:
    - shard-glk:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +2

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-kbl:          PASS -> FAIL [fdo#109016] +1

  * igt@kms_setmode@basic:
    - shard-kbl:          PASS -> FAIL [fdo#99912]

  * igt@kms_vblank@pipe-a-ts-continuation-dpms-rpm:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +5
    - shard-kbl:          PASS -> FAIL [fdo#104894]

  * igt@kms_vblank@pipe-b-ts-continuation-dpms-rpm:
    - shard-apl:          PASS -> FAIL [fdo#104894] +3

  
#### Possible fixes ####

  * igt@gem_create@create-clear:
    - shard-snb:          INCOMPLETE [fdo#105411] -> PASS

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-a:
    - shard-hsw:          DMESG-WARN [fdo#110222] -> PASS

  * igt@kms_cursor_crc@cursor-64x64-sliding:
    - shard-glk:          FAIL [fdo#103232] -> PASS +1

  * igt@kms_cursor_legacy@2x-long-nonblocking-modeset-vs-cursor-atomic:
    - shard-glk:          FAIL [fdo#106509] / [fdo#107409] -> PASS

  * igt@kms_plane@pixel-format-pipe-c-planes-source-clamping:
    - shard-glk:          SKIP [fdo#109271] -> PASS

  * igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend:
    - shard-kbl:          INCOMPLETE [fdo#103665] -> PASS

  * igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend:
    - shard-apl:          FAIL [fdo#104894] -> PASS

  
#### Warnings ####

  * igt@gem_tiled_swapping@non-threaded:
    - shard-hsw:          FAIL [fdo#108686] -> INCOMPLETE [fdo#103540]

  * igt@kms_frontbuffer_tracking@psr-2p-primscrn-pri-shrfb-draw-render:
    - shard-hsw:          INCOMPLETE [fdo#103540] -> SKIP [fdo#109271]

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#106509]: https://bugs.freedesktop.org/show_bug.cgi?id=106509
  [fdo#106641]: https://bugs.freedesktop.org/show_bug.cgi?id=106641
  [fdo#107409]: https://bugs.freedesktop.org/show_bug.cgi?id=107409
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108590]: https://bugs.freedesktop.org/show_bug.cgi?id=108590
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#109016]: https://bugs.freedesktop.org/show_bug.cgi?id=109016
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109660]: https://bugs.freedesktop.org/show_bug.cgi?id=109660
  [fdo#110222]: https://bugs.freedesktop.org/show_bug.cgi?id=110222
  [fdo#110279]: https://bugs.freedesktop.org/show_bug.cgi?id=110279
  [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 (10 -> 5)
------------------------------

  Missing    (5): shard-skl pig-hsw-4770r pig-glk-j5005 shard-iclb pig-skl-6260u 


Build changes
-------------

    * IGT: IGT_4922 -> IGTPW_2766
    * Piglit: piglit_4509 -> None

  CI_DRM_5856: 55074bd825098a71779cf65a69786547f0eccbe9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2766: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2766/
  IGT_4922: e941e4a29438c7130554492e4daf52afbc99ffdf @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2766/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Petri Latvala]

On Tue, Apr 02, 2019 at 12:31:19PM -0700, Manasi Navare wrote:
> DSC enable gets configured during compute_config and needs
> a full modeset to force DSC.
> Sometimes in between the tests, if the initial output is same as the
> mode being set for DSC then it will not do a full modeset.
> So we disable the output before forcing a mode with DSC enable.
> 
> Fixes: db19bccc1c22 ("test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP")
> Cc: Petri Latvala <petri.latvala@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  tests/kms_dp_dsc.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/kms_dp_dsc.c b/tests/kms_dp_dsc.c
> index da93cd74..f3a61029 100644
> --- a/tests/kms_dp_dsc.c
> +++ b/tests/kms_dp_dsc.c
> @@ -174,6 +174,11 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
>  				      &data->fb_test_pattern);
>  		primary = igt_output_get_plane_type(data->output,
>  						    DRM_PLANE_TYPE_PRIMARY);
> +		/* Disable the output first */
> +		igt_plane_set_fb(primary, NULL);
> +		igt_output_set_pipe(data->output, PIPE_NONE);
> +
> +		/* Now set the output to the desired mode */
>  		igt_plane_set_fb(primary, &data->fb_test_pattern);
>  		igt_display_commit(&data->display);
>  
> @@ -187,7 +192,7 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
>  		clear_dp_dsc_enable(data);
>  
>  		igt_assert_f(enabled,
> -			     "Default DSC enable failed on Connector: %s Pipe: %s",
> +			     "\nDefault DSC enable failed on Connector: %s Pipe: %s",


???

Did you mean to put that \n at the end of the string? As Arek did in
https://patchwork.freedesktop.org/series/58831/


-- 
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Imre Deak]

On Tue, Apr 02, 2019 at 12:31:19PM -0700, Manasi Navare wrote:
> DSC enable gets configured during compute_config and needs
> a full modeset to force DSC.
> Sometimes in between the tests, if the initial output is same as the
> mode being set for DSC then it will not do a full modeset.
> So we disable the output before forcing a mode with DSC enable.
> 
> Fixes: db19bccc1c22 ("test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP")
> Cc: Petri Latvala <petri.latvala@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  tests/kms_dp_dsc.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/kms_dp_dsc.c b/tests/kms_dp_dsc.c
> index da93cd74..f3a61029 100644
> --- a/tests/kms_dp_dsc.c
> +++ b/tests/kms_dp_dsc.c
> @@ -174,6 +174,11 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
>  				      &data->fb_test_pattern);
>  		primary = igt_output_get_plane_type(data->output,
>  						    DRM_PLANE_TYPE_PRIMARY);
> +		/* Disable the output first */
> +		igt_plane_set_fb(primary, NULL);

I think no need to reset the plane, it's enough to set PIPE_NONE to turn
off the output.

> +		igt_output_set_pipe(data->output, PIPE_NONE);

You are missing here the additional commit needed to turn off the output
and then set back the output to data->pipe before the commit below. The
igt_output_set_pipe() call in run_test() won't be needed either.

> +
> +		/* Now set the output to the desired mode */
>  		igt_plane_set_fb(primary, &data->fb_test_pattern);
>  		igt_display_commit(&data->display);
>  
> @@ -187,7 +192,7 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
>  		clear_dp_dsc_enable(data);

Instead of this we should restore the original value of
i915_dsc_fec_support and also make sure we restore it if the test fails
and aborts somewhere before this call. Take a look at a look at the
various places changing debugfs entries that call
igt_install_exit_handler() for an idea how to do this.

>  
>  		igt_assert_f(enabled,
> -			     "Default DSC enable failed on Connector: %s Pipe: %s",
> +			     "\nDefault DSC enable failed on Connector: %s Pipe: %s",
>  			     data->conn_name,
>  			     kmstest_pipe_name(data->pipe));
>  	} else {
> -- 
> 2.19.1
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Srivatsa, Anusha]

I tried the below changes on ICL system. The changes below as they are is also failing the test. 

Anusha 

>-----Original Message-----
>From: Navare, Manasi D
>Sent: Tuesday, April 2, 2019 12:31 PM
>To: igt-dev@lists.freedesktop.org
>Cc: Navare, Manasi D <manasi.d.navare@intel.com>; Latvala, Petri
><petri.latvala@intel.com>; Srivatsa, Anusha <anusha.srivatsa@intel.com>; Deak,
>Imre <imre.deak@intel.com>
>Subject: [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc
>enable
>
>DSC enable gets configured during compute_config and needs a full modeset to
>force DSC.
>Sometimes in between the tests, if the initial output is same as the mode being
>set for DSC then it will not do a full modeset.
>So we disable the output before forcing a mode with DSC enable.
>
>Fixes: db19bccc1c22 ("test/kms_dp_dsc: Basic KMS test to validate VESA DSC on
>DP/eDP")
>Cc: Petri Latvala <petri.latvala@intel.com>
>Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>Cc: Imre Deak <imre.deak@intel.com>
>Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
>---
> tests/kms_dp_dsc.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
>diff --git a/tests/kms_dp_dsc.c b/tests/kms_dp_dsc.c index da93cd74..f3a61029
>100644
>--- a/tests/kms_dp_dsc.c
>+++ b/tests/kms_dp_dsc.c
>@@ -174,6 +174,11 @@ static void update_display(data_t *data, enum
>dsc_test_type test_type)
> 				      &data->fb_test_pattern);
> 		primary = igt_output_get_plane_type(data->output,
>
>DRM_PLANE_TYPE_PRIMARY);
>+		/* Disable the output first */
>+		igt_plane_set_fb(primary, NULL);
>+		igt_output_set_pipe(data->output, PIPE_NONE);
>+
>+		/* Now set the output to the desired mode */
> 		igt_plane_set_fb(primary, &data->fb_test_pattern);
> 		igt_display_commit(&data->display);
>
>@@ -187,7 +192,7 @@ static void update_display(data_t *data, enum
>dsc_test_type test_type)
> 		clear_dp_dsc_enable(data);
>
> 		igt_assert_f(enabled,
>-			     "Default DSC enable failed on Connector: %s Pipe:
>%s",
>+			     "\nDefault DSC enable failed on Connector: %s Pipe:
>%s",
> 			     data->conn_name,
> 			     kmstest_pipe_name(data->pipe));
> 	} else {
>--
>2.19.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Manasi Navare]

On Wed, Apr 03, 2019 at 04:25:10PM +0300, Imre Deak wrote:
> On Tue, Apr 02, 2019 at 12:31:19PM -0700, Manasi Navare wrote:
> > DSC enable gets configured during compute_config and needs
> > a full modeset to force DSC.
> > Sometimes in between the tests, if the initial output is same as the
> > mode being set for DSC then it will not do a full modeset.
> > So we disable the output before forcing a mode with DSC enable.
> > 
> > Fixes: db19bccc1c22 ("test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP")
> > Cc: Petri Latvala <petri.latvala@intel.com>
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  tests/kms_dp_dsc.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/kms_dp_dsc.c b/tests/kms_dp_dsc.c
> > index da93cd74..f3a61029 100644
> > --- a/tests/kms_dp_dsc.c
> > +++ b/tests/kms_dp_dsc.c
> > @@ -174,6 +174,11 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
> >  				      &data->fb_test_pattern);
> >  		primary = igt_output_get_plane_type(data->output,
> >  						    DRM_PLANE_TYPE_PRIMARY);
> > +		/* Disable the output first */
> > +		igt_plane_set_fb(primary, NULL);
> 
> I think no need to reset the plane, it's enough to set PIPE_NONE to turn
> off the output.
> 
> > +		igt_output_set_pipe(data->output, PIPE_NONE);
> 
> You are missing here the additional commit needed to turn off the output
> and then set back the output to data->pipe before the commit below. The
> igt_output_set_pipe() call in run_test() won't be needed either.

Oh I looked through all the other kms tests and there we always set the fb for
PRIMARY plane to NULL then set the pipe to NULL.
But I guess if we disable to pipe output, the plane if its NULL or not shouldnt really matter.
So I should just have:

igt_plane_set_fb(primary, NULL);
igt_display_commit(&data->display)

Then set fb to the test pattern and display commit to set output to
the desired mode corerct?


So why is it that I wont need the igt_output_set_pipe(output, pipe) in run_test()?
I see that all the kms tests use this call for_each_valid_output_on_pipe iterator
> 
> > +
> > +		/* Now set the output to the desired mode */
> >  		igt_plane_set_fb(primary, &data->fb_test_pattern);
> >  		igt_display_commit(&data->display);
> >  
> > @@ -187,7 +192,7 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
> >  		clear_dp_dsc_enable(data);
> 
> Instead of this we should restore the original value of
> i915_dsc_fec_support and also make sure we restore it if the test fails
> and aborts somewhere before this call. Take a look at a look at the
> various places changing debugfs entries that call
> igt_install_exit_handler() for an idea how to do this.

So save the original value of i915_dsc_fec_support() and restore that in test_cleanup() and
in the igt_fixture(), have:
kmstest_set_vt_graphics_mode();
igt_install_exit_handler()

Manasi

> 
> >  
> >  		igt_assert_f(enabled,
> > -			     "Default DSC enable failed on Connector: %s Pipe: %s",
> > +			     "\nDefault DSC enable failed on Connector: %s Pipe: %s",
> >  			     data->conn_name,
> >  			     kmstest_pipe_name(data->pipe));
> >  	} else {
> > -- 
> > 2.19.1
> > 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Imre Deak]

On Wed, Apr 03, 2019 at 11:20:58AM -0700, Manasi Navare wrote:
> On Wed, Apr 03, 2019 at 04:25:10PM +0300, Imre Deak wrote:
> > On Tue, Apr 02, 2019 at 12:31:19PM -0700, Manasi Navare wrote:
> > > DSC enable gets configured during compute_config and needs
> > > a full modeset to force DSC.
> > > Sometimes in between the tests, if the initial output is same as the
> > > mode being set for DSC then it will not do a full modeset.
> > > So we disable the output before forcing a mode with DSC enable.
> > > 
> > > Fixes: db19bccc1c22 ("test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP")
> > > Cc: Petri Latvala <petri.latvala@intel.com>
> > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  tests/kms_dp_dsc.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/kms_dp_dsc.c b/tests/kms_dp_dsc.c
> > > index da93cd74..f3a61029 100644
> > > --- a/tests/kms_dp_dsc.c
> > > +++ b/tests/kms_dp_dsc.c
> > > @@ -174,6 +174,11 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
> > >  				      &data->fb_test_pattern);
> > >  		primary = igt_output_get_plane_type(data->output,
> > >  						    DRM_PLANE_TYPE_PRIMARY);
> > > +		/* Disable the output first */
> > > +		igt_plane_set_fb(primary, NULL);
> > 
> > I think no need to reset the plane, it's enough to set PIPE_NONE to turn
> > off the output.
> > 
> > > +		igt_output_set_pipe(data->output, PIPE_NONE);
> > 
> > You are missing here the additional commit needed to turn off the output
> > and then set back the output to data->pipe before the commit below. The
> > igt_output_set_pipe() call in run_test() won't be needed either.
> 
> Oh I looked through all the other kms tests and there we always set
> the fb for PRIMARY plane to NULL then set the pipe to NULL.  But I
> guess if we disable to pipe output, the plane if its NULL or not
> shouldnt really matter.  So I should just have:
> 
> igt_plane_set_fb(primary, NULL);
> igt_display_commit(&data->display);

You need to set PIPE_NONE before the commit and then the actual pipe
before the rest of the sequence, like:

	igt_debug("DSC is supported on %s\n", data->conn_name);

	igt_output_set_pipe(data->output, PIPE_NONE);
	igt_display_commit(&data->display);

	force_dp_dsc_enable(...);

	igt_output_set_pipe(data->output, data->pipe);
	igt_create_pattern_fb(...);
	primary = ...
	igt_plane_set_fb(...);
	igt_display_commit(&data->display);

> Then set fb to the test pattern and display commit to set output to
> the desired mode corerct?
> 

> So why is it that I wont need the igt_output_set_pipe(output, pipe) in
> run_test()?  I see that all the kms tests use this call
> for_each_valid_output_on_pipe iterator

Because you will anyway set it to PIPE_NONE right afterwards in
update_display().

> > > +
> > > +		/* Now set the output to the desired mode */
> > >  		igt_plane_set_fb(primary, &data->fb_test_pattern);
> > >  		igt_display_commit(&data->display);
> > >  
> > > @@ -187,7 +192,7 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
> > >  		clear_dp_dsc_enable(data);
> > 
> > Instead of this we should restore the original value of
> > i915_dsc_fec_support and also make sure we restore it if the test fails
> > and aborts somewhere before this call. Take a look at a look at the
> > various places changing debugfs entries that call
> > igt_install_exit_handler() for an idea how to do this.
> 

> So save the original value of i915_dsc_fec_support() and restore that
> in test_cleanup()

Save the original value in force_dp_dsc_enable() and restore it in a new
restore_dp_dsc_enable() which you call instead of clear_dp_dsc_enable()
in update_display().

> and in the igt_fixture(), have:
> kmstest_set_vt_graphics_mode();
> igt_install_exit_handler()

The exit handler should be installed only after saving the original
value, so the handler will have the proper value to restore. Save/install
the exit handler only once even if force_dp_dsc_enable() is called
multiple times.

> 
> Manasi
> 
> > 
> > >  
> > >  		igt_assert_f(enabled,
> > > -			     "Default DSC enable failed on Connector: %s Pipe: %s",
> > > +			     "\nDefault DSC enable failed on Connector: %s Pipe: %s",
> > >  			     data->conn_name,
> > >  			     kmstest_pipe_name(data->pipe));
> > >  	} else {
> > > -- 
> > > 2.19.1
> > > 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Manasi Navare]

On Wed, Apr 03, 2019 at 10:33:41PM +0300, Imre Deak wrote:
> On Wed, Apr 03, 2019 at 11:20:58AM -0700, Manasi Navare wrote:
> > On Wed, Apr 03, 2019 at 04:25:10PM +0300, Imre Deak wrote:
> > > On Tue, Apr 02, 2019 at 12:31:19PM -0700, Manasi Navare wrote:
> > > > DSC enable gets configured during compute_config and needs
> > > > a full modeset to force DSC.
> > > > Sometimes in between the tests, if the initial output is same as the
> > > > mode being set for DSC then it will not do a full modeset.
> > > > So we disable the output before forcing a mode with DSC enable.
> > > > 
> > > > Fixes: db19bccc1c22 ("test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP")
> > > > Cc: Petri Latvala <petri.latvala@intel.com>
> > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  tests/kms_dp_dsc.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tests/kms_dp_dsc.c b/tests/kms_dp_dsc.c
> > > > index da93cd74..f3a61029 100644
> > > > --- a/tests/kms_dp_dsc.c
> > > > +++ b/tests/kms_dp_dsc.c
> > > > @@ -174,6 +174,11 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
> > > >  				      &data->fb_test_pattern);
> > > >  		primary = igt_output_get_plane_type(data->output,
> > > >  						    DRM_PLANE_TYPE_PRIMARY);
> > > > +		/* Disable the output first */
> > > > +		igt_plane_set_fb(primary, NULL);
> > > 
> > > I think no need to reset the plane, it's enough to set PIPE_NONE to turn
> > > off the output.
> > > 
> > > > +		igt_output_set_pipe(data->output, PIPE_NONE);
> > > 
> > > You are missing here the additional commit needed to turn off the output
> > > and then set back the output to data->pipe before the commit below. The
> > > igt_output_set_pipe() call in run_test() won't be needed either.
> > 
> > Oh I looked through all the other kms tests and there we always set
> > the fb for PRIMARY plane to NULL then set the pipe to NULL.  But I
> > guess if we disable to pipe output, the plane if its NULL or not
> > shouldnt really matter.  So I should just have:
> > 
> > igt_plane_set_fb(primary, NULL);
> > igt_display_commit(&data->display);
> 
> You need to set PIPE_NONE before the commit and then the actual pipe
> before the rest of the sequence, like:
> 
> 	igt_debug("DSC is supported on %s\n", data->conn_name);
> 
> 	igt_output_set_pipe(data->output, PIPE_NONE);
> 	igt_display_commit(&data->display);
> 
> 	force_dp_dsc_enable(...);
> 
> 	igt_output_set_pipe(data->output, data->pipe);
> 	igt_create_pattern_fb(...);
> 	primary = ...
> 	igt_plane_set_fb(...);
> 	igt_display_commit(&data->display);

Yea, got this part.
Testing is in progress.

> 
> > Then set fb to the test pattern and display commit to set output to
> > the desired mode corerct?
> > 
> 
> > So why is it that I wont need the igt_output_set_pipe(output, pipe) in
> > run_test()?  I see that all the kms tests use this call
> > for_each_valid_output_on_pipe iterator
> 
> Because you will anyway set it to PIPE_NONE right afterwards in
> update_display().

Yup got it.

> 
> > > > +
> > > > +		/* Now set the output to the desired mode */
> > > >  		igt_plane_set_fb(primary, &data->fb_test_pattern);
> > > >  		igt_display_commit(&data->display);
> > > >  
> > > > @@ -187,7 +192,7 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
> > > >  		clear_dp_dsc_enable(data);
> > > 
> > > Instead of this we should restore the original value of
> > > i915_dsc_fec_support and also make sure we restore it if the test fails
> > > and aborts somewhere before this call. Take a look at a look at the
> > > various places changing debugfs entries that call
> > > igt_install_exit_handler() for an idea how to do this.
> > 
> 
> > So save the original value of i915_dsc_fec_support() and restore that
> > in test_cleanup()

I think this part will require lot of changes in the debugfs code in the kernel and
also the IGT, since now I just write a 1 to the node to force dsc enable and then a 0 to clear it and in the kernel we accept kstr_to_bool
to get a value in this node to set force_dsc_enable() to true or false.
But now basically the sysfs_write in IGT will need to write the entire original file that was
read from i915_dsc_fec_support() and in the kernel we will need to accept the whole file
and then parse the DSC_enabled field to take its value to use it for force_dsc_enable.

Is all this what you are suggesting?

And the exit_handler() that we install in that we call this test_cleanup()?

Manasi

> 
> Save the original value in force_dp_dsc_enable() and restore it in a new
> restore_dp_dsc_enable() which you call instead of clear_dp_dsc_enable()
> in update_display().
> 
> > and in the igt_fixture(), have:
> > kmstest_set_vt_graphics_mode();
> > igt_install_exit_handler()
> 
> The exit handler should be installed only after saving the original
> value, so the handler will have the proper value to restore. Save/install
> the exit handler only once even if force_dp_dsc_enable() is called
> multiple times.
> 
> > 
> > Manasi
> > 
> > > 
> > > >  
> > > >  		igt_assert_f(enabled,
> > > > -			     "Default DSC enable failed on Connector: %s Pipe: %s",
> > > > +			     "\nDefault DSC enable failed on Connector: %s Pipe: %s",
> > > >  			     data->conn_name,
> > > >  			     kmstest_pipe_name(data->pipe));
> > > >  	} else {
> > > > -- 
> > > > 2.19.1
> > > > 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Imre Deak]

On Wed, Apr 03, 2019 at 01:17:17PM -0700, Manasi Navare wrote:
> On Wed, Apr 03, 2019 at 10:33:41PM +0300, Imre Deak wrote:
> > On Wed, Apr 03, 2019 at 11:20:58AM -0700, Manasi Navare wrote:
> > > On Wed, Apr 03, 2019 at 04:25:10PM +0300, Imre Deak wrote:
> > > > On Tue, Apr 02, 2019 at 12:31:19PM -0700, Manasi Navare wrote:
> > > > > DSC enable gets configured during compute_config and needs
> > > > > a full modeset to force DSC.
> > > > > Sometimes in between the tests, if the initial output is same as the
> > > > > mode being set for DSC then it will not do a full modeset.
> > > > > So we disable the output before forcing a mode with DSC enable.
> > > > > 
> > > > > Fixes: db19bccc1c22 ("test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP")
> > > > > Cc: Petri Latvala <petri.latvala@intel.com>
> > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > ---
> > > > >  tests/kms_dp_dsc.c | 7 ++++++-
> > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tests/kms_dp_dsc.c b/tests/kms_dp_dsc.c
> > > > > index da93cd74..f3a61029 100644
> > > > > --- a/tests/kms_dp_dsc.c
> > > > > +++ b/tests/kms_dp_dsc.c
> > > > > @@ -174,6 +174,11 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
> > > > >  				      &data->fb_test_pattern);
> > > > >  		primary = igt_output_get_plane_type(data->output,
> > > > >  						    DRM_PLANE_TYPE_PRIMARY);
> > > > > +		/* Disable the output first */
> > > > > +		igt_plane_set_fb(primary, NULL);
> > > > 
> > > > I think no need to reset the plane, it's enough to set PIPE_NONE to turn
> > > > off the output.
> > > > 
> > > > > +		igt_output_set_pipe(data->output, PIPE_NONE);
> > > > 
> > > > You are missing here the additional commit needed to turn off the output
> > > > and then set back the output to data->pipe before the commit below. The
> > > > igt_output_set_pipe() call in run_test() won't be needed either.
> > > 
> > > Oh I looked through all the other kms tests and there we always set
> > > the fb for PRIMARY plane to NULL then set the pipe to NULL.  But I
> > > guess if we disable to pipe output, the plane if its NULL or not
> > > shouldnt really matter.  So I should just have:
> > > 
> > > igt_plane_set_fb(primary, NULL);
> > > igt_display_commit(&data->display);
> > 
> > You need to set PIPE_NONE before the commit and then the actual pipe
> > before the rest of the sequence, like:
> > 
> > 	igt_debug("DSC is supported on %s\n", data->conn_name);
> > 
> > 	igt_output_set_pipe(data->output, PIPE_NONE);
> > 	igt_display_commit(&data->display);
> > 
> > 	force_dp_dsc_enable(...);
> > 
> > 	igt_output_set_pipe(data->output, data->pipe);
> > 	igt_create_pattern_fb(...);
> > 	primary = ...
> > 	igt_plane_set_fb(...);
> > 	igt_display_commit(&data->display);
> 
> Yea, got this part.
> Testing is in progress.
> 
> > 
> > > Then set fb to the test pattern and display commit to set output to
> > > the desired mode corerct?
> > > 
> > 
> > > So why is it that I wont need the igt_output_set_pipe(output, pipe) in
> > > run_test()?  I see that all the kms tests use this call
> > > for_each_valid_output_on_pipe iterator
> > 
> > Because you will anyway set it to PIPE_NONE right afterwards in
> > update_display().
> 
> Yup got it.
> 
> > 
> > > > > +
> > > > > +		/* Now set the output to the desired mode */
> > > > >  		igt_plane_set_fb(primary, &data->fb_test_pattern);
> > > > >  		igt_display_commit(&data->display);
> > > > >  
> > > > > @@ -187,7 +192,7 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
> > > > >  		clear_dp_dsc_enable(data);
> > > > 
> > > > Instead of this we should restore the original value of
> > > > i915_dsc_fec_support and also make sure we restore it if the test fails
> > > > and aborts somewhere before this call. Take a look at a look at the
> > > > various places changing debugfs entries that call
> > > > igt_install_exit_handler() for an idea how to do this.
> > > 
> > 
> > > So save the original value of i915_dsc_fec_support() and restore that
> > > in test_cleanup()
> 
> I think this part will require lot of changes in the debugfs code in
> the kernel and also the IGT, since now I just write a 1 to the node to
> force dsc enable and then a 0 to clear it and in the kernel we accept
> kstr_to_bool to get a value in this node to set force_dsc_enable() to
> true or false.  But now basically the sysfs_write in IGT will need to
> write the entire original file that was read from
> i915_dsc_fec_support() and in the kernel we will need to accept the
> whole file and then parse the DSC_enabled field to take its value to
> use it for force_dsc_enable.
> 
> Is all this what you are suggesting?

Hm, no way to read back the actual debugfs value, that's weird. Then
yes, I'd suggest adding a line to i915_dsc_fec_support_show() printing
there intel_dp->force_dsc_en too.

> And the exit_handler() that we install in that we call this test_cleanup()?

The exit handler will be called automatically when the process exits for
whatever reason.

> 
> Manasi
> 
> > 
> > Save the original value in force_dp_dsc_enable() and restore it in a new
> > restore_dp_dsc_enable() which you call instead of clear_dp_dsc_enable()
> > in update_display().
> > 
> > > and in the igt_fixture(), have:
> > > kmstest_set_vt_graphics_mode();
> > > igt_install_exit_handler()
> > 
> > The exit handler should be installed only after saving the original
> > value, so the handler will have the proper value to restore. Save/install
> > the exit handler only once even if force_dp_dsc_enable() is called
> > multiple times.
> > 
> > > 
> > > Manasi
> > > 
> > > > 
> > > > >  
> > > > >  		igt_assert_f(enabled,
> > > > > -			     "Default DSC enable failed on Connector: %s Pipe: %s",
> > > > > +			     "\nDefault DSC enable failed on Connector: %s Pipe: %s",
> > > > >  			     data->conn_name,
> > > > >  			     kmstest_pipe_name(data->pipe));
> > > > >  	} else {
> > > > > -- 
> > > > > 2.19.1
> > > > > 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Manasi Navare]

On Wed, Apr 03, 2019 at 11:34:05PM +0300, Imre Deak wrote:
> On Wed, Apr 03, 2019 at 01:17:17PM -0700, Manasi Navare wrote:
> > On Wed, Apr 03, 2019 at 10:33:41PM +0300, Imre Deak wrote:
> > > On Wed, Apr 03, 2019 at 11:20:58AM -0700, Manasi Navare wrote:
> > > > On Wed, Apr 03, 2019 at 04:25:10PM +0300, Imre Deak wrote:
> > > > > On Tue, Apr 02, 2019 at 12:31:19PM -0700, Manasi Navare wrote:
> > > > > > DSC enable gets configured during compute_config and needs
> > > > > > a full modeset to force DSC.
> > > > > > Sometimes in between the tests, if the initial output is same as the
> > > > > > mode being set for DSC then it will not do a full modeset.
> > > > > > So we disable the output before forcing a mode with DSC enable.
> > > > > > 
> > > > > > Fixes: db19bccc1c22 ("test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP")
> > > > > > Cc: Petri Latvala <petri.latvala@intel.com>
> > > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > ---
> > > > > >  tests/kms_dp_dsc.c | 7 ++++++-
> > > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/tests/kms_dp_dsc.c b/tests/kms_dp_dsc.c
> > > > > > index da93cd74..f3a61029 100644
> > > > > > --- a/tests/kms_dp_dsc.c
> > > > > > +++ b/tests/kms_dp_dsc.c
> > > > > > @@ -174,6 +174,11 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
> > > > > >  				      &data->fb_test_pattern);
> > > > > >  		primary = igt_output_get_plane_type(data->output,
> > > > > >  						    DRM_PLANE_TYPE_PRIMARY);
> > > > > > +		/* Disable the output first */
> > > > > > +		igt_plane_set_fb(primary, NULL);
> > > > > 
> > > > > I think no need to reset the plane, it's enough to set PIPE_NONE to turn
> > > > > off the output.
> > > > > 
> > > > > > +		igt_output_set_pipe(data->output, PIPE_NONE);
> > > > > 
> > > > > You are missing here the additional commit needed to turn off the output
> > > > > and then set back the output to data->pipe before the commit below. The
> > > > > igt_output_set_pipe() call in run_test() won't be needed either.
> > > > 
> > > > Oh I looked through all the other kms tests and there we always set
> > > > the fb for PRIMARY plane to NULL then set the pipe to NULL.  But I
> > > > guess if we disable to pipe output, the plane if its NULL or not
> > > > shouldnt really matter.  So I should just have:
> > > > 
> > > > igt_plane_set_fb(primary, NULL);
> > > > igt_display_commit(&data->display);
> > > 
> > > You need to set PIPE_NONE before the commit and then the actual pipe
> > > before the rest of the sequence, like:
> > > 
> > > 	igt_debug("DSC is supported on %s\n", data->conn_name);
> > > 
> > > 	igt_output_set_pipe(data->output, PIPE_NONE);
> > > 	igt_display_commit(&data->display);
> > > 
> > > 	force_dp_dsc_enable(...);
> > > 
> > > 	igt_output_set_pipe(data->output, data->pipe);
> > > 	igt_create_pattern_fb(...);
> > > 	primary = ...
> > > 	igt_plane_set_fb(...);
> > > 	igt_display_commit(&data->display);
> > 
> > Yea, got this part.
> > Testing is in progress.
> > 
> > > 
> > > > Then set fb to the test pattern and display commit to set output to
> > > > the desired mode corerct?
> > > > 
> > > 
> > > > So why is it that I wont need the igt_output_set_pipe(output, pipe) in
> > > > run_test()?  I see that all the kms tests use this call
> > > > for_each_valid_output_on_pipe iterator
> > > 
> > > Because you will anyway set it to PIPE_NONE right afterwards in
> > > update_display().
> > 
> > Yup got it.
> > 
> > > 
> > > > > > +
> > > > > > +		/* Now set the output to the desired mode */
> > > > > >  		igt_plane_set_fb(primary, &data->fb_test_pattern);
> > > > > >  		igt_display_commit(&data->display);
> > > > > >  
> > > > > > @@ -187,7 +192,7 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
> > > > > >  		clear_dp_dsc_enable(data);
> > > > > 
> > > > > Instead of this we should restore the original value of
> > > > > i915_dsc_fec_support and also make sure we restore it if the test fails
> > > > > and aborts somewhere before this call. Take a look at a look at the
> > > > > various places changing debugfs entries that call
> > > > > igt_install_exit_handler() for an idea how to do this.
> > > > 
> > > 
> > > > So save the original value of i915_dsc_fec_support() and restore that
> > > > in test_cleanup()
> > 
> > I think this part will require lot of changes in the debugfs code in
> > the kernel and also the IGT, since now I just write a 1 to the node to
> > force dsc enable and then a 0 to clear it and in the kernel we accept
> > kstr_to_bool to get a value in this node to set force_dsc_enable() to
> > true or false.  But now basically the sysfs_write in IGT will need to
> > write the entire original file that was read from
> > i915_dsc_fec_support() and in the kernel we will need to accept the
> > whole file and then parse the DSC_enabled field to take its value to
> > use it for force_dsc_enable.
> > 
> > Is all this what you are suggesting?
> 
> Hm, no way to read back the actual debugfs value, that's weird. Then
> yes, I'd suggest adding a line to i915_dsc_fec_support_show() printing
> there intel_dp->force_dsc_en too.

Currently we have inh i915_dsc_fec_support_show:
DSC_enabled:
DSC_sink_support:
FEC_sink_support:

And this node we open as a read only first and then as write node and force_dsc_enable()
from IGT just writes a 1 to this, so you are saying that in the kernel _write() node,
I accept this and write to force_dsc_enable entry in this file instead of rewriting the whole file?
So in IGT also I will have to parse the file and write 1 for force_dsc_enable entry?

I still cant figure out how the restore() function work, we write the stored_values and write
for each of the entries in the file? And then in the kernel debugfs_write, get these and write back
to each entry?

Are there any existing examples in kernel debugfs and IGT where we do this?

Manasi

> 
> > And the exit_handler() that we install in that we call this test_cleanup()?
> 
> The exit handler will be called automatically when the process exits for
> whatever reason.
> 
> > 
> > Manasi
> > 
> > > 
> > > Save the original value in force_dp_dsc_enable() and restore it in a new
> > > restore_dp_dsc_enable() which you call instead of clear_dp_dsc_enable()
> > > in update_display().
> > > 
> > > > and in the igt_fixture(), have:
> > > > kmstest_set_vt_graphics_mode();
> > > > igt_install_exit_handler()
> > > 
> > > The exit handler should be installed only after saving the original
> > > value, so the handler will have the proper value to restore. Save/install
> > > the exit handler only once even if force_dp_dsc_enable() is called
> > > multiple times.
> > > 
> > > > 
> > > > Manasi
> > > > 
> > > > > 
> > > > > >  
> > > > > >  		igt_assert_f(enabled,
> > > > > > -			     "Default DSC enable failed on Connector: %s Pipe: %s",
> > > > > > +			     "\nDefault DSC enable failed on Connector: %s Pipe: %s",
> > > > > >  			     data->conn_name,
> > > > > >  			     kmstest_pipe_name(data->pipe));
> > > > > >  	} else {
> > > > > > -- 
> > > > > > 2.19.1
> > > > > > 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Imre Deak]

On Wed, Apr 03, 2019 at 01:49:44PM -0700, Manasi Navare wrote:
> [...]
> Currently we have inh i915_dsc_fec_support_show:
> DSC_enabled:
> DSC_sink_support:
> FEC_sink_support:

Yes, and after the kernel change you'll have an additional
  
  Force DSC enable: yes/no

line here.

> And this node we open as a read only first and then as write node and
> force_dsc_enable() from IGT just writes a 1 to this, so you are saying
> that in the kernel _write() node, I accept this and write to
> force_dsc_enable entry in this file instead of rewriting the whole
> file?

I'm not sure what you mean. Currently "1" or "0" is written to the
debugfs file which will set intel_dp->force_dsc_enable. This doesn't
need to be changed at all, no need to rewrite the whole file.

> So in IGT also I will have to parse the file and write 1 for
> force_dsc_enable entry?

You can get the original value of it similarly to how it's done already
for 'DSC_Sink_Support', by reading the file and doing

	strstr(buf, "Force DSC enable: yes");

on the content.

> I still cant figure out how the restore() function work, we write the
> stored_values and write for each of the entries in the file? And then
> in the kernel debugfs_write, get these and write back to each entry?

Only force_dsc_enable need to be restored, the only value that you
change by writing to the debugfs file. All the rest are calculated
values that you don't need to care about.

--Imre
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Manasi Navare]

On Thu, Apr 04, 2019 at 12:42:33AM +0300, Imre Deak wrote:
> On Wed, Apr 03, 2019 at 01:49:44PM -0700, Manasi Navare wrote:
> > [...]
> > Currently we have inh i915_dsc_fec_support_show:
> > DSC_enabled:
> > DSC_sink_support:
> > FEC_sink_support:
> 
> Yes, and after the kernel change you'll have an additional
>   
>   Force DSC enable: yes/no
> 
> line here.
> 
> > And this node we open as a read only first and then as write node and
> > force_dsc_enable() from IGT just writes a 1 to this, so you are saying
> > that in the kernel _write() node, I accept this and write to
> > force_dsc_enable entry in this file instead of rewriting the whole
> > file?
> 
> I'm not sure what you mean. Currently "1" or "0" is written to the
> debugfs file which will set intel_dp->force_dsc_enable. This doesn't
> need to be changed at all, no need to rewrite the whole file.
> 
> > So in IGT also I will have to parse the file and write 1 for
> > force_dsc_enable entry?
> 
> You can get the original value of it similarly to how it's done already
> for 'DSC_Sink_Support', by reading the file and doing
> 
> 	strstr(buf, "Force DSC enable: yes");
> 
> on the content.
> 
> > I still cant figure out how the restore() function work, we write the
> > stored_values and write for each of the entries in the file? And then
> > in the kernel debugfs_write, get these and write back to each entry?
> 
> Only force_dsc_enable need to be restored, the only value that you
> change by writing to the debugfs file. All the rest are calculated
> values that you don't need to care about.

But now that I rething then, force_dsc_enable is always 0, it s a variable
that we use to force DSC only if IGT asks to, so for test we force it to 1
and then after the test always clear it since the original value is always 0.
So why do we need a separate restore() for just force_dsc_enable if the value
is always 0.

Manasi

> 
> --Imre
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Imre Deak]

On Wed, Apr 03, 2019 at 03:48:24PM -0700, Manasi Navare wrote:
> On Thu, Apr 04, 2019 at 12:42:33AM +0300, Imre Deak wrote:
> > On Wed, Apr 03, 2019 at 01:49:44PM -0700, Manasi Navare wrote:
> > > [...]
> > > Currently we have inh i915_dsc_fec_support_show:
> > > DSC_enabled:
> > > DSC_sink_support:
> > > FEC_sink_support:
> > 
> > Yes, and after the kernel change you'll have an additional
> >   
> >   Force DSC enable: yes/no
> > 
> > line here.
> > 
> > > And this node we open as a read only first and then as write node and
> > > force_dsc_enable() from IGT just writes a 1 to this, so you are saying
> > > that in the kernel _write() node, I accept this and write to
> > > force_dsc_enable entry in this file instead of rewriting the whole
> > > file?
> > 
> > I'm not sure what you mean. Currently "1" or "0" is written to the
> > debugfs file which will set intel_dp->force_dsc_enable. This doesn't
> > need to be changed at all, no need to rewrite the whole file.
> > 
> > > So in IGT also I will have to parse the file and write 1 for
> > > force_dsc_enable entry?
> > 
> > You can get the original value of it similarly to how it's done already
> > for 'DSC_Sink_Support', by reading the file and doing
> > 
> > 	strstr(buf, "Force DSC enable: yes");
> > 
> > on the content.
> > 
> > > I still cant figure out how the restore() function work, we write the
> > > stored_values and write for each of the entries in the file? And then
> > > in the kernel debugfs_write, get these and write back to each entry?
> > 
> > Only force_dsc_enable need to be restored, the only value that you
> > change by writing to the debugfs file. All the rest are calculated
> > values that you don't need to care about.
> 
> But now that I rething then, force_dsc_enable is always 0, it s a variable
> that we use to force DSC only if IGT asks to, so for test we force it to 1
> and then after the test always clear it since the original value is always 0.
> So why do we need a separate restore() for just force_dsc_enable if the value
> is always 0.

It's not necessarily 0. Before the test starts someone could've written
1 to that file.

> 
> Manasi
> 
> > 
> > --Imre
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Manasi Navare]

On Thu, Apr 04, 2019 at 02:02:42AM +0300, Imre Deak wrote:
> On Wed, Apr 03, 2019 at 03:48:24PM -0700, Manasi Navare wrote:
> > On Thu, Apr 04, 2019 at 12:42:33AM +0300, Imre Deak wrote:
> > > On Wed, Apr 03, 2019 at 01:49:44PM -0700, Manasi Navare wrote:
> > > > [...]
> > > > Currently we have inh i915_dsc_fec_support_show:
> > > > DSC_enabled:
> > > > DSC_sink_support:
> > > > FEC_sink_support:
> > > 
> > > Yes, and after the kernel change you'll have an additional
> > >   
> > >   Force DSC enable: yes/no
> > > 
> > > line here.
> > > 
> > > > And this node we open as a read only first and then as write node and
> > > > force_dsc_enable() from IGT just writes a 1 to this, so you are saying
> > > > that in the kernel _write() node, I accept this and write to
> > > > force_dsc_enable entry in this file instead of rewriting the whole
> > > > file?
> > > 
> > > I'm not sure what you mean. Currently "1" or "0" is written to the
> > > debugfs file which will set intel_dp->force_dsc_enable. This doesn't
> > > need to be changed at all, no need to rewrite the whole file.
> > > 
> > > > So in IGT also I will have to parse the file and write 1 for
> > > > force_dsc_enable entry?
> > > 
> > > You can get the original value of it similarly to how it's done already
> > > for 'DSC_Sink_Support', by reading the file and doing
> > > 
> > > 	strstr(buf, "Force DSC enable: yes");
> > > 
> > > on the content.
> > > 
> > > > I still cant figure out how the restore() function work, we write the
> > > > stored_values and write for each of the entries in the file? And then
> > > > in the kernel debugfs_write, get these and write back to each entry?
> > > 
> > > Only force_dsc_enable need to be restored, the only value that you
> > > change by writing to the debugfs file. All the rest are calculated
> > > values that you don't need to care about.
> > 
> > But now that I rething then, force_dsc_enable is always 0, it s a variable
> > that we use to force DSC only if IGT asks to, so for test we force it to 1
> > and then after the test always clear it since the original value is always 0.
> > So why do we need a separate restore() for just force_dsc_enable if the value
> > is always 0.
> 
> It's not necessarily 0. Before the test starts someone could've written
> 1 to that file.

Hmm, so now the use of this force_dsc_enable in kernel is only for this specific IGT
and the kernel doesnt touch this. But because in the future the kernel might set it to true,
we want to make sure that the IGT doesnt change the kernel behaviour after the test is run,
is my understanding correct?
So basically adding that now for future proofing it?

Also on the igt_install_exit_handler(), should the test_cleanup be now part of this handler?


Manasi

> 
> > 
> > Manasi
> > 
> > > 
> > > --Imre
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Imre Deak]

On Wed, Apr 03, 2019 at 04:11:49PM -0700, Manasi Navare wrote:
> On Thu, Apr 04, 2019 at 02:02:42AM +0300, Imre Deak wrote:
> > On Wed, Apr 03, 2019 at 03:48:24PM -0700, Manasi Navare wrote:
> > > On Thu, Apr 04, 2019 at 12:42:33AM +0300, Imre Deak wrote:
> > > > On Wed, Apr 03, 2019 at 01:49:44PM -0700, Manasi Navare wrote:
> > > > > [...]
> > > > > Currently we have inh i915_dsc_fec_support_show:
> > > > > DSC_enabled:
> > > > > DSC_sink_support:
> > > > > FEC_sink_support:
> > > > 
> > > > Yes, and after the kernel change you'll have an additional
> > > >   
> > > >   Force DSC enable: yes/no
> > > > 
> > > > line here.
> > > > 
> > > > > And this node we open as a read only first and then as write node and
> > > > > force_dsc_enable() from IGT just writes a 1 to this, so you are saying
> > > > > that in the kernel _write() node, I accept this and write to
> > > > > force_dsc_enable entry in this file instead of rewriting the whole
> > > > > file?
> > > > 
> > > > I'm not sure what you mean. Currently "1" or "0" is written to the
> > > > debugfs file which will set intel_dp->force_dsc_enable. This doesn't
> > > > need to be changed at all, no need to rewrite the whole file.
> > > > 
> > > > > So in IGT also I will have to parse the file and write 1 for
> > > > > force_dsc_enable entry?
> > > > 
> > > > You can get the original value of it similarly to how it's done already
> > > > for 'DSC_Sink_Support', by reading the file and doing
> > > > 
> > > > 	strstr(buf, "Force DSC enable: yes");
> > > > 
> > > > on the content.
> > > > 
> > > > > I still cant figure out how the restore() function work, we write the
> > > > > stored_values and write for each of the entries in the file? And then
> > > > > in the kernel debugfs_write, get these and write back to each entry?
> > > > 
> > > > Only force_dsc_enable need to be restored, the only value that you
> > > > change by writing to the debugfs file. All the rest are calculated
> > > > values that you don't need to care about.
> > > 
> > > But now that I rething then, force_dsc_enable is always 0, it s a variable
> > > that we use to force DSC only if IGT asks to, so for test we force it to 1
> > > and then after the test always clear it since the original value is always 0.
> > > So why do we need a separate restore() for just force_dsc_enable if the value
> > > is always 0.
> > 
> > It's not necessarily 0. Before the test starts someone could've written
> > 1 to that file.
> 
> Hmm, so now the use of this force_dsc_enable in kernel is only for
> this specific IGT and the kernel doesnt touch this. But because in the
> future the kernel might set it to true, we want to make sure that the
> IGT doesnt change the kernel behaviour after the test is run, is my
> understanding correct?  So basically adding that now for future
> proofing it?

No, anybody running tests manually could set it and expect it to be
preserved.

> Also on the igt_install_exit_handler(), should the test_cleanup be now
> part of this handler?

No need for that, any required modeset specific cleanup will be handled
automatically.

> Manasi
> 
> > 
> > > 
> > > Manasi
> > > 
> > > > 
> > > > --Imre
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Manasi Navare]

On Thu, Apr 04, 2019 at 02:15:06AM +0300, Imre Deak wrote:
> On Wed, Apr 03, 2019 at 04:11:49PM -0700, Manasi Navare wrote:
> > On Thu, Apr 04, 2019 at 02:02:42AM +0300, Imre Deak wrote:
> > > On Wed, Apr 03, 2019 at 03:48:24PM -0700, Manasi Navare wrote:
> > > > On Thu, Apr 04, 2019 at 12:42:33AM +0300, Imre Deak wrote:
> > > > > On Wed, Apr 03, 2019 at 01:49:44PM -0700, Manasi Navare wrote:
> > > > > > [...]
> > > > > > Currently we have inh i915_dsc_fec_support_show:
> > > > > > DSC_enabled:
> > > > > > DSC_sink_support:
> > > > > > FEC_sink_support:
> > > > > 
> > > > > Yes, and after the kernel change you'll have an additional
> > > > >   
> > > > >   Force DSC enable: yes/no
> > > > > 
> > > > > line here.
> > > > > 
> > > > > > And this node we open as a read only first and then as write node and
> > > > > > force_dsc_enable() from IGT just writes a 1 to this, so you are saying
> > > > > > that in the kernel _write() node, I accept this and write to
> > > > > > force_dsc_enable entry in this file instead of rewriting the whole
> > > > > > file?
> > > > > 
> > > > > I'm not sure what you mean. Currently "1" or "0" is written to the
> > > > > debugfs file which will set intel_dp->force_dsc_enable. This doesn't
> > > > > need to be changed at all, no need to rewrite the whole file.
> > > > > 
> > > > > > So in IGT also I will have to parse the file and write 1 for
> > > > > > force_dsc_enable entry?
> > > > > 
> > > > > You can get the original value of it similarly to how it's done already
> > > > > for 'DSC_Sink_Support', by reading the file and doing
> > > > > 
> > > > > 	strstr(buf, "Force DSC enable: yes");
> > > > > 
> > > > > on the content.
> > > > > 
> > > > > > I still cant figure out how the restore() function work, we write the
> > > > > > stored_values and write for each of the entries in the file? And then
> > > > > > in the kernel debugfs_write, get these and write back to each entry?
> > > > > 
> > > > > Only force_dsc_enable need to be restored, the only value that you
> > > > > change by writing to the debugfs file. All the rest are calculated
> > > > > values that you don't need to care about.
> > > > 
> > > > But now that I rething then, force_dsc_enable is always 0, it s a variable
> > > > that we use to force DSC only if IGT asks to, so for test we force it to 1
> > > > and then after the test always clear it since the original value is always 0.
> > > > So why do we need a separate restore() for just force_dsc_enable if the value
> > > > is always 0.
> > > 
> > > It's not necessarily 0. Before the test starts someone could've written
> > > 1 to that file.
> > 
> > Hmm, so now the use of this force_dsc_enable in kernel is only for
> > this specific IGT and the kernel doesnt touch this. But because in the
> > future the kernel might set it to true, we want to make sure that the
> > IGT doesnt change the kernel behaviour after the test is run, is my
> > understanding correct?  So basically adding that now for future
> > proofing it?
> 
> No, anybody running tests manually could set it and expect it to be
> preserved.
> 
> > Also on the igt_install_exit_handler(), should the test_cleanup be now
> > part of this handler?
> 
> No need for that, any required modeset specific cleanup will be handled
> automatically.

So just call igt_install_exit_handler() without pasisng any function pointer to do
just the default cleanups?

Manasi

> 
> > Manasi
> > 
> > > 
> > > > 
> > > > Manasi
> > > > 
> > > > > 
> > > > > --Imre
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Imre Deak]

On Thu, Apr 04, 2019 at 04:44:35PM -0700, Manasi Navare wrote:
> [...]
> So just call igt_install_exit_handler() without pasisng any function pointer to do
> just the default cleanups?

You need to pass it a pointer to a new function you add. The new
function should write the original value to the debugfs file.

--Imre
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Manasi Navare]

On Fri, Apr 05, 2019 at 01:06:46PM +0300, Imre Deak wrote:
> On Thu, Apr 04, 2019 at 04:44:35PM -0700, Manasi Navare wrote:
> > [...]
> > So just call igt_install_exit_handler() without pasisng any function pointer to do
> > just the default cleanups?
> 
> You need to pass it a pointer to a new function you add. The new
> function should write the original value to the debugfs file.

Ok, I had added the restore_value() function to the test_cleanup that I call after update_display()
but yes adding that to the exit handler function would be a good idea.

the test cleanup function that I have now does:
if (data->output) {
igt_plane_set_fb(primary, NULL);
igt_display_commit(&data->display);
igt_remove_fb(data->drm_fd, &data->fb_test_pattern);
}

Hmm, I can still keep this after each test as is correct?

Manasi

> 
> --Imre
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev