[Intel-gfx] [PATCH 0/4] Color manager framework

Thu Sep 11 10:49:01 CEST 2014

On Thu, Sep 11, 2014 at 10:18 AM, Sharma, Shashank
<shashank.sharma at intel.com> 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 at ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Thursday, September 11, 2014 1:27 PM
> To: Roper, Matthew D
> Cc: Sharma, Shashank; intel-gfx at lists.freedesktop.org; Vetter, Daniel
> Subject: Re: [Intel-gfx] [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 at intel.com wrote:
>> > >>From: Shashank Sharma <shashank.sharma at 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch