Load and apply a vcgt table from an ICC file to the internal display

Load and apply a vcgt table from an ICC file to the internal display

When the command line switch --internal-display-color-profile-file is provided
load the given ICC file using qcms and extract the VCGT data. Use this VCGT
data to apply a gamma ramp to change the output on the internal display using
drmModeCrtcSetGammaRamp.

BUG=471749
TEST=On a link_freon device add the command line option to load a sample ICC
file (e.g. Bluish.icc to give a blue tint) and observe on startup that the
internal display is blue tinted.

Committed: https://crrev.com/46134a86af1eac76e97b062a68afeaa6c8801216
Cr-Commit-Position: refs/heads/master@{#328318}

[robert.bradford, 2015-03-20 19:53:45]

Patchset #1 (id:1) has been deleted

[robert.bradford, 2015-04-02 12:17:43]

robert.bradford@intel.com changed reviewers:
+ dnicoara@chromium.org, oshima@chromium.org, spang@chromium.org

[robert.bradford, 2015-04-02 12:17:44]

oshima@ PTAL at the changes in ui/display

spang@ & dnicoara@ PTAL at the changes in ui/ozone

Thank you very much!

[oshima, 2015-04-02 15:56:52]

https://codereview.chromium.org/1028563003/diff/40001/ui/display/chromeos/tes...
File ui/display/chromeos/test/action_logger_util.cc (right):

https://codereview.chromium.org/1028563003/diff/40001/ui/display/chromeos/tes...
ui/display/chromeos/test/action_logger_util.cc:58: const std::vector<uint16_t>&
b) {
are you going to use r/g/b later?

[robert.bradford, 2015-04-08 13:03:53]

On 2015/04/02 15:56:52, oshima wrote:
>
https://codereview.chromium.org/1028563003/diff/40001/ui/display/chromeos/tes...
> File ui/display/chromeos/test/action_logger_util.cc (right):
> 
>
https://codereview.chromium.org/1028563003/diff/40001/ui/display/chromeos/tes...
> ui/display/chromeos/test/action_logger_util.cc:58: const
std::vector<uint16_t>&
> b) {
> are you going to use r/g/b later?

I wasn't sure what I wanted from the logger because I hadn't developed a test
that triggered this yet - although it's quite long i've decided to make the
action string the full table of values so that it can be used.

spang, any thoughts on the rest of this CL?

[spang, 2015-04-08 14:08:44]

On 2015/04/08 13:03:53, robert.bradford wrote:
> On 2015/04/02 15:56:52, oshima wrote:
> >
>
https://codereview.chromium.org/1028563003/diff/40001/ui/display/chromeos/tes...
> > File ui/display/chromeos/test/action_logger_util.cc (right):
> > 
> >
>
https://codereview.chromium.org/1028563003/diff/40001/ui/display/chromeos/tes...
> > ui/display/chromeos/test/action_logger_util.cc:58: const
> std::vector<uint16_t>&
> > b) {
> > are you going to use r/g/b later?
> 
> I wasn't sure what I wanted from the logger because I hadn't developed a test
> that triggered this yet - although it's quite long i've decided to make the
> action string the full table of values so that it can be used.
> 
> spang, any thoughts on the rest of this CL?

It seems like an incomplete series. Where's the code to call SetGammaRamp?

[robert.bradford, 2015-04-10 18:22:33]

> It seems like an incomplete series. Where's the code to call SetGammaRamp?

I had been looking for feedback on just the NDD changes with the calling code in
another CL. But you're spot on, without the caller of this code how can you
review it?

So i've now added the code that I was working on to read the ICC file, extract
the vcgt data with qcms and apply it to through
NativeDisplayDelegate::SetGammaRamp.

I'm not sure i've done the right thing by putting it into ash/ I was wondering
if it instead should be ui/display/chromeos ?

All feedback would be appreciated.

[spang, 2015-04-13 18:22:43]

Just one nit.. otherwise the plumbing in ui/ozone looks lgtm.

https://codereview.chromium.org/1028563003/diff/80001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc (right):

https://codereview.chromium.org/1028563003/diff/80001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:255:
const_cast<uint16_t*>(&g[0]), const_cast<uint16_t*>(&b[0]))) {
Please move the casts closer to the ioctl in DrmDevice::SetGammaRamp().

[oshima, 2015-04-15 23:56:13]

https://codereview.chromium.org/1028563003/diff/80001/ash/display/display_col...
File ash/display/display_color_manager_chromeos.cc (right):

https://codereview.chromium.org/1028563003/diff/80001/ash/display/display_col...
ash/display/display_color_manager_chromeos.cc:7: #include
"ash/display/display_info.h"
do you need this?

https://codereview.chromium.org/1028563003/diff/80001/ash/display/display_col...
ash/display/display_color_manager_chromeos.cc:55: return data;
how big the data is? If it's small, just using struct would be simpler, and less
error prone.

https://codereview.chromium.org/1028563003/diff/80001/ash/display/display_col...
ash/display/display_color_manager_chromeos.cc:79: }
nit: nuke {}

https://codereview.chromium.org/1028563003/diff/80001/ash/display/display_col...
ash/display/display_color_manager_chromeos.cc:87: }
ditto

https://codereview.chromium.org/1028563003/diff/80001/ash/display/display_col...
ash/display/display_color_manager_chromeos.cc:103:
gfx::Display::InternalDisplayId()));
FILE thread is obsolete. Please use SequencedWorkerPool and PostTaskAndReply.

Also since this file will depend on content/ please move this to
ash/content/display

https://codereview.chromium.org/1028563003/diff/80001/ash/display/display_col...
File ash/display/display_color_manager_chromeos.h (right):

https://codereview.chromium.org/1028563003/diff/80001/ash/display/display_col...
ash/display/display_color_manager_chromeos.h:25: public
base::RefCountedThreadSafe<DisplayColorManager> {
RefCountedThreadSafe is for the special case, where the object needs to be
shared and destroyed on multiple threads and I believe this does not fall into
that use case.

Instead of using RefCountedThreadSafe, please use WeakPtr.

https://codereview.chromium.org/1028563003/diff/80001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1028563003/diff/80001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:753: }
nuke {}

https://codereview.chromium.org/1028563003/diff/80001/ui/display/chromeos/tes...
File ui/display/chromeos/test/action_logger_util.cc (right):

https://codereview.chromium.org/1028563003/diff/80001/ui/display/chromeos/tes...
ui/display/chromeos/test/action_logger_util.cc:60: DCHECK((r.size() == g.size())
&& (g.size() == b.size()));
DCHECK_EQ(r.size(), g.size());
DCHECK_EQ(g.size(), b.size());

would be better.

https://codereview.chromium.org/1028563003/diff/80001/ui/display/chromeos/tes...
ui/display/chromeos/test/action_logger_util.cc:63: for (unsigned int i = 0; i <
r.size(); ++i) {
size_t

https://codereview.chromium.org/1028563003/diff/80001/ui/display/chromeos/tes...
ui/display/chromeos/test/action_logger_util.cc:65: }
ditto

[robert.bradford, 2015-04-17 16:42:46]

Hi oshima@ and spang@. Thanks for the feedback. All your issues should be been
addressed below.

https://codereview.chromium.org/1028563003/diff/80001/ash/display/display_col...
File ash/display/display_color_manager_chromeos.cc (right):

https://codereview.chromium.org/1028563003/diff/80001/ash/display/display_col...
ash/display/display_color_manager_chromeos.cc:7: #include
"ash/display/display_info.h"
On 2015/04/15 23:56:13, oshima wrote:
> do you need this?

Done.

https://codereview.chromium.org/1028563003/diff/80001/ash/display/display_col...
ash/display/display_color_manager_chromeos.cc:55: return data;
On 2015/04/15 23:56:13, oshima wrote:
> how big the data is? If it's small, just using struct would be simpler, and
less
> error prone.

The data will be 3 * 256 * 2 bytes.

https://codereview.chromium.org/1028563003/diff/80001/ash/display/display_col...
ash/display/display_color_manager_chromeos.cc:79: }
On 2015/04/15 23:56:13, oshima wrote:
> nit: nuke {}

Done.

https://codereview.chromium.org/1028563003/diff/80001/ash/display/display_col...
ash/display/display_color_manager_chromeos.cc:87: }
On 2015/04/15 23:56:13, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/1028563003/diff/80001/ash/display/display_col...
ash/display/display_color_manager_chromeos.cc:103:
gfx::Display::InternalDisplayId()));
On 2015/04/15 23:56:13, oshima wrote:
> FILE thread is obsolete. Please use SequencedWorkerPool and PostTaskAndReply.
> 
> Also since this file will depend on content/ please move this to
> ash/content/display

Ended up using base::PostTaskAndReplyWithResult with BrowserThread::BlockingPool
which allows me to handle success or not of loading the file.

https://codereview.chromium.org/1028563003/diff/80001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1028563003/diff/80001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:753: }
On 2015/04/15 23:56:13, oshima wrote:
> nuke {}

Done.

https://codereview.chromium.org/1028563003/diff/80001/ui/display/chromeos/tes...
File ui/display/chromeos/test/action_logger_util.cc (right):

https://codereview.chromium.org/1028563003/diff/80001/ui/display/chromeos/tes...
ui/display/chromeos/test/action_logger_util.cc:60: DCHECK((r.size() == g.size())
&& (g.size() == b.size()));
On 2015/04/15 23:56:13, oshima wrote:
> DCHECK_EQ(r.size(), g.size());
> DCHECK_EQ(g.size(), b.size());
> 
> would be better.

Done.

https://codereview.chromium.org/1028563003/diff/80001/ui/display/chromeos/tes...
ui/display/chromeos/test/action_logger_util.cc:63: for (unsigned int i = 0; i <
r.size(); ++i) {
On 2015/04/15 23:56:13, oshima wrote:
> size_t

Done.

https://codereview.chromium.org/1028563003/diff/80001/ui/display/chromeos/tes...
ui/display/chromeos/test/action_logger_util.cc:65: }
On 2015/04/15 23:56:13, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/1028563003/diff/80001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc (right):

https://codereview.chromium.org/1028563003/diff/80001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:255:
const_cast<uint16_t*>(&g[0]), const_cast<uint16_t*>(&b[0]))) {
On 2015/04/13 18:22:43, spang wrote:
> Please move the casts closer to the ioctl in DrmDevice::SetGammaRamp().

Done.

[spang, 2015-04-17 17:04:51]

https://codereview.chromium.org/1028563003/diff/100001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_device.h (right):

https://codereview.chromium.org/1028563003/diff/100001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.h:163: std::vector<uint16_t> r,
const &

(or if you do want it to copy, you should remove the const_casts)

[robert.bradford, 2015-04-17 17:17:48]

https://codereview.chromium.org/1028563003/diff/100001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_device.h (right):

https://codereview.chromium.org/1028563003/diff/100001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.h:163: std::vector<uint16_t> r,
On 2015/04/17 17:04:51, spang wrote:
> const &
> 
> (or if you do want it to copy, you should remove the const_casts)

Whoops. Fixed now, thanks for checking this, I didn't want to copy as these
vectors will be relatively large.

[oshima, 2015-04-17 17:46:17]

https://codereview.chromium.org/1028563003/diff/120001/ash/content/display/di...
File ash/content/display/display_color_manager_chromeos.cc (right):

https://codereview.chromium.org/1028563003/diff/120001/ash/content/display/di...
ash/content/display/display_color_manager_chromeos.cc:93: << "for internal
display id: " << gfx::Display::InternalDisplayId();
internal display may not exist. Please check using HasInternalDisplay.

One note: there is a known issue (crbug.com/449631) on ozone where the internal
display information isn't available on startup. (crbug.com/38796 is an issue
bitten by this bug) You may want to track this bug.

https://codereview.chromium.org/1028563003/diff/120001/ash/content/display/di...
File ash/content/display/display_color_manager_chromeos.h (right):

https://codereview.chromium.org/1028563003/diff/120001/ash/content/display/di...
ash/content/display/display_color_manager_chromeos.h:27:
DisplayColorManager(ui::DisplayConfigurator* configurator);
nit: explicit

https://codereview.chromium.org/1028563003/diff/120001/ash/content/display/di...
ash/content/display/display_color_manager_chromeos.h:30: // Overriden from
ui::DisplayConfigurator::Observer:
just 

// ui::DisplayConfigurator::Observer

https://codereview.chromium.org/1028563003/diff/120001/ash/content/display/di...
ash/content/display/display_color_manager_chromeos.h:55: std::map<uint64_t,
ColorCalibrationData*> calibration_map_;
new line before DISALLOW_COPY_AND_ASSIGN

[robert.bradford, 2015-04-17 18:54:47]

Thanks for the feedback oshima; patch updated based on your review.

https://codereview.chromium.org/1028563003/diff/120001/ash/content/display/di...
File ash/content/display/display_color_manager_chromeos.cc (right):

https://codereview.chromium.org/1028563003/diff/120001/ash/content/display/di...
ash/content/display/display_color_manager_chromeos.cc:93: << "for internal
display id: " << gfx::Display::InternalDisplayId();
On 2015/04/17 17:46:17, oshima wrote:
> internal display may not exist. Please check using HasInternalDisplay.

I tackled this slightly differently. Rather than use
DisplayManager::HasInternalDisplay I split this into a separate method where I
test that display_id is not gfx::Display::kInvalidId.

> One note: there is a known issue (crbug.com/449631) on ozone where the
internal
> display information isn't available on startup. (crbug.com/38796 is an issue
> bitten by this bug) You may want to track this bug.

That last bug # looks wrong?

https://codereview.chromium.org/1028563003/diff/120001/ash/content/display/di...
File ash/content/display/display_color_manager_chromeos.h (right):

https://codereview.chromium.org/1028563003/diff/120001/ash/content/display/di...
ash/content/display/display_color_manager_chromeos.h:27:
DisplayColorManager(ui::DisplayConfigurator* configurator);
On 2015/04/17 17:46:17, oshima wrote:
> nit: explicit 

Ah, thanks. I didn't know about that keyword.

https://codereview.chromium.org/1028563003/diff/120001/ash/content/display/di...
ash/content/display/display_color_manager_chromeos.h:30: // Overriden from
ui::DisplayConfigurator::Observer:
On 2015/04/17 17:46:17, oshima wrote:
> just 
> 
> // ui::DisplayConfigurator::Observer

Done.

https://codereview.chromium.org/1028563003/diff/120001/ash/content/display/di...
ash/content/display/display_color_manager_chromeos.h:55: std::map<uint64_t,
ColorCalibrationData*> calibration_map_;
On 2015/04/17 17:46:17, oshima wrote:
> new line before DISALLOW_COPY_AND_ASSIGN

Done.

[spang, 2015-04-17 19:07:42]

https://codereview.chromium.org/1028563003/diff/140001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc (right):

https://codereview.chromium.org/1028563003/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:250: LOG(ERROR) << "Gamma
ramp channels must all the same length.";
I think you need a "return" here.

If r.size() > g.size() then this continues on and passes garbage to the kernel.

[oshima, 2015-04-21 19:49:44]

lgtm my bits with nits.

https://codereview.chromium.org/1028563003/diff/120001/ash/content/display/di...
File ash/content/display/display_color_manager_chromeos.cc (right):

https://codereview.chromium.org/1028563003/diff/120001/ash/content/display/di...
ash/content/display/display_color_manager_chromeos.cc:93: << "for internal
display id: " << gfx::Display::InternalDisplayId();
On 2015/04/17 18:54:47, robert.bradford wrote:
> On 2015/04/17 17:46:17, oshima wrote:
> > internal display may not exist. Please check using HasInternalDisplay.
> 
> I tackled this slightly differently. Rather than use
> DisplayManager::HasInternalDisplay I split this into a separate method where I
> test that display_id is not gfx::Display::kInvalidId.
> 
> > One note: there is a known issue (crbug.com/449631) on ozone where the
> internal
> > display information isn't available on startup. (crbug.com/438796 is an
issue
> > bitten by this bug) You may want to track this bug.
> 
> That last bug # looks wrong?

Sorry, it was the # for internal bug tracker.

https://codereview.chromium.org/1028563003/diff/140001/ui/display/chromeos/te...
File ui/display/chromeos/test/action_logger_util.cc (right):

https://codereview.chromium.org/1028563003/diff/140001/ui/display/chromeos/te...
ui/display/chromeos/test/action_logger_util.cc:64: for (size_t i = 0; i <
r.size(); ++i)
nit: you need {} this case.

[robert.bradford, 2015-04-27 13:49:33]

robert.bradford@intel.com changed reviewers:
+ dcheng@chromium.org, sievers@chromium.org

[robert.bradford, 2015-04-27 13:49:33]

Thanks spang@ and oshima@.

+ dcheng@ for ipc messages security review
+ sievers@ and noel@ as OWNERS of additions to ash/content/display/DEPS

https://codereview.chromium.org/1028563003/diff/140001/ui/display/chromeos/te...
File ui/display/chromeos/test/action_logger_util.cc (right):

https://codereview.chromium.org/1028563003/diff/140001/ui/display/chromeos/te...
ui/display/chromeos/test/action_logger_util.cc:64: for (size_t i = 0; i <
r.size(); ++i)
On 2015/04/21 19:49:44, oshima wrote:
> nit: you need {} this case.

Done.

https://codereview.chromium.org/1028563003/diff/140001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc (right):

https://codereview.chromium.org/1028563003/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:250: LOG(ERROR) << "Gamma
ramp channels must all the same length.";
On 2015/04/17 19:07:42, spang wrote:
> I think you need a "return" here.
> 
> If r.size() > g.size() then this continues on and passes garbage to the
kernel.

Done.

[robert.bradford, 2015-04-27 18:25:11]

robert.bradford@intel.com changed reviewers:
+ noel@chromium.org

[robert.bradford, 2015-04-27 18:25:11]

+ noel@ as OWNER of file added to DEPS.

[dcheng, 2015-04-28 18:45:34]

https://codereview.chromium.org/1028563003/diff/160001/ui/ozone/common/gpu/oz...
File ui/ozone/common/gpu/ozone_gpu_messages.h (right):

https://codereview.chromium.org/1028563003/diff/160001/ui/ozone/common/gpu/oz...
ui/ozone/common/gpu/ozone_gpu_messages.h:110: std::vector<uint16_t>)  // b
From a security perspective, I'd be happier if we sent this as a vector of RGB
triples, rather than having the IPC handler verify that r.size() == g.size() ==
b.size(). This does require repacking the data to pass to the system later, but
I don't think this is perf sensitive, is it?

[Noel Gordon, 2015-04-29 00:49:58]

Also re: security, Robert could you add the ICC file you used for manually
testing you changes to chrome bug 471749 please?

[robert.bradford, 2015-04-29 13:39:50]

On 2015/04/29 00:49:58, noel gordon wrote:
> Also re: security, Robert could you add the ICC file you used for manually
> testing you changes to chrome bug 471749 please?

Done.

[robert.bradford, 2015-04-29 13:40:14]

https://codereview.chromium.org/1028563003/diff/160001/ui/ozone/common/gpu/oz...
File ui/ozone/common/gpu/ozone_gpu_messages.h (right):

https://codereview.chromium.org/1028563003/diff/160001/ui/ozone/common/gpu/oz...
ui/ozone/common/gpu/ozone_gpu_messages.h:110: std::vector<uint16_t>)  // b
dcheng@ please see updated patch. I combine and split the data either side of
the IPC. Is this what you were thinking about?

I had to add a typedef to get IPC macros to accept the nested templated types in
ozone_gpu_messages.h and used the same type name in all the places this vector
of tuples is used.

Let me know what you think.

On 2015/04/28 18:45:34, dcheng wrote:
> From a security perspective, I'd be happier if we sent this as a vector of RGB
> triples, rather than having the IPC handler verify that r.size() == g.size()
==
> b.size(). This does require repacking the data to pass to the system later,
but
> I don't think this is perf sensitive, is it?

[dcheng, 2015-04-29 21:43:01]

https://codereview.chromium.org/1028563003/diff/180001/ui/ozone/common/gpu/oz...
File ui/ozone/common/gpu/ozone_gpu_messages.h (right):

https://codereview.chromium.org/1028563003/diff/180001/ui/ozone/common/gpu/oz...
ui/ozone/common/gpu/ozone_gpu_messages.h:110: int64_t,  // display ID,
Nit: make this comment consistent with the conventions in the rest of this file

https://codereview.chromium.org/1028563003/diff/180001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc (right):

https://codereview.chromium.org/1028563003/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:249: if (r.size() !=
g.size() || g.size() != b.size()) {
How about just having this take a vector of tuples and do the unpacking here? Is
there some other function that calls this? That eliminates the need to check in
several different places.

[dnicoara, 2015-04-29 21:48:10]

https://codereview.chromium.org/1028563003/diff/180001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc (right):

https://codereview.chromium.org/1028563003/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:249: if (r.size() !=
g.size() || g.size() != b.size()) {
On 2015/04/29 21:43:01, dcheng wrote:
> How about just having this take a vector of tuples and do the unpacking here?
Is
> there some other function that calls this? That eliminates the need to check
in
> several different places.

Just wondering if this could be taken one level more? Essentially, create the
tuples in DisplayColorManagerChromeos and unpack them down in DrmDevice.

Also as an extra maybe have a struct { uint16_t r, g, b; } rather than the tuple
for extra clarity.

[dbehr, 2015-04-29 22:12:42]

Hi,

Different devices have different size gamma tables. Intel is 256 entries,
Rockchip is 1024 entries, Exynos (if it ever gets enabled/implemented is 17
entries). At some level we need to reconcile the size of the incoming gamma
table from the color profile and resample it to the size reported by the device.

[Noel Gordon, 2015-04-30 10:41:48]

QCMS could provide ramp data in a single array (a block of R data, followed by a
block of G, then a block of B). Most you'd need to check is that the array
length is exactly divisble by 3.  I see no need for tuples at all.

Also, why are the ramp data components defined as uint16_t?  With what
endianness?  Would uint_8 be sufficent for this application and if not, why not?

[robert.bradford, 2015-04-30 11:34:20]

On 2015/04/30 10:41:48, noel gordon wrote:
> QCMS could provide ramp data in a single array (a block of R data, followed by
a
> block of G, then a block of B). Most you'd need to check is that the array
> length is exactly divisble by 3.  I see no need for tuples at all.

Sure, i'm happy to make that change in the QCMS API.

> Also, why are the ramp data components defined as uint16_t?  With what
> endianness?  Would uint_8 be sufficient for this application and if not, why
not?

The DRM api uses uint16_t, and expects the values in host-endianess as the
numbers are in the range 0 - 65535; It's up to the kernel/hardware to determine
how it interprets this value:

The Intel driver currently shifts the value 8 bits to right reducing it's range
to 0-255 (in essence making the data fixed point 8.8, with the fractional part
ignored) and programs that into it's registers. Newer platforms have support for
a 10-bit lut, to deal with higher bitdepth displays (and thus the values in the
API would be treated as 10.6 fixed point.)

Given that 16-bit per channel is the most likely maximum you'd use, this was
size chosen for that DRM API and in order to ensure no loss of precision we
should propagate that design choice through the ozone API.

[Noel Gordon, 2015-04-30 12:17:41]

On 2015/04/30 11:34:20, robert.bradford wrote:
> On 2015/04/30 10:41:48, noel gordon wrote:
> > QCMS could provide ramp data in a single array (a block of R data, followed
by
> a
> > block of G, then a block of B). Most you'd need to check is that the array
> > length is exactly divisble by 3.  I see no need for tuples at all.
> 
> Sure, i'm happy to make that change in the QCMS API.

Ok, let's get the QCMS part done.

> > Also, why are the ramp data components defined as uint16_t?  With what
> > endianness?  Would uint_8 be sufficient for this application and if not, why
> not?
> 
> The DRM api uses uint16_t, and expects the values in host-endianess as the
> numbers are in the range 0 - 65535; It's up to the kernel/hardware to
determine
> how it interprets this value:

Ok so uint16_t is about precision, understood.  Note that Chrome is designed for
little-endian machines and that decision runs deep in Chrome / ChromeOS. 
Question: vgct is a device independent table in a file.  What endianness are
vcgt tables entries stored in?

[Noel Gordon, 2015-04-30 12:18:20]

On 2015/04/30 11:34:20, robert.bradford wrote:
> On 2015/04/30 10:41:48, noel gordon wrote:
> > QCMS could provide ramp data in a single array (a block of R data, followed
by
> a
> > block of G, then a block of B). Most you'd need to check is that the array
> > length is exactly divisble by 3.  I see no need for tuples at all.
> 
> Sure, i'm happy to make that change in the QCMS API.

Ok, let's get the QCMS part done.

> > Also, why are the ramp data components defined as uint16_t?  With what
> > endianness?  Would uint_8 be sufficient for this application and if not, why
> not?
> 
> The DRM api uses uint16_t, and expects the values in host-endianess as the
> numbers are in the range 0 - 65535; It's up to the kernel/hardware to
determine
> how it interprets this value:

Ok so uint16_t is about precision, understood.  Note that Chrome is designed for
little-endian machines and that decision runs deep in Chrome / ChromeOS. 
Question: vgct is a device independent table in a file.  What endianness are
vcgt tables entries stored in?

[Noel Gordon, 2015-04-30 12:25:31]

On 2015/04/30 11:34:20, robert.bradford wrote:
> On 2015/04/30 10:41:48, noel gordon wrote:
> > QCMS could provide ramp data in a single array (a block of R data, followed
by
> a
> > block of G, then a block of B). Most you'd need to check is that the array
> > length is exactly divisble by 3.  I see no need for tuples at all.
> 
> Sure, i'm happy to make that change in the QCMS API.

Ok let's get the QCMS part done.
 
> > Also, why are the ramp data components defined as uint16_t?  With what
> > endianness?  Would uint_8 be sufficient for this application and if not, why
> not?
> 
> The DRM api uses uint16_t, and expects the values in host-endianess as the
> numbers are in the range 0 - 65535; It's up to the kernel/hardware to
determine
> how it interprets this value:

So uint16_t is about precision, understood.  Note that Chrome is designed for
little endian machines, and that decision runs deep in Chrome / Chrome OS.  ICC
vcgt is device independent table data stored in a file.  What endianness do the
table entries have?

[robert.bradford, 2015-04-30 12:27:25]

> Ok so uint16_t is about precision, understood.  Note that Chrome is designed
for
> little-endian machines and that decision runs deep in Chrome / ChromeOS. 
> Question: vgct is a device independent table in a file.  What endianness are
> vcgt tables entries stored in?

The VCGT data is stored in big endian format (like other numerical values in the
ICC spec), by using the qcms "read_u16()" function this value is converted to
host endianness; as such the table reported by the qcms API is in host endianess
and thus ready to put straight into the kernel api.

[dcheng, 2015-04-30 15:57:01]

When possible, it's nice to have as few manual checks for things as possible.
While packing all the R/G/B values into an array, and validating the length is
evenly divisible by 3 is clever, I think having a vector of tuples is a lot
clearer.

[robert.bradford, 2015-04-30 19:20:30]

dbehr@: Great point. In the latest version of my patch i've added a check along
with a TODO to add resampling (and I think i'll open a bug too.)

dcheng@ / dnicoara@: I've added a struct GammaRampRGBEntry that is shared across
all parts of the code and thus no longer use Tuples. Based on your feedback I
now generate the split entry in DisplayColorManager and then separate it into
individual vectors in DrmDevice.

oshima@ & spang@: Would you mind re-reviewing as this change to the CL is quite
intrusive?

Thank you!

[spang, 2015-04-30 19:32:26]

lgtm

[dcheng, 2015-04-30 20:40:24]

ipc changes lgtm with nits

https://codereview.chromium.org/1028563003/diff/220001/ui/ozone/common/gpu/oz...
File ui/ozone/common/gpu/ozone_gpu_messages.h (right):

https://codereview.chromium.org/1028563003/diff/220001/ui/ozone/common/gpu/oz...
ui/ozone/common/gpu/ozone_gpu_messages.h:52: IPC_STRUCT_TRAITS_MEMBER(r)
Nit: indent. clang-format doesn't handle this correctly, sorry =(

https://codereview.chromium.org/1028563003/diff/220001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_device.cc (right):

https://codereview.chromium.org/1028563003/diff/220001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:520: r.resize(lut.size());
Nit: reserve instead of resize.

[no sievers, 2015-05-01 16:46:21]

lgtm for deps

[robert.bradford, 2015-05-01 17:31:55]

The CQ bit was checked by robert.bradford@intel.com to run a CQ dry run

[robert.bradford, 2015-05-01 17:31:56]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
dcheng@chromium.org, spang@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1028563003/#ps260001
(title: "Rebase")

[commit-bot: I haz the power, 2015-05-01 17:33:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1028563003/260001

[commit-bot: I haz the power, 2015-05-01 17:45:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-01 17:45:36]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Noel Gordon, 2015-05-02 01:47:08]

LGTM for the qcms parts.

[robert.bradford, 2015-05-05 12:00:23]

The CQ bit was checked by robert.bradford@intel.com

[robert.bradford, 2015-05-05 12:00:24]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
dcheng@chromium.org, sievers@chromium.org, noel@chromium.org, spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1028563003/#ps300001
(title: "Fix ozone build")

[commit-bot: I haz the power, 2015-05-05 12:00:56]

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

[commit-bot: I haz the power, 2015-05-05 12:54:12]

Committed patchset #15 (id:300001)

[commit-bot: I haz the power, 2015-05-05 12:55:45]

Patchset 15 (id:??) landed as
https://crrev.com/46134a86af1eac76e97b062a68afeaa6c8801216
Cr-Commit-Position: refs/heads/master@{#328318}

[jochen (gone - plz use gerrit), 2015-05-05 13:47:59]

A revert of this CL (patchset #15 id:300001) has been created in
https://codereview.chromium.org/1118373006/ by jochen@chromium.org.

The reason for reverting is: breaks compilation

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....