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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
