Re: [PATCH 0/4] Color manager framework

[Daniel Vetter <daniel@ffwll.ch>]

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@intel.com wrote:
> > >>From: Shashank Sharma <shashank.sharma@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