Mir

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

  1. fix-1331769
  2. Merge into development-branch
Proposed by Kevin DuBois
Status: Merged
Approved by: Kevin DuBois
Approved revision: no longer in the source branch.
Merged at revision: 1752
Proposed branch: lp:~kdub/mir/fix-1331769
Merge into: lp:mir
Prerequisite: lp:~kdub/mir/hwc-layers-cleanup
Diff against target: 193 lines (+63/-22)
3 files modified
src/platform/graphics/android/hwc_device.cpp (+9/-5)
tests/unit-tests/graphics/android/test_hwc_device.cpp (+54/-15)
tests/unit-tests/graphics/android/test_hwc_layers.cpp (+0/-2)
To merge this branch: bzr merge lp:~kdub/mir/fix-1331769
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Robert Carr (community) Approve
Alan Griffiths Needs Fixing
Alexandros Frantzis (community) Approve
Review via email: mp+224337@code.launchpad.net

Commit message

android: only dup the fence for the FRAMEBUFFER_TARGET layer when the HWC has indicated in prepare that it is interested in that layer

fixes: lp: #1331769

Description of the change

android: only dup the fence for the FRAMEBUFFER_TARGET layer when the HWC has indicated in prepare that it is interested in that layer.

fixes: lp: #1331769

So the reason this was only a problem on the nexus 10 is that the other drivers seem to guarantee that they close all acquireFenceFd's, whereas the nexus10 driver will only close those that they are interested in, causing us to leak fd's in overlay scenarios. This scenario would not be hit in the system image.

testing:
20+ min render_to_fb: nex4, nex7, nex10, mx3
20+ min render_overlays: nex4, nex7, nex10, mx3
tear testing with full u8 stack: nex4, nex7, nex10, mx3

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1734
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1989/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/686
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/692
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/686
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/510
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/510/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/507
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/507/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2022
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2022/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1862
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/8807

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

review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Looks as plausible as any of the HWC code ever does to me.

review: Approve
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

> Looks as plausible as any of the HWC code ever does to me.

+1

review: Approve
Revision history for this message
Kevin DuBois (kdub) wrote :

depends-on a branch, switching back to 'needs review'

Revision history for this message
Kevin DuBois (kdub) wrote :

prereq landed, top approving

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/827/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/777/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/783
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/776/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-autolanding/211/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-autolanding/211/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2335/console

review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> Text conflict in tests/unit-tests/graphics/android/test_hwc_device.cpp
> 1 conflicts encountered.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/831/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/785/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/791
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/784/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-autolanding/215/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-autolanding/215/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2390/console

review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> > Text conflict in tests/unit-tests/graphics/android/test_hwc_device.cpp
> > 1 conflicts encountered.

Kevin, fix the conflict before top-approving again. ;?

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1736
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2079/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/846
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/852
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/845
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/600
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/600/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/598
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/598/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2668
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2668/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1994
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/9408

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

review: Approve (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

> > > Text conflict in tests/unit-tests/graphics/android/test_hwc_device.cpp
> > > 1 conflicts encountered.
>
> Kevin, fix the conflict before top-approving again. ;?

resolved

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/862/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/849
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/855
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/848/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-autolanding/246
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-autolanding/246/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-autolanding/246
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-autolanding/246/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2683
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2683/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1997/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/9422

review: Needs Fixing (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

Error: Connection activation failed: (5) IP configuration could not be reserved (no available address, timeout, etc.).

Revision history for this message
Robert Carr (robertcarr) wrote :

LGTM

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/platform/graphics/android/hwc_device.cpp'
2--- src/platform/graphics/android/hwc_device.cpp 2014-07-04 04:43:42 +0000
3+++ src/platform/graphics/android/hwc_device.cpp 2014-07-07 16:58:01 +0000
4@@ -150,11 +150,14 @@
5 it++;
6 }
7
8- list_compositor.render(rejected_renderables, context);
9+ if (!rejected_renderables.empty())
10+ {
11+ list_compositor.render(rejected_renderables, context);
12
13- buffer = context.last_rendered_buffer();
14- fbtarget.layer.setup_layer(mga::LayerType::framebuffer_target, disp_frame, false, *buffer);
15- fbtarget.layer.set_acquirefence_from(*buffer);
16+ buffer = context.last_rendered_buffer();
17+ fbtarget.layer.setup_layer(mga::LayerType::framebuffer_target, disp_frame, false, *buffer);
18+ fbtarget.layer.set_acquirefence_from(*buffer);
19+ }
20
21 hwc_wrapper->set(*hwc_list.native_list().lock());
22 onscreen_overlay_buffers = std::move(next_onscreen_overlay_buffers);
23@@ -165,7 +168,8 @@
24 it->layer.update_from_releasefence(*renderable->buffer());
25 it++;
26 }
27- fbtarget.layer.update_from_releasefence(*buffer);
28+ if (!rejected_renderables.empty())
29+ fbtarget.layer.update_from_releasefence(*buffer);
30
31 mga::SyncFence retire_fence(sync_ops, hwc_list.retirement_fence());
32 return true;
33
34=== modified file 'tests/unit-tests/graphics/android/test_hwc_device.cpp'
35--- tests/unit-tests/graphics/android/test_hwc_device.cpp 2014-07-04 04:43:42 +0000
36+++ tests/unit-tests/graphics/android/test_hwc_device.cpp 2014-07-07 16:58:01 +0000
37@@ -109,6 +109,7 @@
38 for(auto i = 0u; i < contents.numHwLayers - 1; i++) //-1 because the last layer is the target
39 contents.hwLayers[i].compositionType = HWC_OVERLAY;
40 };
41+ reject_all_layers = [&](hwc_display_contents_1_t&){};
42 }
43
44 hwc_rect_t skip_rect;
45@@ -128,6 +129,7 @@
46 geom::Rectangle const fb_position{{0,0},size3};
47 mtd::StubRenderableListCompositor stub_compositor;
48 std::function<void(hwc_display_contents_1_t&)> set_all_layers_to_overlay;
49+ std::function<void(hwc_display_contents_1_t&)> reject_all_layers;
50
51 std::shared_ptr<mtd::MockAndroidNativeBuffer> const mock_native_buffer1;
52 std::shared_ptr<mtd::MockAndroidNativeBuffer> const mock_native_buffer2;
53@@ -254,22 +256,63 @@
54 device.post_gl(stub_context);
55 }
56
57-TEST_F(HwcDevice, sets_proper_overlay_lists)
58+TEST_F(HwcDevice, commits_correct_list_with_rejected_renderables)
59+{
60+ using namespace testing;
61+ int fb_acquire_fence = 80;
62+ int fb_release_fence = 383;
63+
64+ auto set_fences_fn = [&](hwc_display_contents_1_t& contents)
65+ {
66+ ASSERT_EQ(contents.numHwLayers, 2);
67+ contents.hwLayers[1].releaseFenceFd = fb_release_fence;
68+ contents.retireFenceFd = -1;
69+ };
70+
71+ layer.acquireFenceFd = -1;
72+ target_layer.acquireFenceFd = fb_acquire_fence;
73+
74+ std::list<hwc_layer_1_t*> expected_list
75+ {
76+ &layer,
77+ &target_layer
78+ };
79+
80+ mga::HwcDevice device(mock_device, mock_hwc_device_wrapper, mock_vsync, mock_file_ops);
81+
82+ EXPECT_CALL(*mock_native_buffer1, copy_fence())
83+ .Times(0);
84+ EXPECT_CALL(*mock_native_buffer1, update_usage(_,_))
85+ .Times(0);
86+ Sequence seq;
87+ EXPECT_CALL(*mock_hwc_device_wrapper, prepare(_))
88+ .InSequence(seq)
89+ .WillOnce(Invoke(reject_all_layers));
90+ EXPECT_CALL(*mock_native_buffer3, copy_fence())
91+ .InSequence(seq)
92+ .WillOnce(Return(fb_acquire_fence));
93+ EXPECT_CALL(*mock_hwc_device_wrapper, set(MatchesList(expected_list)))
94+ .InSequence(seq)
95+ .WillOnce(Invoke(set_fences_fn));
96+ EXPECT_CALL(*mock_native_buffer3, update_usage(fb_release_fence, mga::BufferAccess::read))
97+ .InSequence(seq);
98+
99+ EXPECT_TRUE(device.post_overlays(stub_context, {stub_renderable1}, stub_compositor));
100+}
101+
102+TEST_F(HwcDevice, commits_correct_list_when_all_accepted_as_overlays)
103 {
104 using namespace testing;
105 int overlay_acquire_fence1 = 80;
106 int overlay_acquire_fence2 = 81;
107- int fb_acquire_fence = 82;
108 int release_fence1 = 381;
109 int release_fence2 = 382;
110- int release_fence3 = 383;
111
112 auto set_fences_fn = [&](hwc_display_contents_1_t& contents)
113 {
114 ASSERT_EQ(contents.numHwLayers, 3);
115 contents.hwLayers[0].releaseFenceFd = release_fence1;
116 contents.hwLayers[1].releaseFenceFd = release_fence2;
117- contents.hwLayers[2].releaseFenceFd = release_fence3;
118 contents.retireFenceFd = -1;
119 };
120
121@@ -279,8 +322,8 @@
122 layer2.compositionType = HWC_OVERLAY;
123 layer2.acquireFenceFd = overlay_acquire_fence2;
124
125- //should be -1
126- target_layer.acquireFenceFd = fb_acquire_fence;
127+ //all layers are overlays, so we don't copy the fence. lp: #1331769
128+ target_layer.acquireFenceFd = -1;
129
130 std::list<hwc_layer_1_t*> expected_list
131 {
132@@ -291,6 +334,11 @@
133
134 mga::HwcDevice device(mock_device, mock_hwc_device_wrapper, mock_vsync, mock_file_ops);
135
136+ EXPECT_CALL(*mock_native_buffer3, copy_fence())
137+ .Times(0);
138+ EXPECT_CALL(*mock_native_buffer3, update_usage(_,_))
139+ .Times(0);
140+
141 Sequence seq;
142 EXPECT_CALL(*mock_hwc_device_wrapper, prepare(_))
143 .InSequence(seq)
144@@ -303,11 +351,6 @@
145 EXPECT_CALL(*mock_native_buffer2, copy_fence())
146 .InSequence(seq)
147 .WillOnce(Return(overlay_acquire_fence2));
148- EXPECT_CALL(*mock_native_buffer3, copy_fence())
149- .InSequence(seq)
150- .WillOnce(Return(fb_acquire_fence));
151-
152- //set
153 EXPECT_CALL(*mock_hwc_device_wrapper, set(MatchesList(expected_list)))
154 .InSequence(seq)
155 .WillOnce(Invoke(set_fences_fn));
156@@ -315,8 +358,6 @@
157 .InSequence(seq);
158 EXPECT_CALL(*mock_native_buffer2, update_usage(release_fence2, mga::BufferAccess::read))
159 .InSequence(seq);
160- EXPECT_CALL(*mock_native_buffer3, update_usage(release_fence3, mga::BufferAccess::read))
161- .InSequence(seq);
162
163 EXPECT_TRUE(device.post_overlays(stub_context, renderlist, stub_compositor));
164 }
165@@ -404,7 +445,6 @@
166
167 int release_fence1 = 381;
168 int release_fence2 = 382;
169- int release_fence3 = 383;
170 auto native_buffer = std::make_shared<testing::NiceMock<mtd::MockAndroidNativeBuffer>>(size1);
171 auto updated_buffer = std::make_shared<mtd::StubBuffer>(native_buffer, size1);
172 auto set_fences_fn = [&](hwc_display_contents_1_t& contents)
173@@ -412,7 +452,6 @@
174 ASSERT_EQ(contents.numHwLayers, 3);
175 contents.hwLayers[0].releaseFenceFd = release_fence1;
176 contents.hwLayers[1].releaseFenceFd = release_fence2;
177- contents.hwLayers[2].releaseFenceFd = release_fence3;
178 contents.retireFenceFd = -1;
179 };
180
181
182=== modified file 'tests/unit-tests/graphics/android/test_hwc_layers.cpp'
183--- tests/unit-tests/graphics/android/test_hwc_layers.cpp 2014-07-04 04:43:42 +0000
184+++ tests/unit-tests/graphics/android/test_hwc_layers.cpp 2014-07-07 16:58:01 +0000
185@@ -230,8 +230,6 @@
186 false,
187 mock_buffer);
188 EXPECT_THAT(*hwc_layer, MatchesLayer(expected_layer));
189-
190- //TODO: we have to know if the fb target is needed or not. if it is not, we should not copy the fd.
191 }
192
193 TEST_F(HWCLayersTest, buffer_fence_updates)

Subscribers

People subscribed via source and target branches