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@xxxxxxxx] On Behalf Of Daniel Vetter
Sent: Thursday, September 11, 2014 1:27 PM
To: Roper, Matthew D
Cc: Sharma, Shashank; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Vetter, Daniel
Subject: Re: [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@xxxxxxxxx wrote:
> > >>From: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> > >>
> > >>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¢ral 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
