Merge lp:~vanvugt/mir/output-model into lp:mir
- output-model
- Merge into development-branch
Status: | Merged |
---|---|
Approved by: | Daniel van Vugt |
Approved revision: | no longer in the source branch. |
Merged at revision: | 3882 |
Proposed branch: | lp:~vanvugt/mir/output-model |
Merge into: | lp:mir |
Diff against target: |
538 lines (+404/-0) 13 files modified
include/client/mir_toolkit/mir_display_configuration.h (+12/-0) src/client/display_configuration_api.cpp (+25/-0) src/client/symbols.map (+1/-0) src/common/CMakeLists.txt (+1/-0) src/common/edid.cpp (+67/-0) src/common/symbols.map (+7/-0) src/include/common/mir/graphics/edid.h (+111/-0) src/protobuf/mir_protobuf.proto (+1/-0) src/server/report/logging/display_configuration_report.cpp (+17/-0) src/utils/out.c (+4/-0) tests/acceptance-tests/test_new_display_configuration.cpp (+84/-0) tests/unit-tests/CMakeLists.txt (+1/-0) tests/unit-tests/test_edid.cpp (+73/-0) |
To merge this branch: | bzr merge lp:~vanvugt/mir/output-model |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Mir CI Bot | continuous-integration | Approve | |
Alan Griffiths | Needs Fixing | ||
Cemil Azizoglu (community) | Approve | ||
Mir development team | Pending | ||
Review via email: mp+312880@code.launchpad.net |
This proposal supersedes a proposal from 2016-12-01.
Commit message
Add descriptive strings for the connected monitor, if available.
Description of the change
Mir CI Bot (mir-ci-bot) wrote : Posted in a previous version of this proposal | # |
Mir CI Bot (mir-ci-bot) wrote : Posted in a previous version of this proposal | # |
PASSED: Continuous integration, rev:3865
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Hmm, I might have gone overboard with the fallback logic. That's needed apparently on laptops and tablets which don't provide a nice "monitor name". There's a risk though that the fallback vendor and product codes are just confusing to people. Although they're useful to me as someone who likes to modify/fix laptop screens, that's not a user-friendly activity.
Cemil Azizoglu (cemil-azizoglu) wrote : Posted in a previous version of this proposal | # |
Looks good other than the typo ('nul' instead of 'null')
+ * \returns A nul-terminated string or NULL if none available. This string
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
That's not a typo. "nul" refers to character '\0' to distinguish it from "NULL" which is a pointer.
https:/
Although Google suggests the industry has changed in recent years and often likes to spell it as "null" these days. I don't mind too much either way but "nul" with a single L is historically the more accurate term when referring to character zero.
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
Mild "needs fixing" for the magic numbers (in particular there's a relationship between 13 and 14 that ought to be explicit).
Cemil Azizoglu (cemil-azizoglu) wrote : Posted in a previous version of this proposal | # |
> That's not a typo. "nul" refers to character '\0' to distinguish it from
> "NULL" which is a pointer.
>
> https:/
>
> Although Google suggests the industry has changed in recent years and often
> likes to spell it as "null" these days. I don't mind too much either way but
> "nul" with a single L is historically the more accurate term when referring to
> character zero.
Didn't know that. Thanks.
Mir CI Bot (mir-ci-bot) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:3868
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
^^^
Bug 1616312
Mir CI Bot (mir-ci-bot) wrote : Posted in a previous version of this proposal | # |
PASSED: Continuous integration, rev:3869
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
54 and 4 are still "magic"
Mir CI Bot (mir-ci-bot) wrote : | # |
FAILED: Continuous integration, rev:3886
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Mir CI Bot (mir-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:3887
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Cemil Azizoglu (cemil-azizoglu) wrote : | # |
Lots of fields
Alan Griffiths (alan-griffiths) wrote : | # |
+struct EDID
1. I don't see any advantage to having the implementation inline. Can we put it in a .cpp file?
2. Having an UPPERCASE name is potentially confusing - we use that for MACROS.
Mir CI Bot (mir-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:3889
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Mir CI Bot (mir-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:3890
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Alan Griffiths (alan-griffiths) wrote : | # |
*Needs Discussion*
Should Edid belong in namespace mir::graphics in preference to namespace mir?
Daniel van Vugt (vanvugt) wrote : | # |
I think the graphics namespace is a bit pointless (or at least needs a better name). The whole project is about graphics so saying something is in the "graphics" namespace does not really contribute to an understanding of the code, or to namespace safety.
Generally I think mir::graphics:: should be abolished unless someone can think of a better name that justifies its existence.
Alan Griffiths (alan-griffiths) wrote : | # |
> I think the graphics namespace is a bit pointless (or at least needs a better
> name). The whole project is about graphics so saying something is in the
> "graphics" namespace does not really contribute to an understanding of the
> code, or to namespace safety.
>
> Generally I think mir::graphics:: should be abolished unless someone can think
> of a better name that justifies its existence.
Changing our long standing namespaces isn't part of this MP.
Kevin DuBois (kdub) wrote : | # |
I suppose that edid fits in the graphics namespace of mir better than any other namespace. mir:: is more for common stuff, and it doesn't fit in input, frontend, shell, compositor, etc
Chris Halse Rogers (raof) wrote : | # |
+1 on mir::graphics being the sensible spot for it.
213 + enum { minimum_size = 128 };
This would be more idiomatic as static constexpr, but that's non-blocking.
Either here, or in future work, it would make sense to validate the checksum on the EDID. This could be done in a static “constructor” in Edid::. Which would also be nice, rather than have the expectation that you reinterpret_cast<> a bunch of bytes into a mg::Edid.
Daniel van Vugt (vanvugt) wrote : | # |
I considered header and checksum validation and decided intentionally that would be a bad idea. Buggy hardware exists, and we should support it as well as we can, even when they fail to checksum their EDID correctly.
This is based on seeing many logs over the years in which users' displays provided invalid EDIDs. We can't fix all hardware, so we should go best effort instead of assuming that no EDID is better than a badly formed EDID.
Daniel van Vugt (vanvugt) wrote : | # |
Also I intentionally wrote this code to deal with any form or badly formed EDID already.
Mir CI Bot (mir-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:3892
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Daniel van Vugt (vanvugt) wrote : | # |
Alright... two weeks later. I've now resolved all the stylistic complaints raised. That's enough. Trying to land it now.
Chris Halse Rogers (raof) wrote : | # |
I don't mean fail to parse the EDID on checksum failures, but to expose that the EDID is broken. For some things (particularly modes) a broken EDID *is* worse than no EDID.
Sometimes the checksum fails because the manufacturer failed to compute the checksum correctly. More often it's broken because the EDID, or the DDC pins, or something is actually broken and should be trusted less.
Hm. While I think of it, checking that all the strings we return are printable ascii is probably a sensible thing to do, particularly if we're going to accept known-bad EDIDs
Mir CI Bot (mir-ci-bot) : | # |
Preview Diff
1 | === modified file 'include/client/mir_toolkit/mir_display_configuration.h' |
2 | --- include/client/mir_toolkit/mir_display_configuration.h 2016-12-08 04:08:06 +0000 |
3 | +++ include/client/mir_toolkit/mir_display_configuration.h 2016-12-14 04:10:39 +0000 |
4 | @@ -373,6 +373,18 @@ |
5 | void mir_output_disable(MirOutput* output); |
6 | |
7 | /** |
8 | + * Get a descriptive manufacturer/model string for the connected display. |
9 | + * The format of this string is arbitrary and driver-specific but should be |
10 | + * human-readable and helpful for someone to identify which physical display |
11 | + * this is. Note this function is not called get_name because that would imply |
12 | + * the returned value is different for each output, whereas it may not be. |
13 | + * |
14 | + * \returns A nul-terminated string or NULL if none available. This string |
15 | + * remains valid for the lifetime of the MirOutput object. |
16 | + */ |
17 | +char const* mir_output_get_model(MirOutput const* output); |
18 | + |
19 | +/** |
20 | * Get the physical width of the connected display, in millimetres. |
21 | * |
22 | * A best-effort report of the physical width of the display connected to this |
23 | |
24 | === modified file 'src/client/display_configuration_api.cpp' |
25 | --- src/client/display_configuration_api.cpp 2016-12-08 04:08:06 +0000 |
26 | +++ src/client/display_configuration_api.cpp 2016-12-14 04:10:39 +0000 |
27 | @@ -21,6 +21,7 @@ |
28 | #include "mir/output_type_names.h" |
29 | #include "display_configuration.h" |
30 | #include "mir/uncaught.h" |
31 | +#include "mir/graphics/edid.h" |
32 | |
33 | namespace mcl = mir::client; |
34 | namespace mp = mir::protobuf; |
35 | @@ -86,6 +87,30 @@ |
36 | output->set_used(0); |
37 | } |
38 | |
39 | +char const* mir_output_get_model(MirOutput const* output) |
40 | +{ |
41 | + // In future this might be provided by the server itself... |
42 | + if (output->has_model()) |
43 | + return output->model().c_str(); |
44 | + |
45 | + // But if not we use the same member for caching our EDID probe... |
46 | + using mir::graphics::Edid; |
47 | + if (mir_output_get_edid_size(output) >= Edid::minimum_size) |
48 | + { |
49 | + auto edid = reinterpret_cast<Edid const*>(mir_output_get_edid(output)); |
50 | + Edid::MonitorName name; |
51 | + if (!edid->get_monitor_name(name)) |
52 | + { |
53 | + auto len = edid->get_manufacturer(name); |
54 | + snprintf(name+len, sizeof(name)-len, " %hu", edid->product_code()); |
55 | + } |
56 | + const_cast<MirOutput*>(output)->set_model(name); |
57 | + return output->model().c_str(); |
58 | + } |
59 | + |
60 | + return nullptr; |
61 | +} |
62 | + |
63 | int mir_display_config_get_max_simultaneous_outputs(MirDisplayConfig const* config) |
64 | { |
65 | return config->display_card(0).max_simultaneous_outputs(); |
66 | |
67 | === modified file 'src/client/symbols.map' |
68 | --- src/client/symbols.map 2016-12-13 02:50:57 +0000 |
69 | +++ src/client/symbols.map 2016-12-14 04:10:39 +0000 |
70 | @@ -502,4 +502,5 @@ |
71 | mir_touchpad_configuration_set_middle_mouse_button_emulation; |
72 | mir_touchpad_configuration_set_scroll_modes; |
73 | mir_touchpad_configuration_set_tap_to_click; |
74 | + mir_output_get_model; |
75 | } MIR_CLIENT_0.25; |
76 | |
77 | === modified file 'src/common/CMakeLists.txt' |
78 | --- src/common/CMakeLists.txt 2016-11-15 23:48:01 +0000 |
79 | +++ src/common/CMakeLists.txt 2016-12-14 04:10:39 +0000 |
80 | @@ -22,6 +22,7 @@ |
81 | ${CMAKE_CURRENT_SOURCE_DIR}/libname.cpp ${PROJECT_SOURCE_DIR}/include/common/mir/libname.h |
82 | ${PROJECT_SOURCE_DIR}/include/common/mir/posix_rw_mutex.h |
83 | posix_rw_mutex.cpp |
84 | + edid.cpp |
85 | ) |
86 | |
87 | set(PREFIX "${CMAKE_INSTALL_PREFIX}") |
88 | |
89 | === added file 'src/common/edid.cpp' |
90 | --- src/common/edid.cpp 1970-01-01 00:00:00 +0000 |
91 | +++ src/common/edid.cpp 2016-12-14 04:10:39 +0000 |
92 | @@ -0,0 +1,67 @@ |
93 | +/* |
94 | + * Copyright © 2016 Canonical Ltd. |
95 | + * |
96 | + * This program is free software: you can redistribute it and/or modify it |
97 | + * under the terms of the GNU Lesser General Public License version 3, |
98 | + * as published by the Free Software Foundation. |
99 | + * |
100 | + * This program is distributed in the hope that it will be useful, |
101 | + * but WITHOUT ANY WARRANTY; without even the implied warranty of |
102 | + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
103 | + * GNU Lesser General Public License for more details. |
104 | + * |
105 | + * You should have received a copy of the GNU Lesser General Public License |
106 | + * along with this program. If not, see <http://www.gnu.org/licenses/>. |
107 | + * |
108 | + * Authored by: Daniel van Vugt <daniel.van.vugt@canonical.com> |
109 | + */ |
110 | + |
111 | +#include "mir/graphics/edid.h" |
112 | +#include <endian.h> |
113 | +#include <cstring> |
114 | + |
115 | +using mir::graphics::Edid; |
116 | + |
117 | +size_t Edid::get_monitor_name(MonitorName str) const |
118 | +{ |
119 | + size_t len = get_string(string_monitor_name, str); |
120 | + if (char* pad = strchr(str, '\n')) |
121 | + { |
122 | + *pad = '\0'; |
123 | + len = pad - str; |
124 | + } |
125 | + return len; |
126 | +} |
127 | + |
128 | +size_t Edid::get_manufacturer(Manufacturer str) const |
129 | +{ |
130 | + // Confusingly this field is more like big endian. Others are little. |
131 | + auto man = static_cast<uint16_t>(manufacturer[0]) << 8 | manufacturer[1]; |
132 | + str[0] = ((man >> 10) & 31) + 'A' - 1; |
133 | + str[1] = ((man >> 5) & 31) + 'A' - 1; |
134 | + str[2] = (man & 31) + 'A' - 1; |
135 | + str[3] = '\0'; |
136 | + return 3; |
137 | +} |
138 | + |
139 | +uint16_t Edid::product_code() const |
140 | +{ |
141 | + return le16toh(product_code_le); |
142 | +} |
143 | + |
144 | +size_t Edid::get_string(StringDescriptorType type, char str[14]) const |
145 | +{ |
146 | + size_t len = 0; |
147 | + for (int d = 0; d < 4; ++d) |
148 | + { |
149 | + auto& desc = descriptor[d]; |
150 | + if (!desc.other.zero0 && desc.other.type == type) |
151 | + { |
152 | + len = sizeof desc.other.text; |
153 | + memcpy(str, desc.other.text, len); |
154 | + break; |
155 | + } |
156 | + } |
157 | + str[len] = '\0'; |
158 | + return len; |
159 | +} |
160 | |
161 | === modified file 'src/common/symbols.map' |
162 | --- src/common/symbols.map 2016-11-15 23:48:01 +0000 |
163 | +++ src/common/symbols.map 2016-12-14 04:10:39 +0000 |
164 | @@ -403,3 +403,10 @@ |
165 | mir::PosixRWMutex::unlock_shared*; |
166 | }; |
167 | } MIR_COMMON_0.25; |
168 | + |
169 | +MIR_COMMON_0.26 { |
170 | + global: |
171 | + extern "C++" { |
172 | + mir::graphics::Edid::*; |
173 | + }; |
174 | +} MIR_COMMON_0.25; |
175 | |
176 | === added directory 'src/include/common/mir/graphics' |
177 | === added file 'src/include/common/mir/graphics/edid.h' |
178 | --- src/include/common/mir/graphics/edid.h 1970-01-01 00:00:00 +0000 |
179 | +++ src/include/common/mir/graphics/edid.h 2016-12-14 04:10:39 +0000 |
180 | @@ -0,0 +1,111 @@ |
181 | +/* |
182 | + * Copyright © 2016 Canonical Ltd. |
183 | + * |
184 | + * This program is free software: you can redistribute it and/or modify it |
185 | + * under the terms of the GNU Lesser General Public License version 3, |
186 | + * as published by the Free Software Foundation. |
187 | + * |
188 | + * This program is distributed in the hope that it will be useful, |
189 | + * but WITHOUT ANY WARRANTY; without even the implied warranty of |
190 | + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
191 | + * GNU Lesser General Public License for more details. |
192 | + * |
193 | + * You should have received a copy of the GNU Lesser General Public License |
194 | + * along with this program. If not, see <http://www.gnu.org/licenses/>. |
195 | + * |
196 | + * Authored by: Daniel van Vugt <daniel.van.vugt@canonical.com> |
197 | + */ |
198 | + |
199 | +#ifndef MIR_GRAPHICS_EDID_H_ |
200 | +#define MIR_GRAPHICS_EDID_H_ |
201 | + |
202 | +#include <cstdint> |
203 | +#include <cstddef> |
204 | + |
205 | +namespace mir { namespace graphics { |
206 | + |
207 | +struct Edid |
208 | +{ |
209 | + Edid() = delete; |
210 | + Edid(Edid const&) = delete; |
211 | + Edid(Edid const&&) = delete; |
212 | + |
213 | + enum { minimum_size = 128 }; |
214 | + typedef char MonitorName[14]; // up to 13 characters |
215 | + typedef char Manufacturer[4]; // always 3 characters |
216 | + |
217 | + size_t get_monitor_name(MonitorName str) const; |
218 | + size_t get_manufacturer(Manufacturer str) const; |
219 | + uint16_t product_code() const; |
220 | + |
221 | +private: |
222 | + /* Pretty much every field in an EDID requires some kind of conversion |
223 | + and reinterpretation. So keep those details private... */ |
224 | + |
225 | + enum StringDescriptorType |
226 | + { |
227 | + string_monitor_serial_number = 0xff, |
228 | + string_unspecified_text = 0xfe, |
229 | + string_monitor_name = 0xfc, |
230 | + }; |
231 | + |
232 | + size_t get_string(StringDescriptorType type, char str[14]) const; |
233 | + |
234 | + union Descriptor |
235 | + { |
236 | + struct |
237 | + { |
238 | + uint16_t pixel_clock_le; |
239 | + uint8_t todo[16]; |
240 | + } detailed_timing; |
241 | + struct |
242 | + { |
243 | + uint16_t zero0; |
244 | + uint8_t zero2; |
245 | + uint8_t type; |
246 | + uint8_t zero4; |
247 | + uint8_t text[13]; |
248 | + } other; |
249 | + }; |
250 | + |
251 | +#ifdef __clang__ |
252 | +#pragma clang diagnostic push |
253 | +#pragma clang diagnostic ignored "-Wunused-private-field" |
254 | +#endif |
255 | + /* 0x00 */ uint8_t header[8]; |
256 | + /* 0x08 */ uint8_t manufacturer[2]; |
257 | + /* 0x0a */ uint16_t product_code_le; |
258 | + /* 0x0c */ uint32_t serial_number_le; |
259 | + /* 0x10 */ uint8_t week_of_manufacture; |
260 | + /* 0x11 */ uint8_t year_of_manufacture; |
261 | + /* 0x12 */ uint8_t edid_version; |
262 | + /* 0x13 */ uint8_t edid_revision; |
263 | + /* 0x14 */ uint8_t input_bitmap; |
264 | + /* 0x15 */ uint8_t max_horz_cm; |
265 | + /* 0x16 */ uint8_t max_vert_cm; |
266 | + /* 0x17 */ uint8_t gamma; |
267 | + /* 0x18 */ uint8_t features_bitmap; |
268 | + /* 0x19 */ uint8_t red_green_bits_1to0; |
269 | + /* 0x1a */ uint8_t blue_white_bits_1to0; |
270 | + /* 0x1b */ uint8_t red_x_bits_9to2; |
271 | + /* 0x1c */ uint8_t red_y_bits_9to2; |
272 | + /* 0x1d */ uint8_t green_x_bits_9to2; |
273 | + /* 0x1e */ uint8_t green_y_bits_9to2; |
274 | + /* 0x1f */ uint8_t blue_x_bits_9to2; |
275 | + /* 0x20 */ uint8_t blue_y_bits_9to2; |
276 | + /* 0x21 */ uint8_t white_x_bits_9to2; |
277 | + /* 0x22 */ uint8_t white_y_bits_9to2; |
278 | + /* 0x23 */ uint8_t established_timings[2]; |
279 | + /* 0x25 */ uint8_t reserved_timings; |
280 | + /* 0x26 */ uint8_t standard_timings[2][8]; |
281 | + /* 0x36 */ Descriptor descriptor[4]; |
282 | + /* 0x7e */ uint8_t num_extensions; /* each is another 128-byte block */ |
283 | + /* 0x7f */ uint8_t checksum; |
284 | +#ifdef __clang__ |
285 | +#pragma clang diagnostic pop |
286 | +#endif |
287 | +}; |
288 | + |
289 | +} } // namespace mir::graphics |
290 | + |
291 | +#endif // MIR_GRAPHICS_EDID_H_ |
292 | |
293 | === modified file 'src/protobuf/mir_protobuf.proto' |
294 | --- src/protobuf/mir_protobuf.proto 2016-12-08 04:08:06 +0000 |
295 | +++ src/protobuf/mir_protobuf.proto 2016-12-14 04:10:39 +0000 |
296 | @@ -241,6 +241,7 @@ |
297 | optional bytes gamma_blue = 22; |
298 | optional uint32 gamma_supported = 23; |
299 | optional bytes edid = 24; |
300 | + optional string model = 25; |
301 | } |
302 | |
303 | message Connection { |
304 | |
305 | === modified file 'src/server/report/logging/display_configuration_report.cpp' |
306 | --- src/server/report/logging/display_configuration_report.cpp 2016-11-30 03:00:24 +0000 |
307 | +++ src/server/report/logging/display_configuration_report.cpp 2016-12-14 04:10:39 +0000 |
308 | @@ -20,6 +20,7 @@ |
309 | #include "mir/graphics/display_configuration.h" |
310 | #include "mir/output_type_names.h" |
311 | #include "mir/logging/logger.h" |
312 | +#include "mir/graphics/edid.h" |
313 | |
314 | #include <boost/exception/diagnostic_information.hpp> |
315 | #include <cmath> |
316 | @@ -93,6 +94,22 @@ |
317 | ); |
318 | if (out.connected) |
319 | { |
320 | + using mir::graphics::Edid; |
321 | + if (out.edid.size() >= Edid::minimum_size) |
322 | + { |
323 | + auto edid = reinterpret_cast<Edid const*>(out.edid.data()); |
324 | + Edid::MonitorName name; |
325 | + if (edid->get_monitor_name(name)) |
326 | + logger->log(component, severity, |
327 | + "%sEDID monitor name: %s", indent, name); |
328 | + Edid::Manufacturer man; |
329 | + edid->get_manufacturer(man); |
330 | + logger->log(component, severity, |
331 | + "%sEDID manufacturer: %s", indent, man); |
332 | + logger->log(component, severity, |
333 | + "%sEDID product code: %hu", indent, edid->product_code()); |
334 | + } |
335 | + |
336 | int width_mm = out.physical_size_mm.width.as_int(); |
337 | int height_mm = out.physical_size_mm.height.as_int(); |
338 | float inches = |
339 | |
340 | === modified file 'src/utils/out.c' |
341 | --- src/utils/out.c 2016-12-08 04:08:06 +0000 |
342 | +++ src/utils/out.c 2016-12-14 04:10:39 +0000 |
343 | @@ -426,6 +426,10 @@ |
344 | |
345 | if (state == mir_output_connection_state_connected) |
346 | { |
347 | + char const* model = mir_output_get_model(out); |
348 | + if (model) |
349 | + printf(", \"%s\"", model); |
350 | + |
351 | MirOutputMode const* current_mode = |
352 | mir_output_get_current_mode(out); |
353 | if (current_mode) |
354 | |
355 | === modified file 'tests/acceptance-tests/test_new_display_configuration.cpp' |
356 | --- tests/acceptance-tests/test_new_display_configuration.cpp 2016-12-08 04:08:06 +0000 |
357 | +++ tests/acceptance-tests/test_new_display_configuration.cpp 2016-12-14 04:10:39 +0000 |
358 | @@ -1085,6 +1085,90 @@ |
359 | client.disconnect(); |
360 | } |
361 | |
362 | +TEST_F(DisplayConfigurationTest, client_receives_model_string_from_edid) |
363 | +{ |
364 | + static unsigned char const edid[] = |
365 | + "\x00\xff\xff\xff\xff\xff\xff\x00\x10\xac\x46\xf0\x4c\x4a\x31\x41" |
366 | + "\x05\x19\x01\x04\xb5\x34\x20\x78\x3a\x1d\xf5\xae\x4f\x35\xb3\x25" |
367 | + "\x0d\x50\x54\xa5\x4b\x00\x81\x80\xa9\x40\xd1\x00\x71\x4f\x01\x01" |
368 | + "\x01\x01\x01\x01\x01\x01\x28\x3c\x80\xa0\x70\xb0\x23\x40\x30\x20" |
369 | + "\x36\x00\x06\x44\x21\x00\x00\x1a\x00\x00\x00\xff\x00\x59\x43\x4d" |
370 | + "\x30\x46\x35\x31\x52\x41\x31\x4a\x4c\x0a\x00\x00\x00\xfc\x00\x44" |
371 | + "\x45\x4c\x4c\x20\x55\x32\x34\x31\x33\x0a\x20\x20\x00\x00\x00\xfd" |
372 | + "\x00\x38\x4c\x1e\x51\x11\x00\x0a\x20\x20\x20\x20\x20\x20\x01\x42" |
373 | + "\x02\x03\x1d\xf1\x50\x90\x05\x04\x03\x02\x07\x16\x01\x1f\x12\x13" |
374 | + "\x14\x20\x15\x11\x06\x23\x09\x1f\x07\x83\x01\x00\x00\x02\x3a\x80" |
375 | + "\x18\x71\x38\x2d\x40\x58\x2c\x45\x00\x06\x44\x21\x00\x00\x1e\x01" |
376 | + "\x1d\x80\x18\x71\x1c\x16\x20\x58\x2c\x25\x00\x06\x44\x21\x00\x00" |
377 | + "\x9e\x01\x1d\x00\x72\x51\xd0\x1e\x20\x6e\x28\x55\x00\x06\x44\x21" |
378 | + "\x00\x00\x1e\x8c\x0a\xd0\x8a\x20\xe0\x2d\x10\x10\x3e\x96\x00\x06" |
379 | + "\x44\x21\x00\x00\x18\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" |
380 | + "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x09"; |
381 | + |
382 | + mtd::StubDisplayConfigurationOutput monitor{ |
383 | + mg::DisplayConfigurationOutputId{48}, |
384 | + {{{1920, 1200}, 60.0}}, |
385 | + {mir_pixel_format_abgr_8888}}; |
386 | + monitor.edid.assign(edid, edid+sizeof(edid)-1); |
387 | + |
388 | + auto config = std::make_shared<mtd::StubDisplayConfig>( |
389 | + std::vector<mg::DisplayConfigurationOutput>{monitor}); |
390 | + |
391 | + apply_config_change_and_wait_for_propagation(config); |
392 | + |
393 | + DisplayClient client{new_connection()}; |
394 | + client.connect(); |
395 | + |
396 | + auto base_config = client.get_base_config(); |
397 | + auto output = mir_display_config_get_output(base_config.get(), 0); |
398 | + |
399 | + EXPECT_STREQ("DELL U2413", mir_output_get_model(output)); |
400 | + |
401 | + client.disconnect(); |
402 | +} |
403 | + |
404 | +TEST_F(DisplayConfigurationTest, client_receives_fallback_string_from_edid) |
405 | +{ |
406 | + static unsigned char const edid[] = |
407 | + "\x00\xff\xff\xff\xff\xff\xff\x00\x10\xac\x46\xf0\x4c\x4a\x31\x41" |
408 | + "\x05\x19\x01\x04\xb5\x34\x20\x78\x3a\x1d\xf5\xae\x4f\x35\xb3\x25" |
409 | + "\x0d\x50\x54\xa5\x4b\x00\x81\x80\xa9\x40\xd1\x00\x71\x4f\x01\x01" |
410 | + "\x01\x01\x01\x01\x01\x01\x28\x3c\x80\xa0\x70\xb0\x23\x40\x30\x20" |
411 | + "\x36\x00\x06\x44\x21\x00\x00\x1a\x00\x00\x00\xff\x00\x59\x43\x4d" |
412 | + "\x30\x46\x35\x31\x52\x41\x31\x4a\x4c\x0a\x00\x00\x00\x11\x00\x44" |
413 | + "\x45\x4c\x4c\x20\x55\x32\x34\x31\x33\x0a\x20\x20\x00\x00\x00\xfd" |
414 | + "\x00\x38\x4c\x1e\x51\x11\x00\x0a\x20\x20\x20\x20\x20\x20\x01\x42" |
415 | + "\x02\x03\x1d\xf1\x50\x90\x05\x04\x03\x02\x07\x16\x01\x1f\x12\x13" |
416 | + "\x14\x20\x15\x11\x06\x23\x09\x1f\x07\x83\x01\x00\x00\x02\x3a\x80" |
417 | + "\x18\x71\x38\x2d\x40\x58\x2c\x45\x00\x06\x44\x21\x00\x00\x1e\x01" |
418 | + "\x1d\x80\x18\x71\x1c\x16\x20\x58\x2c\x25\x00\x06\x44\x21\x00\x00" |
419 | + "\x9e\x01\x1d\x00\x72\x51\xd0\x1e\x20\x6e\x28\x55\x00\x06\x44\x21" |
420 | + "\x00\x00\x1e\x8c\x0a\xd0\x8a\x20\xe0\x2d\x10\x10\x3e\x96\x00\x06" |
421 | + "\x44\x21\x00\x00\x18\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" |
422 | + "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x09"; |
423 | + |
424 | + mtd::StubDisplayConfigurationOutput monitor{ |
425 | + mg::DisplayConfigurationOutputId{2}, |
426 | + {{{3210, 2800}, 60.0}}, |
427 | + {mir_pixel_format_abgr_8888}}; |
428 | + monitor.edid.assign(edid, edid+sizeof(edid)-1); |
429 | + |
430 | + auto config = std::make_shared<mtd::StubDisplayConfig>( |
431 | + std::vector<mg::DisplayConfigurationOutput>{monitor}); |
432 | + |
433 | + apply_config_change_and_wait_for_propagation(config); |
434 | + |
435 | + DisplayClient client{new_connection()}; |
436 | + client.connect(); |
437 | + |
438 | + auto base_config = client.get_base_config(); |
439 | + auto output = mir_display_config_get_output(base_config.get(), 0); |
440 | + |
441 | + EXPECT_STREQ("DEL 61510", mir_output_get_model(output)); |
442 | + |
443 | + client.disconnect(); |
444 | +} |
445 | + |
446 | namespace |
447 | { |
448 | MATCHER_P(IsSameModeAs, mode, "") |
449 | |
450 | === modified file 'tests/unit-tests/CMakeLists.txt' |
451 | --- tests/unit-tests/CMakeLists.txt 2016-11-15 23:48:01 +0000 |
452 | +++ tests/unit-tests/CMakeLists.txt 2016-12-14 04:10:39 +0000 |
453 | @@ -82,6 +82,7 @@ |
454 | test_posix_rw_mutex.cpp |
455 | test_posix_timestamp.cpp |
456 | test_observer_multiplexer.cpp |
457 | + test_edid.cpp |
458 | ) |
459 | |
460 | CMAKE_DEPENDENT_OPTION( |
461 | |
462 | === added file 'tests/unit-tests/test_edid.cpp' |
463 | --- tests/unit-tests/test_edid.cpp 1970-01-01 00:00:00 +0000 |
464 | +++ tests/unit-tests/test_edid.cpp 2016-12-14 04:10:39 +0000 |
465 | @@ -0,0 +1,73 @@ |
466 | +/* |
467 | + * Copyright © 2016 Canonical Ltd. |
468 | + * |
469 | + * This program is free software: you can redistribute it and/or modify it |
470 | + * under the terms of the GNU General Public License version 3, |
471 | + * as published by the Free Software Foundation. |
472 | + * |
473 | + * This program is distributed in the hope that it will be useful, |
474 | + * but WITHOUT ANY WARRANTY; without even the implied warranty of |
475 | + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
476 | + * GNU General Public License for more details. |
477 | + * |
478 | + * You should have received a copy of the GNU General Public License |
479 | + * along with this program. If not, see <http://www.gnu.org/licenses/>. |
480 | + * |
481 | + * Author: Daniel van Vugt <daniel.van.vugt@canonical.com> |
482 | + */ |
483 | + |
484 | +#include "mir/graphics/edid.h" |
485 | +#include <gtest/gtest.h> |
486 | + |
487 | +using mir::graphics::Edid; |
488 | + |
489 | +namespace |
490 | +{ |
491 | +unsigned char const dell_u2413_edid[] = |
492 | + "\x00\xff\xff\xff\xff\xff\xff\x00\x10\xac\x46\xf0\x4c\x4a\x31\x41" |
493 | + "\x05\x19\x01\x04\xb5\x34\x20\x78\x3a\x1d\xf5\xae\x4f\x35\xb3\x25" |
494 | + "\x0d\x50\x54\xa5\x4b\x00\x81\x80\xa9\x40\xd1\x00\x71\x4f\x01\x01" |
495 | + "\x01\x01\x01\x01\x01\x01\x28\x3c\x80\xa0\x70\xb0\x23\x40\x30\x20" |
496 | + "\x36\x00\x06\x44\x21\x00\x00\x1a\x00\x00\x00\xff\x00\x59\x43\x4d" |
497 | + "\x30\x46\x35\x31\x52\x41\x31\x4a\x4c\x0a\x00\x00\x00\xfc\x00\x44" |
498 | + "\x45\x4c\x4c\x20\x55\x32\x34\x31\x33\x0a\x20\x20\x00\x00\x00\xfd" |
499 | + "\x00\x38\x4c\x1e\x51\x11\x00\x0a\x20\x20\x20\x20\x20\x20\x01\x42" |
500 | + "\x02\x03\x1d\xf1\x50\x90\x05\x04\x03\x02\x07\x16\x01\x1f\x12\x13" |
501 | + "\x14\x20\x15\x11\x06\x23\x09\x1f\x07\x83\x01\x00\x00\x02\x3a\x80" |
502 | + "\x18\x71\x38\x2d\x40\x58\x2c\x45\x00\x06\x44\x21\x00\x00\x1e\x01" |
503 | + "\x1d\x80\x18\x71\x1c\x16\x20\x58\x2c\x25\x00\x06\x44\x21\x00\x00" |
504 | + "\x9e\x01\x1d\x00\x72\x51\xd0\x1e\x20\x6e\x28\x55\x00\x06\x44\x21" |
505 | + "\x00\x00\x1e\x8c\x0a\xd0\x8a\x20\xe0\x2d\x10\x10\x3e\x96\x00\x06" |
506 | + "\x44\x21\x00\x00\x18\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" |
507 | + "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x09"; |
508 | +} // namespace |
509 | + |
510 | +TEST(EDID, has_correct_size) |
511 | +{ |
512 | + EXPECT_EQ(128u, sizeof(Edid)); |
513 | + EXPECT_EQ(128u, Edid::minimum_size); |
514 | +} |
515 | + |
516 | +TEST(EDID, can_get_name) |
517 | +{ |
518 | + auto edid = reinterpret_cast<Edid const*>(dell_u2413_edid); |
519 | + Edid::MonitorName name; |
520 | + int len = edid->get_monitor_name(name); |
521 | + EXPECT_EQ(10, len); |
522 | + EXPECT_STREQ("DELL U2413", name); |
523 | +} |
524 | + |
525 | +TEST(EDID, can_get_manufacturer) |
526 | +{ |
527 | + auto edid = reinterpret_cast<Edid const*>(dell_u2413_edid); |
528 | + Edid::Manufacturer man; |
529 | + int len = edid->get_manufacturer(man); |
530 | + EXPECT_EQ(3, len); |
531 | + EXPECT_STREQ("DEL", man); |
532 | +} |
533 | + |
534 | +TEST(EDID, can_get_product_code) |
535 | +{ |
536 | + auto edid = reinterpret_cast<Edid const*>(dell_u2413_edid); |
537 | + EXPECT_EQ(61510u, edid->product_code()); |
538 | +} |
PASSED: Continuous integration, rev:3864 /mir-jenkins. ubuntu. com/job/ mir-ci/ 2299/ /mir-jenkins. ubuntu. com/job/ build-mir/ 2992 /mir-jenkins. ubuntu. com/job/ build-0- fetch/3057 /mir-jenkins. ubuntu. com/job/ build-1- sourcepkg/ release= vivid+overlay/ 3049 /mir-jenkins. ubuntu. com/job/ build-1- sourcepkg/ release= xenial+ overlay/ 3049 /mir-jenkins. ubuntu. com/job/ build-1- sourcepkg/ release= yakkety/ 3049 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= amd64,compiler= clang,platform= mesa,release= yakkety/ 3021 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= amd64,compiler= clang,platform= mesa,release= yakkety/ 3021/artifact/ output/ *zip*/output. zip /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= amd64,compiler= gcc,platform= mesa,release= xenial+ overlay/ 3021 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= amd64,compiler= gcc,platform= mesa,release= xenial+ overlay/ 3021/artifact/ output/ *zip*/output. zip /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= amd64,compiler= gcc,platform= mesa,release= yakkety/ 3021 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= amd64,compiler= gcc,platform= mesa,release= yakkety/ 3021/artifact/ output/ *zip*/output. zip /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= cross-armhf, compiler= gcc,platform= android, release= vivid+overlay/ 3021 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= cross-armhf, compiler= gcc,platform= android, release= vivid+overlay/ 3021/artifact/ output/ *zip*/output. zip /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= i386,compiler= gcc,platform= android, release= vivid+overlay/ 3021 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= i386,compiler= gcc,platform= android, release= vivid+overlay/ 3021/artifact/ output/ *zip*/output. zip /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= i386,compiler= gcc,platform= mesa,release= xenial+ overlay/ 3021 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= i386,compiler= gcc,platform= mesa,release= xenial+ overlay/ 3021/artifact/ output/ *zip*/output. zip
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild: /mir-jenkins. ubuntu. com/job/ mir-ci/ 2299/rebuild
https:/