Merge lp:~vanvugt/mir/fix-1528082 into lp:mir

[Daniel van Vugt]

Avoid choosing graphics-dummy (or no driver at all) over mesa-kms on a
desktop with DRM/Intel graphics. (LP: #1528082)

The right choice is obviously mesa-kms even if it fails to initialize
DRM/VT things. If however mesa-x11 or another driver can be used instead,
it will be.

Warning: If testing this manually, you may get confusing silence. That's bug 1528135 which can be dealt with separately.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3218
http://jenkins.qa.ubuntu.com/job/mir-ci/5907/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5397
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4304
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5350
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/233
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/233/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/233/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5347
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5347/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7855
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26290

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5907/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3219
http://jenkins.qa.ubuntu.com/job/mir-ci/5912/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5403
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4310
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5356
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/227/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/238
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/238/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/238
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/238/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5353
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5353/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7861
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26308
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/223
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/223/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/81/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26311

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5912/rebuild

review: Approve (continuous-integration)

[Cemil Azizoglu (cemil-azizoglu)]

Ok

review: Approve

[Kevin DuBois (kdub)]

lgtm, although curious if this was tested with the u8 desktop stack (where the recent problems started cropping up in 0.18 release). Also as you note, I agree that the system of selection we have is confusing and needs improvement.

review: Approve

[Andreas Pokorny (andreas-pokorny)]

ah this is an interesting approach. This should solve the nested case, while the host case just fails later. So should we remove the test for nested in the lines above? I am not sure which workaround is better....

review: Needs Information

[Daniel van Vugt (vanvugt)]

kdub: Well, our platform priority logic has been /obviously wrong/ leading up to this. It's not like we've had any seriously tricky bugs, but just dumb code and not enough pairs of eyes looking at it. I have not done full stack testing on this, but bug 1528082 describes a simple test case.

anpok: Good point about the nested case. However I think that section of code is probably correct. One good reason why I have not touched it here; we can discuss that separately another day.

=== modified file 'src/platforms/mesa/server/kms/platform_symbols.cpp'
--- src/platforms/mesa/server/kms/platform_symbols.cpp	2015-12-17 17:13:23 +0000
+++ src/platforms/mesa/server/kms/platform_symbols.cpp	2015-12-22 03:32:54 +0000
@@ -190,7 +190,25 @@
     if (platform_option_used)
         return mg::PlatformPriority::best;
 
-    return mg::PlatformPriority::unsupported;
+    /* We failed to set mastership. However, still on most systems mesa-kms
+     * is the right driver to choose. Landing here just means the user did
+     * not specify --vt or is running from ssh. Still in most cases mesa-kms
+     * is the correct option so give it a go. Better to fail trying to switch
+     * VTs (and tell the user that) than to refuse to load the correct
+     * driver at all. (LP: #1528082)
+     *
+     * Just make sure we are below PlatformPriority::supported in case
+     * mesa-x11 or android can be used instead.
+     *
+     * TODO: Revisit the priority terminology. having a range of values between
+     *       "supported" and "unsupported" is potentially confusing.
+     * TODO: Revisit the return code of this function. We document that
+     *       integer values outside the enum are allowed but C++ disallows it
+     *       without a cast. So we should return an int or typedef to int
+     *       instead.
+     */
+    return static_cast<mg::PlatformPriority>(
+        mg::PlatformPriority::supported - 1);
 }
 
 mir::ModuleProperties const description = {

=== modified file 'tests/unit-tests/graphics/mesa/kms/test_platform.cpp'
--- tests/unit-tests/graphics/mesa/kms/test_platform.cpp	2015-12-17 17:13:23 +0000
+++ tests/unit-tests/graphics/mesa/kms/test_platform.cpp	2015-12-22 03:32:54 +0000
@@ -332,6 +332,26 @@
     EXPECT_EQ(mg::PlatformPriority::best, probe(options));
 }
 
+TEST_F(MesaGraphicsPlatform, probe_returns_in_between_when_cant_set_master)
+{   // Regression test for LP: #1528082
+    using namespace testing;
+
+    mtf::UdevEnvironment udev_environment;
+    boost::program_options::options_description po;
+    mir::options::ProgramOption options;
+
+    udev_environment.add_standard_device("standard-drm-devices");
+
+    EXPECT_CALL(mock_drm, drmSetMaster(_))
+        .WillOnce(Return(-1));
+
+    mir::SharedLibrary platform_lib{mtf::server_platform("graphics-mesa-kms")};
+    auto probe = platform_lib.load_function<mg::PlatformProbe>(probe_platform);
+    auto prio = probe(options);
+    EXPECT_THAT(prio, Gt(mg::PlatformPriority::unsupported));
+    EXPECT_THAT(prio, Lt(mg::PlatformPriority::supported));
+}
+
 TEST_F(MesaGraphicsPlatform, probe_returns_best_when_drm_devices_vt_option_exist)
 {
     mtf::UdevEnvironment udev_environment;