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.sha...@intel.com wrote: > >>From: Shashank Sharma <shashank.sha...@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). 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. Matt > >For attaching properties, I'm not sure I see where that is happening in > >your current patchset. You have an intel_attach_pipe_color_correction() > >function that sounds promising, but the implementation doesn't seem to > >actually be calling drm_object_attach_property() that I can see; instead > >it seems to be creating a blob value and trying to set it on the object. > >Honestly I think all you really need is a single call to: > > > > drm_object_attach_property(intel_crtc->base.base, > > dev->mode_config.csc_property, 0); > > > Yes, I was suppose to do this, and then create a blob. I will change this. > >at the bottom of intel_crtc_init() where you have have your call to > >intel_attach_pipe_color_correction() right now. I'm not sure if this > >code is expected to stay VLV-specific or whether you've only provided a > >single platform for RFC/POC purposes, but if it is expected to stay > >limited to VLV you'll probably also want to do an > >'if (IS_VALLEYVIEW(dev_priv))' before doing the attach so that the property > >won't even show up on platforms where you haven't implemented support > >yet. > This will be platform independent code. For any platform, we will > attach all the CRTC properties here. But as you rightly mentioned, I > will add the check once we are ok with this architecture. > > > >Also note that aside from the design/coding stuff there are a couple > >more bookkeeping things that will need to be done before this patch set > >gets accepted upstream. I think you'll need to update the DocBook > >documentation in Documentation/DocBook/drm.tmpl with a description of > >your new properties (that compiles into documentation like you see at > >https://www.kernel.org/doc/htmldocs/drm/drm-kms-properties.html) and > >you'll need to add some tests to intel-gpu-tools to exercise this new > >functionality. > > > Yes, Once we are OK with the architecture, I will add documentation > for all the color properties together. Right now we have one command > line test app to test this, which I will convert into an IGT. > >I'll provide some more specific feedback on your individual patches. > > > > > >Matt > > > >> > >>Shashank Sharma (4): > >> drm/i915: Color manager framework for valleyview > >> drm/i915: Plug-in color manager attach > >> drm/i915: CSC color correction > >> drm/i915: Add set_protpery function > >> > >> drivers/gpu/drm/drm_crtc.c | 4 +- > >> drivers/gpu/drm/i915/Makefile | 3 +- > >> drivers/gpu/drm/i915/i915_reg.h | 11 ++ > >> drivers/gpu/drm/i915/intel_clrmgr.c | 283 > >> +++++++++++++++++++++++++++++++++++ > >> drivers/gpu/drm/i915/intel_clrmgr.h | 108 +++++++++++++ > >> drivers/gpu/drm/i915/intel_display.c | 25 ++++ > >> drivers/gpu/drm/i915/intel_drv.h | 1 + > >> drivers/gpu/drm/i915/intel_sprite.c | 3 + > >> include/drm/drm_crtc.h | 7 + > >> 9 files changed, 442 insertions(+), 3 deletions(-) > >> create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.c > >> create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.h > >> > >>-- > >>1.9.1 > >> > > -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
