[Intel-gfx] [PATCH 00/18] Color Management for DRM

[Sharma, Shashank]

Hey Rob,
My comments inline.

Regards
Shashank
On 9/8/2015 4:19 PM, Rob Bradford wrote:
> On Thu, 2015-08-06 at 22:08 +0530, Shashank Sharma wrote:
>> This patch set adds Color Manager implementation in DRM layer. Color
>> Manager is
>> an extension in DRM framework to support color
>> correction/enhancement. Various
>> Hardware platforms can support several color correction capabilities.
>> Color Manager provides abstraction of these capabilities and allows a
>> user space UI agent to correct/enhance the display using the DRM
>> property interface.
>
>
> I think this patch series is a bit too fragmented and so it's hard to
> review because the code is spread over too many commits. Could you look
> at consolidating them.
>
The whole idea and requirements we agreed on initially was to implement 
it first for CHV/BSW and then implement it for BDW+. Hence the 
sequencing is like that.
> Some suggestions:
>
> - merge the the data structures with adding the properties that use
> them.
> - merge 02 and 03
> - tidy up the ordering so that you e.g. add CTM infrastructure and then
> program it for BDW and then for BSW, and similarly for gamma and
> degamma.
I am afraid we are a bit late for this.
Matt has done 4 set of reviews in this order, and if I rearrange it, it 
would be too much to ask from him to do it all again.

If we consider this enabling all the features for one platform first and 
then extending it to others, this might not look that fragmented to you, 
try this out :)
> - and conversely i think patch 05 tries to do too much, it sets up
> infrastructure and implements it for CHV.
I think I can split this into two patches.
> - (nit)try and improve the terminology (and their consistency) in the
> patches and commit messages, CTM, CSC, deGamma, gamma.
Its pretty clear, actually, may be this would help.
On DRM layer its CTM, palette_befor_ctm and palette_after_ctm, which is 
as defined in the framework.
On i915 layer its CSC, degamma and gammma, which is specific to intel 
hardware. Does it make sense now ?
>
> Rob
>