Mir

Merge lp:~andreas-pokorny/mir/no-initial-display-configuration-sent-to-hosting-server into lp:mir

no-initial-display-configuration-sent-to-hosting-server
Merge into development-branch

Proposed by Andreas Pokorny on 2014-03-27

Status:

Merged

Approved by:

Andreas Pokorny on 2014-04-02

Approved revision:

no longer in the source branch.

Merged at revision:

1526

Proposed branch:

lp:~andreas-pokorny/mir/no-initial-display-configuration-sent-to-hosting-server

Merge into:

lp:mir

Diff against target:

77 lines (+20/-3)

3 files modified

src/server/graphics/nested/nested_display.cpp (+12/-3)
src/server/graphics/nested/nested_display.h (+2/-0)
tests/unit-tests/graphics/nested/test_nested_display.cpp (+6/-0)

To merge this branch:

bzr merge lp:~andreas-pokorny/mir/no-initial-display-configuration-sent-to-hosting-server

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2014-04-02
Daniel van Vugt			Needs Fixing on 2014-04-02
Alexandros Frantzis (community)			Abstain on 2014-04-01
Alan Griffiths		2014-03-27	Approve on 2014-03-31
Review via email: mp+213126@code.launchpad.net

Commit message

Workaround for Greeter turning the screen on.

This is just a workaround for the issue with the split greeter. While display is off another greeter is spawned to be shown as soon as display is turned on again. A greeter is a nested mir server. On initialization nested servers send the initial display configuration to the hosting server. Obviously that configuration contains power_mode == MirPowerMode::mir_power_mode_on.
u-s-c sets the focus onto the recently started greeter as soon as the session has a surface with buffers in. On that focus change the the stored display configuration (given they have one) is applied. Which turns the display on.

This change disables sending the first/initial nested display configuration.
We clearly need a better solution for that issue so we can remove that workaround as soon as we have one.

Description of the change

Bandaid for greeter turning screen on.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-27:

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-03-28:

Wouldn't it be better and simpler to remove the configure() call in NestedDisplay::NestedDisplay()? Along with the code needed to set up that call.

review: Needs Fixing

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-03-28:

14 initial_conf_policy->apply_to(*conf);
15 configure(*conf);

Since we are not appying the display configuration during startup, we should be configuring the nested display with the pristine display configuration. That is, we should ignore initial_conf_policy completely.

It would be good to also have tests, but the current state of the nested backend makes it difficult. However, please add tests/unit-tests/graphics/nested/test_nested_display.cpp with a DISABLED empty test, so that we can remember to implement it when we fix our infrastructure (see tests/unit-tests/graphics/nested/test_nested_platform.cpp).

I want to raise the priority of improving testability of the nested backend, but, depending on which road we take, this may take a while.

review: Needs Fixing

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-03-28:

> Wouldn't it be better and simpler to remove the configure() call in
> NestedDisplay::NestedDisplay()? Along with the code needed to set up that
> call.

We can't remove the call completely since it's in this call that we set up our outputs/framebuffers.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-03-28:

> > Wouldn't it be better and simpler to remove the configure() call in
> > NestedDisplay::NestedDisplay()? Along with the code needed to set up that
> > call.
>
> We can't remove the call completely since it's in this call that we set up our
> outputs/framebuffers.

I see. We should split out that logic to a separate function and call that from the ctor and configure()

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2014-03-28:

> 14 initial_conf_policy->apply_to(*conf);
> 15 configure(*conf);
>
> Since we are not appying the display configuration during startup, we should
> be configuring the nested display with the pristine display configuration.
> That is, we should ignore initial_conf_policy completely.

I think we need that for pixel format selection. I believe for some screen designs the greeter was meant to be partly transparent. But the current split greeter is not... Will check.

Addressing the other things now.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-29:

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-03-31:

I think there are still problems in this area, but this change doesn't introduce new ones.

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-01:

Seems reasonable. Weak "Needs Fixing" for:

(1) Maybe mention LP: #1299101 in the TODO.

(2) Wrong copyright year:
66 + * Copyright © 2013 Canonical Ltd.

Otherwise consider this Approved.

review: Needs Fixing

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-04-01:

I still don't think this is the right solution. The problem is that the nested server configuration may get out of sync with the host configuration. For example, the initial nested configuration may set different output resolutions which will lead to the creation of surfaces that will not map correctly to the real outputs (since we are not propagating that configuration to the host server).

My understanding is that we have decided that we don't want for the nested server to set a new display configuration at startup. The only aspect of the nested configuration we need to control is the translucency of the created surfaces (i.e. the surface pixel format), which is a fixed decision somewhat orthogonal to the display configuration. We may also not want to force that surface pixel format as the output format in the host display configuration because the host may not support that format for its HW framebuffers. Of course there are other scenarios to consider here: what if an authorized application changes the display configuration and tells the nested server to use an opaque pixel format. Should the nested server respect or ignore that request?

I won't block this workaround if it needs to land urgently, but I am concerned that in the name of urgency we have been pushing band-aid fixes that increase our technical debt, doing more harm than good in the medium/long-term.

review: Abstain

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-01:

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

review: Approve (continuous-integration)

Revision history for this message

Michael Terry (mterry) wrote on 2014-04-01:

I think this has merge conflicts now.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-02:

Conflict: The new file exists now. You can drop it from this proposal...

Conflict adding file tests/unit-tests/graphics/nested/test_nested_display.cpp. Moved existing file to tests/unit-tests/graphics/nested/test_nested_display.cpp.moved.
1 conflicts encountered.

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-02:

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

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Andreas Pokorny

Emanuele Antonio Faraone

Gerry Boland

Mir development team

1	=== modified file 'src/server/graphics/nested/nested_display.cpp'
2	--- src/server/graphics/nested/nested_display.cpp 2014-03-28 16:15:40 +0000
3	+++ src/server/graphics/nested/nested_display.cpp 2014-04-02 08:21:44 +0000
4	@@ -144,10 +144,9 @@
5	egl_display{*connection, gl_config},
6	outputs{}
7	{
8	-
9	std::shared_ptr<DisplayConfiguration> conf(configuration());
10	initial_conf_policy->apply_to(*conf);
11	- configure(*conf);
12	+ create_surfaces(*conf);
13	}
14
15	mgn::NestedDisplay::~NestedDisplay() noexcept
16	@@ -180,6 +179,12 @@
17
18	void mgn::NestedDisplay::configure(mg::DisplayConfiguration const& configuration)
19	{
20	+ create_surfaces(configuration);
21	+ apply_to_connection(configuration);
22	+}
23	+
24	+void mgn::NestedDisplay::create_surfaces(mg::DisplayConfiguration const& configuration)
25	+{
26	if (!configuration.valid())
27	{
28	BOOST_THROW_EXCEPTION(std::logic_error("Invalid or inconsistent display configuration"));
29	@@ -231,12 +236,16 @@
30	if (result.empty())
31	BOOST_THROW_EXCEPTION(std::runtime_error("Nested Mir needs at least one output for display"));
32
33	- auto const& conf = dynamic_cast<NestedDisplayConfiguration const&>(configuration);
34
35	{
36	std::unique_lock<std::mutex> lock(outputs_mutex);
37	outputs.swap(result);
38	}
39	+}
40	+
41	+void mgn::NestedDisplay::apply_to_connection(mg::DisplayConfiguration const& configuration)
42	+{
43	+ auto const& conf = dynamic_cast<NestedDisplayConfiguration const&>(configuration);
44
45	mir_connection_apply_display_config(*connection, conf);
46	}
47
48	=== modified file 'src/server/graphics/nested/nested_display.h'
49	--- src/server/graphics/nested/nested_display.h 2014-03-28 16:15:40 +0000
50	+++ src/server/graphics/nested/nested_display.h 2014-04-02 08:21:44 +0000
51	@@ -133,6 +133,8 @@
52	std::mutex outputs_mutex;
53	std::unordered_map<DisplayConfigurationOutputId, std::shared_ptr<detail::NestedOutput>> outputs;
54	DisplayConfigurationChangeHandler my_conf_change_handler;
55	+ void create_surfaces(mir::graphics::DisplayConfiguration const& configuration);
56	+ void apply_to_connection(mir::graphics::DisplayConfiguration const& configuration);
57	void complete_display_initialization(MirPixelFormat format);
58	};
59
60
61	=== modified file 'tests/unit-tests/graphics/nested/test_nested_display.cpp'
62	--- tests/unit-tests/graphics/nested/test_nested_display.cpp 2014-03-28 16:15:40 +0000
63	+++ tests/unit-tests/graphics/nested/test_nested_display.cpp 2014-04-02 08:21:44 +0000
64	@@ -21,7 +21,13 @@
65	/*
66	* TODO: Improve testability of NestedDisplay, so we can have proper unit tests.
67	* In particular we need a way to fake interactions with the client library.
68	+ * See https://bugs.launchpad.net/mir/+bug/1299101 for tracking.
69	*/
70	TEST(NestedDisplay, DISABLED_respects_gl_config)
71	{
72	}
73	+
74	+TEST(NestedDisplay, DISABLED_nested_display_should_not_change_power_mode_when_display_is_off)
75	+{
76	+}
77	+