Merge lp:~albaguirre/mir/minimize-1498550 into lp:mir

[Alberto Aguirre]

Query HWC for display connection if hwc->set fails.

Avoid using hotplug events to ignore commit errors; instead have the wrapper query the external display connection status and issue a DisplayDisconnectedException in such cases. The DisplayGroup can then choose to ignore that specific exception.

This only minimizes the occurences of lp:1498550, as there are still situations in which hwc->set fails due to unplugging the external display but hwc still reports the display as connected. The only recovery in this situation seems to be to tear down the compositor thread and recreate it as it happens when a display configuration occurs.

This only minimizes the occurences of lp:1498550, as there are still situations in which hwc->set fails due to unplugging the external display but hwc still reports the display as connected. The only recovery in this situation seems to be to tear down the compositor thread and recreate it as it happens when a display configuration occurs.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2962
http://jenkins.qa.ubuntu.com/job/mir-ci/5001/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4106
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3013
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4048
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/6/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1152/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/6/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4049
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4049/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6742
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23622
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/7/console

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

review: Needs Fixing (continuous-integration)

[Alberto Aguirre (albaguirre)]

^--
"/usr/include/EGL/eglplatform.h:100:35: fatal error: android/native_window.h: No such file or directory"

Ummm.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2963
http://jenkins.qa.ubuntu.com/job/mir-ci/5028/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4138
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3045
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4080
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/38/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1179
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1179/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/38/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4081
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4081/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6766
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23675
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/39/console

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Seems reasonable. (But not "obviously correct".)

review: Approve

=== modified file 'src/platforms/android/server/display.cpp'
--- src/platforms/android/server/display.cpp	2015-08-17 06:42:51 +0000
+++ src/platforms/android/server/display.cpp	2015-09-25 14:17:02 +0000
@@ -279,7 +279,6 @@
 {
     std::lock_guard<decltype(configuration_mutex)> lock{configuration_mutex};
     configuration_dirty = true;
-    displays.hotplug_occurred();
     display_change_pipe->notify_change();
 }
 

=== added file 'src/platforms/android/server/display_disconnected_exception.h'
--- src/platforms/android/server/display_disconnected_exception.h	1970-01-01 00:00:00 +0000
+++ src/platforms/android/server/display_disconnected_exception.h	2015-09-25 14:17:02 +0000
@@ -0,0 +1,41 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alberto Aguirre <[e-mail]>
+ */
+
+#ifndef MIR_GRAPHICS_ANDROID_DISPLAY_DISCONNECTED_EXCEPTION_H_
+#define MIR_GRAPHICS_ANDROID_DISPLAY_DISCONNECTED_EXCEPTION_H_
+
+#include <stdexcept>
+
+namespace mir
+{
+namespace graphics
+{
+namespace android
+{
+
+class DisplayDisconnectedException : public std::runtime_error
+{
+public:
+    DisplayDisconnectedException() : std::runtime_error("display disconnected") {}
+};
+
+}
+}
+}
+
+#endif /* MIR_GRAPHICS_ANDROID_DISPLAY_DISCONNECTED_EXCEPTION_H_ */

=== modified file 'src/platforms/android/server/display_group.cpp'
--- src/platforms/android/server/display_group.cpp	2015-07-22 02:54:31 +0000
+++ src/platforms/android/server/display_group.cpp	2015-09-25 14:17:02 +0000
@@ -18,6 +18,7 @@
 
 #include "display_group.h"
 #include "configurable_display_buffer.h"
+#include "display_disconnected_exception.h"
 #include <boost/throw_exception.hpp>
 #include <stdexcept>
 
@@ -28,8 +29,7 @@
 mga::DisplayGroup::DisplayGroup(
     std::shared_ptr<mga::DisplayDevice> const& device,
     std::unique_ptr<mga::ConfigurableDisplayBuffer> primary_buffer) :
-    device(device),
-    hotplugging(false)
+    device(device)
 {
     dbs.emplace(std::make_pair(mga::DisplayName::primary, std::move(primary_buffer)));
 }
@@ -81,18 +81,15 @@
         std::unique_lock<decltype(guard)> lk(guard);
         for(auto const& db : dbs)
             contents.emplace_back(db.second->contents());
-        hotplugging = false;
     }
 
     try
     {
         device->commit(contents);
     }
-    catch (std::runtime_error& e)
+    catch (mga::DisplayDisconnectedException& e)
     {
-        std::unique_lock<decltype(guard)> lk(guard);
-        if (!hotplugging)
-            throw e;
+        //Ignore disconnect errors as they are not fatal
     }
 }
 
@@ -100,9 +97,3 @@
 {
     return device->recommended_sleep();
 }
-
-void mga::DisplayGroup::hotplug_occurred()
-{
-    std::unique_lock<decltype(guard)> lk(guard);
-    hotplugging = true;
-}

=== modified file 'src/platforms/android/server/display_group.h'
--- src/platforms/android/server/display_group.h	2015-07-22 02:54:31 +0000
+++ src/platforms/android/server/display_group.h	2015-09-25 14:17:02 +0000
@@ -50,13 +50,11 @@
     void remove(DisplayName name);
     void configure(DisplayName name, MirPowerMode, MirOrientation, geometry::Displacement);
     bool display_present(DisplayName name) const;
-    void hotplug_occurred();
 
 private:
     std::mutex mutable guard;
     std::shared_ptr<DisplayDevice> const device;
     std::map<DisplayName, std::unique_ptr<ConfigurableDisplayBuffer>> dbs;
-    bool hotplugging;
 };
 
 }

=== modified file 'src/platforms/android/server/real_hwc_wrapper.cpp'
--- src/platforms/android/server/real_hwc_wrapper.cpp	2015-07-22 02:54:31 +0000
+++ src/platforms/android/server/real_hwc_wrapper.cpp	2015-09-25 14:17:02 +0000
@@ -18,6 +18,7 @@
 
 #include "real_hwc_wrapper.h"
 #include "hwc_report.h"
+#include "display_disconnected_exception.h"
 #include <boost/throw_exception.hpp>
 #include <stdexcept>
 #include <sstream>
@@ -115,9 +116,15 @@
     std::array<hwc_display_contents_1_t*, HWC_NUM_DISPLAY_TYPES> const& displays) const
 {
     report->report_set_list(displays);
-    if (auto rc = hwc_device->set(hwc_device.get(), num_displays(displays),
+    auto const num_displays = ::num_displays(displays);
+    if (auto rc = hwc_device->set(hwc_device.get(), num_displays,
         const_cast<hwc_display_contents_1**>(displays.data())))
     {
+        if (num_displays > 1 && !display_connected(DisplayName::external))
+        {
+            BOOST_THROW_EXCEPTION(mga::DisplayDisconnectedException());
+        }
+
         std::stringstream ss;
         ss << "error during hwc set(). rc = " << std::hex << rc;
         BOOST_THROW_EXCEPTION(std::runtime_error(ss.str()));
@@ -290,3 +297,9 @@
     if (rc < 0)
         BOOST_THROW_EXCEPTION(std::system_error(rc, std::system_category(), "unable to set active display config"));
 }
+
+bool mga::RealHwcWrapper::display_connected(DisplayName display_name) const
+{
+    size_t num_configs = 0;
+    return hwc_device->getDisplayConfigs(hwc_device.get(), display_name, nullptr, &num_configs) == 0;
+}

=== modified file 'src/platforms/android/server/real_hwc_wrapper.h'
--- src/platforms/android/server/real_hwc_wrapper.h	2015-06-17 05:20:42 +0000
+++ src/platforms/android/server/real_hwc_wrapper.h	2015-09-25 14:17:02 +0000
@@ -73,6 +73,8 @@
     void vsync(DisplayName, std::chrono::nanoseconds) noexcept;
     void hotplug(DisplayName, bool) noexcept;
     void invalidate() noexcept;
+
+    bool display_connected(DisplayName) const;
 private:
     std::shared_ptr<hwc_composer_device_1> const hwc_device;
     std::shared_ptr<HwcReport> const report;

=== modified file 'tests/unit-tests/graphics/android/test_display_group.cpp'
--- tests/unit-tests/graphics/android/test_display_group.cpp	2015-07-22 02:54:31 +0000
+++ tests/unit-tests/graphics/android/test_display_group.cpp	2015-09-25 14:17:02 +0000
@@ -18,6 +18,7 @@
 
 #include "src/platforms/android/server/configurable_display_buffer.h"
 #include "src/platforms/android/server/display_group.h"
+#include "src/platforms/android/server/display_disconnected_exception.h"
 #include "mir/test/doubles/mock_display_device.h"
 #include "mir/test/doubles/stub_renderable_list_compositor.h"
 #include "mir/test/doubles/stub_swapping_gl_context.h"
@@ -84,7 +85,7 @@
     group.post();
 }
 
-//lp: 1474891: If the driver is processing the external display in set,
+//lp: 1474891, 1498550: If the driver is processing the external display in set,
 //and it gets a hotplug event removing the external display, set() will throw, which we should ignore
 TEST(DisplayGroup, group_ignores_throws_during_hotplug)
 {
@@ -92,17 +93,9 @@
     NiceMock<mtd::MockDisplayDevice> mock_device;
     mga::DisplayGroup group(mt::fake_shared(mock_device), std::make_unique<StubConfigurableDB>());
     EXPECT_CALL(mock_device, commit(_))
-        .WillOnce(Throw(std::runtime_error("")))
-        .WillOnce(InvokeWithoutArgs([&]{
-            group.hotplug_occurred();
-            throw std::runtime_error("");
-        }))
+        .WillOnce(Throw(mga::DisplayDisconnectedException()))
         .WillOnce(Throw(std::runtime_error("")));
 
-    EXPECT_THROW({
-        group.post();
-    }, std::runtime_error);
-
     EXPECT_NO_THROW({group.post();});
 
     EXPECT_THROW({