[Intel-gfx] [PATCH 0/4] Color manager framework

Thu Sep 11 10:18:01 CEST 2014

Thanks Bob, Matt, Daniel for the review. 
I will create a new patch set, complying all (almost :)) review comments from you all, and will float for review. 

Points I noted down overall: 
1.  No need for separate arrays for names, sizes etc (:( )
2.  Create a _set_blob for blob_properties, and take that route for setting values of blob property in blob.
3.  Keep hue and saturation as separate properties, programming the same control register.
4.  Create a generic function _create_crtc/plane_property (instead of create_color_property), and create properties there. This function might go in intel_display.c or intel_modes.c (?) . 
5.  Move some of the existing property creation code in that function, if required.
6.  Attach the property to object while initializing the CRTC/plane.
7.  Keep gamma, brightness, contrast, hue, saturation in mode_config.
8.  CSC coeff might be different for big core and VLV, but still the property is valid, and can be configured. Also, I can see CSC specified in Tegra driver (linux for tegra code, csc.c) which suggests its more than us who might be interested. 
     So lets keep CSC also  in mode_config. 

Please let me know if this sounds ok, or you suggest modifications. 

Regards
Shashank

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
Sent: Thursday, September 11, 2014 1:27 PM
To: Roper, Matthew D
Cc: Sharma, Shashank; intel-gfx at lists.freedesktop.org; Vetter, Daniel
Subject: Re: [Intel-gfx] [PATCH 0/4] Color manager framework

On Wed, Sep 10, 2014 at 11:15:11AM -0700, Matt Roper wrote:
> On Wed, Sep 10, 2014 at 04:38:43PM +0530, Sharma, Shashank wrote:
> > Hello Matt,
> > 
> > Thanks for your detailed descriptions, reviews comments and time.
> > Please find my comments inline.
> > 
> > Regards
> > Shashank
> > On 9/10/2014 6:58 AM, Matt Roper wrote:
> > >On Tue, Sep 09, 2014 at 11:53:12AM +0530, shashank.sharma at intel.com wrote:
> > >>From: Shashank Sharma <shashank.sharma at intel.com>
> > >>
> > >>Color manager is an extention to i915 driver which provides 
> > >>display tuning and color-correction properties to user space, via 
> > >>DRM propery interface.Different Intel platforms support different 
> > >>color tuning capabilities which can be exploited using this framework.
> > >>
> > >>This patch set adds color correction for VLV, and the code is 
> > >>written in such a way that new color properties and support for 
> > >>other platforms can be pluggen in, using the same architecture.
> > >>
> > >>For the design discussion, right now only one color correction 
> > >>property (CSC) has been added. Once we agree on the design, 
> > >>gamma-correction, contrast, brightness, and hue/saturation would be followed by the same.
> > >>
> > >>What this patch set does, is:
> > >>1. Color manager framework for valleyview:
> > >>Add basic functions of framework, to create and destroy DRM 
> > >>properties for color correction. It also adds header, enums and defines.
> > >>
> > >>2. Plug-in color manager attach
> > >>Attach created color properties, with the objects. Basically 
> > >>properties get attached to two type of objects, crtc (pipe level correction) and plane (plane level correction).
> > >>
> > >>3. CSC color correction
> > >>First color correction method.
> > >>- Add color property for CSC during init.
> > >>- Add blob to keep correction values
> > >>- Attach DRM CSC propery with each CRTC object
> > >>- Core CSC correction for VLV
> > >>
> > >>4. Add set_property function for intel CRTC.
> > >>This function checks the type of the property, and calls the 
> > >>appropriate high level function (intel_clrmgr_set_csc), which does 
> > >>platform check and calls the core platform color correction 
> > >>function (vlv_set_csc)
> > >>
> > >>V2: Re-designed based on review comments and suggestions from Matt Roper and Daniel.
> > >>- Creating only one property per color correction, and attaching it to every DRM object.
> > >>- No additional data structures on top of DRM property.
> > >>- No registeration of color property, just create and attach.
> > >>- Added properties in dev->mode_config
> > >
> > >Hi Shashank, thanks for incorporating the feedback from the last 
> > >review round.  This patchset is definitely moving in the right 
> > >direction, although I still feel that there's still a little bit of 
> > >unnecessary work/complexity in creating a "framework" here where we 
> > >probably don't need it.
> > >
> > >I'll give some more detailed responses on your individual patches, 
> > >but at a high level I don't really see the need to treat color 
> > >correction properties (csc, gamma, etc.) in a special way with 
> > >their own registration framework.  There are really three things to consider here:
> > >  * How/where to create the properties
> > >  * How/where to attach properties to CRTC's and/or planes
> > >  * How to handle property changes
> > >
> > >For creating properties, at the moment you're doing that via
> > >intel_modeset_init() -> intel_clrmgr_init() -> 
> > >intel_clrmgr_create_color_properties().  Presumably we'll add other 
> > >(non-color correction) properties to the driver in the future, so 
> > >it seems like it would be simpler if we just renamed your
> > >intel_clrmgr_create_color_properties() to something like
> > >intel_modeset_create_properties() and called it directly from 
> > >intel_modeset_init().  I don't see the intermediate 
> > >intel_clrmgr_init() adding any value at the moment, so I think it can be removed.
> > >
> > I got this point Matt, and we can do this. The only confusion I have 
> > is, we already have places where we create properties, and if we are 
> > intending to create different type of properties in this function, 
> > would that align with the previous property creations places ?
> > 
> > At least we have a common ground among the color properties,  that's 
> > why I am sticking them together.
> > 
> > If you dont like this, the other common ground is CRTC/plane 
> > properties. These are the first of CRTC or plane properties.
> > So I can create a intel_create_crtc_properties()
> 
> We do actually have one plane property (rotation) now that just went 
> into the driver.  But you're right that we also have connector 
> properties that are handled a bit differently at the moment.  There 
> are 'audio' and 'Broadcast RGB' properties that are stored in dev_priv 
> and get created the first time they are attached to a connector.  
> Personally I don't see a problem with moving the creation of those two 
> properties into your new intel_modeset_create_properties() function; 
> it's not like we're using up a ton of memory in cases where we don't 
> have any relevant connectors to attach them to.
> 
> We also have a whole bunch of SDVO/TV connector properties that are 
> actually stored in the connector itself, which seems a bit off to me.
> Generally properties get stored in a central location like dev or 
> dev_priv so that they can be shared between multiple objects.  I'm not 
> really familiar with the SDVO code, so I'm not sure if there's some 
> subtle reason the code was written this way or whether it's just 
> really old code that nobody ever cleaned up (git blame does indicate 
> that most of that logic is from 2009-2010).

Generally there's just one sdvo/tv-out connector, and the properties are highly dynamic. We could refactor that, but just not worth it.

> I'd be inclined to move the audio and broadcast properties in 
> alongside your new color property setup code (and just make it a 
> generic property setup function as noted).  Same with the rotation 
> plane property.  Since the SDVO/TV properties are a bit of a different 
> world (and since I'm not personally familiar enough with it to feel 
> comfortable touching it), I'd leave those as they are for now.  But if 
> any of the OTC reviewers think differently, I'll defer to their judgment.

Hm, I don't care whether we do lazy&distributed or eager&central prop creation. Imo the important part is that attaching the properties is done where the objects get initialized and not in a central place, since I don't see any value in that indirection. And having it per-object all together should make it easier to adjust property-attachment for platform-oddities ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch