Re: [PATCH 1/7] drm/i915: Initialize Color Manager

Re: [PATCH 1/7] drm/i915: Initialize Color Manager

[Jindal, Sonika]

On 6/2/2015 1:22 AM, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
>
> Color Manager is an extension in i915 driver to handle color correction
> and enhancements across various Intel platforms.
>
> This patch initializes color manager framework by :
> 1. Adding two new files, intel_color_manager(.c/.h)
> 2. Introducing new pointers in DRM mode_config structure to
>     carry CSC and Gamma color correction properties.
> 3. Creating these DRM properties in Color Manager initialization
>     sequence.
>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>   drivers/gpu/drm/i915/Makefile              |    3 ++
>   drivers/gpu/drm/i915/intel_color_manager.c |   49 ++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_color_manager.h |   32 ++++++++++++++++++
>   drivers/gpu/drm/i915/intel_display.c       |    5 +++
>   include/drm/drm_crtc.h                     |    4 +++
>   5 files changed, 93 insertions(+)
>   create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
>   create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index b7ddf48..c62d048 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -89,6 +89,9 @@ i915-y += i915_vgpu.o
>   # legacy horrors
>   i915-y += i915_dma.o
>
> +# Color Management
> +i915-y += intel_color_manager.o
> +
>   obj-$(CONFIG_DRM_I915)  += i915.o
>
>   CFLAGS_i915_trace_points.o := -I$(src)
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> new file mode 100644
> index 0000000..c83a212
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright © 2015 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@intel.com>
> + * Kausal Malladi <Kausal.Malladi@intel.com>
> + */
> +
> +#include "intel_color_manager.h"
> +
> +int intel_color_manager_init(struct drm_device *dev)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	/* Create Gamma and CSC properties */
> +	config->gamma_property = drm_property_create(dev,
> +		DRM_MODE_PROP_BLOB, "gamma_property", 0);
> +	if (!config->gamma_property)
> +		DRM_ERROR("Gamma property creation failed\n");
> +
> +	DRM_DEBUG_DRIVER("Created Gamma property\n");
> +
> +	config->csc_property = drm_property_create(dev,
> +			DRM_MODE_PROP_BLOB, "csc_property", 0);
> +	if (!config->csc_property)
> +		DRM_ERROR("CSC property creation failed\n");
> +
> +	DRM_DEBUG_DRIVER("Created CSC property\n");
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
> new file mode 100644
> index 0000000..3cff09d
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright © 2015 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@intel.com>
> + * Kausal Malladi <Kausal.Malladi@intel.com>
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc_helper.h>
> +
> +/* Generic Function prototypes */
> +int intel_color_manager_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 067b1de..93eed31 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -44,6 +44,7 @@
>   #include <drm/drm_plane_helper.h>
>   #include <drm/drm_rect.h>
>   #include <linux/dma_remapping.h>
> +#include "intel_color_manager.h"
>
>   /* Primary plane formats for gen <= 3 */
>   static const uint32_t i8xx_primary_formats[] = {
> @@ -14670,6 +14671,10 @@ void intel_modeset_init(struct drm_device *dev)
>
>   	intel_init_pm(dev);
>
> +	ret = intel_color_manager_init(dev);
This call never fails, you might want to add returning of errors

Also, it is better to move this initialization after the num_pipes 
check, so that we are not initializing the color manager when modeset fails.

> +	if (ret)
> +		DRM_ERROR("Color Manager initialization failed\n");
> +
>   	if (INTEL_INFO(dev)->num_pipes == 0)
>   		return;
>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3b4d8a4..4085339 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1187,6 +1187,10 @@ struct drm_mode_config {
>
>   	/* cursor size */
>   	uint32_t cursor_width, cursor_height;
> +
> +	/* Color Management Properties */
> +	struct drm_property *gamma_property;
> +	struct drm_property *csc_property;
This can be moved near the property section in the same struct.
>   };
>
>   /**
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/7] drm/i915: Add Set property interface for CRTC

[Jindal, Sonika]

On 6/2/2015 1:22 AM, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
>
> This patch adds set property interface for Intel CRTC. This interface
> will be used to set color correction DRM properties.
>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c |    8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f817cea..21e67da 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12978,11 +12978,19 @@ out:
>   	return ret;
>   }
>
> +static int intel_crtc_set_property(struct drm_crtc *crtc,
> +		struct drm_property *property, uint64_t val)
> +{
> +	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> +	return -EINVAL;
> +}
> +
>   static const struct drm_crtc_funcs intel_crtc_funcs = {
>   	.gamma_set = intel_crtc_gamma_set,
>   	.set_config = intel_crtc_set_config,
>   	.destroy = intel_crtc_destroy,
>   	.page_flip = intel_crtc_page_flip,
> +	.set_property = intel_crtc_set_property,
I think it should be done similar to plane set property using atomic 
helpers.

>   	.atomic_duplicate_state = intel_crtc_duplicate_state,
>   	.atomic_destroy_state = intel_crtc_destroy_state,
>   };
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm: Add Gamma correction structure

[Jindal, Sonika]

On 6/2/2015 1:22 AM, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
>
> This patch adds a new structure in DRM layer for Gamma color correction.
> This structure will be used by all user space agents to configure
> appropriate Gamma precision and Gamma level.
>
> struct drm_intel_gamma {
>         __u32 flags;
> 	(The flag variable will indicate if the property to be set/get
> 	is Gamma or DeGamma)
>         __u32 gamma_level;
> 	(The gamma_level variable indicates if the Gamma correction is to be
> 	applied on Pipe/plane)
>         __u32 gamma_precision;
> 	(The Gamma precision indicates the Gamma mode to be applied)
>
> 	Supported precisions are -
> 	#define I915_GAMMA_PRECISION_UNKNOWN	0
> 	#define I915_GAMMA_PRECISION_CURRENT	0xFFFFFFFF
> 	#define I915_GAMMA_PRECISION_LEGACY	(1 << 0)
> 	#define I915_GAMMA_PRECISION_10BIT	(1 << 1)
> 	#define I915_GAMMA_PRECISION_12BIT	(1 << 2)
> 	#define I915_GAMMA_PRECISION_14BIT	(1 << 3)
> 	#define I915_GAMMA_PRECISION_16BIT	(1 << 4)
>
> 	__u32 num_samples;
> 	(The num_samples indicates the number of Gamma correction
> 	coefficients)
>         __u32 reserved;
>         __u64 gamma_ptr;
> 	(Points to the raw Gamma color correction values)
> };
>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>   include/uapi/drm/drm.h |   11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 3801584..fe27e5c 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -829,6 +829,17 @@ struct drm_event_vblank {
>   	__u32 reserved;
>   };
>
> +/* Color Management structure for Gamma */
> +struct drm_intel_gamma {
I suppose, this can be used by other drivers as well? If yes, "intel" 
can be removed.
> +	__u32 obj_id;
> +	__u32 flags;
> +	__u32 gamma_level;
> +	__u32 gamma_precision;
> +	__u32 num_samples;
> +	__u32 reserved;
> +	__u64 gamma_ptr;
> +};
> +
>   /* typedef area */
>   #ifndef __KERNEL__
>   typedef struct drm_clip_rect drm_clip_rect_t;
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 4/7] drm: Add Gamma correction structure

[Daniel Stone]

Hi,

On 2 June 2015 at 12:25, Jindal, Sonika <sonika.jindal@intel.com> wrote:
> On 6/2/2015 1:22 AM, Kausal Malladi wrote:
>> struct drm_intel_gamma {
>>         __u32 flags;
>>         (The flag variable will indicate if the property to be set/get
>>         is Gamma or DeGamma)
>>         __u32 gamma_level;
>>         (The gamma_level variable indicates if the Gamma correction is to
>> be
>>         applied on Pipe/plane)
>>         __u32 gamma_precision;
>>         (The Gamma precision indicates the Gamma mode to be applied)
>>
>>         Supported precisions are -
>>         #define I915_GAMMA_PRECISION_UNKNOWN    0
>>         #define I915_GAMMA_PRECISION_CURRENT    0xFFFFFFFF
>>         #define I915_GAMMA_PRECISION_LEGACY     (1 << 0)
>>         #define I915_GAMMA_PRECISION_10BIT      (1 << 1)
>>         #define I915_GAMMA_PRECISION_12BIT      (1 << 2)
>>         #define I915_GAMMA_PRECISION_14BIT      (1 << 3)
>>         #define I915_GAMMA_PRECISION_16BIT      (1 << 4)
>>
>>         __u32 num_samples;
>>         (The num_samples indicates the number of Gamma correction
>>         coefficients)
>>         __u32 reserved;
>>         __u64 gamma_ptr;
>>         (Points to the raw Gamma color correction values)
>> };

Please use the atomic interface and blob properties for this instead.
A user-created blob property can replace the contents of gamma_ptr,
you can replace gamma_level by choosing whether you apply the
properties to a CRTC (pipe) or plane, and then you just need separate
properties for precision and num_samples.

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/7] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Jindal, Sonika]

On 6/2/2015 1:22 AM, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
>
> This patch does the following:
> 1. Adds the core function to program Gamma correction values for CHV/BSW
>     platform
> 2. Adds Gamma correction macros/defines
> 3. Adds drm_mode_crtc_update_color_property function, which replaces the
>     old blob for the property with the new one
> 4. Adds a pointer to hold blob for Gamma property in drm_crtc
>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>   drivers/gpu/drm/drm_crtc.c                 |   14 ++
>   drivers/gpu/drm/i915/intel_color_manager.c |  194 ++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_color_manager.h |   61 +++++++++
>   drivers/gpu/drm/i915/intel_display.c       |    9 +-
>   include/drm/drm_crtc.h                     |    8 ++
>   5 files changed, 285 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 77f87b2..50b925b 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4691,6 +4691,20 @@ int drm_mode_connector_set_tile_property(struct drm_connector *connector)
>   }
>   EXPORT_SYMBOL(drm_mode_connector_set_tile_property);
>
> +int drm_mode_crtc_update_color_property(struct drm_device *dev,
> +		struct drm_property_blob **blob,
> +		size_t length, const void *color_data,
> +		struct drm_mode_object *obj_holds_id,
> +		struct drm_property *prop_holds_id)
This can be simplified.. No need to pass so many params.
> +{
> +	int ret;
> +
> +	ret = drm_property_replace_global_blob(dev,
> +			blob, length, color_data, obj_holds_id, prop_holds_id);
> +
> +	return ret;
> +}
> +
Split the patch to add drm specific changes in a separate patch. Also 
you need to export this function.

Will review the rest of the file in some time.

Regards,
Sonika
>   /**
>    * drm_mode_connector_update_edid_property - update the edid property of a connector
>    * @connector: drm connector
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> index b0eb679..f46857f 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,6 +27,200 @@
>
>   #include "intel_color_manager.h"
>
> +int chv_set_gamma(struct drm_device *dev, uint64_t blob_id,
> +		struct drm_crtc *crtc)
> +{
> +	struct drm_intel_gamma *gamma_data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_property_blob *blob;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	u32 cgm_control_reg = 0;
> +	u32 cgm_gamma_reg = 0;
> +	u32 reg, val, pipe;
> +	u16 red, green, blue;
> +	struct rgb_pixel correct_rgb;
> +	u32 count = 0;
> +	struct rgb_pixel *correction_values = NULL;
> +	u32 num_samples;
> +	u32 word;
> +	u32 palette;
> +	int ret = 0, length;
> +
> +	blob = drm_property_lookup_blob(dev, blob_id);
> +	if (!blob) {
> +		DRM_ERROR("Invalid Blob ID\n");
> +		return -EINVAL;
> +	}
> +
> +	gamma_data = kzalloc(sizeof(struct drm_intel_gamma), GFP_KERNEL);
> +	if (!gamma_data) {
> +		DRM_ERROR("Memory unavailable\n");
> +		return -ENOMEM;
> +	}
> +	gamma_data = (struct drm_intel_gamma *)blob->data;
> +	pipe = to_intel_crtc(crtc)->pipe;
> +	num_samples = gamma_data->num_samples;
> +	length = num_samples * sizeof(struct rgb_pixel);
> +
> +	if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_UNKNOWN) {
> +
> +		/* Disable Gamma functionality on Pipe - CGM Block */
> +		cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
> +		cgm_control_reg &= ~CGM_GAMMA_EN;
> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
> +
> +		DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
> +				pipe_name(pipe));
> +		ret = 0;
> +		goto release_memory;
> +	}
> +
> +	if (pipe >= CHV_MAX_PIPES) {
> +		DRM_ERROR("Incorrect Pipe ID\n");
> +		ret = -EFAULT;
> +		goto release_memory;
> +	}
> +
> +	correction_values = kzalloc(length, GFP_KERNEL);
> +	if (!correction_values) {
> +		DRM_ERROR("Out of Memory\n");
> +		ret = -ENOMEM;
> +		goto release_memory;
> +	}
> +
> +	ret = copy_from_user((void *)correction_values,
> +		(const void __user *)gamma_data->gamma_ptr, length);
> +	if (ret) {
> +		DRM_ERROR("Error copying user data\n");
> +		ret = -EFAULT;
> +		goto release_memory;
> +	}
> +
> +	ret = drm_mode_crtc_update_color_property(dev,
> +		&crtc->gamma_blob, length, (void *) correction_values,
> +		&crtc->base, config->gamma_property);
> +	if (ret) {
> +		DRM_ERROR("Error updating Gamma blob\n");
> +		ret = -EFAULT;
> +		goto release_memory;
> +	}
> +
> +	if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_LEGACY) {
> +
> +		if (num_samples != CHV_8BIT_GAMMA_MAX_VALS) {
> +			DRM_ERROR("Incorrect number of samples received\n");
> +			goto release_memory;
> +		}
> +
> +		/* First, disable CGM Gamma, if already set */
> +		cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
> +		cgm_control_reg &= ~CGM_GAMMA_EN;
> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
> +
> +		/* Enable (Legacy) Gamma on Pipe gamma_data.__obj_id */
> +		palette = _PIPE_GAMMA_BASE(pipe);
> +
> +		count = 0;
> +		while (count < num_samples) {
> +			correct_rgb = correction_values[count];
> +			blue = correct_rgb.blue;
> +			green = correct_rgb.green;
> +			red = correct_rgb.red;
> +
> +			blue = blue >> CHV_8BIT_GAMMA_MSB_SHIFT;
> +			green = green >> CHV_8BIT_GAMMA_MSB_SHIFT;
> +			red = red >> CHV_8BIT_GAMMA_MSB_SHIFT;
> +
> +			/* Red (23:16), Green (15:8), Blue (7:0) */
> +			word = (red << CHV_8BIT_GAMMA_SHIFT_RED_REG) |
> +				(green <<
> +				 CHV_8BIT_GAMMA_SHIFT_GREEN_REG) |
> +				blue;
> +			I915_WRITE(palette, word);
> +
> +			palette += 4;
> +			count++;
> +		}
> +		reg = PIPECONF(pipe);
> +		val = I915_READ(reg) | PIPECONF_GAMMA;
> +		I915_WRITE(reg, val);
> +
> +		DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n",
> +			pipe_name(pipe));
> +		ret = 0;
> +		goto release_memory;
> +	} else if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_10BIT) {
> +
> +		if (num_samples != CHV_10BIT_GAMMA_MAX_VALS) {
> +			DRM_ERROR("Incorrect number of samples received\n");
> +			ret = -EINVAL;
> +			goto release_memory;
> +		}
> +
> +		/* Enable (CGM) Gamma on Pipe gamma_data.__obj_id */
> +		cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
> +
> +		count = 0;
> +		while (count < num_samples) {
> +			correct_rgb = correction_values[count];
> +			blue = correct_rgb.blue;
> +			green = correct_rgb.green;
> +			red = correct_rgb.red;
> +
> +			blue = blue >> CHV_10BIT_GAMMA_MSB_SHIFT;
> +			green = green >> CHV_10BIT_GAMMA_MSB_SHIFT;
> +			red = red >> CHV_10BIT_GAMMA_MSB_SHIFT;
> +
> +			/* Green (25:16) and Blue (9:0) to be written */
> +			word = (green << CHV_GAMMA_SHIFT_GREEN) | blue;
> +			I915_WRITE(cgm_gamma_reg, word);
> +			cgm_gamma_reg += 4;
> +
> +			/* Red (9:0) to be written */
> +			word = red;
> +			I915_WRITE(cgm_gamma_reg, word);
> +
> +			cgm_gamma_reg += 4;
> +			count++;
> +		}
> +
> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe),
> +			I915_READ(_PIPE_CGM_CONTROL(pipe))
> +			| CGM_GAMMA_EN);
> +
> +		DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n",
> +			pipe_name(pipe));
> +		ret = 0;
> +		goto release_memory;
> +	} else {
> +		DRM_ERROR("Invalid gamma_level received\n");
> +		ret = -EFAULT;
> +		goto release_memory;
> +	}
> +
> +release_memory:
> +
> +	/* kfree is NULL protected */
> +	kfree(correction_values);
> +	kfree(gamma_data);
> +
> +	return ret;
> +}
> +
> +int intel_color_manager_set_gamma(struct drm_device *dev,
> +		struct drm_mode_object *obj, uint64_t blob_id)
> +{
> +	struct drm_crtc *crtc = obj_to_crtc(obj);
> +
> +	if (IS_CHERRYVIEW(dev))
> +		return chv_set_gamma(dev, blob_id, crtc);
> +	else
> +		DRM_ERROR("This platform is not yet supported\n");
> +
> +	return -EINVAL;
> +}
> +
>   void intel_color_manager_attach(struct drm_device *dev,
>   		struct drm_mode_object *mode_obj)
>   {
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
> index e5876fa..2c8b7da 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.h
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -27,8 +27,69 @@
>
>   #include <drm/drmP.h>
>   #include <drm/drm_crtc_helper.h>
> +#include "i915_drv.h"
> +
> +/* Color Management macros for Gamma */
> +#define I915_GAMMA_FLAG_DEGAMMA			(1 << 0)
> +#define I915_PIPE_GAMMA				(1 << 0)
> +#define I915_PLANE_GAMMA			(1 << 1)
> +#define I915_GAMMA_PRECISION_UNKNOWN		0
> +#define I915_GAMMA_PRECISION_CURRENT		0xFFFFFFFF
> +#define I915_GAMMA_PRECISION_LEGACY		(1 << 0)
> +#define I915_GAMMA_PRECISION_10BIT		(1 << 1)
> +#define I915_GAMMA_PRECISION_12BIT		(1 << 2)
> +#define I915_GAMMA_PRECISION_14BIT		(1 << 3)
> +#define I915_GAMMA_PRECISION_16BIT		(1 << 4)
> +
> +#define CHV_MAX_PIPES				3
> +#define CHV_DISPLAY_BASE			0x180000
> +
> +struct rgb_pixel {
> +	u16 red;
> +	u16 green;
> +	u16 blue;
> +};
> +
> +/* CHV CGM Block */
> +/* Bit 2 to be enabled in CGM block for CHV */
> +#define CGM_GAMMA_EN				4
> +
> +/* Gamma */
> +#define CHV_GAMMA_VALS				257
> +#define CHV_10BIT_GAMMA_MAX_INDEX		256
> +#define CHV_8BIT_GAMMA_MAX_INDEX		255
> +#define CHV_10BIT_GAMMA_MSB_SHIFT		6
> +#define CHV_GAMMA_EVEN_MASK			0xFF
> +#define CHV_GAMMA_SHIFT_BLUE			0
> +#define CHV_GAMMA_SHIFT_GREEN			16
> +#define CHV_GAMMA_SHIFT_RED			0
> +#define CHV_GAMMA_ODD_SHIFT			8
> +#define CHV_CLRMGR_GAMMA_GCMAX_SHIFT		17
> +#define CHV_GAMMA_GCMAX_MASK			0x1FFFF
> +#define CHV_GAMMA_GCMAX_MAX			0x400
> +#define CHV_10BIT_GAMMA_MAX_VALS		(CHV_10BIT_GAMMA_MAX_INDEX + 1)
> +#define CHV_8BIT_GAMMA_MAX_VALS			(CHV_8BIT_GAMMA_MAX_INDEX + 1)
> +#define CHV_8BIT_GAMMA_MSB_SHIFT		8
> +#define CHV_8BIT_GAMMA_SHIFT_GREEN_REG		8
> +#define CHV_8BIT_GAMMA_SHIFT_RED_REG		16
> +
> +/* CGM Registers */
> +#define CGM_OFFSET				0x2000
> +#define GAMMA_OFFSET				0x2000
> +#define PIPEA_CGM_CONTROL			(CHV_DISPLAY_BASE + 0x67A00)
> +#define PIPEA_CGM_GAMMA_MIN			(CHV_DISPLAY_BASE + 0x67000)
> +#define _PIPE_CGM_CONTROL(pipe) \
> +	(PIPEA_CGM_CONTROL + (pipe * CGM_OFFSET))
> +#define _PIPE_GAMMA_BASE(pipe) \
> +	(PIPEA_CGM_GAMMA_MIN + (pipe * GAMMA_OFFSET))
>
>   /* Generic Function prototypes */
>   int intel_color_manager_init(struct drm_device *dev);
>   void intel_color_manager_attach(struct drm_device *dev,
>   		struct drm_mode_object *mode_obj);
> +extern int intel_color_manager_set_gamma(struct drm_device *dev,
> +		struct drm_mode_object *obj, uint64_t blob_id);
> +
> +/* Platform specific function prototypes */
> +extern int chv_set_gamma(struct drm_device *dev,
> +		uint64_t blob_id, struct drm_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 21e67da..876fde0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12981,7 +12981,14 @@ out:
>   static int intel_crtc_set_property(struct drm_crtc *crtc,
>   		struct drm_property *property, uint64_t val)
>   {
> -	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->gamma_property)
> +		return intel_color_manager_set_gamma(dev, &crtc->base, val);
> +	else
> +		DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> +
>   	return -EINVAL;
>   }
>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 4085339..654c098 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -483,6 +483,9 @@ struct drm_crtc {
>   	 * acquire context.
>   	 */
>   	struct drm_modeset_acquire_ctx *acquire_ctx;
> +
> +	/* Color Management Blobs */
> +	struct drm_property_blob *gamma_blob;
>   };
>
>   /**
> @@ -1341,6 +1344,11 @@ extern void drm_mode_config_cleanup(struct drm_device *dev);
>   extern int drm_mode_connector_set_path_property(struct drm_connector *connector,
>   						const char *path);
>   int drm_mode_connector_set_tile_property(struct drm_connector *connector);
> +extern int drm_mode_crtc_update_color_property(struct drm_device *dev,
> +		struct drm_property_blob **blob,
> +		size_t length, const void *color_data,
> +		struct drm_mode_object *obj_holds_id,
> +		struct drm_property *prop_holds_id);
>   extern int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>   						   const struct edid *edid);
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Daniel Stone]

Hi,

On 2 June 2015 at 12:38, Jindal, Sonika <sonika.jindal@intel.com> wrote:
> On 6/2/2015 1:22 AM, Kausal Malladi wrote:
>> +int drm_mode_crtc_update_color_property(struct drm_device *dev,
>> +               struct drm_property_blob **blob,
>> +               size_t length, const void *color_data,
>> +               struct drm_mode_object *obj_holds_id,
>> +               struct drm_property *prop_holds_id)
>
> This can be simplified.. No need to pass so many params.
>>
>> +{
>> +       int ret;
>> +
>> +       ret = drm_property_replace_global_blob(dev,
>> +                       blob, length, color_data, obj_holds_id,
>> prop_holds_id);
>> +
>> +       return ret;
>> +}
>> +
>
> Split the patch to add drm specific changes in a separate patch. Also you
> need to export this function.

Or just remove the function entirely. It literally adds no value, and
is just an alias for drm_property_replace_global_blob. So just use
that directly.

>> +int chv_set_gamma(struct drm_device *dev, uint64_t blob_id,
>> +               struct drm_crtc *crtc)
>> +{
>> +       struct drm_intel_gamma *gamma_data;
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +       struct drm_property_blob *blob;
>> +       struct drm_mode_config *config = &dev->mode_config;
>> +
>> +       u32 cgm_control_reg = 0;
>> +       u32 cgm_gamma_reg = 0;
>> +       u32 reg, val, pipe;

pipe should be an enum pipe.

>> +       u16 red, green, blue;

Isn't this the literal definition of struct rgb_pixel, which you added
separately in this series?

>> +       if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_UNKNOWN) {
>> +
>> +               /* Disable Gamma functionality on Pipe - CGM Block */
>> +               cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
>> +               cgm_control_reg &= ~CGM_GAMMA_EN;
>> +               I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
>> +
>> +               DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
>> +                               pipe_name(pipe));
>> +               ret = 0;
>> +               goto release_memory;
>> +       }

This branch never updates the property.

>> +       if (pipe >= CHV_MAX_PIPES) {
>> +               DRM_ERROR("Incorrect Pipe ID\n");
>> +               ret = -EFAULT;
>> +               goto release_memory;
>> +       }

How could this ever happen? This should be a WARN_ON at least.

>> +       correction_values = kzalloc(length, GFP_KERNEL);
>> +       if (!correction_values) {
>> +               DRM_ERROR("Out of Memory\n");
>> +               ret = -ENOMEM;
>> +               goto release_memory;
>> +       }
>> +
>> +       ret = copy_from_user((void *)correction_values,
>> +               (const void __user *)gamma_data->gamma_ptr, length);
>> +       if (ret) {
>> +               DRM_ERROR("Error copying user data\n");
>> +               ret = -EFAULT;
>> +               goto release_memory;
>> +       }

I think I've managed to work out the userspace API now:
 - allocate drm_intel_gamma structure
 - allocate correction values
 - insert pointer to correction values into gamma structure
 - create blob with pointer to gamma structure

This seems pretty backwards. The correction values - the large data we
need to avoid copying around - is what should be a blob property. With
your approach, the correction data (big) will be copied quite a few
times, where the supporting structure (very small) will never be
copied.

At the very least, the correction data must be a blob property. I
don't think there is much use in having drm_intel_gamma itself be a
blob property, but I can see why you might want it to be.

>> +       ret = drm_mode_crtc_update_color_property(dev,
>> +               &crtc->gamma_blob, length, (void *) correction_values,
>> +               &crtc->base, config->gamma_property);

As discussed, this function is a pure alias, and can be removed.

>> +       if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_LEGACY) {
>> +
>> +               if (num_samples != CHV_8BIT_GAMMA_MAX_VALS) {
>> +                       DRM_ERROR("Incorrect number of samples
>> received\n");
>> +                       goto release_memory;
>> +               }

This should be checked before the property is updated.

>> +               /* First, disable CGM Gamma, if already set */
>> +               cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
>> +               cgm_control_reg &= ~CGM_GAMMA_EN;
>> +               I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
>> +
>> +               /* Enable (Legacy) Gamma on Pipe gamma_data.__obj_id */
>> +               palette = _PIPE_GAMMA_BASE(pipe);

The comment is misleading. pipe is calculated from crtc->pipe, not
gamma_data.__obj_id.

Also, should all these operations be performed under vblank evasion?

>> +       } else if (gamma_data->gamma_precision ==
>> I915_GAMMA_PRECISION_10BIT) {
>> +
>> +               if (num_samples != CHV_10BIT_GAMMA_MAX_VALS) {
>> +                       DRM_ERROR("Incorrect number of samples
>> received\n");
>> +                       ret = -EINVAL;
>> +                       goto release_memory;
>> +               }

Same comment.

>> +               /* Enable (CGM) Gamma on Pipe gamma_data.__obj_id */
>> +               cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);

Same comment.

>> +release_memory:
>> +
>> +       /* kfree is NULL protected */

Probably no need to comment this.

>> +       kfree(correction_values);
>> +       kfree(gamma_data);

gamma_data is the blob data; you cannot free this. If you ever try to
set the same gamma twice, the kernel will OOPS.

>> +       return ret;
>> +}
>> +
>> +int intel_color_manager_set_gamma(struct drm_device *dev,
>> +               struct drm_mode_object *obj, uint64_t blob_id)
>> +{
>> +       struct drm_crtc *crtc = obj_to_crtc(obj);
>> +
>> +       if (IS_CHERRYVIEW(dev))
>> +               return chv_set_gamma(dev, blob_id, crtc);
>> +       else
>> +               DRM_ERROR("This platform is not yet supported\n");
>> +
>> +       return -EINVAL;
>> +}

If a platform is not supported, then it should not export the gamma
property to userspace.

>> +struct rgb_pixel {
>> +       u16 red;
>> +       u16 green;
>> +       u16 blue;
>> +};

The name of rgb_pixel here is quite generic, especially as 16bpc is
still quite unusual.

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/7] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Sharma, Shashank]

Hi Daniel,

Thanks for the review.
Please find my comments inline.

Regards
Shashank

On 6/2/2015 5:23 PM, Daniel Stone wrote:
> Hi,
>
> On 2 June 2015 at 12:38, Jindal, Sonika <sonika.jindal@intel.com> wrote:
>> On 6/2/2015 1:22 AM, Kausal Malladi wrote:
>>> +int drm_mode_crtc_update_color_property(struct drm_device *dev,
>>> +               struct drm_property_blob **blob,
>>> +               size_t length, const void *color_data,
>>> +               struct drm_mode_object *obj_holds_id,
>>> +               struct drm_property *prop_holds_id)
>>
>> This can be simplified.. No need to pass so many params.
>>>
>>> +{
>>> +       int ret;
>>> +
>>> +       ret = drm_property_replace_global_blob(dev,
>>> +                       blob, length, color_data, obj_holds_id,
>>> prop_holds_id);
>>> +
>>> +       return ret;
>>> +}
>>> +
>>
>> Split the patch to add drm specific changes in a separate patch. Also you
>> need to export this function.
>
> Or just remove the function entirely. It literally adds no value, and
> is just an alias for drm_property_replace_global_blob. So just use
> that directly.
>
This function(drm_property_replace_global_blob) is a static function
and I can see few other properties have created wrapper function in this
file built around it (like path_property, edid_prop etc), so seems like 
that's the right way to do it. Do you still think we should remove the 
wrapper ?
>>> +int chv_set_gamma(struct drm_device *dev, uint64_t blob_id,
>>> +               struct drm_crtc *crtc)
>>> +{
>>> +       struct drm_intel_gamma *gamma_data;
>>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>>> +       struct drm_property_blob *blob;
>>> +       struct drm_mode_config *config = &dev->mode_config;
>>> +
>>> +       u32 cgm_control_reg = 0;
>>> +       u32 cgm_gamma_reg = 0;
>>> +       u32 reg, val, pipe;
>
> pipe should be an enum pipe.
Got it.
>
>>> +       u16 red, green, blue;
>
> Isn't this the literal definition of struct rgb_pixel, which you added
> separately in this series?
Yes, they are same, but we are using these local variables to 
shift/adjust according to the Palette register format, so thought it 
would be more readable if we make direct u16 variables
>
>>> +       if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_UNKNOWN) {
>>> +
>>> +               /* Disable Gamma functionality on Pipe - CGM Block */
>>> +               cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
>>> +               cgm_control_reg &= ~CGM_GAMMA_EN;
>>> +               I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
>>> +
>>> +               DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
>>> +                               pipe_name(pipe));
>>> +               ret = 0;
>>> +               goto release_memory;
>>> +       }
>
> This branch never updates the property.
Oops, thanks for catching this.
>
>>> +       if (pipe >= CHV_MAX_PIPES) {
>>> +               DRM_ERROR("Incorrect Pipe ID\n");
>>> +               ret = -EFAULT;
>>> +               goto release_memory;
>>> +       }
>
> How could this ever happen? This should be a WARN_ON at least.
In the first design, user space was sending this variable, so a check 
was required. But then we changed the design, and kept the check :).
We will remove this.
>
>>> +       correction_values = kzalloc(length, GFP_KERNEL);
>>> +       if (!correction_values) {
>>> +               DRM_ERROR("Out of Memory\n");
>>> +               ret = -ENOMEM;
>>> +               goto release_memory;
>>> +       }
>>> +
>>> +       ret = copy_from_user((void *)correction_values,
>>> +               (const void __user *)gamma_data->gamma_ptr, length);
>>> +       if (ret) {
>>> +               DRM_ERROR("Error copying user data\n");
>>> +               ret = -EFAULT;
>>> +               goto release_memory;
>>> +       }
>
> I think I've managed to work out the userspace API now:
>   - allocate drm_intel_gamma structure
>   - allocate correction values
>   - insert pointer to correction values into gamma structure
>   - create blob with pointer to gamma structure
>
> This seems pretty backwards. The correction values - the large data we
> need to avoid copying around - is what should be a blob property. With
> your approach, the correction data (big) will be copied quite a few
> times, where the supporting structure (very small) will never be
> copied.
>
> At the very least, the correction data must be a blob property. I
> don't think there is much use in having drm_intel_gamma itself be a
> blob property, but I can see why you might want it to be.
>
Yes, right, that will be the better way. We were slightly unclear about 
the best way to use the whole set_blob stuff, so this dint go well. Now 
we are planning to do it like this:
1. set_blob_ioctl() to set the gamma_correction values, get the blob_id
2. Add a new variable blob_id in drm_intel_gamma struct
3. Pack the gamma_struct, and send that as set_porperty value.

Does it sound better to you now ?
>>> +       ret = drm_mode_crtc_update_color_property(dev,
>>> +               &crtc->gamma_blob, length, (void *) correction_values,
>>> +               &crtc->base, config->gamma_property);
>
> As discussed, this function is a pure alias, and can be removed.
Same explanation as previous.
>
>>> +       if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_LEGACY) {
>>> +
>>> +               if (num_samples != CHV_8BIT_GAMMA_MAX_VALS) {
>>> +                       DRM_ERROR("Incorrect number of samples
>>> received\n");
>>> +                       goto release_memory;
>>> +               }
>
> This should be checked before the property is updated.
Yep, Got it. update property should happen in the end, after the last 
check.
>
>>> +               /* First, disable CGM Gamma, if already set */
>>> +               cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
>>> +               cgm_control_reg &= ~CGM_GAMMA_EN;
>>> +               I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
>>> +
>>> +               /* Enable (Legacy) Gamma on Pipe gamma_data.__obj_id */
>>> +               palette = _PIPE_GAMMA_BASE(pipe);
>
> The comment is misleading. pipe is calculated from crtc->pipe, not
> gamma_data.__obj_id.
Yep, agree. Will change this.
>
> Also, should all these operations be performed under vblank evasion?
Currently, due to missing set_crtc infrastructure, we are not planning 
to make the atomicity as primary target. Once the infrastructure is 
ready, we will move on to that, and the fwk will take care of that.
>
>>> +       } else if (gamma_data->gamma_precision ==
>>> I915_GAMMA_PRECISION_10BIT) {
>>> +
>>> +               if (num_samples != CHV_10BIT_GAMMA_MAX_VALS) {
>>> +                       DRM_ERROR("Incorrect number of samples
>>> received\n");
>>> +                       ret = -EINVAL;
>>> +                       goto release_memory;
>>> +               }
>
> Same comment.
Got it.
>
>>> +               /* Enable (CGM) Gamma on Pipe gamma_data.__obj_id */
>>> +               cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
>
> Same comment.
Got it.
>
>>> +release_memory:
>>> +
>>> +       /* kfree is NULL protected */
>
> Probably no need to comment this.
I remember receiving comments for this long back, but yeah. :)
>
>>> +       kfree(correction_values);
>>> +       kfree(gamma_data);
>
> gamma_data is the blob data; you cannot free this. If you ever try to
> set the same gamma twice, the kernel will OOPS.
Oops, yes, this was bad. There was a mistake while allocating 
gamma_data, it doesnt need that.
>
>>> +       return ret;
>>> +}
>>> +
>>> +int intel_color_manager_set_gamma(struct drm_device *dev,
>>> +               struct drm_mode_object *obj, uint64_t blob_id)
>>> +{
>>> +       struct drm_crtc *crtc = obj_to_crtc(obj);
>>> +
>>> +       if (IS_CHERRYVIEW(dev))
>>> +               return chv_set_gamma(dev, blob_id, crtc);
>>> +       else
>>> +               DRM_ERROR("This platform is not yet supported\n");
>>> +
>>> +       return -EINVAL;
>>> +}
>
> If a platform is not supported, then it should not export the gamma
> property to userspace.
The idea is to extend this fwk to other platforms slowly. but we can 
remove the error message.
>
>>> +struct rgb_pixel {
>>> +       u16 red;
>>> +       u16 green;
>>> +       u16 blue;
>>> +};
>
> The name of rgb_pixel here is quite generic, especially as 16bpc is
> still quite unusual.
mostly 10.6 gamma format or 16.16 CSC.
>
> Cheers,
> Daniel
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

RE: [PATCH 5/7] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Sharma, Shashank]

Thanks for the review Sonika,

We will incorporate the review comments and send the updated patch set soon. 

Regards
Shashank

-----Original Message-----
From: Jindal, Sonika 
Sent: Tuesday, June 02, 2015 5:08 PM
To: Malladi, Kausal; Roper, Matthew D; Barnes, Jesse; Lespiau, Damien; R, Durgadoss; Purushothaman, Vijay A; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Cc: Vetter, Daniel; Sharma, Shashank; Kamath, Sunil; Mukherjee, Indranil; annie.j.matherson@intel.com; R, Dhanya p; Palleti, Avinash Reddy
Subject: Re: [PATCH 5/7] drm/i915: Add pipe level Gamma correction for CHV/BSW



On 6/2/2015 1:22 AM, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
>
> This patch does the following:
> 1. Adds the core function to program Gamma correction values for CHV/BSW
>     platform
> 2. Adds Gamma correction macros/defines 3. Adds 
> drm_mode_crtc_update_color_property function, which replaces the
>     old blob for the property with the new one 4. Adds a pointer to 
> hold blob for Gamma property in drm_crtc
>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>   drivers/gpu/drm/drm_crtc.c                 |   14 ++
>   drivers/gpu/drm/i915/intel_color_manager.c |  194 ++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_color_manager.h |   61 +++++++++
>   drivers/gpu/drm/i915/intel_display.c       |    9 +-
>   include/drm/drm_crtc.h                     |    8 ++
>   5 files changed, 285 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c 
> index 77f87b2..50b925b 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4691,6 +4691,20 @@ int drm_mode_connector_set_tile_property(struct drm_connector *connector)
>   }
>   EXPORT_SYMBOL(drm_mode_connector_set_tile_property);
>
> +int drm_mode_crtc_update_color_property(struct drm_device *dev,
> +		struct drm_property_blob **blob,
> +		size_t length, const void *color_data,
> +		struct drm_mode_object *obj_holds_id,
> +		struct drm_property *prop_holds_id)
This can be simplified.. No need to pass so many params.
> +{
> +	int ret;
> +
> +	ret = drm_property_replace_global_blob(dev,
> +			blob, length, color_data, obj_holds_id, prop_holds_id);
> +
> +	return ret;
> +}
> +
Split the patch to add drm specific changes in a separate patch. Also you need to export this function.

Will review the rest of the file in some time.

Regards,
Sonika
>   /**
>    * drm_mode_connector_update_edid_property - update the edid property of a connector
>    * @connector: drm connector
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c 
> b/drivers/gpu/drm/i915/intel_color_manager.c
> index b0eb679..f46857f 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,6 +27,200 @@
>
>   #include "intel_color_manager.h"
>
> +int chv_set_gamma(struct drm_device *dev, uint64_t blob_id,
> +		struct drm_crtc *crtc)
> +{
> +	struct drm_intel_gamma *gamma_data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_property_blob *blob;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	u32 cgm_control_reg = 0;
> +	u32 cgm_gamma_reg = 0;
> +	u32 reg, val, pipe;
> +	u16 red, green, blue;
> +	struct rgb_pixel correct_rgb;
> +	u32 count = 0;
> +	struct rgb_pixel *correction_values = NULL;
> +	u32 num_samples;
> +	u32 word;
> +	u32 palette;
> +	int ret = 0, length;
> +
> +	blob = drm_property_lookup_blob(dev, blob_id);
> +	if (!blob) {
> +		DRM_ERROR("Invalid Blob ID\n");
> +		return -EINVAL;
> +	}
> +
> +	gamma_data = kzalloc(sizeof(struct drm_intel_gamma), GFP_KERNEL);
> +	if (!gamma_data) {
> +		DRM_ERROR("Memory unavailable\n");
> +		return -ENOMEM;
> +	}
> +	gamma_data = (struct drm_intel_gamma *)blob->data;
> +	pipe = to_intel_crtc(crtc)->pipe;
> +	num_samples = gamma_data->num_samples;
> +	length = num_samples * sizeof(struct rgb_pixel);
> +
> +	if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_UNKNOWN) {
> +
> +		/* Disable Gamma functionality on Pipe - CGM Block */
> +		cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
> +		cgm_control_reg &= ~CGM_GAMMA_EN;
> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
> +
> +		DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
> +				pipe_name(pipe));
> +		ret = 0;
> +		goto release_memory;
> +	}
> +
> +	if (pipe >= CHV_MAX_PIPES) {
> +		DRM_ERROR("Incorrect Pipe ID\n");
> +		ret = -EFAULT;
> +		goto release_memory;
> +	}
> +
> +	correction_values = kzalloc(length, GFP_KERNEL);
> +	if (!correction_values) {
> +		DRM_ERROR("Out of Memory\n");
> +		ret = -ENOMEM;
> +		goto release_memory;
> +	}
> +
> +	ret = copy_from_user((void *)correction_values,
> +		(const void __user *)gamma_data->gamma_ptr, length);
> +	if (ret) {
> +		DRM_ERROR("Error copying user data\n");
> +		ret = -EFAULT;
> +		goto release_memory;
> +	}
> +
> +	ret = drm_mode_crtc_update_color_property(dev,
> +		&crtc->gamma_blob, length, (void *) correction_values,
> +		&crtc->base, config->gamma_property);
> +	if (ret) {
> +		DRM_ERROR("Error updating Gamma blob\n");
> +		ret = -EFAULT;
> +		goto release_memory;
> +	}
> +
> +	if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_LEGACY) {
> +
> +		if (num_samples != CHV_8BIT_GAMMA_MAX_VALS) {
> +			DRM_ERROR("Incorrect number of samples received\n");
> +			goto release_memory;
> +		}
> +
> +		/* First, disable CGM Gamma, if already set */
> +		cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
> +		cgm_control_reg &= ~CGM_GAMMA_EN;
> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
> +
> +		/* Enable (Legacy) Gamma on Pipe gamma_data.__obj_id */
> +		palette = _PIPE_GAMMA_BASE(pipe);
> +
> +		count = 0;
> +		while (count < num_samples) {
> +			correct_rgb = correction_values[count];
> +			blue = correct_rgb.blue;
> +			green = correct_rgb.green;
> +			red = correct_rgb.red;
> +
> +			blue = blue >> CHV_8BIT_GAMMA_MSB_SHIFT;
> +			green = green >> CHV_8BIT_GAMMA_MSB_SHIFT;
> +			red = red >> CHV_8BIT_GAMMA_MSB_SHIFT;
> +
> +			/* Red (23:16), Green (15:8), Blue (7:0) */
> +			word = (red << CHV_8BIT_GAMMA_SHIFT_RED_REG) |
> +				(green <<
> +				 CHV_8BIT_GAMMA_SHIFT_GREEN_REG) |
> +				blue;
> +			I915_WRITE(palette, word);
> +
> +			palette += 4;
> +			count++;
> +		}
> +		reg = PIPECONF(pipe);
> +		val = I915_READ(reg) | PIPECONF_GAMMA;
> +		I915_WRITE(reg, val);
> +
> +		DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n",
> +			pipe_name(pipe));
> +		ret = 0;
> +		goto release_memory;
> +	} else if (gamma_data->gamma_precision == 
> +I915_GAMMA_PRECISION_10BIT) {
> +
> +		if (num_samples != CHV_10BIT_GAMMA_MAX_VALS) {
> +			DRM_ERROR("Incorrect number of samples received\n");
> +			ret = -EINVAL;
> +			goto release_memory;
> +		}
> +
> +		/* Enable (CGM) Gamma on Pipe gamma_data.__obj_id */
> +		cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
> +
> +		count = 0;
> +		while (count < num_samples) {
> +			correct_rgb = correction_values[count];
> +			blue = correct_rgb.blue;
> +			green = correct_rgb.green;
> +			red = correct_rgb.red;
> +
> +			blue = blue >> CHV_10BIT_GAMMA_MSB_SHIFT;
> +			green = green >> CHV_10BIT_GAMMA_MSB_SHIFT;
> +			red = red >> CHV_10BIT_GAMMA_MSB_SHIFT;
> +
> +			/* Green (25:16) and Blue (9:0) to be written */
> +			word = (green << CHV_GAMMA_SHIFT_GREEN) | blue;
> +			I915_WRITE(cgm_gamma_reg, word);
> +			cgm_gamma_reg += 4;
> +
> +			/* Red (9:0) to be written */
> +			word = red;
> +			I915_WRITE(cgm_gamma_reg, word);
> +
> +			cgm_gamma_reg += 4;
> +			count++;
> +		}
> +
> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe),
> +			I915_READ(_PIPE_CGM_CONTROL(pipe))
> +			| CGM_GAMMA_EN);
> +
> +		DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n",
> +			pipe_name(pipe));
> +		ret = 0;
> +		goto release_memory;
> +	} else {
> +		DRM_ERROR("Invalid gamma_level received\n");
> +		ret = -EFAULT;
> +		goto release_memory;
> +	}
> +
> +release_memory:
> +
> +	/* kfree is NULL protected */
> +	kfree(correction_values);
> +	kfree(gamma_data);
> +
> +	return ret;
> +}
> +
> +int intel_color_manager_set_gamma(struct drm_device *dev,
> +		struct drm_mode_object *obj, uint64_t blob_id) {
> +	struct drm_crtc *crtc = obj_to_crtc(obj);
> +
> +	if (IS_CHERRYVIEW(dev))
> +		return chv_set_gamma(dev, blob_id, crtc);
> +	else
> +		DRM_ERROR("This platform is not yet supported\n");
> +
> +	return -EINVAL;
> +}
> +
>   void intel_color_manager_attach(struct drm_device *dev,
>   		struct drm_mode_object *mode_obj)
>   {
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h 
> b/drivers/gpu/drm/i915/intel_color_manager.h
> index e5876fa..2c8b7da 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.h
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -27,8 +27,69 @@
>
>   #include <drm/drmP.h>
>   #include <drm/drm_crtc_helper.h>
> +#include "i915_drv.h"
> +
> +/* Color Management macros for Gamma */
> +#define I915_GAMMA_FLAG_DEGAMMA			(1 << 0)
> +#define I915_PIPE_GAMMA				(1 << 0)
> +#define I915_PLANE_GAMMA			(1 << 1)
> +#define I915_GAMMA_PRECISION_UNKNOWN		0
> +#define I915_GAMMA_PRECISION_CURRENT		0xFFFFFFFF
> +#define I915_GAMMA_PRECISION_LEGACY		(1 << 0)
> +#define I915_GAMMA_PRECISION_10BIT		(1 << 1)
> +#define I915_GAMMA_PRECISION_12BIT		(1 << 2)
> +#define I915_GAMMA_PRECISION_14BIT		(1 << 3)
> +#define I915_GAMMA_PRECISION_16BIT		(1 << 4)
> +
> +#define CHV_MAX_PIPES				3
> +#define CHV_DISPLAY_BASE			0x180000
> +
> +struct rgb_pixel {
> +	u16 red;
> +	u16 green;
> +	u16 blue;
> +};
> +
> +/* CHV CGM Block */
> +/* Bit 2 to be enabled in CGM block for CHV */
> +#define CGM_GAMMA_EN				4
> +
> +/* Gamma */
> +#define CHV_GAMMA_VALS				257
> +#define CHV_10BIT_GAMMA_MAX_INDEX		256
> +#define CHV_8BIT_GAMMA_MAX_INDEX		255
> +#define CHV_10BIT_GAMMA_MSB_SHIFT		6
> +#define CHV_GAMMA_EVEN_MASK			0xFF
> +#define CHV_GAMMA_SHIFT_BLUE			0
> +#define CHV_GAMMA_SHIFT_GREEN			16
> +#define CHV_GAMMA_SHIFT_RED			0
> +#define CHV_GAMMA_ODD_SHIFT			8
> +#define CHV_CLRMGR_GAMMA_GCMAX_SHIFT		17
> +#define CHV_GAMMA_GCMAX_MASK			0x1FFFF
> +#define CHV_GAMMA_GCMAX_MAX			0x400
> +#define CHV_10BIT_GAMMA_MAX_VALS		(CHV_10BIT_GAMMA_MAX_INDEX + 1)
> +#define CHV_8BIT_GAMMA_MAX_VALS			(CHV_8BIT_GAMMA_MAX_INDEX + 1)
> +#define CHV_8BIT_GAMMA_MSB_SHIFT		8
> +#define CHV_8BIT_GAMMA_SHIFT_GREEN_REG		8
> +#define CHV_8BIT_GAMMA_SHIFT_RED_REG		16
> +
> +/* CGM Registers */
> +#define CGM_OFFSET				0x2000
> +#define GAMMA_OFFSET				0x2000
> +#define PIPEA_CGM_CONTROL			(CHV_DISPLAY_BASE + 0x67A00)
> +#define PIPEA_CGM_GAMMA_MIN			(CHV_DISPLAY_BASE + 0x67000)
> +#define _PIPE_CGM_CONTROL(pipe) \
> +	(PIPEA_CGM_CONTROL + (pipe * CGM_OFFSET)) #define 
> +_PIPE_GAMMA_BASE(pipe) \
> +	(PIPEA_CGM_GAMMA_MIN + (pipe * GAMMA_OFFSET))
>
>   /* Generic Function prototypes */
>   int intel_color_manager_init(struct drm_device *dev);
>   void intel_color_manager_attach(struct drm_device *dev,
>   		struct drm_mode_object *mode_obj);
> +extern int intel_color_manager_set_gamma(struct drm_device *dev,
> +		struct drm_mode_object *obj, uint64_t blob_id);
> +
> +/* Platform specific function prototypes */ extern int 
> +chv_set_gamma(struct drm_device *dev,
> +		uint64_t blob_id, struct drm_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 21e67da..876fde0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12981,7 +12981,14 @@ out:
>   static int intel_crtc_set_property(struct drm_crtc *crtc,
>   		struct drm_property *property, uint64_t val)
>   {
> -	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->gamma_property)
> +		return intel_color_manager_set_gamma(dev, &crtc->base, val);
> +	else
> +		DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> +
>   	return -EINVAL;
>   }
>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 
> 4085339..654c098 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -483,6 +483,9 @@ struct drm_crtc {
>   	 * acquire context.
>   	 */
>   	struct drm_modeset_acquire_ctx *acquire_ctx;
> +
> +	/* Color Management Blobs */
> +	struct drm_property_blob *gamma_blob;
>   };
>
>   /**
> @@ -1341,6 +1344,11 @@ extern void drm_mode_config_cleanup(struct drm_device *dev);
>   extern int drm_mode_connector_set_path_property(struct drm_connector *connector,
>   						const char *path);
>   int drm_mode_connector_set_tile_property(struct drm_connector 
> *connector);
> +extern int drm_mode_crtc_update_color_property(struct drm_device *dev,
> +		struct drm_property_blob **blob,
> +		size_t length, const void *color_data,
> +		struct drm_mode_object *obj_holds_id,
> +		struct drm_property *prop_holds_id);
>   extern int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>   						   const struct edid *edid);
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/7] Color Manager Implementation

[Damien Lespiau]

On Tue, Jun 02, 2015 at 01:22:42AM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> This patch set adds color manager implementation in drm/i915 layer.

Is anyone working on tests/test plan?

Thanks,

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/7] Color Manager Implementation

[Sharma, Shashank]

Hi Damien, 

Yes we are getting the IGT's ready, and already we have a test tool to apply CSC/Gamma already, which we used for ULT.
As discussed in the parallel forums, we will finally use Chrome UI to test the end-to-end UI level effects 

Regards
Shashank

-----Original Message-----
From: Lespiau, Damien 
Sent: Tuesday, June 02, 2015 5:31 PM
To: Malladi, Kausal
Cc: Roper, Matthew D; Barnes, Jesse; Jindal, Sonika; R, Durgadoss; Purushothaman, Vijay A; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Vetter, Daniel; Sharma, Shashank; Kamath, Sunil; Mukherjee, Indranil; annie.j.matherson@intel.com; R, Dhanya p; Palleti, Avinash Reddy
Subject: Re: [PATCH 0/7] Color Manager Implementation

On Tue, Jun 02, 2015 at 01:22:42AM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> This patch set adds color manager implementation in drm/i915 layer.

Is anyone working on tests/test plan?

Thanks,

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx