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
