Re: [PATCH 0/4] Color manager framework

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, Sep 09, 2014 at 11:53:12AM +0530, shashank.sharma@intel.com wrote:
> From: Shashank Sharma <shashank.sharma@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.

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

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.

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.

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