Merge lp:~kdub/mir/workaround-qcom-display-request into lp:mir

[Kevin DuBois]

the stock qcom 8960 hwcomposer chokes on getDisplayAttributes if the submitted arrays are not at least size 6. I patched the qcom android 4.2 hwcomposer driver on the ubuntu touch images to work properly, but this causes us problems with in-the wild drivers, and the new 4.4 drivers. Make sure we always submit a larger-than-needed array to this function

the stock qcom 8960 hwcomposer chokes on getDisplayAttributes if the submitted arrays are not at least size 6. I patched the qcom android 4.2 hwcomposer driver on the ubuntu touch images to work properly, but this causes us problems with in-the wild drivers, and the new 4.4 drivers. Make sure we always submit a larger-than-needed array to this function

getDisplayAttributes() is supposed to work like eglChooseConfig, but the qcom implementation [1] breaks if the array is smaller than the array that surfaceflinger submits.

[1] https://android.googlesource.com/platform/hardware/qcom/display/+/master/msm8960/libhwcomposer/hwc.cpp line 527

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1336
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~kdub/mir/workaround-qcom-display-request/+merge/201859/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/671/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/652
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/648
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/255
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/401
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/401/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/405
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/405/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/255
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/255/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/273
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/3135

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

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

jenkins failure was just me not setting a commit msg.

[Daniel van Vugt (vanvugt)]

Looks good.

I'm just a little concerned about a pre-existing problem with test_fb_simple_swapper.cpp ...
It looks like display_attribute_handler() writes to the "values" array without any knowledge of the array length!?

review: Needs Information

[Andreas Pokorny (andreas-pokorny)]

> Looks good.
>
> I'm just a little concerned about a pre-existing problem with
> test_fb_simple_swapper.cpp ...
> It looks like display_attribute_handler() writes to the "values" array without
> any knowledge of the array length!?

The size is passed - currently unnamed it should check that that value is exactly 6 for now.

review: Needs Fixing

[Kevin DuBois (kdub)]

> Looks good.
>
> I'm just a little concerned about a pre-existing problem with
> test_fb_simple_swapper.cpp ...
> It looks like display_attribute_handler() writes to the "values" array without
> any knowledge of the array length!?

Yes, its ugly. However, the driver code does not implement this correctly, and to accommodate the driver, the testing code is just as bad as the driver code. This at least gives us the safety net that if this is changed incorrectly in the future, we have a comment, a segfault location, and a comment explaining why we had to accommodate this quirk.

[Alan Griffiths (alan-griffiths)]

26 + int32_t size_values[sizeof(display_attribute_request) / sizeof (display_attribute_request[0])];

We really ought to have a lengthof function defined somewhere. Like:

    template <size_t Length, typename Element>
    constexpr size_t lengthof(Element (&)[Length]) { return Length; }

At least that fails to compile when passed a pointer.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1336
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/674/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/655
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/651
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/258
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/404
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/404/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/408
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/408/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/258
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/258/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/276
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/3150

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

review: Approve (continuous-integration)

[Andreas Pokorny (andreas-pokorny)]

> > Looks good.
> >
> > I'm just a little concerned about a pre-existing problem with
> > test_fb_simple_swapper.cpp ...
> > It looks like display_attribute_handler() writes to the "values" array
> without
> > any knowledge of the array length!?
>
> Yes, its ugly. However, the driver code does not implement this correctly, and
> to accommodate the driver, the testing code is just as bad as the driver code.
> This at least gives us the safety net that if this is changed incorrectly in
> the future, we have a comment, a segfault location, and a comment explaining
> why we had to accommodate this quirk.

I was just afraid that we might not get a seg fault/test failure when we pass an array of wrong size.
So i thought the test should fail if we dont pass 6 elements - which we can only guess by the size parameter..

ok maybe valgrind would notice?

[Kevin DuBois (kdub)]

> I was just afraid that we might not get a seg fault/test failure when we pass
> an array of wrong size.
> So i thought the test should fail if we dont pass 6 elements - which we can
> only guess by the size parameter..

we don't have a size parameter to check, the function signature is
    int (*getDisplayAttributes)(struct hwc_composer_device_1* dev, int disp,
            uint32_t config, const uint32_t* attributes, int32_t* values);

you pass in the list of attributes you want in "attributes", and where you want those attributes filled in "values". The size is negotiated by the position of HWC_DISPLAY_NO_ATTRIBUTE in attributes.

[Daniel van Vugt (vanvugt)]

If the array is terminated by a HWC_DISPLAY_NO_ATTRIBUTE then we should be using ASSERT_EQ in place of EXPECT_EQ. That way the test will fail gracefully if anything changes, instead of segfaulting (or worse, failing with reads past the end of array).

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

53 + for(auto i = 2; i <= 6; i++)

Off by one error: i <= 5, or even i < 5 since I don't think HWC_DISPLAY_NO_ATTRIBUTE is expected to return a value (not that it matters for the code currently).

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> 53 + for(auto i = 2; i <= 6; i++)
>
> Off by one error: i <= 5, or even i < 5 since I don't think
> HWC_DISPLAY_NO_ATTRIBUTE is expected to return a value (not that it matters
> for the code currently).

I guess we should be using named constants not magic numbers too.

Oh, and the principle that "the loop condition is the inverse of the termination invariant" says the condition should be "!=".

But given the above discussion, shouldn't this be:

for (auto p = values+2; *p != HWC_DISPLAY_NO_ATTRIBUTE; ++p)

review: Needs Fixing

[Kevin DuBois (kdub)]

this now scans for HWC_DISPLAY_NO_ATTRIBUTE and then checks the size, should be more robust.

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

[Andreas Pokorny (andreas-pokorny)]

ack

review: Approve

[Kevin DuBois (kdub)]

top, approving, seems people's concerns addressed.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1337
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/682/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/666
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/662
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/268
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/412
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/412/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/416
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/416/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/268
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/268/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/284
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/3183

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

review: Approve (continuous-integration)

=== modified file 'src/platform/graphics/android/framebuffers.cpp'
--- src/platform/graphics/android/framebuffers.cpp	2014-01-10 03:59:43 +0000
+++ src/platform/graphics/android/framebuffers.cpp	2014-01-17 17:18:01 +0000
@@ -78,13 +78,22 @@
     {
         BOOST_THROW_EXCEPTION(std::runtime_error("could not determine hwc display config"));
     }
-    static uint32_t size_request[3] = { HWC_DISPLAY_WIDTH,
-                                        HWC_DISPLAY_HEIGHT,
-                                        HWC_DISPLAY_NO_ATTRIBUTE};
-
-    int size_values[2];
+
+    /* note: some drivers (qcom msm8960) choke if this is not the same size array
+       as the one surfaceflinger submits */
+    static uint32_t const display_attribute_request[] =
+    {
+        HWC_DISPLAY_WIDTH,
+        HWC_DISPLAY_HEIGHT,
+        HWC_DISPLAY_VSYNC_PERIOD,
+        HWC_DISPLAY_DPI_X,
+        HWC_DISPLAY_DPI_Y,
+        HWC_DISPLAY_NO_ATTRIBUTE,
+    };
+
+    int32_t size_values[sizeof(display_attribute_request) / sizeof (display_attribute_request[0])];
     hwc_device->getDisplayAttributes(hwc_device.get(), HWC_DISPLAY_PRIMARY, primary_display_config,
-                                     size_request, size_values);
+                                     display_attribute_request, size_values);
 
     return {size_values[0], size_values[1]};
 }

=== modified file 'tests/unit-tests/graphics/android/test_fb_simple_swapper.cpp'
--- tests/unit-tests/graphics/android/test_fb_simple_swapper.cpp	2014-01-10 03:59:43 +0000
+++ tests/unit-tests/graphics/android/test_fb_simple_swapper.cpp	2014-01-17 17:18:01 +0000
@@ -81,12 +81,25 @@
 static int display_attribute_handler(struct hwc_composer_device_1*, int, uint32_t,
                                      const uint32_t* attribute_list, int32_t* values)
 {
-    EXPECT_EQ(attribute_list[0], HWC_DISPLAY_WIDTH);
-    EXPECT_EQ(attribute_list[1], HWC_DISPLAY_HEIGHT);
-    EXPECT_EQ(attribute_list[2], HWC_DISPLAY_NO_ATTRIBUTE);
+    int i = 0;
+    while(attribute_list[i] != HWC_DISPLAY_NO_ATTRIBUTE)
+    {
+        switch(attribute_list[i])
+        {
+            case HWC_DISPLAY_WIDTH:
+                values[i] = display_width;
+                break;
+            case HWC_DISPLAY_HEIGHT:
+                values[i] = display_height;
+                break;
+            default:
+                break;
+        }
+        i++;
+    }
 
-    values[0] = display_width;
-    values[1] = display_height;
+    //the attribute list should be at least this long, as some qcom drivers always deref attribute_list[5]
+    EXPECT_EQ(5, i);
     return 0;
 }
 }