Merge lp:~kdub/mir/fix-1274189 into lp:mir

[Kevin DuBois]

graphics: android: remove wait on retireFenceFd. On some platforms (specifically the 4.4-based flo and grouper), this wait was causing us to miss vsync.

fixes lp: #1274189

graphics: android: remove wait on retireFenceFd. On some platforms (specifically the 4.4-based flo and grouper), this wait was causing us to miss vsync.

top level summary:
retireFenceFd as described (line 296 https://android.googlesource.com/platform/hardware/libhardware/+/android-4.4.2_r1/include/hardware/hwcomposer.h ) says that the fence will signal when the composition has been replaced on screen by subsequent set. It seems that waiting for the last post to leave the screen is not necessarily the same thing as the just-called commit to get to the screen, resulting in missing vsync. hwc api guarantees that the post will get to the screen without artifacts, and we use fences more extensively now to guarantee no tears for the gpu or the fb.

Lastly, surfaceflinger doesn't wait for this fence for the physical display, rather it seems used to coordinate with the 'virtual' display (which mir doesn't implement yet). We still have the responsibility to close the fd, which we do in this MP.

historical note:
I think this code was put in when we weren't implementing fences correctly in the system, circa october 2013-ish. It didn't manifest itself as a problem until now because none of the drivers we were commonly running on would either 1) not wait here at all because the driver didn't set it, or 2) just wait here as opposed to waiting on the subsequent gpu render of the framebuffer.

fixes lp: #1274189

test procedure:
Since this affects display, was tested with the full stack manually to check for tears and the frame times were logged.

tested (no tears, ~16 ms frame time):
flo (android 4.4, hwc 1.2)
manta (android 4.4, hwc 1.1)
mako (android 4.4, hwc 1.2)
grouper (android 4.4, hwc 1.1)
mako (android 4.2, hwc 1.1)
manta (android 4.2, hwc 1.1)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1365
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/759/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/779
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/776
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/380
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/489
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/489/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/493
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/493/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/381
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/381/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/383
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/3544

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Seems to work for me. At least as well as before.

review: Approve

[Gerry Boland (gerboland)]

Big improvement on Nexus7(2013)/Flo

review: Approve (functional)

[Ricardo Salveti (rsalveti)]

Flo is really fast now, +1.

review: Approve

[Alan Griffiths (alan-griffiths)]

Code looks reasonable and there's enough approvals. Top approving.

=== modified file 'include/test/mir_test_doubles/mock_hwc_composer_device_1.h'
--- include/test/mir_test_doubles/mock_hwc_composer_device_1.h	2014-01-23 17:26:51 +0000
+++ include/test/mir_test_doubles/mock_hwc_composer_device_1.h	2014-01-31 18:37:40 +0000
@@ -76,6 +76,11 @@
         fb_fence = fence;
     }
 
+    void hwc_set_retire_fence(int fence)
+    {
+        retire_fence = fence;
+    }
+
     int save_last_prepare_arguments(struct hwc_composer_device_1 *, size_t size, hwc_display_contents_1_t** displays)
     {
         if ((size == 0) || (!displays)) 
@@ -122,12 +127,14 @@
                     set_layerlist.back().visibleRegionScreen = {0, nullptr};
                 }
 
+                save_args(&display0_set_content, displays);
+
                 if (displays[0]->numHwLayers >= 2)
                 {
                     displays[0]->hwLayers[1].releaseFenceFd = fb_fence;
+                    displays[0]->retireFenceFd = retire_fence;
                 }
 
-                save_args(&display0_set_content, displays);
             default:
                 break;
         }
@@ -186,6 +193,7 @@
     std::vector<hwc_layer_1> prepare_layerlist;
     hwc_display_contents_1_t display0_prepare_content;
     int fb_fence;
+    int retire_fence;
 };
 
 }

=== modified file 'src/platform/graphics/android/hwc_device.cpp'
--- src/platform/graphics/android/hwc_device.cpp	2014-01-23 18:01:19 +0000
+++ src/platform/graphics/android/hwc_device.cpp	2014-01-31 18:37:40 +0000
@@ -35,10 +35,11 @@
 namespace geom = mir::geometry;
 
 mga::HwcDevice::HwcDevice(std::shared_ptr<hwc_composer_device_1> const& hwc_device,
-                              std::shared_ptr<HWCVsyncCoordinator> const& coordinator)
+                          std::shared_ptr<HWCVsyncCoordinator> const& coordinator,
+                          std::shared_ptr<SyncFileOps> const& sync_ops)
     : HWCCommonDevice(hwc_device, coordinator),
       layer_list({mga::ForceGLLayer{}, mga::FramebufferLayer{}}),
-      sync_ops(std::make_shared<mga::RealSyncFileOps>())
+      sync_ops(sync_ops)
 {
 }
 
@@ -79,12 +80,8 @@
         BOOST_THROW_EXCEPTION(std::runtime_error("error during hwc set()"));
     }
 
-    if (last_display_fence)
-        last_display_fence->wait();
+    mga::SyncFence retire_fence(sync_ops, displays[HWC_DISPLAY_PRIMARY]->retireFenceFd);
 
     int framebuffer_fence = layer_list.framebuffer_fence();
     native_buffer->update_fence(framebuffer_fence);
-
-    last_display_fence = std::make_shared<mga::SyncFence>(
-        sync_ops, displays[HWC_DISPLAY_PRIMARY]->retireFenceFd);
 }

=== modified file 'src/platform/graphics/android/hwc_device.h'
--- src/platform/graphics/android/hwc_device.h	2014-01-23 18:01:19 +0000
+++ src/platform/graphics/android/hwc_device.h	2014-01-31 18:37:40 +0000
@@ -34,13 +34,13 @@
 {
 class HWCVsyncCoordinator;
 class SyncFileOps;
-class SyncFence;
 
 class HwcDevice : public HWCCommonDevice
 {
 public:
     HwcDevice(std::shared_ptr<hwc_composer_device_1> const& hwc_device,
-                std::shared_ptr<HWCVsyncCoordinator> const& coordinator);
+              std::shared_ptr<HWCVsyncCoordinator> const& coordinator,
+              std::shared_ptr<SyncFileOps> const& sync_ops);
 
     void prepare_gl();
     void prepare_gl_and_overlays(std::list<std::shared_ptr<Renderable>> const& list); 
@@ -50,7 +50,6 @@
 private:
     LayerList layer_list;
 
-    std::shared_ptr<SyncFence> last_display_fence;
     std::shared_ptr<SyncFileOps> const sync_ops;
     unsigned int primary_display_config;
     MirPixelFormat fb_format;

=== modified file 'src/platform/graphics/android/resource_factory.cpp'
--- src/platform/graphics/android/resource_factory.cpp	2014-01-21 18:09:35 +0000
+++ src/platform/graphics/android/resource_factory.cpp	2014-01-31 18:37:40 +0000
@@ -18,6 +18,7 @@
  */
 
 #include "mir/graphics/android/mir_native_window.h"
+#include "mir/graphics/android/sync_fence.h"
 #include "buffer.h"
 #include "resource_factory.h"
 #include "fb_device.h"
@@ -91,7 +92,8 @@
     std::shared_ptr<hwc_composer_device_1> const& hwc_native_device) const
 {
     auto syncer = std::make_shared<mga::HWCVsync>();
-    return std::make_shared<mga::HwcDevice>(hwc_native_device, syncer);
+    auto file_ops = std::make_shared<mga::RealSyncFileOps>();
+    return std::make_shared<mga::HwcDevice>(hwc_native_device, syncer, file_ops);
 }
 
 std::shared_ptr<mga::DisplayDevice> mga::ResourceFactory::create_hwc_fb_device(

=== modified file 'tests/unit-tests/graphics/android/test_hwc_common_device.cpp'
--- tests/unit-tests/graphics/android/test_hwc_common_device.cpp	2014-01-21 18:09:35 +0000
+++ tests/unit-tests/graphics/android/test_hwc_common_device.cpp	2014-01-31 18:37:40 +0000
@@ -16,6 +16,7 @@
  * Authored by: Kevin DuBois <[e-mail]>
  */
 
+#include "mir/graphics/android/sync_fence.h"
 #include "src/platform/graphics/android/hwc_fb_device.h"
 #include "src/platform/graphics/android/hwc_device.h"
 #include "src/platform/graphics/android/hwc_layerlist.h"
@@ -58,7 +59,8 @@
     std::shared_ptr<framebuffer_device_t> const&,
     std::shared_ptr<mga::HWCVsyncCoordinator> const& coordinator)
 {
-    return std::make_shared<mga::HwcDevice>(hwc_device, coordinator);
+    auto file_ops = std::make_shared<mga::RealSyncFileOps>();
+    return std::make_shared<mga::HwcDevice>(hwc_device, coordinator, file_ops);
 }
 
 template<typename T>

=== modified file 'tests/unit-tests/graphics/android/test_hwc_device.cpp'
--- tests/unit-tests/graphics/android/test_hwc_device.cpp	2014-01-23 17:26:51 +0000
+++ tests/unit-tests/graphics/android/test_hwc_device.cpp	2014-01-31 18:37:40 +0000
@@ -16,6 +16,7 @@
  * Authored by: Kevin DuBois <[e-mail]>
  */
 
+#include "mir/graphics/android/sync_fence.h"
 #include "src/platform/graphics/android/framebuffer_bundle.h"
 #include "src/platform/graphics/android/hwc_device.h"
 #include "src/platform/graphics/android/hwc_layerlist.h"
@@ -35,6 +36,16 @@
 namespace mtd=mir::test::doubles;
 namespace geom=mir::geometry;
 
+namespace
+{
+struct MockFileOps : public mga::SyncFileOps
+{
+    MOCK_METHOD3(ioctl, int(int,int,void*));
+    MOCK_METHOD1(dup, int(int));
+    MOCK_METHOD1(close, int(int));
+};
+}
+
 class HwcDevice : public ::testing::Test
 {
 protected:
@@ -48,10 +59,13 @@
         mock_buffer = std::make_shared<testing::NiceMock<mtd::MockBuffer>>();
         mock_device = std::make_shared<testing::NiceMock<mtd::MockHWCComposerDevice1>>();
         mock_vsync = std::make_shared<testing::NiceMock<mtd::MockVsyncCoordinator>>();
+        mock_file_ops = std::make_shared<MockFileOps>();
 
         ON_CALL(*mock_buffer, native_buffer_handle())
             .WillByDefault(Return(mock_native_buffer));
     }
+
+    std::shared_ptr<MockFileOps> mock_file_ops;
     std::shared_ptr<mtd::MockVsyncCoordinator> mock_vsync;
     std::shared_ptr<mtd::MockHWCComposerDevice1> mock_device;
     std::shared_ptr<mtd::MockAndroidNativeBuffer> mock_native_buffer;
@@ -69,7 +83,7 @@
     EXPECT_CALL(*mock_device, set_interface(mock_device.get(),_,_))
         .Times(1);
 
-    mga::HwcDevice device(mock_device, mock_vsync);
+    mga::HwcDevice device(mock_device, mock_vsync, mock_file_ops);
     device.prepare_gl();
     device.post(*mock_buffer);
 
@@ -90,7 +104,7 @@
     EXPECT_CALL(*mock_device, prepare_interface(mock_device.get(), 1, _))
         .Times(1);
 
-    mga::HwcDevice device(mock_device, mock_vsync);
+    mga::HwcDevice device(mock_device, mock_vsync, mock_file_ops);
     device.prepare_gl();
     EXPECT_EQ(2, mock_device->display0_prepare_content.numHwLayers);
     EXPECT_EQ(-1, mock_device->display0_prepare_content.retireFenceFd);
@@ -102,7 +116,7 @@
     EXPECT_CALL(*mock_device, prepare_interface(mock_device.get(), 1, _))
         .Times(1);
 
-    mga::HwcDevice device(mock_device, mock_vsync);
+    mga::HwcDevice device(mock_device, mock_vsync, mock_file_ops);
     std::list<std::shared_ptr<mg::Renderable>> renderlist;
     device.prepare_gl_and_overlays(renderlist);
 
@@ -114,7 +128,7 @@
 {
     EXPECT_CALL(mock_egl, eglSwapBuffers(dpy,surf))
         .Times(1);
-    mga::HwcDevice device(mock_device, mock_vsync);
+    mga::HwcDevice device(mock_device, mock_vsync, mock_file_ops);
     device.gpu_render(dpy, surf);
 }
 
@@ -125,7 +139,7 @@
         .Times(1)
         .WillOnce(Return(EGL_FALSE));
 
-    mga::HwcDevice device(mock_device, mock_vsync);
+    mga::HwcDevice device(mock_device, mock_vsync, mock_file_ops);
 
     EXPECT_THROW({
         device.gpu_render(dpy, surf);
@@ -136,15 +150,19 @@
 {
     using namespace testing;
     int hwc_return_fence = 94;
+    int hwc_retire_fence = 74;
     mock_device->hwc_set_return_fence(hwc_return_fence);
+    mock_device->hwc_set_retire_fence(hwc_retire_fence);
 
-    mga::HwcDevice device(mock_device, mock_vsync);
+    mga::HwcDevice device(mock_device, mock_vsync, mock_file_ops);
 
     InSequence seq;
     EXPECT_CALL(*mock_device, set_interface(mock_device.get(), 1, _))
         .Times(1);
     EXPECT_CALL(*mock_native_buffer, update_fence(hwc_return_fence))
         .Times(1);
+    EXPECT_CALL(*mock_file_ops, close(hwc_retire_fence))
+        .Times(1);
 
     device.post(*mock_buffer);
 
@@ -161,7 +179,7 @@
 {
     using namespace testing;
 
-    mga::HwcDevice device(mock_device, mock_vsync);
+    mga::HwcDevice device(mock_device, mock_vsync, mock_file_ops);
 
     EXPECT_CALL(*mock_device, set_interface(mock_device.get(), _, _))
         .Times(1)