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

[Sharma, Shashank]

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()
> 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
>>
>