Add support for more advanced color correction

Add support for more advanced color correction

Rather than use just a gamma ramp (specified in a VCGT file) this change loads
a correction matrix from the ICC file. As correction matrix needs to be applied
in a linear colorspace it is also necesary to apply a "degamma" to adjust from
rendered gamma (sRGB) to linear space. The display's gamma is then applied
after the correction matrix.

TEST=New unit tests in ash_unittests, interactive testing on Broadwell (like
samus) device.
BUG=495196
Committed: https://crrev.com/dda9fb755a50b7ea24346f9315e0d2f6a93a7864
Cr-Commit-Position: refs/heads/master@{#384525}

[robert.bradford, 2015-10-26 12:26:13]

Patchset #3 (id:40001) has been deleted

[robert.bradford, 2015-11-09 19:15:42]

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

[robert.bradford, 2015-11-09 19:15:43]

spang@ / dnicoara@ Can you guys take a look over this change to tell me what you
think. In particular the DRM/IPC bits.

Do you think that I should reuse SetColorCorrection for the legacy gamma table
support (i.e. drop SetGammaRamp and use legacy ioctl iff degamma_lut.size() ==
0)

Am I doing the right thing with the new threaded DRM code in the Ozone backend?

Any testing thoughts beyond interactive testing?

If you're interested, the kernel patches that this relies on are here:
http://lists.freedesktop.org/archives/intel-gfx/2015-October/078341.html

I've got patches prepared on top of chromeos-kernel v3.18 that are undergoing
some testing now.

[robert.bradford, 2015-11-09 19:16:44]

Description was changed from

==========
WIP: Add support for more advanced color correction

Rather than use just a gamma ramp this change loads a correction matrix
from the ICC file. As correction matrix needs to be applied in a linear
colorspace it is also necesary to apply a "degamma" to adjust from
rendered gamma (sRGB) to linear space. The display's gamma is then
applied after the correction matrix.

BUG=495196
==========

to

==========
WIP: Add support for more advanced color correction

Rather than use just a gamma ramp this change loads a correction matrix
from the ICC file. As correction matrix needs to be applied in a linear
colorspace it is also necesary to apply a "degamma" to adjust from
rendered gamma (sRGB) to linear space. The display's gamma is then
applied after the correction matrix.

BUG=495196
==========

[spang, 2015-11-09 19:28:13]

On 2015/11/09 19:15:43, robert.bradford wrote:
> spang@ / dnicoara@ Can you guys take a look over this change to tell me what
you
> think. In particular the DRM/IPC bits.
> 
> Do you think that I should reuse SetColorCorrection for the legacy gamma table
> support (i.e. drop SetGammaRamp and use legacy ioctl iff degamma_lut.size() ==
> 0)

I think that would be preferable yes.

> 
> Am I doing the right thing with the new threaded DRM code in the Ozone
backend?

It looks fine to me.

> 
> Any testing thoughts beyond interactive testing?
> 
> If you're interested, the kernel patches that this relies on are here:
> http://lists.freedesktop.org/archives/intel-gfx/2015-October/078341.html
> 
> I've got patches prepared on top of chromeos-kernel v3.18 that are undergoing
> some testing now.

[spang, 2015-11-09 19:40:47]

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

https://codereview.chromium.org/1182063002/diff/120001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:214: #define RESAMPLE(array, m, i, r) \
Please put a function in the anonymous namespace instead of using a macro.

https://codereview.chromium.org/1182063002/diff/120001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_display.h (right):

https://codereview.chromium.org/1182063002/diff/120001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_display.h:46: const float correction_matrix[9]);
This declaration just turns into const float*, the bound isn't preserved.
Probably just use vector..

[robert.bradford, 2016-02-04 18:45:55]

Description was changed from

==========
WIP: Add support for more advanced color correction

Rather than use just a gamma ramp this change loads a correction matrix
from the ICC file. As correction matrix needs to be applied in a linear
colorspace it is also necesary to apply a "degamma" to adjust from
rendered gamma (sRGB) to linear space. The display's gamma is then
applied after the correction matrix.

BUG=495196
==========

to

==========
Add support for more advanced color correction

Rather than use just a gamma ramp (specified in a VCGT file) this change loads
a correction matrix from the ICC file. As correction matrix needs to be applied
in a linear colorspace it is also necesary to apply a "degamma" to adjust from
rendered gamma (sRGB) to linear space. The display's gamma is then applied
after the correction matrix.

TEST=New unit tests in ash_unittests, interactive testing on Broadwell (like
samus) device.
BUG=495196
==========

[robert.bradford, 2016-02-04 18:46:04]

Description was changed from

==========
Add support for more advanced color correction

Rather than use just a gamma ramp (specified in a VCGT file) this change loads
a correction matrix from the ICC file. As correction matrix needs to be applied
in a linear colorspace it is also necesary to apply a "degamma" to adjust from
rendered gamma (sRGB) to linear space. The display's gamma is then applied
after the correction matrix.

TEST=New unit tests in ash_unittests, interactive testing on Broadwell (like
samus) device.
BUG=495196
==========

to

==========
Add support for more advanced color correction

Rather than use just a gamma ramp (specified in a VCGT file) this change loads
a correction matrix from the ICC file. As correction matrix needs to be applied
in a linear colorspace it is also necesary to apply a "degamma" to adjust from
rendered gamma (sRGB) to linear space. The display's gamma is then applied
after the correction matrix.

TEST=New unit tests in ash_unittests, interactive testing on Broadwell (like
samus) device.
BUG=495196
==========

[robert.bradford, 2016-02-04 18:46:46]

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

[robert.bradford, 2016-02-04 18:52:27]

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

[robert.bradford, 2016-02-04 18:52:28]

oshima@ Please review the changes in ash/

spang@ & dnicoara@ Please take a look at the ozone bits. I did as you requested
and collapsed everything into using SetColorCorrection IPC.

glevin@ FYI

For some more background please see this document:
https://docs.google.com/document/d/1a21jCKtcogqFc8F9CVNpmp1c6Wh9z5cuodPpO0PM6fo/

Although the new functionality requires kernel patches (adding new properties)
this code is designed to not require them for legacy functionality. i.e. this is
safe to land out -of-order with the kernel patches.

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

https://codereview.chromium.org/1182063002/diff/120001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:214: #define RESAMPLE(array, m, i, r) \
On 2015/11/09 19:40:47, spang wrote:
> Please put a function in the anonymous namespace instead of using a macro.

Ended up with it inline which worked better than a function.

https://codereview.chromium.org/1182063002/diff/120001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_display.h (right):

https://codereview.chromium.org/1182063002/diff/120001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_display.h:46: const float correction_matrix[9]);
On 2015/11/09 19:40:47, spang wrote:
> This declaration just turns into const float*, the bound isn't preserved.
> Probably just use vector..

Done.

[dnicoara, 2016-02-10 20:33:31]

Sorry for the late response. Here are some initial comments. I'm not
particularly familiar with the math behind color correction, so I'll leave it to
glevin@ to make sure that looks correct.

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

https://codereview.chromium.org/1182063002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:117: struct _drm_color_lut {
structs should follow the Chromium naming convention: DrmColorLut

Is this supposed to be from a libdrm header file?

https://codereview.chromium.org/1182063002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:118: __u16 red;
Use uint16_t

https://codereview.chromium.org/1182063002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:124: struct _drm_color_ctm {
Same: DrmColorCtm

https://codereview.chromium.org/1182063002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:125: __s64 ctm_coeff[9];
Use int64_t.

https://codereview.chromium.org/1182063002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:174: res = drmModeCreatePropertyBlob(fd,
&data[0], data.size(), &blob_id);
nit: Use vector_as_array(&data) from base/stl_util.h

https://codereview.chromium.org/1182063002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:687: if (property &&
!strcmp(property->name, "DEGAMMA_LUT")) {
nit: Maybe check the property is valid once. Something like:

if (!property)
  continue;

[dnicoara, 2016-02-11 14:57:31]

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

https://codereview.chromium.org/1182063002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:174: res = drmModeCreatePropertyBlob(fd,
&data[0], data.size(), &blob_id);
On 2016/02/10 20:33:31, dnicoara wrote:
> nit: Use vector_as_array(&data) from base/stl_util.h 

Actually, looks like this got deprecated in favor of std::vector::data

[robert.bradford, 2016-02-12 14:51:13]

Patchset #8 (id:160001) has been deleted

[robert.bradford, 2016-02-12 14:55:47]

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

[robert.bradford, 2016-02-12 14:55:50]

Hi dnicoara@ thanks for the feedback. Should be all integrated in new patch set.

+ dcheng@ IPC reviewer of ozone_gpu_messages*.h

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

https://codereview.chromium.org/1182063002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:117: struct _drm_color_lut {
On 2016/02/10 20:33:31, dnicoara wrote:
> structs should follow the Chromium naming convention: DrmColorLut
> 
> Is this supposed to be from a libdrm header file?

Yes ultimately but there is delay in getting changes to from the kernel API
header into libdrm. For now i'll make them Chromium style and mark with a //TODO
and a bug to roll libdrm.

https://codereview.chromium.org/1182063002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:118: __u16 red;
On 2016/02/10 20:33:31, dnicoara wrote:
> Use uint16_t

Done.

https://codereview.chromium.org/1182063002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:124: struct _drm_color_ctm {
On 2016/02/10 20:33:31, dnicoara wrote:
> Same: DrmColorCtm

Done.

https://codereview.chromium.org/1182063002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:125: __s64 ctm_coeff[9];
On 2016/02/10 20:33:31, dnicoara wrote:
> Use int64_t.

Done.

https://codereview.chromium.org/1182063002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:174: res = drmModeCreatePropertyBlob(fd,
&data[0], data.size(), &blob_id);
On 2016/02/11 14:57:31, dnicoara wrote:
> On 2016/02/10 20:33:31, dnicoara wrote:
> > nit: Use vector_as_array(&data) from base/stl_util.h 
> 
> Actually, looks like this got deprecated in favor of std::vector::data

Done.

https://codereview.chromium.org/1182063002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:687: if (property &&
!strcmp(property->name, "DEGAMMA_LUT")) {
On 2016/02/10 20:33:31, dnicoara wrote:
> nit: Maybe check the property is valid once. Something like:
> 
> if (!property)
>   continue;

Done.

[robert.bradford, 2016-02-22 16:44:31]

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

[robert.bradford, 2016-02-22 16:44:32]

spang@/oshima@/dcheng@ ping! Looking for review on this change (please disregard
the "WIP" in the email subject - that no longer applies.)

[spang, 2016-02-22 18:57:51]

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

https://codereview.chromium.org/1182063002/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:137: DrmColorLut* lut =
reinterpret_cast<DrmColorLut*>(data.data());
Please use a vector<DrmColorLut> and cast to unsigned char, not the other way
around. Casting unsigned char* to DrmColorLut* is against the type aliasing
rules and undefined behavior.

https://codereview.chromium.org/1182063002/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:151: DrmColorCtm* ctm =
reinterpret_cast<DrmColorCtm*>(data.data());
Same here, it's not OK to cast unsigned char* to a struct in general. Casting a
struct to char* is allowed.

https://codereview.chromium.org/1182063002/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:171: std::vector<uint8_t> data) {
const &

https://codereview.chromium.org/1182063002/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:176: LOG(ERROR) << "Error creating
property blob: " << base::safe_strerror(res);
PLOG(ERROR)

https://codereview.chromium.org/1182063002/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:181: LOG(ERROR) << "Error updating
property: " << base::safe_strerror(res);
PLOG(ERROR)

https://codereview.chromium.org/1182063002/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:185: drmModeDestroyPropertyBlob(fd,
blob_id);
I don't understand the lifetime of the blob. Who's responsible for destroying
the blob after this function returns? Are we just leaking it every time?

https://codereview.chromium.org/1182063002/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:674: LOG(WARNING) << "No available
(de)gamma tables.";
Will we hit this if color correction isn't supported or does it not get called
in that case?

https://codereview.chromium.org/1182063002/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:694: LOG(WARNING) << "Error setting
degamma property.";
Can you pass the property name into SetBlobProperty, add it to the internal
LOG(ERROR), and then remove these duplicate logs?

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

https://codereview.chromium.org/1182063002/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_display.cc:180: << " size = " << gamma_lut.size();
PLOG?

[spang, 2016-02-22 19:01:46]

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

https://codereview.chromium.org/1182063002/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:176: LOG(ERROR) << "Error creating
property blob: " << base::safe_strerror(res);
On 2016/02/22 18:57:50, spang wrote:
> PLOG(ERROR)

Nevermind that's for strerror(errno).

[robert.bradford, 2016-02-23 19:23:01]

Hi spang@ thanks for the review. I dealt with all the memory & casting issues.
Could you take another look.

As regards the PLOG feedback, i'm going to try and work out if returned value is
always equal to errno on the error conditions and if so then use PLOG.

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

https://codereview.chromium.org/1182063002/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:137: DrmColorLut* lut =
reinterpret_cast<DrmColorLut*>(data.data());
On 2016/02/22 18:57:50, spang wrote:
> Please use a vector<DrmColorLut> and cast to unsigned char, not the other way
> around. Casting unsigned char* to DrmColorLut* is against the type aliasing
> rules and undefined behavior.

Ack. Replaced with a scoped_ptr w/ base::FreeDeleter and malloc based
allocations. I think this makes the memory management much clearer.

https://codereview.chromium.org/1182063002/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:151: DrmColorCtm* ctm =
reinterpret_cast<DrmColorCtm*>(data.data());
On 2016/02/22 18:57:50, spang wrote:
> Same here, it's not OK to cast unsigned char* to a struct in general. Casting
a
> struct to char* is allowed.

Done. The cast is migrated now only to the SetBlobProperty() call point

https://codereview.chromium.org/1182063002/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:171: std::vector<uint8_t> data) {
On 2016/02/22 18:57:50, spang wrote:
> const &

Replaced with a raw pointer now using scoped_ptr and direct memory allocations.

https://codereview.chromium.org/1182063002/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:185: drmModeDestroyPropertyBlob(fd,
blob_id);
On 2016/02/22 18:57:50, spang wrote:
> I don't understand the lifetime of the blob. Who's responsible for destroying
> the blob after this function returns? Are we just leaking it every time?

I was using the stack allocation of the std::vector<unsigned char>, in the
calling stack frame, as the memory management so that the contents of what goes
into the blob isn't leaked. However as you pointed out that wasn't safe so I
went with scoped_ptr instead.

The DRM blobs themselves aren't leaked as we call drmModeDestroyPropertyBlob
(which is really a ref count, the kernel still holds onto the blob internally
whilst it's using it.)

https://codereview.chromium.org/1182063002/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:674: LOG(WARNING) << "No available
(de)gamma tables.";
On 2016/02/22 18:57:50, spang wrote:
> Will we hit this if color correction isn't supported or does it not get called
> in that case?

This is protection against a broken kernel implementation. The properties are
generic DRM APIs. If you don't have a degamma table then there is no way to
correctly apply a matrix transformation (they only make sense in a linear colour
space.) Similarly if you don't have a gamma then you can't display anything
useful to the user as linear output looks strange.

https://codereview.chromium.org/1182063002/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:694: LOG(WARNING) << "Error setting
degamma property.";
On 2016/02/22 18:57:50, spang wrote:
> Can you pass the property name into SetBlobProperty, add it to the internal
> LOG(ERROR), and then remove these duplicate logs?

Done.

[robert.bradford, 2016-02-23 19:24:21]

https://codereview.chromium.org/1182063002/diff/200001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/common/drm_util.cc (right):

https://codereview.chromium.org/1182063002/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/common/drm_util.cc:166: if (property &&
!strcmp(property->name, "CTM")) {
Property name changed in kernel interface.

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

https://codereview.chromium.org/1182063002/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:702: if (!strcmp(property->name, "CTM"))
{
CTM_MATRIX -> CTM in kernel API.

[dcheng, 2016-02-23 20:01:55]

https://codereview.chromium.org/1182063002/diff/200001/ash/display/display_co...
File ash/display/display_color_manager_chromeos.cc (right):

https://codereview.chromium.org/1182063002/diff/200001/ash/display/display_co...
ash/display/display_color_manager_chromeos.cc:34: base::FilePath
PathForProductID(int64_t product_id) {
#include <stdint.h>

https://codereview.chromium.org/1182063002/diff/200001/ash/display/display_co...
ash/display/display_color_manager_chromeos.cc:216: request =
base::Callback<bool(void)>(
What's this cast for?

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

https://codereview.chromium.org/1182063002/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:130: DrmColorLut* CreateLutBlob(const
std::vector<GammaRampRGBEntry>& source) {
Return a scoped_ptr here and below.

https://codereview.chromium.org/1182063002/diff/200001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_display.cc (right):

https://codereview.chromium.org/1182063002/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_display.cc:177: if (degamma_lut.size() == 0) {
.empty()

[robert.bradford, 2016-02-24 12:34:20]

Thanks for the review dcheng@! New patch uploaded.

https://codereview.chromium.org/1182063002/diff/200001/ash/display/display_co...
File ash/display/display_color_manager_chromeos.cc (right):

https://codereview.chromium.org/1182063002/diff/200001/ash/display/display_co...
ash/display/display_color_manager_chromeos.cc:34: base::FilePath
PathForProductID(int64_t product_id) {
On 2016/02/23 20:01:55, dcheng wrote:
> #include <stdint.h>

Done.

https://codereview.chromium.org/1182063002/diff/200001/ash/display/display_co...
ash/display/display_color_manager_chromeos.cc:216: request =
base::Callback<bool(void)>(
On 2016/02/23 20:01:54, dcheng wrote:
> What's this cast for?

Spurious. Fixed!

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

https://codereview.chromium.org/1182063002/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:130: DrmColorLut* CreateLutBlob(const
std::vector<GammaRampRGBEntry>& source) {
On 2016/02/23 20:01:55, dcheng wrote:
> Return a scoped_ptr here and below.

Done.

https://codereview.chromium.org/1182063002/diff/200001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_display.cc (right):

https://codereview.chromium.org/1182063002/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_display.cc:177: if (degamma_lut.size() == 0) {
On 2016/02/23 20:01:55, dcheng wrote:
> .empty()

Done.

[dnicoara, 2016-02-24 14:24:34]

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

https://codereview.chromium.org/1182063002/diff/220001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:130: typedef scoped_ptr<DrmColorLut,
base::FreeDeleter> ScopedDrmColorLut;
nit: Add a "Ptr" suffix to match the other ScopedDrm*Ptr typedefs.

https://codereview.chromium.org/1182063002/diff/220001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:147: ScopedDrmColorCtm ctm(new
DrmColorCtm);
You're allocating with new() but freeing with free(). Since you're not passing
ownership to DRM is there any reason to use malloc/free?

The one thing that I could think of is to be closer to the DRM allocation type,
especially since these will be done via a DRM API (once that's available). Is
this the case?

[robert.bradford, 2016-02-24 15:22:05]

Thanks dnicoara@ typedefs renamed and malloc used in place of new.

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

https://codereview.chromium.org/1182063002/diff/220001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:130: typedef scoped_ptr<DrmColorLut,
base::FreeDeleter> ScopedDrmColorLut;
On 2016/02/24 14:24:34, dnicoara wrote:
> nit: Add a "Ptr" suffix to match the other ScopedDrm*Ptr typedefs.

Done.

https://codereview.chromium.org/1182063002/diff/220001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:147: ScopedDrmColorCtm ctm(new
DrmColorCtm);
On 2016/02/24 14:24:34, dnicoara wrote:
> You're allocating with new() but freeing with free(). Since you're not passing
> ownership to DRM is there any reason to use malloc/free?

You're right, malloc & free is best.

> The one thing that I could think of is to be closer to the DRM allocation
type,
> especially since these will be done via a DRM API (once that's available). Is
> this the case?

These are just raw structures, you could allocate them however you like as the
ioctl() will ultimately copy from userspace to kernel space. libdrm won't ever
provide any kind of allocation mechanism for these.

[spang, 2016-02-24 16:05:45]

lgtm

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

https://codereview.chromium.org/1182063002/diff/240001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:149: for (int i = 0; i < 9; ++i) {
i < arraysize(ctm->ctm_coeff)

[dcheng, 2016-02-26 09:37:48]

ipc lgtm

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

https://codereview.chromium.org/1182063002/diff/240001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:130: typedef scoped_ptr<DrmColorLut,
base::FreeDeleter> ScopedDrmColorLutPtr;
Nit: prefer using ScopedDrmColorLutPtr = ... to typedefs in new code.

https://codereview.chromium.org/1182063002/diff/240001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:136: DrmColorLut* lut =
ScopedDrmColorLutPtr lut(static_cast<DrmColorLut*>(malloc(sizeof(DrmColorLut) *
source.size())));

And just return lut on 143. Ditto for CreateCTMBlob.

[robert.bradford, 2016-02-26 15:15:22]

Much appreciated spang@ & dcheng@. oshima@ please review when you can.

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

https://codereview.chromium.org/1182063002/diff/240001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:130: typedef scoped_ptr<DrmColorLut,
base::FreeDeleter> ScopedDrmColorLutPtr;
On 2016/02/26 09:37:48, dcheng wrote:
> Nit: prefer using ScopedDrmColorLutPtr = ... to typedefs in new code.

Ah good to know. Done.

https://codereview.chromium.org/1182063002/diff/240001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:136: DrmColorLut* lut =
On 2016/02/26 09:37:48, dcheng wrote:
> ScopedDrmColorLutPtr lut(static_cast<DrmColorLut*>(malloc(sizeof(DrmColorLut)
*
> source.size())));
> 
> And just return lut on 143. Ditto for CreateCTMBlob.

Done.

https://codereview.chromium.org/1182063002/diff/240001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:149: for (int i = 0; i < 9; ++i) {
On 2016/02/24 16:05:44, spang wrote:
> i < arraysize(ctm->ctm_coeff)

Done.

[robert.bradford, 2016-03-08 11:38:21]

oshima@ ping? We're almost ready with the kernel patches so it would be nice to
get any work needed on this CL done this week.

[oshima, 2016-03-11 01:35:41]

lgtm with nits

https://codereview.chromium.org/1182063002/diff/250001/ash/display/display_co...
File ash/display/display_color_manager_chromeos.cc (right):

https://codereview.chromium.org/1182063002/diff/250001/ash/display/display_co...
ash/display/display_color_manager_chromeos.cc:75: }
nit: VLOG_IF

https://codereview.chromium.org/1182063002/diff/250001/ash/display/display_co...
File ash/display/display_color_manager_chromeos_unittest.cc (right):

https://codereview.chromium.org/1182063002/diff/250001/ash/display/display_co...
ash/display/display_color_manager_chromeos_unittest.cc:129:
modes.push_back(&mode);
nit: can this just be on stack? same for the rest.

[robert.bradford, 2016-03-22 12:01:57]

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

[commit-bot: I haz the power, 2016-03-22 12:02:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1182063002/310001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1182063002/310001

[commit-bot: I haz the power, 2016-03-22 12:40:07]

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

[commit-bot: I haz the power, 2016-03-22 12:40:09]

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

[robert.bradford, 2016-03-22 16:26:21]

Hi spang@ requesting re-review after "importing" the missing drm function calls
into drm_device.cc directly.

[spang, 2016-03-23 16:19:37]

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

https://codereview.chromium.org/1182063002/diff/320001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:130: struct _drm_mode_create_blob {
Follow our naming style.

DrmModeCreateBlob

https://codereview.chromium.org/1182063002/diff/320001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:136: struct _drm_mode_destroy_blob {
DrmModeDestroyBlob

https://codereview.chromium.org/1182063002/diff/320001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:140: #define
_DRM_IOCTL_MODE_CREATEPROPBLOB \
Should be able to just #ifdef these.

https://codereview.chromium.org/1182063002/diff/320001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:145: int _drmModeCreatePropertyBlob(int
fd,
CreatePropertyBlob()

https://codereview.chromium.org/1182063002/diff/320001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:171: int _drmModeDestroyPropertyBlob(int
fd, uint32_t id) {
DestroyPropertyBlob()

[robert.bradford, 2016-03-23 20:52:33]

Thank you spang@ for how to deal with the imported code, I wasn't sure to what
extent I should look like the DRM API.

I've chatted to glevin@ and he's got a CL almost ready to land that touches
ash/display/display_color_manager_chromeos* too. I'll wait for his to land and
then rebase mine on top.

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

https://codereview.chromium.org/1182063002/diff/320001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:130: struct _drm_mode_create_blob {
On 2016/03/23 16:19:37, spang wrote:
> Follow our naming style.
> 
> DrmModeCreateBlob

Done.

https://codereview.chromium.org/1182063002/diff/320001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:136: struct _drm_mode_destroy_blob {
On 2016/03/23 16:19:37, spang wrote:
> DrmModeDestroyBlob

Done.

https://codereview.chromium.org/1182063002/diff/320001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:140: #define
_DRM_IOCTL_MODE_CREATEPROPBLOB \
On 2016/03/23 16:19:37, spang wrote:
> Should be able to just #ifdef these.

Done.

https://codereview.chromium.org/1182063002/diff/320001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:145: int _drmModeCreatePropertyBlob(int
fd,
On 2016/03/23 16:19:37, spang wrote:
> CreatePropertyBlob()

Done.

https://codereview.chromium.org/1182063002/diff/320001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device.cc:171: int _drmModeDestroyPropertyBlob(int
fd, uint32_t id) {
On 2016/03/23 16:19:37, spang wrote:
> DestroyPropertyBlob()

Done.

[spang, 2016-03-23 21:49:37]

lgtm

[robert.bradford, 2016-04-01 08:14:06]

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

[robert.bradford, 2016-04-01 08:14:07]

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

[commit-bot: I haz the power, 2016-04-01 08:14:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1182063002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1182063002/360001

[commit-bot: I haz the power, 2016-04-01 09:08:44]

Description was changed from

==========
Add support for more advanced color correction

Rather than use just a gamma ramp (specified in a VCGT file) this change loads
a correction matrix from the ICC file. As correction matrix needs to be applied
in a linear colorspace it is also necesary to apply a "degamma" to adjust from
rendered gamma (sRGB) to linear space. The display's gamma is then applied
after the correction matrix.

TEST=New unit tests in ash_unittests, interactive testing on Broadwell (like
samus) device.
BUG=495196
==========

to

==========
Add support for more advanced color correction

Rather than use just a gamma ramp (specified in a VCGT file) this change loads
a correction matrix from the ICC file. As correction matrix needs to be applied
in a linear colorspace it is also necesary to apply a "degamma" to adjust from
rendered gamma (sRGB) to linear space. The display's gamma is then applied
after the correction matrix.

TEST=New unit tests in ash_unittests, interactive testing on Broadwell (like
samus) device.
BUG=495196
==========

[commit-bot: I haz the power, 2016-04-01 09:08:46]

Committed patchset #18 (id:360001)

[commit-bot: I haz the power, 2016-04-01 09:10:17]

Description was changed from

==========
Add support for more advanced color correction

Rather than use just a gamma ramp (specified in a VCGT file) this change loads
a correction matrix from the ICC file. As correction matrix needs to be applied
in a linear colorspace it is also necesary to apply a "degamma" to adjust from
rendered gamma (sRGB) to linear space. The display's gamma is then applied
after the correction matrix.

TEST=New unit tests in ash_unittests, interactive testing on Broadwell (like
samus) device.
BUG=495196
==========

to

==========
Add support for more advanced color correction

Rather than use just a gamma ramp (specified in a VCGT file) this change loads
a correction matrix from the ICC file. As correction matrix needs to be applied
in a linear colorspace it is also necesary to apply a "degamma" to adjust from
rendered gamma (sRGB) to linear space. The display's gamma is then applied
after the correction matrix.

TEST=New unit tests in ash_unittests, interactive testing on Broadwell (like
samus) device.
BUG=495196

Committed: https://crrev.com/dda9fb755a50b7ea24346f9315e0d2f6a93a7864
Cr-Commit-Position: refs/heads/master@{#384525}
==========

[commit-bot: I haz the power, 2016-04-01 09:10:18]

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