[Intel-gfx] [PATCH 1/4] drm/i915: Color manager framework for valleyview

[Bob Paauwe]

On Wed, 10 Sep 2014 14:10:01 +0530
"Sharma, Shashank" <shashank.sharma at intel.com> wrote:

> Hello Bob,
> 
> Thanks for your time and review comments.
> Please find my comments inline.
> 
> Regards
> Shashank
> On 9/10/2014 4:21 AM, Bob Paauwe wrote:
> > On Tue, 9 Sep 2014 11:53:13 +0530
> > <shashank.sharma at intel.com> wrote:
> >
> >> From: Shashank Sharma <shashank.sharma at intel.com>
> >>
> >> Color manager is a framework which adds drm properties for
> >> color correction in I915 driver. This framework creates DRM
> >> properties for each color correction feature, and attaches
> >> it to appropriate CRTC/plane based on the property type.
> >> This allows userspace to fine tune the display as per the panel.
> >>
> >> This is the first patch of the series, what this patch does is:
> >> 1. Create 2 new files
> >> 	intel_clrmgr.c
> >> 	intel_clrmgr.h
> >> 2. Add color manager init function, This function will create
> >>     DRM properties for color correction.
> >> 3. Add color manager exit function. This function will destroy
> >>     registered DRM color properties.
> >> 4. Adds a enum for currently listed color correction properties:
> >>     they are:
> >>     CSC correction (wide gamut), Gamma correction, Contrast,
> >>     Brightness, Hue and Saturation
> >>     This enum will be further used to index color properties
> >>     from array of elements.
> >> 5. Add names for vlv color properties.
> >
> > I'd suggest maybe dropping the name "color manager" and "clrmgr" in
> > favor of "color properties" and maybe "color props" as a shorter form.
> >
> I will try to comply with this suggestion.
> >> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/Makefile        |  3 +-
> >>   drivers/gpu/drm/i915/intel_clrmgr.c  | 80 ++++++++++++++++++++++++++++++++++++
> >>   drivers/gpu/drm/i915/intel_clrmgr.h  | 70 +++++++++++++++++++++++++++++++
> >>   drivers/gpu/drm/i915/intel_display.c |  5 +++
> >>   4 files changed, 157 insertions(+), 1 deletion(-)
> >>   create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.c
> >>   create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.h
> >>
> >> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> >> index c1dd485..6361c9b 100644
> >> --- a/drivers/gpu/drm/i915/Makefile
> >> +++ b/drivers/gpu/drm/i915/Makefile
> >> @@ -46,7 +46,8 @@ i915-y += intel_bios.o \
> >>   	  intel_modes.o \
> >>   	  intel_overlay.o \
> >>   	  intel_sideband.o \
> >> -	  intel_sprite.o
> >> +	  intel_sprite.o \
> >> +	  intel_clrmgr.o
> >>   i915-$(CONFIG_ACPI)		+= intel_acpi.o intel_opregion.o
> >>   i915-$(CONFIG_DRM_I915_FBDEV)	+= intel_fbdev.o
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c b/drivers/gpu/drm/i915/intel_clrmgr.c
> >> new file mode 100644
> >> index 0000000..0def917
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/i915/intel_clrmgr.c
> >> @@ -0,0 +1,80 @@
> >> +/*
> >> + * Copyright © 2014 Intel Corporation
> >> + *
> >> + * Permission is hereby granted, free of charge, to any person obtaining a
> >> + * copy of this software and associated documentation files (the "Software"),
> >> + * to deal in the Software without restriction, including without limitation
> >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> >> + * and/or sell copies of the Software, and to permit persons to whom the
> >> + * Software is furnished to do so, subject to the following conditions:
> >> + *
> >> + * The above copyright notice and this permission notice (including the next
> >> + * paragraph) shall be included in all copies or substantial portions of the
> >> + * Software.
> >> + *
> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> >> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> >> + * IN THE SOFTWARE.
> >> + *
> >> + * Authors:
> >> + * ======
> >> + * Shashank Sharma <shashank.sharma at intel.com>
> >> + * Uma Shankar <uma.shankar at intel.com>
> >> + * Sonika Jindal <sonika.jindal at intel.com>
> >> + */
> >> +
> >> +#include "i915_drm.h"
> >> +#include "i915_drv.h"
> >> +#include "i915_reg.h"
> >> +#include "intel_clrmgr.h"
> >> +
> >> +/* Names to register with color properties */
> >> +const char *clrmgr_property_names[] = {
> >> +	/* csc */
> >> +	"csc-correction",
> >> +	/* gamma */
> >> +	"gamma-correction",
> >> +	/* contrast */
> >> +	"contrast",
> >> +	/* brightness */
> >> +	"brightness",
> >> +	/* hue_saturation */
> >> +	"hue_saturation"
> >> +	/* add a new prop name here */
> >> +};
> >
> > I don't think you need the comments for each of the names.  The names
> > themselves are descriptive.
> >
> > I haven't looked at the rest of the series yet, but in embedded we've
> > considered hue and saturation separate properties.
> >
> Actually they are, but in VLV hardware they are getting 
> programmed/adjusted via the same register (single), so I am force to 
> combine them together.

But the interface to them should be generic. You should be able to
program them separately by masking out only what needs to change.  At
least that's what we've been doing in the embedded driver for VLV.

I'm not real familiar with the this part of the code but here's what we
have in our driver today.


sat_hue_reg is the contents of the saturation / hue register

To change the saturation:

  (sat_hue_reg & 0xffffc00) | (saturation_val & 0x3ff);

to change the hue:

  (sat_hue_reg & 0xf800ffff) | ((hue_val & 0x7ff) << 16) 

Just looking at our code, we may have bug in the hue calculation, we
limit the max hue to 0x3ff, but then do the masking with 0x7ff. One of
those may be wrong and I'm not sure which.  You probably have something
similar to this in your user-space code today if you're passing in the
actual value for the register.

In general, I don't think we want user-space to have to know the format
of the saturation/hue register.