Mir

Merge lp:~cemil-azizoglu/mir/more-accurate-probing into lp:mir

more-accurate-probing
Merge into development-branch

Proposed by Cemil Azizoglu on 2015-11-05

Status:

Merged

Approved by:

Cemil Azizoglu on 2015-11-10

Approved revision:

no longer in the source branch.

Merged at revision:

3098

Proposed branch:

lp:~cemil-azizoglu/mir/more-accurate-probing

Merge into:

lp:mir

Diff against target:

140 lines (+36/-11)

5 files modified

src/platforms/mesa/server/kms/platform_symbols.cpp (+21/-3)
src/platforms/mesa/server/x11/graphics/graphics.cpp (+1/-1)
tests/unit-tests/graphics/mesa/kms/test_platform.cpp (+2/-2)
tests/unit-tests/graphics/mesa/x11/test_platform.cpp (+2/-2)
tests/unit-tests/graphics/test_platform_prober.cpp (+10/-3)

To merge this branch:

bzr merge lp:~cemil-azizoglu/mir/more-accurate-probing

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2015-11-10
Andreas Pokorny (community)			Approve on 2015-11-10
Alan Griffiths		2015-11-05	Approve on 2015-11-10
Alexandros Frantzis (community)			Approve on 2015-11-10
Review via email: mp+276825@code.launchpad.net

Commit message

Tighten probing rules

Description of the change

Tighten probing rules to better differentiate between mir-on-x and kms platforms by using the ability to become 'master'.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-06:

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-11-06:

I'm not convinced that checking explicitly for an option used by a different platform scales well.

I assume checking for options not recognized by /this/ platform would give false positives by detecting options used by downstreams (I'm looking at you U8) that don't use Mir's boost.Options support.

Could we instead add a bonus to our PlatformPriority score for seeing options we recognize? (So, in this case, mesa-kms would "win" if it sees --vt.)

review: Needs Information

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-11-06:

> I'm not convinced that checking explicitly for an option used by a different platform scales well.
...
> Could we instead add a bonus to our PlatformPriority score for seeing options we recognize? (So, in > this case, mesa-kms would "win" if it sees --vt.)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-11-06:

Yeah, either give a boost for platforms that understand the option or have a filter at the platform load stage that only loads platforms that understand the options.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-07:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-07:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-07:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-09:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-11-09:

Needs Information: If the caller has insufficient rights, the call to drmSetMaster will fail.That way it would be a lot harder to lock yourself out when launching a mir_demo_server. Unless of course someone specifies the platform on command line and bypasses probing entirely.

If so I would move the drmMaster above the platform option check - or as proposed above change the early return into += x of the probing result.

review: Needs Information

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-09:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-09:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-11-10:

+ const char *argv[] = {"dummy", "--vt"};
+ options.parse_arguments(po, 2, argv);

These are not needed for any of the existing tests and also skew the results since they include a platform specific argument.

TEST_F(ServerPlatformProbeMockDRM, LoadsMesaPlatformWhenDrmDevicePresent)

The test name is now misleading. This should be renamed to LoadsMesaPlatformWhenDrmMasterCanBeAcquired or similar.

review: Needs Fixing

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-11-10:

Looks good.

review: Approve

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-11-10:

LGTM

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-10:

FAILED: Continuous integration, rev:3100
http://jenkins.qa.ubuntu.com/job/mir-ci/5537/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4848
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3755
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4790
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/748/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1691/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/748
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/748/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4791
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4791/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7389
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/25073
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/749/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-11-10:

yes

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) on 2015-11-10:

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:

Cemil Azizoglu

Emanuele Antonio Faraone

Gerry Boland

Mir development team

1	=== modified file 'src/platforms/mesa/server/kms/platform_symbols.cpp'
2	--- src/platforms/mesa/server/kms/platform_symbols.cpp 2015-11-04 07:43:28 +0000
3	+++ src/platforms/mesa/server/kms/platform_symbols.cpp 2015-11-10 15:26:16 +0000
4	@@ -164,15 +164,33 @@
5	drm_devices.match_sysname("card[0-9]*");
6	drm_devices.scan_devices();
7
8	+ if (drm_devices.begin() == drm_devices.end())
9	+ return mg::PlatformPriority::unsupported;
10	+
11	+ // Check for master
12	+ int tmp_fd = -1;
13	for (auto& device : drm_devices)
14	{
15	- static_cast<void>(device);
16	- if (platform_option_used)
17	+ tmp_fd = open(device.devnode(), O_RDWR \| O_CLOEXEC);
18	+ if (tmp_fd >= 0)
19	+ break;
20	+ }
21	+
22	+ if (tmp_fd >= 0)
23	+ {
24	+ if (drmSetMaster(tmp_fd) >= 0)
25	+ {
26	+ drmDropMaster(tmp_fd);
27	+ drmClose(tmp_fd);
28	return mg::PlatformPriority::best;
29	+ }
30	else
31	- return mg::PlatformPriority::supported;
32	+ drmClose(tmp_fd);
33	}
34
35	+ if (platform_option_used)
36	+ return mg::PlatformPriority::best;
37	+
38	return mg::PlatformPriority::unsupported;
39	}
40
41
42	=== modified file 'src/platforms/mesa/server/x11/graphics/graphics.cpp'
43	--- src/platforms/mesa/server/x11/graphics/graphics.cpp 2015-11-04 07:43:28 +0000
44	+++ src/platforms/mesa/server/x11/graphics/graphics.cpp 2015-11-10 15:26:16 +0000
45	@@ -86,7 +86,7 @@
46	drm_devices.scan_devices();
47
48	if (drm_devices.begin() != drm_devices.end())
49	- return mg::PlatformPriority::best;
50	+ return mg::PlatformPriority::supported;
51	}
52	return mg::PlatformPriority::unsupported;
53	}
54
55	=== modified file 'tests/unit-tests/graphics/mesa/kms/test_platform.cpp'
56	--- tests/unit-tests/graphics/mesa/kms/test_platform.cpp 2015-11-09 11:13:52 +0000
57	+++ tests/unit-tests/graphics/mesa/kms/test_platform.cpp 2015-11-10 15:26:16 +0000
58	@@ -319,7 +319,7 @@
59	EXPECT_EQ(mg::PlatformPriority::unsupported, probe(options));
60	}
61
62	-TEST_F(MesaGraphicsPlatform, probe_returns_supported_when_drm_devices_exist)
63	+TEST_F(MesaGraphicsPlatform, probe_returns_best_when_master)
64	{
65	mtf::UdevEnvironment udev_environment;
66	boost::program_options::options_description po;
67	@@ -329,7 +329,7 @@
68
69	mir::SharedLibrary platform_lib{mtf::server_platform("graphics-mesa-kms")};
70	auto probe = platform_lib.load_function<mg::PlatformProbe>(probe_platform);
71	- EXPECT_EQ(mg::PlatformPriority::supported, probe(options));
72	+ EXPECT_EQ(mg::PlatformPriority::best, probe(options));
73	}
74
75	TEST_F(MesaGraphicsPlatform, probe_returns_best_when_drm_devices_vt_option_exist)
76
77	=== modified file 'tests/unit-tests/graphics/mesa/x11/test_platform.cpp'
78	--- tests/unit-tests/graphics/mesa/x11/test_platform.cpp 2015-09-30 23:50:10 +0000
79	+++ tests/unit-tests/graphics/mesa/x11/test_platform.cpp 2015-11-10 15:26:16 +0000
80	@@ -116,7 +116,7 @@
81	EXPECT_EQ(mg::PlatformPriority::unsupported, probe(options));
82	}
83
84	-TEST_F(X11GraphicsPlatformTest, probe_returns_best_when_drm_render_nodes_exist)
85	+TEST_F(X11GraphicsPlatformTest, probe_returns_supported_when_drm_render_nodes_exist)
86	{
87	mtf::UdevEnvironment udev_environment;
88	mir::options::ProgramOption options;
89	@@ -125,5 +125,5 @@
90
91	mir::SharedLibrary platform_lib{mtf::server_platform("server-mesa-x11")};
92	auto probe = platform_lib.load_function<mg::PlatformProbe>(probe_platform);
93	- EXPECT_EQ(mg::PlatformPriority::best, probe(options));
94	+ EXPECT_EQ(mg::PlatformPriority::supported, probe(options));
95	}
96
97	=== modified file 'tests/unit-tests/graphics/test_platform_prober.cpp'
98	--- tests/unit-tests/graphics/test_platform_prober.cpp 2015-09-30 19:34:21 +0000
99	+++ tests/unit-tests/graphics/test_platform_prober.cpp 2015-11-10 15:26:16 +0000
100	@@ -100,6 +100,13 @@
101	return std::shared_ptr<void>{};
102	#endif
103	}
104	+
105	+class ServerPlatformProbeMockDRM : public ::testing::Test
106	+{
107	+public:
108	+ ::testing::NiceMock<mtd::MockDRM> mock_drm;
109	+};
110	+
111	}
112
113	TEST(ServerPlatformProbe, ConstructingWithNoModulesIsAnError)
114	@@ -112,7 +119,7 @@
115	}
116
117	#ifdef MIR_BUILD_PLATFORM_MESA_KMS
118	-TEST(ServerPlatformProbe, LoadsMesaPlatformWhenDrmDevicePresent)
119	+TEST_F(ServerPlatformProbeMockDRM, LoadsMesaPlatformWhenDrmMasterCanBeAcquired)
120	{
121	using namespace testing;
122	mir::options::ProgramOption options;
123	@@ -183,7 +190,7 @@
124	}
125
126	#if defined(MIR_BUILD_PLATFORM_MESA_KMS) \|\| defined(MIR_BUILD_PLATFORM_MESA_X11) \|\| defined(MIR_BUILD_PLATFORM_ANDROID)
127	-TEST(ServerPlatformProbe, LoadsMesaOrAndroidInPreferenceToDummy)
128	+TEST_F(ServerPlatformProbeMockDRM, LoadsMesaOrAndroidInPreferenceToDummy)
129	{
130	using namespace testing;
131	mir::options::ProgramOption options;
132	@@ -203,7 +210,7 @@
133	}
134	#endif
135
136	-TEST(ServerPlatformProbe, IgnoresNonPlatformModules)
137	+TEST_F(ServerPlatformProbeMockDRM, IgnoresNonPlatformModules)
138	{
139	using namespace testing;
140	mir::options::ProgramOption options;