[igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs

[Arkadiusz Hiler]

On Fri, Oct 12, 2018 at 03:57:59PM +0300, Arkadiusz Hiler wrote:
> On Fri, Oct 12, 2018 at 02:32:44PM +0200, Daniel Vetter wrote:
> > On Fri, Oct 12, 2018 at 2:02 PM Arkadiusz Hiler
> > So the question is, what do do. We have 2 options for this function
> > here (revert or igt_warn), and we have a bunch of options for how to
> > make things more explicit in the tests (either my patch series here,
> > with v2 of patch 1, or maybe more explicit control flow with a
> > __must_check annotation for igt_display_init). Imo all combinations
> > would give us a solid api in the end, I just prefer not to implement
> > them all first, but decide first :-)
> > -Daniel
> 
> IMO:
> 
> 1. revert and either:
>  a) fixing try_commit so that it does not assert
>  b) moving the if n_pipes == 0 || n_outputs == 0 logic to debugfs tests
>     (or a block igt_with_display_on(display) { } ?)
> 2. respin of your patch series
> 
> But let's see what others have to say.
> 
> -Arek

I reverted the patch locally to see whether we stumble upon any of the
internal asserts while running debugfs tests with display disabled.
Tested with today's drm-tip and igt.

To my surprise - everything went just fine:

--------------------------------------------------------------------------------
$ dmesg | tail -n 9
[  166.643232] [drm] Display disabled (module parameter)
[  166.643407] [drm] Replacing VGA console driver
[  166.655672] i915 0000:00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=io+mem:owns=io+mem
[  166.668362] [drm] Initialized i915 1.6.0 20180921 for 0000:00:02.0 on minor 0
[  166.670518] [drm] DRM_I915_DEBUG enabled
[  166.670520] [drm] DRM_I915_DEBUG_GEM enabled
[  166.670522] [drm] DRM_I915_DEBUG_RUNTIME_PM enabled
[  188.818475] random: crng init done
[  188.818484] random: 7 urandom warning(s) missed due to ratelimiting

$ git show
commit ff89f7bce0111404ae480b7e901b827dedecf9fc (HEAD -> master)
Author: testrunner <testrunner at dev-bdw>
Date:   Wed Oct 17 11:42:35 2018 +0300

    Revert "lib/kms: Skip no-op display updates"

    This reverts commit 212b71372bfbb73663d872df31118d6b396ada4f.

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 6e499f48..e4165065 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -3295,9 +3295,6 @@ static int do_display_commit(igt_display_t *display,
        enum pipe pipe;
        LOG_INDENT(display, "commit");

-       if (!display->n_pipes || !display->n_outputs)
-               return 0; /* nothing to do */
-
        igt_display_refresh(display);

        if (s == COMMIT_ATOMIC) {
@@ -3348,9 +3345,6 @@ int igt_display_try_commit_atomic(igt_display_t *display, uint32_t flags, void *
 {
        int ret;

-       if (!display->n_pipes || !display->n_outputs)
-               return 0; /* nothing to do */
-
        LOG_INDENT(display, "commit");

        igt_display_refresh(display);

$ git diff
diff --git a/tests/debugfs_test.c b/tests/debugfs_test.c
index 2e87e442..01f3422f 100644
--- a/tests/debugfs_test.c
+++ b/tests/debugfs_test.c
@@ -131,7 +131,8 @@ igt_main
                        }
                }

-               igt_display_commit2(&display, display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
+               int ret = igt_display_commit2(&display, display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
+               printf("igt_display_commit2: %d\n", ret);

                read_and_discard_sysfs_entries(debugfs, 0);
        }
@@ -147,7 +148,8 @@ igt_main
                        for_each_plane_on_pipe(&display, pipe, plane)
                                igt_plane_set_fb(plane, NULL);

-               igt_display_commit2(&display, display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
+               int ret = igt_display_commit2(&display, display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
+               printf("igt_display_commit2: %d\n", ret);

                read_and_discard_sysfs_entries(debugfs, 0);
        }

$ ninja
[1/342] Generating version.h with a custom command.

$ sudo ./tests/debugfs_test
IGT-Version: 1.23-gff89f7bc (x86_64) (Linux: 4.19.0-rc8-CI-CI_DRM_4994+ x86_64)
Starting subtest: read_all_entries
igt_display_commit2: 0
Subtest read_all_entries: SUCCESS (0,017s)
Starting subtest: read_all_entries_display_off
igt_display_commit2: 0
Subtest read_all_entries_display_off: SUCCESS (0,000s)
Starting subtest: emon_crash
Test requirement not met in function __real_main90, file ../tests/debugfs_test.c:168:
Test requirement: !(!buf && !i)
i915_emon_status could not be read
Last errno: 9, Bad file descriptor
Subtest emon_crash: SKIP (0,000s)
--------------------------------------------------------------------------------

@Chris: What am I missing?

-Arek