Merge lp:~kdub/mir/deactive-notifications-when-display-off into lp:mir

[Kevin DuBois]

graphics: android: disable vsync notifications when screen is off as per hwc specs. rename unblank_or_blank_display to apply_display_state() to remove ambiguity about what is being done.

fixes: lp1233870

graphics: android: disable vsync notifications when screen is off as per hwc specs. rename unblank_or_blank_display to apply_display_state() to remove ambiguity about what is being done.

fixes: lp1233870

as seen here:
http://androidxref.com/4.2.2_r1/xref/hardware/libhardware/include/hardware/hwcomposer.h#337
the vsync notifications should be off when the screen is off. This was causing the nexus4 vsync notification thread in the driver to spin.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1099
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/66/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2194
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2079
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/66
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/66/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-mir-development-branch-ci/66/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

In terms of binary logic, I'd prefer to see these values reversed:

63 +enum class DisplayState
64 +{
65 + DisplayOn = 0,
66 + DisplayOff = 1,
67 +};

But then you have to ask... why am I not just using a boolean with a more obvious name?

Alternatively, use the existing MirPowerMode enum?

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

48 + display_provider->apply_display_state(mga::DisplayState::DisplayOn);

I approve the desire to have meaningful parameter values, but repeating "display state" and vague words like "apply" don't make for easy reading.

display_provider->state(mga::DisplayState::DisplayOn);

Or:

display_provider->mode(mir_power_mode_on);

review: Needs Fixing

[Kevin DuBois (kdub)]

@daniel, alan
should be fixed, it is much better to use the existing enum. thanks!

[Alan Griffiths (alan-griffiths)]

202 + if(err)

Whitespace nit

review: Approve

[Ricardo Mendoza (ricmm)]

Fixes the power issue for me on N4.

review: Approve

=== modified file 'include/test/mir_test_doubles/mock_display_support_provider.h'
--- include/test/mir_test_doubles/mock_display_support_provider.h	2013-09-16 19:16:30 +0000
+++ include/test/mir_test_doubles/mock_display_support_provider.h	2013-10-02 17:00:39 +0000
@@ -37,7 +37,7 @@
     MOCK_CONST_METHOD0(number_of_framebuffers_available, unsigned int());
     MOCK_METHOD1(set_next_frontbuffer, void(std::shared_ptr<mir::graphics::Buffer> const&));
     MOCK_METHOD1(sync_to_display, void(bool));
-    MOCK_METHOD1(blank_or_unblank_screen, void(bool));
+    MOCK_METHOD1(mode, void(MirPowerMode));
 };
 }
 }

=== modified file 'include/test/mir_test_doubles/mock_hwc_interface.h'
--- include/test/mir_test_doubles/mock_hwc_interface.h	2013-09-11 19:50:31 +0000
+++ include/test/mir_test_doubles/mock_hwc_interface.h	2013-10-02 17:00:39 +0000
@@ -41,7 +41,7 @@
     MOCK_CONST_METHOD0(number_of_framebuffers_available, unsigned int());
     MOCK_METHOD1(set_next_frontbuffer, void(std::shared_ptr<mir::graphics::Buffer> const&));
     MOCK_METHOD1(sync_to_display, void(bool));
-    MOCK_METHOD1(blank_or_unblank_screen, void(bool));
+    MOCK_METHOD1(mode, void(MirPowerMode));
 };
 
 }

=== modified file 'include/test/mir_test_doubles/stub_display_support_provider.h'
--- include/test/mir_test_doubles/stub_display_support_provider.h	2013-09-16 21:00:32 +0000
+++ include/test/mir_test_doubles/stub_display_support_provider.h	2013-10-02 17:00:39 +0000
@@ -36,7 +36,7 @@
     unsigned int number_of_framebuffers_available() const { return 0; }
     void set_next_frontbuffer(std::shared_ptr<mir::graphics::Buffer> const&) {}
     void sync_to_display(bool) {}
-    void blank_or_unblank_screen(bool) {}
+    void mode(MirPowerMode) {}
 };
 
 }

=== modified file 'src/server/graphics/android/android_display.cpp'
--- src/server/graphics/android/android_display.cpp	2013-09-17 19:46:15 +0000
+++ src/server/graphics/android/android_display.cpp	2013-10-02 17:00:39 +0000
@@ -156,13 +156,10 @@
 
 void mga::AndroidDisplay::configure(mg::DisplayConfiguration const& configuration)
 {
-    configuration.for_each_output([&](mg::DisplayConfigurationOutput const& output) -> void
+    configuration.for_each_output([&](mg::DisplayConfigurationOutput const& output)
     {
         // TODO: Properly support multiple outputs
-        if (output.power_mode == mir_power_mode_on)
-            display_provider->blank_or_unblank_screen(false);
-        else
-            display_provider->blank_or_unblank_screen(true);
+        display_provider->mode(output.power_mode);
     });
     current_configuration = dynamic_cast<mga::AndroidDisplayConfiguration const&>(configuration);
 }

=== modified file 'src/server/graphics/android/display_support_provider.h'
--- src/server/graphics/android/display_support_provider.h	2013-09-16 19:16:30 +0000
+++ src/server/graphics/android/display_support_provider.h	2013-10-02 17:00:39 +0000
@@ -19,6 +19,7 @@
 #ifndef MIR_GRAPHICS_ANDROID_DISPLAY_SUPPORT_PROVIDER_H_
 #define MIR_GRAPHICS_ANDROID_DISPLAY_SUPPORT_PROVIDER_H_
 
+#include "mir_toolkit/common.h"
 #include "mir/geometry/size.h"
 #include "mir/geometry/pixel_format.h"
 #include <memory>
@@ -44,8 +45,8 @@
     //post immediately, or be deferred.
     virtual void set_next_frontbuffer(std::shared_ptr<graphics::Buffer> const& buffer) = 0;
     virtual void sync_to_display(bool sync) = 0;
-    
-    virtual void blank_or_unblank_screen(bool blank) = 0;
+    virtual void mode(MirPowerMode mode) = 0;
+
 protected:
     DisplaySupportProvider() = default;
     DisplaySupportProvider& operator=(DisplaySupportProvider const&) = delete;

=== modified file 'src/server/graphics/android/fb_device.cpp'
--- src/server/graphics/android/fb_device.cpp	2013-09-16 19:16:30 +0000
+++ src/server/graphics/android/fb_device.cpp	2013-10-02 17:00:39 +0000
@@ -79,8 +79,8 @@
     }
 }
 
-void mga::FBDevice::blank_or_unblank_screen(bool blank)
+void mga::FBDevice::mode(MirPowerMode mode)
 {
     // TODO: Implement
-    (void) blank;
+    (void) mode;
 }

=== modified file 'src/server/graphics/android/fb_device.h'
--- src/server/graphics/android/fb_device.h	2013-09-16 19:16:30 +0000
+++ src/server/graphics/android/fb_device.h	2013-10-02 17:00:39 +0000
@@ -42,7 +42,7 @@
     void set_next_frontbuffer(std::shared_ptr<graphics::Buffer> const& buffer);
     void sync_to_display(bool sync);
     
-    void blank_or_unblank_screen(bool blank);
+    virtual void mode(MirPowerMode mode);
 private:
     std::shared_ptr<framebuffer_device_t> const fb_device;
 };

=== modified file 'src/server/graphics/android/hwc_common_device.cpp'
--- src/server/graphics/android/hwc_common_device.cpp	2013-09-26 18:17:11 +0000
+++ src/server/graphics/android/hwc_common_device.cpp	2013-10-02 17:00:39 +0000
@@ -47,7 +47,7 @@
                                       std::shared_ptr<mga::HWCVsyncCoordinator> const& coordinator)
     : hwc_device(hwc_device),
       coordinator(coordinator),
-      blanked(false)
+      current_mode(mir_power_mode_on)
 {
     callbacks.hooks.invalidate = invalidate_hook;
     callbacks.hooks.vsync = vsync_hook;
@@ -56,30 +56,20 @@
 
     hwc_device->registerProcs(hwc_device.get(), &callbacks.hooks);
 
-    int err = hwc_device->blank(hwc_device.get(), HWC_DISPLAY_PRIMARY, 0);
-    if (err)
+    if (auto err = turn_screen_on())
     {
         BOOST_THROW_EXCEPTION(
             boost::enable_error_info(
                 std::runtime_error("Could not unblank display")) <<
             boost::errinfo_errno(-err));
     }
-    
-    err = hwc_device->eventControl(hwc_device.get(), 0, HWC_EVENT_VSYNC, 1);
-    if (err)
-    {
-        BOOST_THROW_EXCEPTION(
-            boost::enable_error_info(
-                std::runtime_error("Could not enable HWC vsync "
-                                   "notifications")) <<
-            boost::errinfo_errno(-err));
-    }
 }
 
 mga::HWCCommonDevice::~HWCCommonDevice() noexcept
 {
-    hwc_device->eventControl(hwc_device.get(), 0, HWC_EVENT_VSYNC, 0);
-    hwc_device->blank(hwc_device.get(), HWC_DISPLAY_PRIMARY, 1);
+    std::unique_lock<std::mutex> lg(blanked_mutex);
+    if (current_mode == mir_power_mode_on)
+        turn_screen_off();
 }
 
 geom::PixelFormat mga::HWCCommonDevice::display_format() const
@@ -97,30 +87,62 @@
     coordinator->notify_vsync();
 }
 
-void mga::HWCCommonDevice::blank_or_unblank_screen(bool blank_request)
+void mga::HWCCommonDevice::mode(MirPowerMode mode_request)
 {
     std::unique_lock<std::mutex> lg(blanked_mutex);
-
-    if (blank_request != blanked)
-    {
-        if(auto err = hwc_device->blank(hwc_device.get(), HWC_DISPLAY_PRIMARY, blank_request))
-        {
-            std::string blanking_status_msg = "Could not " + 
-                (blank_request ? std::string("blank") : std::string("unblank")) + " display";
-            BOOST_THROW_EXCEPTION(
-                boost::enable_error_info(
-                    std::runtime_error(blanking_status_msg)) <<
-                boost::errinfo_errno(-err));
-        }    
-        blanked = blank_request;
-        blanked_cond.notify_all();
-    }
+    int err = 0;
+
+    //note: mir_power_mode_standby, mir_power_mode_suspend, mir_power_mode_off
+    //      are all treated like mir_power_mode_off
+    if ((mode_request == mir_power_mode_suspend) || 
+        (mode_request == mir_power_mode_standby))
+    {
+        mode_request = mir_power_mode_off;
+    }
+
+    if ((mode_request == mir_power_mode_on) && 
+        (current_mode == mir_power_mode_off))
+    {
+        err = turn_screen_on();
+    }
+    else if ((mode_request == mir_power_mode_off) && 
+             (current_mode == mir_power_mode_on))
+    {
+        err = turn_screen_off();
+    }
+
+    if (err)
+    {
+        std::string blanking_status_msg = "Could not " + 
+            ((mode_request == mir_power_mode_off) ? std::string("blank") : std::string("unblank")) + " display";
+        BOOST_THROW_EXCEPTION(
+            boost::enable_error_info(
+                std::runtime_error(blanking_status_msg)) <<
+            boost::errinfo_errno(-err));
+    }
+   
+    current_mode = mode_request; 
+    blanked_cond.notify_all();
 }
 
 std::unique_lock<std::mutex> mga::HWCCommonDevice::lock_unblanked()
 {
     std::unique_lock<std::mutex> lg(blanked_mutex);
-    while(blanked)
+    while(current_mode == mir_power_mode_off)
         blanked_cond.wait(lg);
     return std::move(lg);
 }
+
+int mga::HWCCommonDevice::turn_screen_on() const noexcept(true)
+{
+    if (auto err = hwc_device->blank(hwc_device.get(), HWC_DISPLAY_PRIMARY, 0))
+        return err;
+    return hwc_device->eventControl(hwc_device.get(), 0, HWC_EVENT_VSYNC, 1);
+}
+
+int mga::HWCCommonDevice::turn_screen_off() const noexcept(true)
+{
+    if (auto err = hwc_device->eventControl(hwc_device.get(), 0, HWC_EVENT_VSYNC, 0))
+        return err;
+    return hwc_device->blank(hwc_device.get(), HWC_DISPLAY_PRIMARY, 1);
+}

=== modified file 'src/server/graphics/android/hwc_common_device.h'
--- src/server/graphics/android/hwc_common_device.h	2013-09-18 16:08:14 +0000
+++ src/server/graphics/android/hwc_common_device.h	2013-10-02 17:00:39 +0000
@@ -53,7 +53,7 @@
     virtual void set_next_frontbuffer(std::shared_ptr<Buffer> const& buffer) = 0;
     virtual void commit_frame(EGLDisplay dpy, EGLSurface sur) = 0;
     
-    void blank_or_unblank_screen(bool blank);
+    virtual void mode(MirPowerMode mode);
 
     void notify_vsync();
 protected:
@@ -66,11 +66,14 @@
     std::unique_lock<std::mutex> lock_unblanked();
 
 private:
+    int turn_screen_on() const noexcept(true);
+    int turn_screen_off() const noexcept(true);
+
     HWCCallbacks callbacks;
 
     std::mutex blanked_mutex;
     std::condition_variable blanked_cond;
-    bool blanked;
+    MirPowerMode current_mode; 
 };
 
 }

=== modified file 'src/server/graphics/android/hwc_device.h'
--- src/server/graphics/android/hwc_device.h	2013-09-05 17:46:22 +0000
+++ src/server/graphics/android/hwc_device.h	2013-10-02 17:00:39 +0000
@@ -43,7 +43,7 @@
 
     virtual void commit_frame(EGLDisplay dpy, EGLSurface sur) = 0;
     
-    virtual void blank_or_unblank_screen(bool blank) = 0;
+    virtual void mode(MirPowerMode mode) = 0;
 private:
     HWCDevice(HWCDevice const&) = delete;
     HWCDevice& operator=(HWCDevice const&) = delete;

=== modified file 'tests/unit-tests/graphics/android/test_hwc_device.cpp'
--- tests/unit-tests/graphics/android/test_hwc_device.cpp	2013-09-26 18:17:11 +0000
+++ tests/unit-tests/graphics/android/test_hwc_device.cpp	2013-10-02 17:00:39 +0000
@@ -176,7 +176,7 @@
     }, std::runtime_error);
 }
 
-TYPED_TEST(HWCCommon, test_hwc_throws_on_blank_or_unblank_error)
+TYPED_TEST(HWCCommon, test_hwc_throws_on_blanking_error)
 {
     using namespace testing;
 
@@ -194,10 +194,52 @@
     auto device = make_hwc_device<TypeParam>(this->mock_device, this->mock_layer_list,
                                              this->mock_fbdev, this->mock_vsync);
     EXPECT_THROW({
-            device->blank_or_unblank_screen(true);
+        device->mode(mir_power_mode_off);
     }, std::runtime_error);
 }
 
+TYPED_TEST(HWCCommon, test_hwc_suspend_standby_turn_off)
+{
+    using namespace testing;
+
+    EXPECT_CALL(*this->mock_device, blank_interface(this->mock_device.get(), HWC_DISPLAY_PRIMARY, 0))
+        .Times(3)
+        .WillOnce(Return(0));
+    EXPECT_CALL(*this->mock_device, blank_interface(this->mock_device.get(), HWC_DISPLAY_PRIMARY, 1))
+        .Times(3)
+        .WillOnce(Return(0));
+
+    auto device = make_hwc_device<TypeParam>(this->mock_device, this->mock_layer_list,
+                                             this->mock_fbdev, this->mock_vsync);
+    device->mode(mir_power_mode_off);
+    device->mode(mir_power_mode_on);
+    device->mode(mir_power_mode_suspend);
+    device->mode(mir_power_mode_on);
+    device->mode(mir_power_mode_standby);
+}
+
+TYPED_TEST(HWCCommon, test_hwc_deactivates_vsync_on_blank)
+{
+    using namespace testing;
+
+    InSequence seq;
+    EXPECT_CALL(*this->mock_device, blank_interface(this->mock_device.get(), HWC_DISPLAY_PRIMARY, 0))
+        .Times(1)
+        .WillOnce(Return(0));
+    EXPECT_CALL(*this->mock_device, eventControl_interface(this->mock_device.get(), HWC_DISPLAY_PRIMARY, HWC_EVENT_VSYNC, 1))
+        .Times(1);
+
+    EXPECT_CALL(*this->mock_device, eventControl_interface(this->mock_device.get(), HWC_DISPLAY_PRIMARY, HWC_EVENT_VSYNC, 0))
+        .Times(1);
+    EXPECT_CALL(*this->mock_device, blank_interface(this->mock_device.get(), HWC_DISPLAY_PRIMARY, 1))
+        .Times(1)
+        .WillOnce(Return(0));
+
+    auto device = make_hwc_device<TypeParam>(this->mock_device, this->mock_layer_list,
+                                             this->mock_fbdev, this->mock_vsync);
+    device->mode(mir_power_mode_off);
+}
+
 TYPED_TEST(HWCCommon, test_blank_is_ignored_if_already_in_correct_state)
 {
     using namespace testing;
@@ -216,7 +258,7 @@
 
     auto device = make_hwc_device<TypeParam>(this->mock_device, this->mock_layer_list,
                                              this->mock_fbdev, this->mock_vsync);
-    device->blank_or_unblank_screen(false);
+    device->mode(mir_power_mode_on);
 }
 
 TYPED_TEST(HWCCommon, test_hwc_display_is_deactivated_on_destroy)

=== modified file 'tests/unit-tests/graphics/android/test_hwc_display.cpp'
--- tests/unit-tests/graphics/android/test_hwc_display.cpp	2013-09-16 19:16:30 +0000
+++ tests/unit-tests/graphics/android/test_hwc_display.cpp	2013-10-02 17:00:39 +0000
@@ -142,13 +142,13 @@
 
     {
         InSequence seq;
-        EXPECT_CALL(*mock_hwc_device, blank_or_unblank_screen(false));
-        EXPECT_CALL(*mock_hwc_device, blank_or_unblank_screen(true));
-        EXPECT_CALL(*mock_hwc_device, blank_or_unblank_screen(true));
-        EXPECT_CALL(*mock_hwc_device, blank_or_unblank_screen(true));
+        EXPECT_CALL(*mock_hwc_device, mode(mir_power_mode_on));
+        EXPECT_CALL(*mock_hwc_device, mode(mir_power_mode_off));
+        EXPECT_CALL(*mock_hwc_device, mode(mir_power_mode_suspend));
+        EXPECT_CALL(*mock_hwc_device, mode(mir_power_mode_standby));
     }
-    display->configure(*on_configuration.get());
-    display->configure(*off_configuration.get());
-    display->configure(*suspend_configuration.get());
-    display->configure(*standby_configuration.get());
+    display->configure(*on_configuration);
+    display->configure(*off_configuration);
+    display->configure(*suspend_configuration);
+    display->configure(*standby_configuration);
 }