[Intel-gfx] [PATCH 1/4] drm/i915: Color manager framework for valleyview

[Daniel Vetter]

On Wed, Sep 10, 2014 at 02:17:02PM -0700, Matt Roper wrote:
> On Wed, Sep 10, 2014 at 04:50:56PM +0530, Sharma, Shashank wrote:
> ...
> > >>+
> > >>+/* Properties */
> > >>+enum clrmgr_tweaks {
> > >>+	csc = 0,
> > >>+	gamma,
> > >>+	contrast,
> > >>+	brightness,
> > >>+	hue_saturation,
> > >>+	clrmgr_tweak_invalid
> > >>+};
> > >
> > >These are just enums into your property name array, right?   I'm not
> > >sure if we need these either.
> > >
> > >
> > Actually my plan was like this:
> > One enum(like this), which assigns color property id to each property.
> > Arrays, arranged in order of this enum for sizes, name and
> > init_values of these properties. So it would be easy to access, easy
> > to plug in new property, and less hard coding.
> > +/* Properties */
> > enum clrmgr_tweaks {
> > 	csc = 0,
> > 	gamma,
> > 	contrast,
> > 	brightness,
> > 	hue_saturation,
> > 	clrmgr_tweak_invalid
> > };
> > 
> > array color_prop_sizes{size_csc, size_gamma, size_cont, size_bright};
> > array color_prop_name{"csc", "gamma", "cont", "bright"};
> > array init_values{{9 init values for CSC} {129 init values for
> > gamma} {1 default value of contrast} {1 default val for brightness}}
> > 
> > Does it look that bad :) ?
> 
> Okay, I think I see what you were going for, but I'm still not convinced
> that pulling these values out into separate arrays is more clear at the
> moment.  You still have to make sure your arrays stay in sync and if
> there's any control flow (e.g., some properties are only relevant on
> certain platforms), then that gets more complicated as well.

Yeah, aiming for too much clever structure in your code usually means you
get to toss it all away again for the next generation. Some of the
properties here also look generic enough that we want to make them
universally known, i.e. put them into dev->mode_config so that all drivers
can use them (contrast, brightness, hue_saturation and gamma imo fall into
this cathegory).

> I'd suggest not using the arrays for now while we only have a handful of
> properties.  Once we have a large list of properties in the driver maybe
> we can revisit whether there's a nicer way to stick them in a table and 
> simplify the code.

Yeah, refactoring after the fact is what I recommend when creating
completely new stuff like the color management support here. It's a
different story if there's already existing support infrastructure around,
where it's generally a good idea to reuse them (and refactor first to make
that possible, if it doesn't fit perfectly).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch