Read EDID for the 1st display for startup

Read EDID for the 1st display for startup

BUG=449662, chrome-os-partner:35334
TEST=covered by unittest. Also tested on link_freon

Committed: https://crrev.com/a2b7df9d21af3c0d19fed3628caf13ae36fee233
Cr-Commit-Position: refs/heads/master@{#312947}

[oshima, 2015-01-16 19:45:45]

Patchset #1 (id:1) has been deleted

[oshima, 2015-01-16 19:50:54]

Patchset #1 (id:20001) has been deleted

[oshima, 2015-01-16 19:52:24]

oshima@chromium.org changed reviewers:
+ dnicoara@chromium.org

[dnicoara, 2015-01-16 20:39:51]

I think the bug number if wrong.

https://codereview.chromium.org/854203002/diff/60001/ui/display/util/edid_par...
File ui/display/util/edid_parser.cc (right):

https://codereview.chromium.org/854203002/diff/60001/ui/display/util/edid_par...
ui/display/util/edid_parser.cc:194: if (edid.size() <= kNumExtensionsOffset)
nit: Missing space

https://codereview.chromium.org/854203002/diff/60001/ui/display/util/edid_par...
File ui/display/util/edid_parser.h (right):

https://codereview.chromium.org/854203002/diff/60001/ui/display/util/edid_par...
ui/display/util/edid_parser.h:32: // |human_readable_name|, |pixel_out| and
size_out, then returns true. NULL can
nit: |size_out|

https://codereview.chromium.org/854203002/diff/60001/ui/display/util/edid_par...
ui/display/util/edid_parser.h:39: gfx::Size* pixel_out,
nit: I think it would be clearer if |pixel_out| and |size_out| would have a more
expressive name (like mode_size and physical_display_size)

https://codereview.chromium.org/854203002/diff/60001/ui/ozone/common/display_...
File ui/ozone/common/display_util.h (right):

https://codereview.chromium.org/854203002/diff/60001/ui/ozone/common/display_...
ui/ozone/common/display_util.h:48: std::vector<uint8_t> edid,
const std::vector<uint8_t>&

[oshima, 2015-01-16 21:50:19]

filed a new bug.

https://codereview.chromium.org/854203002/diff/60001/ui/display/util/edid_par...
File ui/display/util/edid_parser.cc (right):

https://codereview.chromium.org/854203002/diff/60001/ui/display/util/edid_par...
ui/display/util/edid_parser.cc:194: if (edid.size() <= kNumExtensionsOffset)
On 2015/01/16 20:39:50, dnicoara wrote:
> nit: Missing space

Done.

https://codereview.chromium.org/854203002/diff/60001/ui/display/util/edid_par...
File ui/display/util/edid_parser.h (right):

https://codereview.chromium.org/854203002/diff/60001/ui/display/util/edid_par...
ui/display/util/edid_parser.h:32: // |human_readable_name|, |pixel_out| and
size_out, then returns true. NULL can
On 2015/01/16 20:39:51, dnicoara wrote:
> nit: |size_out|

Done.

https://codereview.chromium.org/854203002/diff/60001/ui/display/util/edid_par...
ui/display/util/edid_parser.h:39: gfx::Size* pixel_out,
On 2015/01/16 20:39:51, dnicoara wrote:
> nit: I think it would be clearer if |pixel_out| and |size_out| would have a
more
> expressive name (like mode_size and physical_display_size)

changed to active_pixel_out and physical_display_size_out (as described in
http://en.wikipedia.org/wiki/Extended_display_identification_data

https://codereview.chromium.org/854203002/diff/60001/ui/ozone/common/display_...
File ui/ozone/common/display_util.h (right):

https://codereview.chromium.org/854203002/diff/60001/ui/ozone/common/display_...
ui/ozone/common/display_util.h:48: std::vector<uint8_t> edid,
On 2015/01/16 20:39:51, dnicoara wrote:
> const std::vector<uint8_t>&

Done.

[dnicoara, 2015-01-17 05:17:43]

dnicoara@chromium.org changed reviewers:
+ spang@chromium.org

[dnicoara, 2015-01-17 05:17:44]

lgtm

+spang@ for ui/ozone OWNERS

[oshima, 2015-01-22 02:02:20]

On 2015/01/17 05:17:44, dnicoara wrote:
> lgtm
> 
> +spang@ for ui/ozone OWNERS

spang@: friendly ping

[oshima, 2015-01-22 02:02:21]

On 2015/01/17 05:17:44, dnicoara wrote:
> lgtm
> 
> +spang@ for ui/ozone OWNERS

spang@: friendly ping

[Jun Mukai, 2015-01-22 02:07:39]

mukai@chromium.org changed reviewers:
+ mukai@chromium.org

[Jun Mukai, 2015-01-22 02:07:39]

https://codereview.chromium.org/854203002/diff/80001/ui/display/util/edid_par...
File ui/display/util/edid_parser.cc (right):

https://codereview.chromium.org/854203002/diff/80001/ui/display/util/edid_par...
ui/display/util/edid_parser.cc:61: gfx::Size* active_active_pixel_out,
nit: active_active_pixel_out -> active_pixel_out

[spang, 2015-01-22 02:25:26]

https://codereview.chromium.org/854203002/diff/80001/ui/ozone/common/display_...
File ui/ozone/common/display_util.cc (right):

https://codereview.chromium.org/854203002/diff/80001/ui/ozone/common/display_...
ui/ozone/common/display_util.cc:137: base::ThreadRestrictions::ScopedAllowIO
allow_io;
This function is banned, see the red try run for chromium_presubmit.

To submit, you'll need an exception in the top level PRESUBMIT.py.

[spang, 2015-01-22 02:31:14]

On 2015/01/22 02:25:26, spang wrote:
>
https://codereview.chromium.org/854203002/diff/80001/ui/ozone/common/display_...
> File ui/ozone/common/display_util.cc (right):
> 
>
https://codereview.chromium.org/854203002/diff/80001/ui/ozone/common/display_...
> ui/ozone/common/display_util.cc:137: base::ThreadRestrictions::ScopedAllowIO
> allow_io;
> This function is banned, see the red try run for chromium_presubmit.
> 
> To submit, you'll need an exception in the top level PRESUBMIT.py.

and of course you'll also need to socialize this hack with the PRESUBMIT owners.

[oshima, 2015-01-22 02:34:01]

On 2015/01/22 02:31:14, spang wrote:
> On 2015/01/22 02:25:26, spang wrote:
> >
>
https://codereview.chromium.org/854203002/diff/80001/ui/ozone/common/display_...
> > File ui/ozone/common/display_util.cc (right):
> > 
> >
>
https://codereview.chromium.org/854203002/diff/80001/ui/ozone/common/display_...
> > ui/ozone/common/display_util.cc:137: base::ThreadRestrictions::ScopedAllowIO
> > allow_io;
> > This function is banned, see the red try run for chromium_presubmit.
> > 
> > To submit, you'll need an exception in the top level PRESUBMIT.py.
> 
> and of course you'll also need to socialize this hack with the PRESUBMIT
owners.

Spawining off this to another thread doesn't really make sense and will slow
down the process,
as this has to be read to boot the chrome.

[spang, 2015-01-22 02:39:29]

On 2015/01/22 02:34:01, oshima wrote:
> On 2015/01/22 02:31:14, spang wrote:
> > On 2015/01/22 02:25:26, spang wrote:
> > >
> >
>
https://codereview.chromium.org/854203002/diff/80001/ui/ozone/common/display_...
> > > File ui/ozone/common/display_util.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/854203002/diff/80001/ui/ozone/common/display_...
> > > ui/ozone/common/display_util.cc:137:
base::ThreadRestrictions::ScopedAllowIO
> > > allow_io;
> > > This function is banned, see the red try run for chromium_presubmit.
> > > 
> > > To submit, you'll need an exception in the top level PRESUBMIT.py.
> > 
> > and of course you'll also need to socialize this hack with the PRESUBMIT
> owners.
> 
> Spawining off this to another thread doesn't really make sense and will slow
> down the process,
> as this has to be read to boot the chrome.

I'm sympathetic to the problem; I also want chrome to start up at the correct
resolution without having to do a huge refactor and without having to pass it on
the command line. But even if I ack this pack you would not be able to land it.
Please get approval from a top level presubmit owner such as jochen first.

[oshima, 2015-01-22 02:45:13]

On 2015/01/22 02:39:29, spang wrote:
> On 2015/01/22 02:34:01, oshima wrote:
> > On 2015/01/22 02:31:14, spang wrote:
> > > On 2015/01/22 02:25:26, spang wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/854203002/diff/80001/ui/ozone/common/display_...
> > > > File ui/ozone/common/display_util.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/854203002/diff/80001/ui/ozone/common/display_...
> > > > ui/ozone/common/display_util.cc:137:
> base::ThreadRestrictions::ScopedAllowIO
> > > > allow_io;
> > > > This function is banned, see the red try run for chromium_presubmit.
> > > > 
> > > > To submit, you'll need an exception in the top level PRESUBMIT.py.
> > > 
> > > and of course you'll also need to socialize this hack with the PRESUBMIT
> > owners.
> > 
> > Spawining off this to another thread doesn't really make sense and will slow
> > down the process,
> > as this has to be read to boot the chrome.
> 
> I'm sympathetic to the problem; I also want chrome to start up at the correct
> resolution without having to do a huge refactor and without having to pass it
on
> the command line. But even if I ack this pack you would not be able to land
it.
> Please get approval from a top level presubmit owner such as jochen first.

I'll get the review. Can you at least review ui/ozone?

[spang, 2015-01-22 02:54:57]

On 2015/01/22 02:45:13, oshima wrote:
> On 2015/01/22 02:39:29, spang wrote:
> > On 2015/01/22 02:34:01, oshima wrote:
> > > On 2015/01/22 02:31:14, spang wrote:
> > > > On 2015/01/22 02:25:26, spang wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/854203002/diff/80001/ui/ozone/common/display_...
> > > > > File ui/ozone/common/display_util.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/854203002/diff/80001/ui/ozone/common/display_...
> > > > > ui/ozone/common/display_util.cc:137:
> > base::ThreadRestrictions::ScopedAllowIO
> > > > > allow_io;
> > > > > This function is banned, see the red try run for chromium_presubmit.
> > > > > 
> > > > > To submit, you'll need an exception in the top level PRESUBMIT.py.
> > > > 
> > > > and of course you'll also need to socialize this hack with the PRESUBMIT
> > > owners.
> > > 
> > > Spawining off this to another thread doesn't really make sense and will
slow
> > > down the process,
> > > as this has to be read to boot the chrome.
> > 
> > I'm sympathetic to the problem; I also want chrome to start up at the
correct
> > resolution without having to do a huge refactor and without having to pass
it
> on
> > the command line. But even if I ack this pack you would not be able to land
> it.
> > Please get approval from a top level presubmit owner such as jochen first.
> 
> I'll get the review. Can you at least review ui/ozone?

everything else lgtm. Or at least it's an improvement over the command line
flag. But I really do think we should fix the underlying issue so that we do not
need to rely on frecon passing us this information via /tmp.

[oshima, 2015-01-22 03:13:38]

On 2015/01/22 02:54:57, spang wrote:
> On 2015/01/22 02:45:13, oshima wrote:
> > On 2015/01/22 02:39:29, spang wrote:
> > > On 2015/01/22 02:34:01, oshima wrote:
> > > > On 2015/01/22 02:31:14, spang wrote:
> > > > > On 2015/01/22 02:25:26, spang wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/854203002/diff/80001/ui/ozone/common/display_...
> > > > > > File ui/ozone/common/display_util.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/854203002/diff/80001/ui/ozone/common/display_...
> > > > > > ui/ozone/common/display_util.cc:137:
> > > base::ThreadRestrictions::ScopedAllowIO
> > > > > > allow_io;
> > > > > > This function is banned, see the red try run for chromium_presubmit.
> > > > > > 
> > > > > > To submit, you'll need an exception in the top level PRESUBMIT.py.
> > > > > 
> > > > > and of course you'll also need to socialize this hack with the
PRESUBMIT
> > > > owners.
> > > > 
> > > > Spawining off this to another thread doesn't really make sense and will
> slow
> > > > down the process,
> > > > as this has to be read to boot the chrome.
> > > 
> > > I'm sympathetic to the problem; I also want chrome to start up at the
> correct
> > > resolution without having to do a huge refactor and without having to pass
> it
> > on
> > > the command line. But even if I ack this pack you would not be able to
land
> > it.
> > > Please get approval from a top level presubmit owner such as jochen first.
> > 
> > I'll get the review. Can you at least review ui/ozone?
> 
> everything else lgtm. Or at least it's an improvement over the command line
> flag. But I really do think we should fix the underlying issue so that we do
not
> need to rely on frecon passing us this information via /tmp.

Thanks.

AFAIK, alternative solutions either require architectural change (move the
display configuration
to browser process), or make boot slower (wait for display change notification
from GPU process).

I do agree that it'd be better if there is a better alternative. I think we're
open
to any suggestion, and we should keep the issue open.

[oshima, 2015-01-22 04:12:58]

On 2015/01/22 02:54:57, spang wrote:
> On 2015/01/22 02:45:13, oshima wrote:
> > On 2015/01/22 02:39:29, spang wrote:
> > > On 2015/01/22 02:34:01, oshima wrote:
> > > > On 2015/01/22 02:31:14, spang wrote:
> > > > > On 2015/01/22 02:25:26, spang wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/854203002/diff/80001/ui/ozone/common/display_...
> > > > > > File ui/ozone/common/display_util.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/854203002/diff/80001/ui/ozone/common/display_...
> > > > > > ui/ozone/common/display_util.cc:137:
> > > base::ThreadRestrictions::ScopedAllowIO
> > > > > > allow_io;
> > > > > > This function is banned, see the red try run for chromium_presubmit.
> > > > > > 
> > > > > > To submit, you'll need an exception in the top level PRESUBMIT.py.
> > > > > 
> > > > > and of course you'll also need to socialize this hack with the
PRESUBMIT
> > > > owners.
> > > > 
> > > > Spawining off this to another thread doesn't really make sense and will
> slow
> > > > down the process,
> > > > as this has to be read to boot the chrome.
> > > 
> > > I'm sympathetic to the problem; I also want chrome to start up at the
> correct
> > > resolution without having to do a huge refactor and without having to pass
> it
> > on
> > > the command line. But even if I ack this pack you would not be able to
land
> > it.
> > > Please get approval from a top level presubmit owner such as jochen first.
> > 
> > I'll get the review. Can you at least review ui/ozone?
> 
> everything else lgtm. Or at least it's an improvement over the command line
> flag. But I really do think we should fix the underlying issue so that we do
not
> need to rely on frecon passing us this information via /tmp.

I moved AllowIO to native_display_delegate_proxy.cc, which should prevent
mis-use of
AllowIO. PTAL.

[oshima, 2015-01-22 04:16:21]

oshima@chromium.org changed reviewers:
+ jam@chromium.org

[oshima, 2015-01-22 04:16:22]

+jam@ for requesting an exception for AllowIO on tmpfs file.
(native_display_delegate_proxy.cc)

context:
ChromeOS ozone+freon build (graphics stack without X) needs to read a display
information from file (which is generated by the other process that run prior to
chrome) during boot. This happens only once during the startup time, and it
reads only from tmpfs which is on memory.

[jam, 2015-01-22 17:17:51]

On 2015/01/22 04:16:22, oshima wrote:
> +jam@ for requesting an exception for AllowIO on tmpfs file.
> (native_display_delegate_proxy.cc)
> 
> context:
> ChromeOS ozone+freon build (graphics stack without X) needs to read a display
> information from file (which is generated by the other process that run prior
to
> chrome) during boot. This happens only once during the startup time, and it
> reads only from tmpfs which is on memory.

this seems reasonable.

you don't need me to approve this :)

[spang, 2015-01-22 18:12:17]

On 2015/01/22 17:17:51, jam wrote:
> On 2015/01/22 04:16:22, oshima wrote:
> > +jam@ for requesting an exception for AllowIO on tmpfs file.
> > (native_display_delegate_proxy.cc)
> > 
> > context:
> > ChromeOS ozone+freon build (graphics stack without X) needs to read a
display
> > information from file (which is generated by the other process that run
prior
> to
> > chrome) during boot. This happens only once during the startup time, and it
> > reads only from tmpfs which is on memory.
> 
> this seems reasonable.
> 
> you don't need me to approve this :)

the patch needs to update PRESUBMIT.py, we'll need your approval once that's
done

[oshima, 2015-01-22 19:09:29]

On 2015/01/22 18:12:17, spang wrote:
> On 2015/01/22 17:17:51, jam wrote:
> > On 2015/01/22 04:16:22, oshima wrote:
> > > +jam@ for requesting an exception for AllowIO on tmpfs file.
> > > (native_display_delegate_proxy.cc)
> > > 
> > > context:
> > > ChromeOS ozone+freon build (graphics stack without X) needs to read a
> display
> > > information from file (which is generated by the other process that run
> prior
> > to
> > > chrome) during boot. This happens only once during the startup time, and
it
> > > reads only from tmpfs which is on memory.
> > 
> > this seems reasonable.
> > 
> > you don't need me to approve this :)

I sent this to you as you'r the owner of base/threading/thread_restrictions* :).

> 
> the patch needs to update PRESUBMIT.py, we'll need your approval once that's
> done

I'll run try jobs and land manually.

[oshima, 2015-01-22 19:29:10]

On 2015/01/22 18:12:17, spang wrote:
> On 2015/01/22 17:17:51, jam wrote:
> > On 2015/01/22 04:16:22, oshima wrote:
> > > +jam@ for requesting an exception for AllowIO on tmpfs file.
> > > (native_display_delegate_proxy.cc)
> > > 
> > > context:
> > > ChromeOS ozone+freon build (graphics stack without X) needs to read a
> display
> > > information from file (which is generated by the other process that run
> prior
> > to
> > > chrome) during boot. This happens only once during the startup time, and
it
> > > reads only from tmpfs which is on memory.
> > 
> > this seems reasonable.
> > 
> > you don't need me to approve this :)
> 
> the patch needs to update PRESUBMIT.py, we'll need your approval once that's
> done


Ah, I understood what you meant. I'll do that in a separate CL.

[oshima, 2015-01-23 21:49:26]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2015-01-23 21:50:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/854203002/120001

[commit-bot: I haz the power, 2015-01-23 22:22:28]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-01-23 22:23:31]

Patchset 5 (id:??) landed as
https://crrev.com/a2b7df9d21af3c0d19fed3628caf13ae36fee233
Cr-Commit-Position: refs/heads/master@{#312947}

[gunsch, 2015-01-23 23:19:58]

On 2015/01/23 22:23:31, I haz the power (commit-bot) wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/a2b7df9d21af3c0d19fed3628caf13ae36fee233
> Cr-Commit-Position: refs/heads/master@{#312947}

FYI: this broke the Chromecast build (no trybot yet, we're working on it) that
uses Ozone + Linux. Error below.

It looks like ui/ozone.gyp:ozone should now depend on
ui/display/display.gyp:display_util to correctly link in the new dependency.

FAILED: ../../third_party/llvm-build/Release+Asserts/bin/clang++ -Wl,-z,now
-Wl,-z,relro -Wl,--fatal-warnings -pthread -Wl,-z,noexecstack -fPIC
-fuse-ld=gold
-B/usr/local/google/home/mosaic-role/slave/repo_clients/chromium_tot/chromium/src/third_party/binutils/Linux_x64/Release/bin
-Wl,--disable-new-dtags -Wl,-uIsHeapProfilerRunning,-uProfilerStart
-Wl,-u_Z21InitialMallocHook_NewPKvj,-u_Z22InitialMallocHook_MMapPKvS0_jiiix,-u_Z22InitialMallocHook_SbrkPKvi
-Wl,-u_Z21InitialMallocHook_NewPKvm,-u_Z22InitialMallocHook_MMapPKvS0_miiil,-u_Z22InitialMallocHook_SbrkPKvl
-Wl,-u_ZN15HeapLeakChecker12IgnoreObjectEPKv,-u_ZN15HeapLeakChecker14UnIgnoreObjectEPKv
-m64 -Wl,--icf=safe -o cast_shell [ ... command snipped ]
../../ui/ozone/common/display_util.cc:108: error: undefined reference to
'ui::ParseOutputDeviceData(std::__debug::vector<unsigned char,
std::allocator<unsigned char> > const&, unsigned short*, std::string*,
gfx::Size*, gfx::Size*)'
../../ui/ozone/common/display_util.cc:111: error: undefined reference to
'ui::GetDisplayIdFromEDID(std::__debug::vector<unsigned char,
std::allocator<unsigned char> > const&, unsigned char, long*)'
../../ui/ozone/common/display_util.cc:114: error: undefined reference to
'ui::ParseOutputOverscanFlag(std::__debug::vector<unsigned char,
std::allocator<unsigned char> > const&, bool*)'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[oshima, 2015-01-24 01:29:53]

On 2015/01/23 23:19:58, gunsch wrote:
> On 2015/01/23 22:23:31, I haz the power (commit-bot) wrote:
> > Patchset 5 (id:??) landed as
> > https://crrev.com/a2b7df9d21af3c0d19fed3628caf13ae36fee233
> > Cr-Commit-Position: refs/heads/master@{#312947}
> 
> FYI: this broke the Chromecast build (no trybot yet, we're working on it) that
> uses Ozone + Linux. Error below.
> 
> It looks like ui/ozone.gyp:ozone should now depend on
> ui/display/display.gyp:display_util to correctly link in the new dependency.
> 
> FAILED: ../../third_party/llvm-build/Release+Asserts/bin/clang++ -Wl,-z,now
> -Wl,-z,relro -Wl,--fatal-warnings -pthread -Wl,-z,noexecstack -fPIC
> -fuse-ld=gold
>
-B/usr/local/google/home/mosaic-role/slave/repo_clients/chromium_tot/chromium/src/third_party/binutils/Linux_x64/Release/bin
> -Wl,--disable-new-dtags -Wl,-uIsHeapProfilerRunning,-uProfilerStart
>
-Wl,-u_Z21InitialMallocHook_NewPKvj,-u_Z22InitialMallocHook_MMapPKvS0_jiiix,-u_Z22InitialMallocHook_SbrkPKvi
>
-Wl,-u_Z21InitialMallocHook_NewPKvm,-u_Z22InitialMallocHook_MMapPKvS0_miiil,-u_Z22InitialMallocHook_SbrkPKvl
>
-Wl,-u_ZN15HeapLeakChecker12IgnoreObjectEPKv,-u_ZN15HeapLeakChecker14UnIgnoreObjectEPKv
> -m64 -Wl,--icf=safe -o cast_shell [ ... command snipped ]
> ../../ui/ozone/common/display_util.cc:108: error: undefined reference to
> 'ui::ParseOutputDeviceData(std::__debug::vector<unsigned char,
> std::allocator<unsigned char> > const&, unsigned short*, std::string*,
> gfx::Size*, gfx::Size*)'
> ../../ui/ozone/common/display_util.cc:111: error: undefined reference to
> 'ui::GetDisplayIdFromEDID(std::__debug::vector<unsigned char,
> std::allocator<unsigned char> > const&, unsigned char, long*)'
> ../../ui/ozone/common/display_util.cc:114: error: undefined reference to
> 'ui::ParseOutputOverscanFlag(std::__debug::vector<unsigned char,
> std::allocator<unsigned char> > const&, bool*)'
> clang: error: linker command failed with exit code 1 (use -v to see
invocation)

Sorry for breakage. I'll land fix soon.