Mir

Merge lp:~robertcarr/mir/remove-ensure-display-powered into lp:mir

  1. remove-ensure-display-powered
  2. Merge into development-branch
Proposed by Robert Carr
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
Merged at revision: 1484
Proposed branch: lp:~robertcarr/mir/remove-ensure-display-powered
Merge into: lp:mir
Diff against target: 127 lines (+0/-43)
8 files modified
include/server/mir/frontend/display_changer.h (+0/-1)
include/test/mir_test_doubles/mock_display_changer.h (+0/-1)
include/test/mir_test_doubles/null_display_changer.h (+0/-3)
src/server/frontend/session_mediator.cpp (+0/-6)
src/server/frontend/unauthorized_display_changer.cpp (+0/-5)
src/server/frontend/unauthorized_display_changer.h (+0/-2)
src/server/scene/mediating_display_changer.cpp (+0/-23)
src/server/scene/mediating_display_changer.h (+0/-2)
To merge this branch: bzr merge lp:~robertcarr/mir/remove-ensure-display-powered
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Andreas Pokorny (community) Approve
Daniel van Vugt Approve
Alberto Aguirre (community) Approve
Alan Griffiths Needs Information
Kevin DuBois Pending
Review via email: mp+209734@code.launchpad.net

This proposal supersedes a proposal from 2014-03-05.

Commit message

Remove ensure_display_powered logic around client buffer swapping.

Description of the change

As alan points out, swap buffers is async now so we dont need this funny bit of logic. Remove ensure_display_powered and demonstrate with an acceptance tests that clients are not blocking.

OLD:
While discussing this feature today with mterry I noticed we have no acceptance test for it!

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

PASSED: Continuous integration, rev:1453
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/995/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1141
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1139
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/724
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/727
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/727/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/732
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/732/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/725
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/725/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/687
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4502

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

review: Approve (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote : Posted in a previous version of this proposal

looks okay to me

review: Approve
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

80 +// when the client makes a swap buffers request, swap buffers will block until the screen is reenabled. However even if on the
81 +// client side, swap buffers is called asynchronously, the server side RPC thread will not process another request from this client
82 +// until swap buffers completes

As swap_buffers is non-blocking I don't think this has been true since -r 1398. Am I missing something?

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

Good stuff. Love having a test for the required behavior!!!

Small nits:

224 +using ClientPowerControl = BespokeDisplayServerTestFixture;
...
234 +TEST_F(ClientPowerControl, client_may_turn_power_back_on_while_blocked_on_swap_buffers)
235 +{
236 + TestingServerConfiguration server_config;
237 + launch_server_process(server_config);

Even easier to use DefaultDisplayServerTestFixture

~~~~

266 + // Creating the surface will try to swap buffers, so this can not complete
267 + // while the display is off.

There should probably be an ASSERT to ensure that this is correct.

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

FAILED: Continuous integration, rev:1454
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/~robertcarr/mir/remove-ensure-display-powered/+merge/209734/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1006/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1156
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1154
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/738
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/738
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/738/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/743
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/743/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/739
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/739/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/700
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4554

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

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

266 + // Creating the surface will try to swap buffers, so this can not complete
267 + // while the display is off.
268 + auto s_handle = mir_connection_create_surface(connection, &params, surface_callback, this);

This is wrong.

SwitchingBundle will allocate three buffers for a surface before requiring the client to wait for compositing to release the first one.

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

True...this was a long time ago I guess (though I could have sworn we had this logic in Boston when I was originally adding the ensure display powered...I guess maybe there was a bug in the bundle at that point).

Removed the deceptive test.

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

PASSED: Continuous integration, rev:1455
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1041/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1200
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1198
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/782
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/773
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/773/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/778
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/778/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/783
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/783/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/734
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4694

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

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

I was so happy we had a test.

Why didn't you fix the test instead of deleting it?

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

I couldnt figure out a way to comment the regression test. In particular I guess I dont know why this code was ever required

this landed after bypass so triple buffering was in place. Why did it ever block?

Revision history for this message
Michael Terry (mterry) wrote :

I just tested this. It fixed an annoying bug with my split greeter branches, where after turning the screen off, it turns right back on when the greeter comes up. +1 from me from the functionality side.

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

(1) Needs fixing: The commit message disagrees with the change.
Commit message: "Add acceptance test"...
Code diff: Only deletions

(2) mterry's comment seems to suggest this might be a fix for bug 1231857. I shall have to check...

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

Nope, bug 1231857 is not helped by this.

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> I couldnt figure out a way to comment the regression test. In particular I
> guess I dont know why this code was ever required
>
> this landed after bypass so triple buffering was in place. Why did it ever
> block?

Bypass and triple buffering don't prevent swap_buffers blocking. AFAIK it has never blocked create_surface.

However, there are limited buffers associated with a surface and if the compositor doesn't consume them then swap_buffers will eventually block.

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

Yes I guess the acceptance test never would have blocked it was only if the client consumes all the buffers.

Anyway updated commit message.

Revision history for this message
Alberto Aguirre (albaguirre) wrote :

Yes, the display power/off policy should be mostly driven by the user.

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) :
review: Approve
Revision history for this message
Andreas Pokorny (andreas-pokorny) :
review: Approve
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/477/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1267
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1265
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/848/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-autolanding/382
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-autolanding/382/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-autolanding/383
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-autolanding/383/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/849
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/849/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/792/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4887

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

PASSED: Continuous integration, rev:1456
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1100/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1281
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1279
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/861
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/832
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/832/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/837
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/837/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/862
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/862/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/804
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4947

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

review: Approve (continuous-integration)
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 'include/server/mir/frontend/display_changer.h'
2--- include/server/mir/frontend/display_changer.h 2013-09-17 20:03:16 +0000
3+++ include/server/mir/frontend/display_changer.h 2014-03-18 20:43:48 +0000
4@@ -38,7 +38,6 @@
5
6 virtual std::shared_ptr<graphics::DisplayConfiguration> active_configuration() = 0;
7 virtual void configure(std::shared_ptr<Session> const&, std::shared_ptr<graphics::DisplayConfiguration> const&) = 0;
8- virtual void ensure_display_powered(std::shared_ptr<Session> const& session) = 0;
9
10 protected:
11 DisplayChanger() = default;
12
13=== modified file 'include/test/mir_test_doubles/mock_display_changer.h'
14--- include/test/mir_test_doubles/mock_display_changer.h 2013-09-17 20:03:16 +0000
15+++ include/test/mir_test_doubles/mock_display_changer.h 2014-03-18 20:43:48 +0000
16@@ -35,7 +35,6 @@
17 MOCK_METHOD0(active_configuration, std::shared_ptr<graphics::DisplayConfiguration>());
18 MOCK_METHOD2(configure,
19 void(std::shared_ptr<frontend::Session> const&, std::shared_ptr<graphics::DisplayConfiguration> const&));
20- MOCK_METHOD1(ensure_display_powered, void(std::shared_ptr<frontend::Session> const&));
21 };
22
23 }
24
25=== modified file 'include/test/mir_test_doubles/null_display_changer.h'
26--- include/test/mir_test_doubles/null_display_changer.h 2013-09-17 20:03:16 +0000
27+++ include/test/mir_test_doubles/null_display_changer.h 2014-03-18 20:43:48 +0000
28@@ -39,9 +39,6 @@
29 virtual void configure(std::shared_ptr<frontend::Session> const&, std::shared_ptr<graphics::DisplayConfiguration> const&)
30 {
31 }
32- virtual void ensure_display_powered(std::shared_ptr<frontend::Session> const&)
33- {
34- }
35 };
36 }
37 }
38
39=== modified file 'src/server/frontend/session_mediator.cpp'
40--- src/server/frontend/session_mediator.cpp 2014-03-11 16:19:27 +0000
41+++ src/server/frontend/session_mediator.cpp 2014-03-18 20:43:48 +0000
42@@ -212,12 +212,6 @@
43
44 report->session_next_buffer_called(session->name());
45
46- // We ensure the client has not powered down the outputs, so that
47- // swap_buffer will not block indefinitely, leaving the client
48- // in a position where it can not turn back on the
49- // outputs.
50- display_changer->ensure_display_powered(session);
51-
52 auto surface = session->get_surface(surf_id);
53
54 advance_buffer(surf_id, *surface,
55
56=== modified file 'src/server/frontend/unauthorized_display_changer.cpp'
57--- src/server/frontend/unauthorized_display_changer.cpp 2014-03-06 06:05:17 +0000
58+++ src/server/frontend/unauthorized_display_changer.cpp 2014-03-18 20:43:48 +0000
59@@ -37,8 +37,3 @@
60 {
61 BOOST_THROW_EXCEPTION(std::runtime_error("not authorized to apply display configurations"));
62 }
63-
64-void mf::UnauthorizedDisplayChanger::ensure_display_powered(std::shared_ptr<mf::Session> const&)
65-{
66- BOOST_THROW_EXCEPTION(std::runtime_error("not authorized to apply display configurations"));
67-}
68
69=== modified file 'src/server/frontend/unauthorized_display_changer.h'
70--- src/server/frontend/unauthorized_display_changer.h 2014-03-06 06:05:17 +0000
71+++ src/server/frontend/unauthorized_display_changer.h 2014-03-18 20:43:48 +0000
72@@ -34,8 +34,6 @@
73 std::shared_ptr<graphics::DisplayConfiguration> active_configuration();
74 void configure(std::shared_ptr<frontend::Session> const&, std::shared_ptr<graphics::DisplayConfiguration> const&);
75
76- void ensure_display_powered(std::shared_ptr<frontend::Session> const& session);
77-
78 private:
79 std::shared_ptr<frontend::DisplayChanger> const changer;
80 };
81
82=== modified file 'src/server/scene/mediating_display_changer.cpp'
83--- src/server/scene/mediating_display_changer.cpp 2014-03-06 06:05:17 +0000
84+++ src/server/scene/mediating_display_changer.cpp 2014-03-18 20:43:48 +0000
85@@ -117,29 +117,6 @@
86
87 }
88
89-void ms::MediatingDisplayChanger::ensure_display_powered(std::shared_ptr<mf::Session> const& session)
90-{
91- std::lock_guard<std::mutex> lg{configuration_mutex};
92- bool switched = false;
93-
94- auto it = config_map.find(session);
95- if (it == config_map.end())
96- return;
97- auto conf = it->second;
98- conf->for_each_output([&](mg::UserDisplayConfigurationOutput& output) -> void
99- {
100- if (!output.used) return;
101-
102- if (output.power_mode != mir_power_mode_on)
103- {
104- switched = true;
105- output.power_mode = mir_power_mode_on;
106- }
107- });
108- if (switched)
109- configure(session, conf);
110-}
111-
112 void ms::MediatingDisplayChanger::configure(
113 std::shared_ptr<mf::Session> const& session,
114 std::shared_ptr<mg::DisplayConfiguration> const& conf)
115
116=== modified file 'src/server/scene/mediating_display_changer.h'
117--- src/server/scene/mediating_display_changer.h 2014-03-06 06:05:17 +0000
118+++ src/server/scene/mediating_display_changer.h 2014-03-18 20:43:48 +0000
119@@ -55,8 +55,6 @@
120 void configure(std::shared_ptr<frontend::Session> const& session,
121 std::shared_ptr<graphics::DisplayConfiguration> const& conf);
122
123- void ensure_display_powered(std::shared_ptr<frontend::Session> const& session);
124-
125 /* From mir::DisplayChanger */
126 void configure_for_hardware_change(
127 std::shared_ptr<graphics::DisplayConfiguration> const& conf,

Subscribers

People subscribed via source and target branches