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

[Kevin DuBois]

android: explicitly wait for the synchronization fence before the framebuffer EGL context is posted in hwc::set() for some mali-based devices.

fixes: lp: #1270245

android: explicitly wait for the synchronization fence before the framebuffer EGL context is posted in hwc::set() for some mali-based devices.

note: this branch, in tandem with https://code.launchpad.net/~kdub/mir/egl-sync-fences/+merge/278181 should fix the software corruption on the mali devices. Running this should fix the egltriangle skipping back, but you could still see corruption from lp: #1517205 with this branch.

fixes: lp: #1270245

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3126
http://jenkins.qa.ubuntu.com/job/mir-ci/5657/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5017
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3923
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4964/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/920/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1809
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1809/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/917
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/917/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4964
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4964/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7506/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/25470
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/922/console

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

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

I am not able to reproduce the original bug locally to check if the fix works properly, but the code looks sensible.

+ if (clear_fence)
+ mga::SyncFence(std::make_shared<RealSyncFileOps>(), mir::Fd(fence_fd)).wait();

Since RealSyncFileOps is a stateless object we could instantiate it once as a member variable and reuse it to avoid unnecessary memory allocation on every frame. Of course there is still mir::Fd and probably many other objects we are allocating dynamically on every frame, but let's cut where we can.

review: Approve

[Andreas Pokorny (andreas-pokorny)]

why mx4? Shouldnt that be nexus10? Or is that problem not related to the gpu but the display controller itself?

review: Needs Information

[Alan Griffiths (alan-griffiths)]

+ return device_name == std::string{"krillin"} || device_name == std::string{"mx4"};

I know there's existing code in this style, but it seem unnecessary to construct temporary string objects for these constants. As std::string has an operator==(std::string const&, const CharT*) the same test can be written more efficiently:

    return device_name == "krillin" || device_name == "mx4";

review: Abstain

[Kevin DuBois (kdub)]

@N10, This device isn't supported anymore by mir, and I haven't verified that this fix works for the device. It might be better to detect the device class eventually (eg, mali) and autodetect the workaround based on that.

Other issues fixed.

[Andreas Pokorny (andreas-pokorny)]

Confused - I tried your branch ..

1) without a || device_name == "manta" :
 corruption in mir_demo_client_flicker
 strange inconsistent updates in fingerpaint .. always somwehere on the right side of the buffer
 no jumps or flickering in egltriangle
2) with an additional || device_name "manta"
 no corruption in mir_demo_client_flicker
 no update problems in fingerpaint!
 no jumps or flickering in egltriangle

It seems to fix lp:1517205

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3128
http://jenkins.qa.ubuntu.com/job/mir-ci/5716/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5102
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4008
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5050
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/50/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/45
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/45/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/45
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/45/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5050
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5050/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7585
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/25628
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/48/console

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

review: Approve (continuous-integration)

[Andreas Pokorny (andreas-pokorny)]

> @N10, This device isn't supported anymore by mir, and I haven't verified that
> this fix works for the device. It might be better to detect the device class
> eventually (eg, mali) and autodetect the workaround based on that.
>
> Other issues fixed.

can we add manta here? But the problem is not limited to mali since mx4 has the quirk?

[Kevin DuBois (kdub)]

@Andreas, thanks for verifying, will add the n10 as well

[Andreas Pokorny (andreas-pokorny)]

thx!

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3131
http://jenkins.qa.ubuntu.com/job/mir-ci/5725/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5113
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4019
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5061
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/54
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/54/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/54/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5061
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5061/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7595
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/25648

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/platforms/android/server/device_quirks.cpp'
--- src/platforms/android/server/device_quirks.cpp	2015-11-25 12:59:13 +0000
+++ src/platforms/android/server/device_quirks.cpp	2015-11-30 18:34:59 +0000
@@ -49,7 +49,7 @@
 
 unsigned int num_framebuffers_for(std::string const& device_name, bool quirk_enabled)
 {
-    if (quirk_enabled && device_name == std::string{"mx3"})
+    if (quirk_enabled && device_name == "mx3")
         return 3;
     else
         return 2;
@@ -57,15 +57,22 @@
 
 bool gralloc_cannot_be_closed_safely_for(std::string const& device_name, bool quirk_enabled)
 {
-    return quirk_enabled && device_name == std::string{"krillin"};
-}
+    return quirk_enabled && device_name == "krillin";
+}
+
+bool clear_fb_context_fence_for(std::string const& device_name)
+{
+    return device_name == "krillin" || device_name == "mx4" || device_name == "manta";
+}
+
 }
 
 mga::DeviceQuirks::DeviceQuirks(PropertiesWrapper const& properties)
     : device_name(determine_device_name(properties)),
       num_framebuffers_(num_framebuffers_for(device_name, true)),
       gralloc_cannot_be_closed_safely_(gralloc_cannot_be_closed_safely_for(device_name, true)),
-      enable_width_alignment_quirk{true}
+      enable_width_alignment_quirk{true},
+      clear_fb_context_fence_{clear_fb_context_fence_for(device_name)}
 {
 }
 
@@ -73,7 +80,8 @@
     : device_name(determine_device_name(properties)),
       num_framebuffers_(num_framebuffers_for(device_name, options.get(num_framebuffers_opt, true))),
       gralloc_cannot_be_closed_safely_(gralloc_cannot_be_closed_safely_for(device_name, options.get(gralloc_cannot_be_closed_safely_opt, true))),
-      enable_width_alignment_quirk(options.get(width_alignment_opt, true))
+      enable_width_alignment_quirk(options.get(width_alignment_opt, true)),
+      clear_fb_context_fence_{clear_fb_context_fence_for(device_name)}
 {
 }
 
@@ -89,11 +97,16 @@
 
 int mga::DeviceQuirks::aligned_width(int width) const
 {
-    if (enable_width_alignment_quirk && width == 720 && device_name == std::string{"vegetahd"})
+    if (enable_width_alignment_quirk && width == 720 && device_name == "vegetahd")
         return 736;
     return width;
 }
 
+bool mga::DeviceQuirks::clear_fb_context_fence() const
+{
+    return clear_fb_context_fence_;
+}
+
 void mga::DeviceQuirks::add_options(boost::program_options::options_description& config)
 {
     config.add_options()

=== modified file 'src/platforms/android/server/device_quirks.h'
--- src/platforms/android/server/device_quirks.h	2015-11-25 12:59:13 +0000
+++ src/platforms/android/server/device_quirks.h	2015-11-30 18:34:59 +0000
@@ -63,6 +63,7 @@
     unsigned int num_framebuffers() const;
     bool gralloc_cannot_be_closed_safely() const;
     int aligned_width(int width) const;
+    bool clear_fb_context_fence() const;
 
     static void add_options(boost::program_options::options_description& config);
 
@@ -73,6 +74,7 @@
     unsigned int const num_framebuffers_;
     bool const gralloc_cannot_be_closed_safely_;
     bool const enable_width_alignment_quirk;
+    bool const clear_fb_context_fence_;
 };
 }
 }

=== modified file 'src/platforms/android/server/display.cpp'
--- src/platforms/android/server/display.cpp	2015-11-16 10:52:06 +0000
+++ src/platforms/android/server/display.cpp	2015-11-30 18:34:59 +0000
@@ -119,7 +119,8 @@
 {
     std::shared_ptr<mga::FramebufferBundle> fbs{display_buffer_builder.create_framebuffers(config)};
     auto cache = std::make_shared<mga::InterpreterCache>();
-    auto interpreter = std::make_shared<mga::ServerRenderWindow>(fbs, config.current_format, cache);
+    mga::DeviceQuirks quirks(mga::PropertiesOps{});
+    auto interpreter = std::make_shared<mga::ServerRenderWindow>(fbs, config.current_format, cache, quirks); 
     auto native_window = std::make_shared<mga::MirNativeWindow>(interpreter);
     return std::unique_ptr<mga::ConfigurableDisplayBuffer>(new mga::DisplayBuffer(
         name,

=== modified file 'src/platforms/android/server/server_render_window.cpp'
--- src/platforms/android/server/server_render_window.cpp	2015-09-04 02:42:45 +0000
+++ src/platforms/android/server/server_render_window.cpp	2015-11-30 18:34:59 +0000
@@ -37,10 +37,12 @@
 mga::ServerRenderWindow::ServerRenderWindow(
     std::shared_ptr<mga::FramebufferBundle> const& fb_bundle,
     MirPixelFormat format,
-    std::shared_ptr<InterpreterResourceCache> const& cache)
+    std::shared_ptr<InterpreterResourceCache> const& cache,
+    DeviceQuirks& quirks)
     : fb_bundle(fb_bundle),
       resource_cache(cache),
-      format(mga::to_android_format(format))
+      format(mga::to_android_format(format)),
+      clear_fence(quirks.clear_fb_context_fence())
 {
 }
 
@@ -54,7 +56,13 @@
 
 void mga::ServerRenderWindow::driver_returns_buffer(ANativeWindowBuffer* buffer, int fence_fd)
 {
-    resource_cache->update_native_fence(buffer, fence_fd);
+    //depending on the quirk, some mali drivers won't synchronize the fb context fence before posting.
+    //if this bug is present, we synchronize here to avoid tearing or other artifacts.
+    if (clear_fence)
+        mga::SyncFence(std::make_shared<RealSyncFileOps>(), mir::Fd(fence_fd)).wait();
+    else
+        resource_cache->update_native_fence(buffer, fence_fd);
+
     resource_cache->retrieve_buffer(buffer);
 }
 

=== modified file 'src/platforms/android/server/server_render_window.h'
--- src/platforms/android/server/server_render_window.h	2015-06-17 05:20:42 +0000
+++ src/platforms/android/server/server_render_window.h	2015-11-30 18:34:59 +0000
@@ -21,6 +21,7 @@
 #define MIR_GRAPHICS_ANDROID_SERVER_RENDER_WINDOW_H_
 
 #include "mir/graphics/android/android_driver_interpreter.h"
+#include "device_quirks.h"
 #include "mir_toolkit/common.h"
 
 #include <memory>
@@ -39,7 +40,8 @@
 public:
     ServerRenderWindow(std::shared_ptr<FramebufferBundle> const& fb_bundle,
                        MirPixelFormat format,
-                       std::shared_ptr<InterpreterResourceCache> const&);
+                       std::shared_ptr<InterpreterResourceCache> const&,
+                       DeviceQuirks& quirks);
 
     graphics::NativeBuffer* driver_requests_buffer() override;
     void driver_returns_buffer(ANativeWindowBuffer*, int fence_fd) override;
@@ -52,6 +54,7 @@
     std::shared_ptr<FramebufferBundle> const fb_bundle;
     std::shared_ptr<InterpreterResourceCache> const resource_cache;
     int format;
+    bool const clear_fence;
 };
 
 }

=== modified file 'tests/unit-tests/graphics/android/test_device_detection.cpp'
--- tests/unit-tests/graphics/android/test_device_detection.cpp	2015-11-25 12:59:13 +0000
+++ tests/unit-tests/graphics/android/test_device_detection.cpp	2015-11-30 18:34:59 +0000
@@ -132,6 +132,82 @@
     EXPECT_THAT(quirks.aligned_width(720), Eq(736));
 }
 
+TEST(DeviceDetection, clears_gl_context_fence_on_manta)
+{
+    using namespace testing;
+    char const default_str[] = "";
+    char const mx4_name_str[] = "manta";
+
+    MockOps mock_ops;
+    EXPECT_CALL(mock_ops, property_get(StrEq("ro.product.device"), _, StrEq(default_str)))
+        .Times(1)
+        .WillOnce(Invoke([&](char const*, char* value, char const*)
+        {
+            strncpy(value, mx4_name_str, PROP_VALUE_MAX);
+            return 0;
+        }));
+
+    mga::DeviceQuirks quirks(mock_ops);
+    EXPECT_TRUE(quirks.clear_fb_context_fence());
+}
+
+TEST(DeviceDetection, clears_gl_context_fence_on_mx4)
+{
+    using namespace testing;
+    char const default_str[] = "";
+    char const mx4_name_str[] = "mx4";
+
+    MockOps mock_ops;
+    EXPECT_CALL(mock_ops, property_get(StrEq("ro.product.device"), _, StrEq(default_str)))
+        .Times(1)
+        .WillOnce(Invoke([&](char const*, char* value, char const*)
+        {
+            strncpy(value, mx4_name_str, PROP_VALUE_MAX);
+            return 0;
+        }));
+
+    mga::DeviceQuirks quirks(mock_ops);
+    EXPECT_TRUE(quirks.clear_fb_context_fence());
+}
+
+TEST(DeviceDetection, clears_gl_context_fence_on_krillin)
+{
+    using namespace testing;
+    char const default_str[] = "";
+    char const krillin_name_str[] = "krillin";
+
+    MockOps mock_ops;
+    EXPECT_CALL(mock_ops, property_get(StrEq("ro.product.device"), _, StrEq(default_str)))
+        .Times(1)
+        .WillOnce(Invoke([&](char const*, char* value, char const*)
+        {
+            strncpy(value, krillin_name_str, PROP_VALUE_MAX);
+            return 0;
+        }));
+
+    mga::DeviceQuirks quirks(mock_ops);
+    EXPECT_TRUE(quirks.clear_fb_context_fence());
+}
+
+TEST(DeviceDetection, does_not_clear_gl_context_fence_on_others)
+{
+    using namespace testing;
+    char const default_str[] = "";
+    char const mx4_name_str[] = "others";
+
+    MockOps mock_ops;
+    EXPECT_CALL(mock_ops, property_get(StrEq("ro.product.device"), _, StrEq(default_str)))
+        .Times(1)
+        .WillOnce(Invoke([&](char const*, char* value, char const*)
+        {
+            strncpy(value, mx4_name_str, PROP_VALUE_MAX);
+            return 0;
+        }));
+
+    mga::DeviceQuirks quirks(mock_ops);
+    EXPECT_FALSE(quirks.clear_fb_context_fence());
+}
+
 struct DeviceQuirks : testing::Test
 {
     void SetUp()

=== modified file 'tests/unit-tests/graphics/android/test_server_interpreter.cpp'
--- tests/unit-tests/graphics/android/test_server_interpreter.cpp	2015-09-04 02:42:45 +0000
+++ tests/unit-tests/graphics/android/test_server_interpreter.cpp	2015-11-30 18:34:59 +0000
@@ -39,6 +39,26 @@
 
 namespace
 {
+//krillin and mx4 need to clear their fences before hwc commit.
+struct StubPropertiesWrapper : mga::PropertiesWrapper
+{
+    StubPropertiesWrapper(bool should_clear_fence) :
+        name(should_clear_fence ? "mx4" : "otherdevice")
+    {
+    }
+
+    int property_get(char const* key, char* value, char const* default_value) const override
+    {
+        if (strncmp(key, "ro.product.device", PROP_VALUE_MAX) == 0)
+            strncpy(value, name.c_str(), name.size());
+        else
+            strncpy(value, default_value, PROP_VALUE_MAX);
+        return 0;    
+    }
+
+    std::string name;
+};
+
 struct ServerRenderWindow : public ::testing::Test
 {
     std::shared_ptr<mtd::MockBuffer> mock_buffer{std::make_shared<testing::NiceMock<mtd::MockBuffer>>()};
@@ -47,7 +67,9 @@
     std::shared_ptr<mtd::MockFBBundle> mock_fb_bundle{
         std::make_shared<testing::NiceMock<mtd::MockFBBundle>>()};
     MirPixelFormat format{mir_pixel_format_abgr_8888};
-    mga::ServerRenderWindow render_window{mock_fb_bundle, format, mock_cache};
+    StubPropertiesWrapper wrapper{false};
+    mga::DeviceQuirks quirks{wrapper};
+    mga::ServerRenderWindow render_window{mock_fb_bundle, format, mock_cache, quirks};
 };
 }
 
@@ -92,6 +114,34 @@
     Mock::VerifyAndClearExpectations(mock_fb_bundle.get());
 }
 
+TEST_F(ServerRenderWindow, clears_fence_when_quirk_present)
+{
+    using namespace testing;
+    StubPropertiesWrapper wrapper{true};
+    mga::DeviceQuirks quirks{wrapper};
+    mga::ServerRenderWindow render_window{mock_fb_bundle, format, mock_cache, quirks};
+
+    int fake_fence = 488;
+    auto stub_buffer = std::make_shared<mtd::StubAndroidNativeBuffer>();
+
+    EXPECT_CALL(*mock_fb_bundle, buffer_for_render())
+        .WillOnce(Return(mock_buffer));
+    EXPECT_CALL(*mock_buffer, native_buffer_handle())
+        .WillOnce(Return(stub_buffer));
+
+    render_window.driver_requests_buffer();
+    Mock::VerifyAndClearExpectations(mock_fb_bundle.get());
+
+    std::shared_ptr<mg::Buffer> buf1 = mock_buffer;
+    EXPECT_CALL(*mock_cache, update_native_fence(stub_buffer->anwb(), fake_fence))
+        .Times(0);
+    EXPECT_CALL(*mock_cache, retrieve_buffer(stub_buffer->anwb()))
+        .WillOnce(Return(mock_buffer));
+
+    render_window.driver_returns_buffer(stub_buffer->anwb(), fake_fence);
+    Mock::VerifyAndClearExpectations(mock_fb_bundle.get());
+}
+
 TEST_F(ServerRenderWindow, returns_format)
 {
     EXPECT_EQ(HAL_PIXEL_FORMAT_RGBA_8888, render_window.driver_requests_info(NATIVE_WINDOW_FORMAT));