Merge lp:~afrantzis/mir/android-gralloc-can-be-safely-closed-quirk into lp:mir

[Alexandros Frantzis]

android: Don't close gralloc if it's not safe to do so on particular devices

android: Don't close gralloc if it's not safe to do so on particular devices

Based on the success of the experimental branch [1], this branch applies the same workaround (don't close gralloc) as a quirk for krillin.

[1] See https://code.launchpad.net/~afrantzis/mir/ci-failure-experiment-1/+merge/278344
and also additional manually triggered runs of the test runner which were all successful: https://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7508/
https://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7509/
https://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7510/

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3123
http://jenkins.qa.ubuntu.com/job/mir-ci/5660/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5020/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3926/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4967/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/923/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1812/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/920/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4967/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/924/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3124
http://jenkins.qa.ubuntu.com/job/mir-ci/5661/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5021
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3927
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4968
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/924/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1813
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1813/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/921
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/921/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4968
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4968/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7512
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/25488
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/925/console

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I'd prefer more optimistic phrasing than "gralloc_can_be_safely_closed()". Vis: "gralloc_cannot_be_closed_safely()" but if it works...

review: Approve

[Kevin DuBois (kdub)]

I'm ambivalent about the name. Its unfortunate we need a quirk like this, but barring support from the driver vendor, its the best we can do.

review: Approve

[Andreas Pokorny (andreas-pokorny)]

no strong opinion on either of the names (negation vs api-intentions)

review: Approve

[Alexandros Frantzis (afrantzis)]

> Vis: "gralloc_cannot_be_closed_safely()" but if it works...

Renamed.

I also scheduled some manual runner jobs, which were successful:

https://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7513/
https://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7514/
https://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7515/

[Alberto Aguirre (albaguirre)]

I wonder if well need this on vegetahd as well...

=== modified file 'src/platforms/android/server/android_buffer_allocator.cpp'
--- src/platforms/android/server/android_buffer_allocator.cpp	2015-08-07 15:55:22 +0000
+++ src/platforms/android/server/android_buffer_allocator.cpp	2015-11-25 12:59:55 +0000
@@ -25,6 +25,7 @@
 #include "android_graphic_buffer_allocator.h"
 #include "android_alloc_adaptor.h"
 #include "buffer.h"
+#include "device_quirks.h"
 
 #include <boost/throw_exception.hpp>
 
@@ -36,14 +37,17 @@
 
 namespace
 {
-struct AllocDevDeleter
-{
-    void operator()(alloc_device_t* t)
-    {
-        /* android takes care of delete for us */
-        t->common.close((hw_device_t*)t);
-    }
-};
+
+void alloc_dev_deleter(alloc_device_t* t)
+{
+    /* android takes care of delete for us */
+    t->common.close((hw_device_t*)t);
+}
+
+void null_alloc_dev_deleter(alloc_device_t*)
+{
+}
+
 }
 
 mga::AndroidGraphicBufferAllocator::AndroidGraphicBufferAllocator(std::shared_ptr<DeviceQuirks> const& quirks)
@@ -63,8 +67,9 @@
     /* note for future use: at this point, the hardware module should be filled with vendor information
        that we can determine different courses of action based upon */
 
-    AllocDevDeleter del;
-    std::shared_ptr<struct alloc_device_t> alloc_dev_ptr(alloc_dev, del);
+    std::shared_ptr<struct alloc_device_t> alloc_dev_ptr(
+        alloc_dev,
+        quirks->gralloc_cannot_be_closed_safely() ? null_alloc_dev_deleter : alloc_dev_deleter);
     alloc_device = std::shared_ptr<mga::GraphicAllocAdaptor>(new AndroidAllocAdaptor(alloc_dev_ptr, quirks));
 }
 

=== modified file 'src/platforms/android/server/device_quirks.cpp'
--- src/platforms/android/server/device_quirks.cpp	2015-06-24 11:33:02 +0000
+++ src/platforms/android/server/device_quirks.cpp	2015-11-25 12:59:55 +0000
@@ -35,7 +35,7 @@
 namespace
 {
 char const* const num_framebuffers_opt = "enable-num-framebuffers-quirk";
-char const* const single_gralloc_instance_opt = "enable-single-gralloc-instance-quirk";
+char const* const gralloc_cannot_be_closed_safely_opt = "enable-gralloc-cannot-be-closed-safely-quirk";
 char const* const width_alignment_opt = "enable-width-alignment-quirk";
 
 std::string determine_device_name(mga::PropertiesWrapper const& properties)
@@ -55,16 +55,16 @@
         return 2;
 }
 
-unsigned int gralloc_reopenable_after_close_for(std::string const& device_name, bool quirk_enabled)
+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 == std::string{"krillin"};
 }
 }
 
 mga::DeviceQuirks::DeviceQuirks(PropertiesWrapper const& properties)
     : device_name(determine_device_name(properties)),
       num_framebuffers_(num_framebuffers_for(device_name, true)),
-      gralloc_reopenable_after_close_(gralloc_reopenable_after_close_for(device_name, true)),
+      gralloc_cannot_be_closed_safely_(gralloc_cannot_be_closed_safely_for(device_name, true)),
       enable_width_alignment_quirk{true}
 {
 }
@@ -72,7 +72,7 @@
 mga::DeviceQuirks::DeviceQuirks(PropertiesWrapper const& properties, mo::Option const& options)
     : device_name(determine_device_name(properties)),
       num_framebuffers_(num_framebuffers_for(device_name, options.get(num_framebuffers_opt, true))),
-      gralloc_reopenable_after_close_(gralloc_reopenable_after_close_for(device_name, options.get(single_gralloc_instance_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))
 {
 }
@@ -82,9 +82,9 @@
     return num_framebuffers_;
 }
 
-bool mga::DeviceQuirks::gralloc_reopenable_after_close() const
+bool mga::DeviceQuirks::gralloc_cannot_be_closed_safely() const
 {
-    return gralloc_reopenable_after_close_;
+    return gralloc_cannot_be_closed_safely_;
 }
 
 int mga::DeviceQuirks::aligned_width(int width) const
@@ -100,9 +100,9 @@
         (num_framebuffers_opt,
          boost::program_options::value<bool>()->default_value(true),
          "[platform-specific] Enable allocating 3 framebuffers (MX3 quirk) [{true,false}]")
-        (single_gralloc_instance_opt,
+        (gralloc_cannot_be_closed_safely_opt,
          boost::program_options::value<bool>()->default_value(true),
-         "[platform-specific] Allocate a single gralloc instance (krillin quirk)  [{true,false}]")
+         "[platform-specific] Only close gralloc if it is safe to do so (krillin quirk)  [{true,false}]")
          (width_alignment_opt,
           boost::program_options::value<bool>()->default_value(true),
           "[platform-specific] Enable width alignment (vegetahd quirk) [{true,false}]");

=== modified file 'src/platforms/android/server/device_quirks.h'
--- src/platforms/android/server/device_quirks.h	2015-06-24 11:33:02 +0000
+++ src/platforms/android/server/device_quirks.h	2015-11-25 12:59:55 +0000
@@ -61,7 +61,7 @@
     DeviceQuirks(PropertiesWrapper const& properties, mir::options::Option const& options);
 
     unsigned int num_framebuffers() const;
-    bool gralloc_reopenable_after_close() const;
+    bool gralloc_cannot_be_closed_safely() const;
     int aligned_width(int width) const;
 
     static void add_options(boost::program_options::options_description& config);
@@ -71,7 +71,7 @@
     DeviceQuirks & operator=(DeviceQuirks const&) = delete;
     std::string const device_name;
     unsigned int const num_framebuffers_;
-    bool const gralloc_reopenable_after_close_;
+    bool const gralloc_cannot_be_closed_safely_;
     bool const enable_width_alignment_quirk;
 };
 }

=== modified file 'tests/unit-tests/graphics/android/test_device_detection.cpp'
--- tests/unit-tests/graphics/android/test_device_detection.cpp	2015-06-24 11:33:02 +0000
+++ tests/unit-tests/graphics/android/test_device_detection.cpp	2015-11-25 12:59:55 +0000
@@ -75,7 +75,7 @@
 }
 
 //LP: 1371619, 1370555
-TEST(DeviceDetection, reports_gralloc_reopenable_after_close_by_default)
+TEST(DeviceDetection, reports_gralloc_can_be_closed_safely_by_default)
 {
     using namespace testing;
     char const default_str[] = "";
@@ -91,10 +91,10 @@
         }));
 
     mga::DeviceQuirks quirks(mock_ops);
-    EXPECT_TRUE(quirks.gralloc_reopenable_after_close());
+    EXPECT_FALSE(quirks.gralloc_cannot_be_closed_safely());
 }
 
-TEST(DeviceDetection, reports_gralloc_not_reopenable_after_close_on_krillin)
+TEST(DeviceDetection, reports_gralloc_cannot_be_closed_safely_on_krillin)
 {
     using namespace testing;
     char const default_str[] = "";
@@ -110,7 +110,7 @@
         }));
 
     mga::DeviceQuirks quirks(mock_ops);
-    EXPECT_FALSE(quirks.gralloc_reopenable_after_close());
+    EXPECT_TRUE(quirks.gralloc_cannot_be_closed_safely());
 }
 
 TEST(DeviceDetection, aligns_width_on_vegetahd)
@@ -150,12 +150,12 @@
         options.parse_arguments(desc, argc, argv);
     }
 
-    void disable_single_gralloc_instance_quirk()
+    void disable_gralloc_cannot_be_closed_safely_quirk()
     {
         int const argc = 2;
         char const* argv[argc] = {
             __PRETTY_FUNCTION__,
-            "--enable-single-gralloc-instance-quirk=false"
+            "--enable-gralloc-cannot-be-closed-safely-quirk=false"
         };
 
         options.parse_arguments(desc, argc, argv);
@@ -197,7 +197,7 @@
     EXPECT_THAT(quirks.num_framebuffers(), Ne(3));
 }
 
-TEST_F(DeviceQuirks, single_gralloc_instance_quirk_can_be_disabled)
+TEST_F(DeviceQuirks, gralloc_cannot_be_closed_safely_quirk_can_be_disabled)
 {
     using namespace testing;
     char const default_str[] = "";
@@ -212,9 +212,9 @@
             return 0;
         }));
 
-    disable_single_gralloc_instance_quirk();
+    disable_gralloc_cannot_be_closed_safely_quirk();
     mga::DeviceQuirks quirks(mock_ops, options);
-    EXPECT_TRUE(quirks.gralloc_reopenable_after_close());
+    EXPECT_FALSE(quirks.gralloc_cannot_be_closed_safely());
 }
 
 TEST_F(DeviceQuirks, width_alignment_quirk_can_be_disabled)