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.
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
