Mir

Merge lp:~albaguirre/mir/fix-1501927 into lp:mir

  1. fix-1501927
  2. Merge into development-branch
Proposed by Alberto Aguirre
Status: Merged
Approved by: Alexandros Frantzis
Approved revision: no longer in the source branch.
Merged at revision: 3001
Proposed branch: lp:~albaguirre/mir/fix-1501927
Merge into: lp:mir
Diff against target: 171 lines (+85/-22)
2 files modified
src/platforms/android/server/display.cpp (+14/-15)
tests/unit-tests/graphics/android/test_display.cpp (+71/-7)
To merge this branch: bzr merge lp:~albaguirre/mir/fix-1501927
Reviewer Review Type Date Requested Status
Alexandros Frantzis (community) Approve
Daniel van Vugt Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+273146@code.launchpad.net

Commit message

[android] Update display sync group during configure where it's safe to delete display buffers. (LP: #1501927)

Fix removing display buffers during for_each_display_sync_group, as that can be called from
threads other than the compositor thread, which may still be using the display buffers. Instead,
remove unused display buffers during the configure phase where it's safe to do so as the compositor
thread is stopped before applying a new configuration.

Description of the change

[android] Update display sync group during configure where it's safe to delete display buffers.

Fix removing display buffers during for_each_display_sync_group, as that can be called from
threads other than the compositor thread, which may still be using the display buffers. Instead,
remove unused display buffers during the configure phase where it's safe to do so as the compositor
thread is stopped before applying a new configuration.

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

FAILED: Continuous integration, rev:2983
http://jenkins.qa.ubuntu.com/job/mir-ci/5104/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4230
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3137
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4172/console
    None: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/130/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1258
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1258/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/130/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4173/console
    None: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/131/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

no test?

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I sometimes wonder about tests for crash fixes. Sometimes they're not feasible because crashes occur in the implementation which is stubbed or mocked. It's not impossible to write tests, but sometimes it's not feasible to add all the testing infrastructure required to emulate the root cause of the problem, in a short time.

Similarly: https://code.launchpad.net/~vanvugt/mir/fix-1493721/+merge/273021

I'd probably approve this one if Jenkins was happier with it.

review: Abstain
Revision history for this message
Chris Halse Rogers (raof) wrote :

Code change seems sensible, but yeah, no test :)

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

FAILED: Continuous integration, rev:2984
http://jenkins.qa.ubuntu.com/job/mir-ci/5172/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4316
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3223
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4258/console
    None: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/216/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1326
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1326/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/216/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4259/console
    None: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/217/console

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

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

PASSED: Continuous integration, rev:2985
http://jenkins.qa.ubuntu.com/job/mir-ci/5179/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4323
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3230
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4265
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/223/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1333
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1333/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/223/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4266
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4266/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/6925
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24008
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/224/console

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

review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Cool

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

Looks good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/platforms/android/server/display.cpp'
--- src/platforms/android/server/display.cpp 2015-10-05 08:19:39 +0000
+++ src/platforms/android/server/display.cpp 2015-10-06 18:42:44 +0000
@@ -221,21 +221,6 @@
221void mga::Display::for_each_display_sync_group(std::function<void(mg::DisplaySyncGroup&)> const& f)221void mga::Display::for_each_display_sync_group(std::function<void(mg::DisplaySyncGroup&)> const& f)
222{222{
223 std::lock_guard<decltype(configuration_mutex)> lock{configuration_mutex};223 std::lock_guard<decltype(configuration_mutex)> lock{configuration_mutex};
224 update_configuration(lock);
225 if ((config.external().connected) && !displays.display_present(mga::DisplayName::external))
226 displays.add(mga::DisplayName::external,
227 create_display_buffer(
228 display_device,
229 mga::DisplayName::external,
230 *display_buffer_builder,
231 config.external(),
232 gl_program_factory,
233 gl_context,
234 config.external().top_left - origin,
235 overlay_option));
236 if ((!config.external().connected) && displays.display_present(mga::DisplayName::external))
237 displays.remove(mga::DisplayName::external);
238
239 f(displays);224 f(displays);
240}225}
241226
@@ -254,6 +239,20 @@
254239
255 std::lock_guard<decltype(configuration_mutex)> lock{configuration_mutex};240 std::lock_guard<decltype(configuration_mutex)> lock{configuration_mutex};
256241
242 if ((config.external().connected) && !displays.display_present(mga::DisplayName::external))
243 displays.add(mga::DisplayName::external,
244 create_display_buffer(
245 display_device,
246 mga::DisplayName::external,
247 *display_buffer_builder,
248 config.external(),
249 gl_program_factory,
250 gl_context,
251 config.external().top_left - origin,
252 overlay_option));
253 if ((!config.external().connected) && displays.display_present(mga::DisplayName::external))
254 displays.remove(mga::DisplayName::external);
255
257 new_configuration.for_each_output([this](mg::DisplayConfigurationOutput const& output)256 new_configuration.for_each_output([this](mg::DisplayConfigurationOutput const& output)
258 {257 {
259 if (output.current_format != config[output.id].current_format)258 if (output.current_format != config[output.id].current_format)
260259
=== modified file 'tests/unit-tests/graphics/android/test_display.cpp'
--- tests/unit-tests/graphics/android/test_display.cpp 2015-10-05 17:36:04 +0000
+++ tests/unit-tests/graphics/android/test_display.cpp 2015-10-06 18:42:44 +0000
@@ -665,6 +665,8 @@
665 group_count++;665 group_count++;
666 group.for_each_display_buffer([&](mg::DisplayBuffer&) {db_count++;});666 group.for_each_display_buffer([&](mg::DisplayBuffer&) {db_count++;});
667 };667 };
668 auto conf = display.configuration();
669 display.configure(*conf);
668 display.for_each_display_sync_group(db_group_counter);670 display.for_each_display_sync_group(db_group_counter);
669 EXPECT_THAT(group_count, Eq(1));671 EXPECT_THAT(group_count, Eq(1));
670 EXPECT_THAT(db_count, Eq(2));672 EXPECT_THAT(db_count, Eq(2));
@@ -672,6 +674,8 @@
672 //hotplug external away674 //hotplug external away
673 external_connected = false;675 external_connected = false;
674 hotplug_fn();676 hotplug_fn();
677 conf = display.configuration();
678 display.configure(*conf);
675679
676 group_count = 0;680 group_count = 0;
677 db_count = 0;681 db_count = 0;
@@ -682,6 +686,8 @@
682 //hotplug external back 686 //hotplug external back
683 external_connected = true;687 external_connected = true;
684 hotplug_fn();688 hotplug_fn();
689 conf = display.configuration();
690 display.configure(*conf);
685691
686 group_count = 0;692 group_count = 0;
687 db_count = 0;693 db_count = 0;
@@ -709,11 +715,20 @@
709 {20,20}, {4,4}, mir_pixel_format_abgr_8888, 50.0f, external_connected};715 {20,20}, {4,4}, mir_pixel_format_abgr_8888, 50.0f, external_connected};
710 }));716 }));
711717
712718 InSequence seq;
713 testing::InSequence seq;719
714 EXPECT_CALL(mock_config, power_mode(mga::DisplayName::primary, _));720 //At construction, we expect external display to be connected
715 EXPECT_CALL(mock_config, power_mode(mga::DisplayName::external, mir_power_mode_on));721 EXPECT_CALL(mock_config, power_mode(mga::DisplayName::primary, _));
716 EXPECT_CALL(mock_config, power_mode(mga::DisplayName::external, mir_power_mode_on));722 EXPECT_CALL(mock_config, power_mode(mga::DisplayName::external, mir_power_mode_on));
723
724 //Unplugging
725 EXPECT_CALL(mock_config, power_mode(mga::DisplayName::primary, _));
726
727 //Hotplugging external back again
728 EXPECT_CALL(mock_config, power_mode(mga::DisplayName::external, mir_power_mode_on));
729 EXPECT_CALL(mock_config, power_mode(mga::DisplayName::primary, _));
730
731 //Expected after destructor
717 EXPECT_CALL(mock_config, power_mode(mga::DisplayName::primary, _));732 EXPECT_CALL(mock_config, power_mode(mga::DisplayName::primary, _));
718 EXPECT_CALL(mock_config, power_mode(mga::DisplayName::external, mir_power_mode_off));733 EXPECT_CALL(mock_config, power_mode(mga::DisplayName::external, mir_power_mode_off));
719 });734 });
@@ -728,12 +743,15 @@
728 //hotplug external away743 //hotplug external away
729 external_connected = false;744 external_connected = false;
730 hotplug_fn();745 hotplug_fn();
731 display.for_each_display_sync_group([](mg::DisplaySyncGroup&){});746 auto conf = display.configuration();
747 display.configure(*conf);
732748
733 //hotplug external back 749 //hotplug external back
734 external_connected = true;750 external_connected = true;
735 hotplug_fn();751 hotplug_fn();
736 display.for_each_display_sync_group([](mg::DisplaySyncGroup&){});752 conf = display.configuration();
753 display.configure(*conf);
754
737}755}
738756
739TEST_F(Display, configures_external_display)757TEST_F(Display, configures_external_display)
@@ -925,3 +943,49 @@
925 });943 });
926 });944 });
927}945}
946
947TEST_F(Display, does_not_remove_dbs_when_enumerating_display_groups)
948{
949 using namespace testing;
950 std::function<void()> hotplug_fn = []{};
951 bool external_connected = true;
952 stub_db_factory->with_next_config([&](mtd::MockHwcConfiguration& mock_config)
953 {
954 ON_CALL(mock_config, active_config_for(mga::DisplayName::primary))
955 .WillByDefault(Return(mtd::StubDisplayConfigurationOutput{
956 mg::DisplayConfigurationOutputId{0}, {20,20}, {4,4}, mir_pixel_format_abgr_8888, 50.0f, true}));
957
958 ON_CALL(mock_config, active_config_for(mga::DisplayName::external))
959 .WillByDefault(Invoke([&](mga::DisplayName)
960 {
961 return mtd::StubDisplayConfigurationOutput{mg::DisplayConfigurationOutputId{1},
962 {20,20}, {4,4}, mir_pixel_format_abgr_8888, 50.0f, external_connected};
963 }));
964 EXPECT_CALL(mock_config, subscribe_to_config_changes(_,_))
965 .WillOnce(DoAll(SaveArg<0>(&hotplug_fn), Return(std::make_shared<char>('2'))));
966 });
967
968 mga::Display display(
969 stub_db_factory,
970 stub_gl_program_factory,
971 stub_gl_config,
972 null_display_report,
973 mga::OverlayOptimization::enabled);
974
975 auto db_count = 0;
976 auto db_group_counter = [&](mg::DisplaySyncGroup& group) {
977 group.for_each_display_buffer([&](mg::DisplayBuffer&) {db_count++;});
978 };
979
980 display.for_each_display_sync_group(db_group_counter);
981 auto const expected_buffer_count = 2;
982 EXPECT_THAT(db_count, Eq(expected_buffer_count));
983
984 //hotplug external away
985 external_connected = false;
986 hotplug_fn();
987
988 db_count = 0;
989 display.for_each_display_sync_group(db_group_counter);
990 EXPECT_THAT(db_count, Eq(expected_buffer_count));
991}

Subscribers

People subscribed via source and target branches