On Thu, Sep 11, 2014 at 10:18 AM, Sharma, Shashank <shashank.sharma@xxxxxxxxx> wrote: > 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. Please coordinate with Rob Clark/Ville on this, since the atomic ioctl also needs a set_blob. I'm not sure whether they have this already even. Adding them. Also there's no need to block brightness/hue/... and such simpler properties on this, so best to submit patches in parallel and prioritize some of the simpler ones to get those in first. > 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 (?) . intel_display.c sounds ok. Splitting that monster is an entire task in itself, and atm no one has a good idea really ;-) > 5. Move some of the existing property creation code in that function, if required. Not required imo, see my reply to Matt's mail. > 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. Cross-driver properties only make sense if they work the same everywhere. Of course every gpu has some CSC control or even a matrix somewhere, but they'll work differently on different platforms. Shared properties (in mode_config) imo only make sense if a generic driver can make sense of them (like xf86-video-modesetting has support for the plane rotation stuff we've added). > Please let me know if this sounds ok, or you suggest modifications. Minor comments, but sounds good as a plan. -Daniel > > 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 -- 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
