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

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

[Damien Lespiau]

On Wed, Jul 01, 2015 at 09:18:19PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> CHV/BSW platform supports various Gamma correction modes, which are:
> 1. Legacy 8-bit mode
> 2. 10-bit CGM (Color Gamut Mapping) mode
> 
> 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
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h            |  10 ++
>  drivers/gpu/drm/i915/intel_atomic.c        |   6 ++
>  drivers/gpu/drm/i915/intel_color_manager.c | 154 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color_manager.h |  12 +++
>  drivers/gpu/drm/i915/intel_drv.h           |   2 +
>  5 files changed, 184 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 313b1f9..36672e7 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7900,4 +7900,14 @@ enum skl_disp_power_wells {
>  #define _PALETTE_A (dev_priv->info.display_mmio_offset + 0xa000)
>  #define _PALETTE_B (dev_priv->info.display_mmio_offset + 0xa800)
>  
> +/* Color Management */
> +#define PIPEA_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x67A00)
> +#define PIPEA_CGM_GAMMA_MIN			(VLV_DISPLAY_BASE + 0x67000)
> +#define CGM_OFFSET				0x2000
> +#define GAMMA_OFFSET				0x2000
> +#define _PIPE_CGM_CONTROL(pipe) \
> +	(PIPEA_CGM_CONTROL + (pipe * CGM_OFFSET))
> +#define _PIPE_GAMMA_BASE(pipe) \
> +	(PIPEA_CGM_GAMMA_MIN + (pipe * GAMMA_OFFSET))

We use the _PIPE() macro with tha pipe A and B registers for those ,
instead of having to defies the offsets.

>  #endif /* _I915_REG_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index d2674a6..21f0ac2 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -473,6 +473,12 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
>  				   struct drm_property *property,
>  				   uint64_t val)
>  {
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->prop_palette_after_ctm)
> +		return intel_color_manager_set_gamma(dev, &crtc->base, val);
> +
>  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>  	return -EINVAL;
>  }

You are touching the hardware instead of staging the configuration?

> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> index 71b4c05..84cc3e47 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,6 +27,160 @@
>  
>  #include "intel_color_manager.h"
>  
> +int chv_set_gamma(struct drm_device *dev, uint32_t blob_id,
> +		  struct drm_crtc *crtc)
> +{
> +	if (num_samples == 0) {

[...]

> +	} else if (num_samples == CHV_8BIT_GAMMA_MAX_VALS) {

[...]
> +	} else if (num_samples == CHV_10BIT_GAMMA_MAX_VALS) {

[...]

> +	} else {
> +		DRM_ERROR("Invalid number of samples for Gamma LUT\n");
> +		return -EINVAL;
> +	}

This means you're not accepting 256 values, something we do today with
the legacy ioctl() and something we probably want for generic userspace.

> +	ret = drm_property_replace_global_blob(dev, &blob, length,
> +			(void *) gamma_data, &crtc->base,
> +			config->prop_palette_after_ctm);
> +
> +	if (ret) {
> +		DRM_ERROR("Error updating Gamma blob\n");
> +		return -EFAULT;
> +	}
> +
> +	return ret;
> +}
> +
> +int intel_color_manager_set_gamma(struct drm_device *dev,
> +		struct drm_mode_object *obj, uint32_t blob_id)
> +{
> +	struct drm_crtc *crtc = obj_to_crtc(obj);
> +
> +	if (IS_CHERRYVIEW(dev))
> +		return chv_set_gamma(dev, blob_id, crtc);
> +
> +	return -EINVAL;
> +}
> +
>  int get_chv_pipe_capabilities(struct drm_device *dev,
>  		struct drm_color_caps *color_caps, struct drm_crtc *crtc)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
> index 32262ac..d83567a 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.h
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -31,6 +31,8 @@
>  #define CHV_PALETTE_STRUCT_VERSION		1
>  #define CHV_CTM_STRUCT_VERSION			1
>  #define CHV_PLATFORM_STRUCT_VERSION		1
> +#define CHV_GAMMA_DATA_STRUCT_VERSION		1
> +
>  #define CHV_MAX_PALETTE_CAPS_BEFORE_CTM		1
>  #define CHV_MAX_PALETTE_CAPS_AFTER_CTM		2
>  #define CHV_DEGAMMA_PRECISION			14
> @@ -43,6 +45,16 @@
>  #define CHV_10BIT_GAMMA_MAX_VALS		257
>  #define CHV_8BIT_GAMMA_PRECISION		8
>  #define CHV_8BIT_GAMMA_MAX_VALS			256
> +#define CHV_8BIT_GAMMA_MSB_SHIFT		8
> +#define CHV_8BIT_GAMMA_SHIFT_RED_REG		16
> +#define CHV_8BIT_GAMMA_SHIFT_GREEN_REG		8
> +#define CHV_10BIT_GAMMA_MSB_SHIFT		6
> +#define CHV_GAMMA_SHIFT_GREEN			16
> +
>  #define CHV_CSC_COEFF_MAX_PRECISION		12
>  #define CHV_CSC_COEFF_MAX_INT			7
>  #define CHV_CSC_COEFF_MIN_INT			-7
> +
> +/* CHV CGM Block */
> +/* Bit 2 to be enabled in CGM block for CHV */
> +#define CGM_GAMMA_EN				4
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 053ceb0..a7aaadf 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1453,5 +1453,7 @@ extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
>  /* intel_color_manager.c */
>  void intel_color_manager_attach(struct drm_device *dev,
>  		struct drm_mode_object *mode_obj);
> +int intel_color_manager_set_gamma(struct drm_device *dev,
> +		struct drm_mode_object *obj, uint32_t blob_id);
>  
>  #endif /* __INTEL_DRV_H__ */
> -- 
> 2.4.5
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/12] drm: Add structures for querying color capabilities

[Damien Lespiau]

On Wed, Jul 01, 2015 at 09:18:14PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> The DRM color management framework is targeting various hardware
> platforms and drivers. Different platforms can have different color
> correction and enhancement capabilities.
> 
> A commom user space application can query these capabilities using the
> DRM property interface. Each driver can fill this property with its
> platform's color capabilities.
> 
> This patch adds new structures in DRM layer for querying color
> capabilities. These structures will be used by all user space
> agents to configure appropriate color configurations.

As I indicated before, I don't think we should go for a full fledged 
query API, because, I don't believe we can ever make it good enough to 
cover future hardware (and probably not today's hardware across 
vendors). 
 
These kinds of query APIs have been frown upon in the past for that 
exact reason. 
 
- Accept configurations that are mostly likely to be working across vendors 
(256 enties for 8 bits) That should be enough for basic functionality. 
 
- To support things that are really hw specific: make sure the kernel API can 
accept those, put the hw specific knowledge into a user-space HAL where APIs 
can evolve. What you're trying to do here with queries about per-platform 
details can go into userspace and still have a generic compositor code using 
those limits. Let's just not put the limits into the kernel API, we won't be 
able to get it right. 
 
Now maybe there's a middle ground and we can expose basic limits. In this case, 
maybe a list of supported LUT sizes, but the sampling details don't belong to a 
kernel interface IMHO. I'm even hesitant putting the hw precision at that 
level. 

> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>  include/uapi/drm/drm.h | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 3801584..d9562a2 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -829,6 +829,40 @@ struct drm_event_vblank {
>  	__u32 reserved;
>  };
>  
> +struct drm_palette_sampling_details {
> +	__u32 sample_fract_precision;
> +	__u32 last_sample_int_max;
> +	__u32 remaining_sample_int_max;
> +	__u32 num_samples;
> +};

If we're are indeed going with this, it will need documentation in the
code. Why do we need the precision for instance? It's not clear to me.

Also, we don't describe what the values beyond 1.0 represent, how the
interpolation is done, etc... (to support my first point to no try
describing too much).

> +struct drm_palette_caps {
> +	__u32 version;
> +	__u32 num_supported_types;
> +	struct drm_palette_sampling_details palette_sampling_types[4];
> +};
> +
> +struct drm_ctm_caps {
> +	__u32 version;
> +	__u32 ctm_coeff_fract_precision;
> +	__u32 ctm_coeff_int_max;
> +	__s32 ctm_coeff_int_min;
> +};

Same story for those precision, int_min and int_max fields. Why do we need
those? if we get 256 enties, we can take the values in the interface form
(whatever it is, say 15.16 + sign bit) and clip the values to what the hardware
supports.

> +struct drm_cge_caps {
> +	__u32 version;
> +	__u32 cge_max_weight;
> +};

This doesn't beling to DRM
> +struct drm_color_caps {
> +	__u32 version;
> +	__u32 reserved;

Why do we need reserved here? isn't version enough to make sure we can grow the
structure?

> +	struct drm_palette_caps palette_caps_after_ctm;
> +	struct drm_palette_caps palette_caps_before_ctm;
> +	struct drm_ctm_caps ctm_caps;
> +	struct drm_cge_caps cge_caps;

No CGE in the DRM core, other hw won't have it.

> +};
> +
>  /* typedef area */
>  #ifndef __KERNEL__
>  typedef struct drm_clip_rect drm_clip_rect_t;
> -- 
> 2.4.5
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/12] drm/i915: Attach color properties to CRTC

[Damien Lespiau]

On Wed, Jul 01, 2015 at 09:18:13PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> This patch does the following:
> 1. Adds new files intel_color_manager(.c/.h)
> 2. Attaches color properties to CRTC while initialization

A small note that we could remove manager from the whole API name,
intel_color_ looks enough of a prefix to me and will save some typing.

> 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 | 61 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color_manager.h | 28 ++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c       |  3 ++
>  drivers/gpu/drm/i915/intel_drv.h           |  4 ++
>  5 files changed, 98 insertions(+), 1 deletion(-)
>  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 de21965..ad928d8 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -56,7 +56,8 @@ i915-y += intel_audio.o \
>  	  intel_overlay.o \
>  	  intel_psr.o \
>  	  intel_sideband.o \
> -	  intel_sprite.o
> +	  intel_sprite.o \
> +	  intel_color_manager.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_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> new file mode 100644
> index 0000000..9280ea6
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -0,0 +1,61 @@
> +/*
> + * 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"
> +
> +void intel_color_manager_attach(struct drm_device *dev,
> +		struct drm_mode_object *mode_obj)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (mode_obj->type == DRM_MODE_OBJECT_CRTC) {
> +		if (config->prop_color_capabilities) {
> +			drm_object_attach_property(mode_obj,
> +					config->prop_color_capabilities, 0);
> +			DRM_DEBUG_DRIVER("Attached Color Caps property to CRTC\n");
> +		} else
> +			DRM_ERROR("Error attaching Color Capabilities property to CRTC\n");
> +		if (config->prop_palette_before_ctm) {
> +			drm_object_attach_property(mode_obj,
> +					config->prop_palette_before_ctm, 0);
> +			DRM_DEBUG_DRIVER("Attached Palette (before CTM) property to CRTC\n");
> +		} else
> +			DRM_ERROR("Error attaching Palette (before CTM) property to CRTC\n");
> +		if (config->prop_palette_after_ctm) {
> +			drm_object_attach_property(mode_obj,
> +					config->prop_palette_after_ctm, 0);
> +			DRM_DEBUG_DRIVER("Attached Palette (after CTM) property to CRTC\n");
> +		} else
> +			DRM_ERROR("Error attaching Palette (after CTM) property to CRTC\n");
> +		if (config->prop_ctm) {
> +			drm_object_attach_property(mode_obj,
> +					config->prop_ctm, 0);
> +			DRM_DEBUG_DRIVER("Attached CTM property to CRTC\n");
> +		} else
> +			DRM_ERROR("Error attaching CTM property to CRTC\n");
> +	}

Way too verbose, don't add any of those log message to the driver, I can
see how they may be useful for the initial effort, but it's enough to
inspect the properties actually installed on the DRM objects from
userspace.

> +}
> 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..c564761
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -0,0 +1,28 @@
> +/*
> + * 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>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 724b0e3..e175df2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14105,6 +14105,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  
>  	intel_crtc->wm.cxsr_allowed = true;
>  
> +	/* Attaching color properties to the CRTC */
> +	intel_color_manager_attach(dev, &intel_crtc->base.base);

Those kind of comments are frowned upon, don't add any more information
that the next line doesn't contain.

>  	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>  	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
>  	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3f0a890..1e18a7e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1446,4 +1446,8 @@ void intel_plane_destroy_state(struct drm_plane *plane,
>  			       struct drm_plane_state *state);
>  extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
>  
> +/* intel_color_manager.c */
> +void intel_color_manager_attach(struct drm_device *dev,
> +		struct drm_mode_object *mode_obj);
> +
>  #endif /* __INTEL_DRV_H__ */
> -- 
> 2.4.5
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/12] drm: Add structures to set/get a palette color property

[Damien Lespiau]

On Wed, Jul 01, 2015 at 09:18:17PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> This patch adds new structures in DRM layer for Palette color correction.
> These structures will be used by user space agents to configure
> appropriate number of samples and Palette LUT for a platform.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>  include/uapi/drm/drm.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index d9562a2..04a8f2a 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -863,6 +863,18 @@ struct drm_color_caps {
>  	struct drm_cge_caps cge_caps;
>  };
>  
> +struct drm_r32g32b32 {
> +	__u32 r32;
> +	__u32 g32;
> +	__u32 b32;
> +};

r32 means red is an u32 to me. Is that the right encoding? I thought we
need values > 1.0 for our hw?

> +
> +struct drm_palette {
> +	__u32 version;
> +	__u32 palette_num_samples;

No need to prefix the fields with the structure name.

> +	struct drm_r32g32b32 palette_lut[0];
> +};
> +
>  /* typedef area */
>  #ifndef __KERNEL__
>  typedef struct drm_clip_rect drm_clip_rect_t;
> -- 
> 2.4.5
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/12] drm: Export drm_property_replace_global_blob function

[Damien Lespiau]

On Wed, Jul 01, 2015 at 09:18:18PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> drm_property_replace_global_blob() is getting used by many wrapper
> functions to replace an existing blob with new values. Because this
> function was static, modules are forced to create wrapper functions in
> same file. Exporting this function will remove need for such wrapper
> functions.
> 
> This patch makes the function drm_property_replace_global_blob() be
> accessible globally.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 2 +-
>  include/drm/drm_crtc.h     | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 5d12ea9..15263a1 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4458,7 +4458,7 @@ EXPORT_SYMBOL(drm_property_lookup_blob);
>   * a completely atomic update. The access to path_blob_ptr is protected by the
>   * caller holding a lock on the connector.
>   */
> -static int drm_property_replace_global_blob(struct drm_device *dev,
> +int drm_property_replace_global_blob(struct drm_device *dev,
>                                              struct drm_property_blob **replace,
>                                              size_t length,
>                                              const void *data,

Whether this is a good idea or not, you probably want an EXPORT_SYMBOL().

-- 
Damien

> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 408d39a..821424e 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1541,6 +1541,12 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device
>  							      unsigned int supported_rotations);
>  extern unsigned int drm_rotation_simplify(unsigned int rotation,
>  					  unsigned int supported_rotations);
> +extern int drm_property_replace_global_blob(struct drm_device *dev,
> +					struct drm_property_blob **replace,
> +					size_t length,
> +					const void *data,
> +					struct drm_mode_object *obj_holds_id,
> +					struct drm_property *prop_holds_id);
>  
>  /* Helpers */
>  
> -- 
> 2.4.5
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/12] drm/i915: Attach color properties to CRTC

[Damien Lespiau]

On Wed, Jul 01, 2015 at 09:18:13PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> This patch does the following:
> 1. Adds new files intel_color_manager(.c/.h)
> 2. Attaches color properties to CRTC while initialization

We usually order the series so that "it always works". In this case, you
are attaching the properties to the CRTC before having any code to
handle them, so if we are bisecting the tree and end up in the middle of
the series we get a broken behaviour.

If you put the attach() at the end of the series, we'll only expose the
properties when we do have to handle them.

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

Re: [PATCH 04/12] drm: Add structures for querying color capabilities

[Damien Lespiau]

On Thu, Jul 02, 2015 at 05:20:45PM +0100, Damien Lespiau wrote:
> On Wed, Jul 01, 2015 at 09:18:14PM +0530, Kausal Malladi wrote:
> > From: Kausal Malladi <Kausal.Malladi@intel.com>
> > 
> > The DRM color management framework is targeting various hardware
> > platforms and drivers. Different platforms can have different color
> > correction and enhancement capabilities.
> > 
> > A commom user space application can query these capabilities using the
> > DRM property interface. Each driver can fill this property with its
> > platform's color capabilities.
> > 
> > This patch adds new structures in DRM layer for querying color
> > capabilities. These structures will be used by all user space
> > agents to configure appropriate color configurations.
> 
> As I indicated before, I don't think we should go for a full fledged 
> query API, because, I don't believe we can ever make it good enough to 
> cover future hardware (and probably not today's hardware across 
> vendors). 
>  
> These kinds of query APIs have been frown upon in the past for that 
> exact reason. 
>  
> - Accept configurations that are mostly likely to be working across vendors 
> (256 enties for 8 bits) That should be enough for basic functionality. 
>  
> - To support things that are really hw specific: make sure the kernel API can 
> accept those, put the hw specific knowledge into a user-space HAL where APIs 
> can evolve. What you're trying to do here with queries about per-platform 
> details can go into userspace and still have a generic compositor code using 
> those limits. Let's just not put the limits into the kernel API, we won't be 
> able to get it right. 
>  
> Now maybe there's a middle ground and we can expose basic limits. In this case, 
> maybe a list of supported LUT sizes, but the sampling details don't belong to a 
> kernel interface IMHO. I'm even hesitant putting the hw precision at that 
> level. 

To re-iterate the point with actual examples, the proposed query API
doesn't seem to handle things we'd want to know today:

- What are the extra values are coding for. eg. for 8 bits, the values
  after index 255 are special and we have no description for those. (and
  it can get fiddly to describe them, you may want to add the type of
  interpolation for instance).
- How to you represent capabilities across hardware unit. Eg. split
  gamma lowering the number of LUT entries.

That somewhat demonstrates my point I think. We won't be able to get the
query API right.

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

Re: [Intel-gfx] [PATCH 04/12] drm: Add structures for querying color capabilities

[Ville Syrjälä]

On Thu, Jul 02, 2015 at 05:45:32PM +0100, Damien Lespiau wrote:
> On Thu, Jul 02, 2015 at 05:20:45PM +0100, Damien Lespiau wrote:
> > On Wed, Jul 01, 2015 at 09:18:14PM +0530, Kausal Malladi wrote:
> > > From: Kausal Malladi <Kausal.Malladi@intel.com>
> > > 
> > > The DRM color management framework is targeting various hardware
> > > platforms and drivers. Different platforms can have different color
> > > correction and enhancement capabilities.
> > > 
> > > A commom user space application can query these capabilities using the
> > > DRM property interface. Each driver can fill this property with its
> > > platform's color capabilities.
> > > 
> > > This patch adds new structures in DRM layer for querying color
> > > capabilities. These structures will be used by all user space
> > > agents to configure appropriate color configurations.
> > 
> > As I indicated before, I don't think we should go for a full fledged 
> > query API, because, I don't believe we can ever make it good enough to 
> > cover future hardware (and probably not today's hardware across 
> > vendors). 
> >  
> > These kinds of query APIs have been frown upon in the past for that 
> > exact reason. 
> >  
> > - Accept configurations that are mostly likely to be working across vendors 
> > (256 enties for 8 bits) That should be enough for basic functionality. 
> >  
> > - To support things that are really hw specific: make sure the kernel API can 
> > accept those, put the hw specific knowledge into a user-space HAL where APIs 
> > can evolve. What you're trying to do here with queries about per-platform 
> > details can go into userspace and still have a generic compositor code using 
> > those limits. Let's just not put the limits into the kernel API, we won't be 
> > able to get it right. 
> >  
> > Now maybe there's a middle ground and we can expose basic limits. In this case, 
> > maybe a list of supported LUT sizes, but the sampling details don't belong to a 
> > kernel interface IMHO. I'm even hesitant putting the hw precision at that 
> > level. 
> 
> To re-iterate the point with actual examples, the proposed query API
> doesn't seem to handle things we'd want to know today:
> 
> - What are the extra values are coding for. eg. for 8 bits, the values
>   after index 255 are special and we have no description for those. (and
>   it can get fiddly to describe them, you may want to add the type of
>   interpolation for instance).
> - How to you represent capabilities across hardware unit. Eg. split
>   gamma lowering the number of LUT entries.
> 
> That somewhat demonstrates my point I think. We won't be able to get the
> query API right.

Yeah. My first idea for the gamma stuff was that we'd simply have the
blob property for the data, and then a separate enum property which
decides how we interpret the blob contents. The default for the enum
would be the 8bpc/256 entries thing always, but the other values could
be potentially hardware specific. Obviously this would limit the use of
the fancier modes to the atomic API since you'd need to configure both
the blob and enum at the same time. But I don't see why we shouldn't
be allowed to rely on atomic from now on.

Well, I guess we could allow the user to change just the enum if the
expected blob size will match what was already provided, although
thinking up a valid use case for this is a bit hard :)

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 04/12] drm: Add structures for querying color capabilities

[Daniel Vetter]

On Thu, Jul 02, 2015 at 08:04:40PM +0300, Ville Syrjälä wrote:
> On Thu, Jul 02, 2015 at 05:45:32PM +0100, Damien Lespiau wrote:
> > On Thu, Jul 02, 2015 at 05:20:45PM +0100, Damien Lespiau wrote:
> > > On Wed, Jul 01, 2015 at 09:18:14PM +0530, Kausal Malladi wrote:
> > > > From: Kausal Malladi <Kausal.Malladi@intel.com>
> > > > 
> > > > The DRM color management framework is targeting various hardware
> > > > platforms and drivers. Different platforms can have different color
> > > > correction and enhancement capabilities.
> > > > 
> > > > A commom user space application can query these capabilities using the
> > > > DRM property interface. Each driver can fill this property with its
> > > > platform's color capabilities.
> > > > 
> > > > This patch adds new structures in DRM layer for querying color
> > > > capabilities. These structures will be used by all user space
> > > > agents to configure appropriate color configurations.
> > > 
> > > As I indicated before, I don't think we should go for a full fledged 
> > > query API, because, I don't believe we can ever make it good enough to 
> > > cover future hardware (and probably not today's hardware across 
> > > vendors). 
> > >  
> > > These kinds of query APIs have been frown upon in the past for that 
> > > exact reason. 
> > >  
> > > - Accept configurations that are mostly likely to be working across vendors 
> > > (256 enties for 8 bits) That should be enough for basic functionality. 
> > >  
> > > - To support things that are really hw specific: make sure the kernel API can 
> > > accept those, put the hw specific knowledge into a user-space HAL where APIs 
> > > can evolve. What you're trying to do here with queries about per-platform 
> > > details can go into userspace and still have a generic compositor code using 
> > > those limits. Let's just not put the limits into the kernel API, we won't be 
> > > able to get it right. 
> > >  
> > > Now maybe there's a middle ground and we can expose basic limits. In this case, 
> > > maybe a list of supported LUT sizes, but the sampling details don't belong to a 
> > > kernel interface IMHO. I'm even hesitant putting the hw precision at that 
> > > level. 
> > 
> > To re-iterate the point with actual examples, the proposed query API
> > doesn't seem to handle things we'd want to know today:
> > 
> > - What are the extra values are coding for. eg. for 8 bits, the values
> >   after index 255 are special and we have no description for those. (and
> >   it can get fiddly to describe them, you may want to add the type of
> >   interpolation for instance).
> > - How to you represent capabilities across hardware unit. Eg. split
> >   gamma lowering the number of LUT entries.
> > 
> > That somewhat demonstrates my point I think. We won't be able to get the
> > query API right.
> 
> Yeah. My first idea for the gamma stuff was that we'd simply have the
> blob property for the data, and then a separate enum property which
> decides how we interpret the blob contents. The default for the enum
> would be the 8bpc/256 entries thing always, but the other values could
> be potentially hardware specific. Obviously this would limit the use of
> the fancier modes to the atomic API since you'd need to configure both
> the blob and enum at the same time. But I don't see why we shouldn't
> be allowed to rely on atomic from now on.

Yeah, enum+blob is what I like too. We can do one gamma table format enum
in drm core. And drivers can then create it with just the enum values they
actually support, similar to how we have rotation/mirror defines for
everything, but the driver doesn't necessarily support it all. And the
enum would fully encode everything there is to know about a layout, i.e.
including excat precision and stuff like the 1025th/513th additional entry
we have in some intel gamma tables.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Jani Nikula]

On Thu, 02 Jul 2015, Damien Lespiau <damien.lespiau@intel.com> wrote:
> On Wed, Jul 01, 2015 at 09:18:19PM +0530, Kausal Malladi wrote:
>> From: Kausal Malladi <Kausal.Malladi@intel.com>

I didn't get the series at all, and it's not in the moderation queue
either. The same happened to the last series from Kausal. What gives?

BR,
Jani.


>> 
>> CHV/BSW platform supports various Gamma correction modes, which are:
>> 1. Legacy 8-bit mode
>> 2. 10-bit CGM (Color Gamut Mapping) mode
>> 
>> 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
>> 
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h            |  10 ++
>>  drivers/gpu/drm/i915/intel_atomic.c        |   6 ++
>>  drivers/gpu/drm/i915/intel_color_manager.c | 154 +++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_color_manager.h |  12 +++
>>  drivers/gpu/drm/i915/intel_drv.h           |   2 +
>>  5 files changed, 184 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 313b1f9..36672e7 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7900,4 +7900,14 @@ enum skl_disp_power_wells {
>>  #define _PALETTE_A (dev_priv->info.display_mmio_offset + 0xa000)
>>  #define _PALETTE_B (dev_priv->info.display_mmio_offset + 0xa800)
>>  
>> +/* Color Management */
>> +#define PIPEA_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x67A00)
>> +#define PIPEA_CGM_GAMMA_MIN			(VLV_DISPLAY_BASE + 0x67000)
>> +#define CGM_OFFSET				0x2000
>> +#define GAMMA_OFFSET				0x2000
>> +#define _PIPE_CGM_CONTROL(pipe) \
>> +	(PIPEA_CGM_CONTROL + (pipe * CGM_OFFSET))
>> +#define _PIPE_GAMMA_BASE(pipe) \
>> +	(PIPEA_CGM_GAMMA_MIN + (pipe * GAMMA_OFFSET))
>
> We use the _PIPE() macro with tha pipe A and B registers for those ,
> instead of having to defies the offsets.
>
>>  #endif /* _I915_REG_H_ */
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index d2674a6..21f0ac2 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -473,6 +473,12 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
>>  				   struct drm_property *property,
>>  				   uint64_t val)
>>  {
>> +	struct drm_device *dev = crtc->dev;
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +
>> +	if (property == config->prop_palette_after_ctm)
>> +		return intel_color_manager_set_gamma(dev, &crtc->base, val);
>> +
>>  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>>  	return -EINVAL;
>>  }
>
> You are touching the hardware instead of staging the configuration?
>
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
>> index 71b4c05..84cc3e47 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -27,6 +27,160 @@
>>  
>>  #include "intel_color_manager.h"
>>  
>> +int chv_set_gamma(struct drm_device *dev, uint32_t blob_id,
>> +		  struct drm_crtc *crtc)
>> +{
>> +	if (num_samples == 0) {
>
> [...]
>
>> +	} else if (num_samples == CHV_8BIT_GAMMA_MAX_VALS) {
>
> [...]
>> +	} else if (num_samples == CHV_10BIT_GAMMA_MAX_VALS) {
>
> [...]
>
>> +	} else {
>> +		DRM_ERROR("Invalid number of samples for Gamma LUT\n");
>> +		return -EINVAL;
>> +	}
>
> This means you're not accepting 256 values, something we do today with
> the legacy ioctl() and something we probably want for generic userspace.
>
>> +	ret = drm_property_replace_global_blob(dev, &blob, length,
>> +			(void *) gamma_data, &crtc->base,
>> +			config->prop_palette_after_ctm);
>> +
>> +	if (ret) {
>> +		DRM_ERROR("Error updating Gamma blob\n");
>> +		return -EFAULT;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +int intel_color_manager_set_gamma(struct drm_device *dev,
>> +		struct drm_mode_object *obj, uint32_t blob_id)
>> +{
>> +	struct drm_crtc *crtc = obj_to_crtc(obj);
>> +
>> +	if (IS_CHERRYVIEW(dev))
>> +		return chv_set_gamma(dev, blob_id, crtc);
>> +
>> +	return -EINVAL;
>> +}
>> +
>>  int get_chv_pipe_capabilities(struct drm_device *dev,
>>  		struct drm_color_caps *color_caps, struct drm_crtc *crtc)
>>  {
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
>> index 32262ac..d83567a 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.h
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
>> @@ -31,6 +31,8 @@
>>  #define CHV_PALETTE_STRUCT_VERSION		1
>>  #define CHV_CTM_STRUCT_VERSION			1
>>  #define CHV_PLATFORM_STRUCT_VERSION		1
>> +#define CHV_GAMMA_DATA_STRUCT_VERSION		1
>> +
>>  #define CHV_MAX_PALETTE_CAPS_BEFORE_CTM		1
>>  #define CHV_MAX_PALETTE_CAPS_AFTER_CTM		2
>>  #define CHV_DEGAMMA_PRECISION			14
>> @@ -43,6 +45,16 @@
>>  #define CHV_10BIT_GAMMA_MAX_VALS		257
>>  #define CHV_8BIT_GAMMA_PRECISION		8
>>  #define CHV_8BIT_GAMMA_MAX_VALS			256
>> +#define CHV_8BIT_GAMMA_MSB_SHIFT		8
>> +#define CHV_8BIT_GAMMA_SHIFT_RED_REG		16
>> +#define CHV_8BIT_GAMMA_SHIFT_GREEN_REG		8
>> +#define CHV_10BIT_GAMMA_MSB_SHIFT		6
>> +#define CHV_GAMMA_SHIFT_GREEN			16
>> +
>>  #define CHV_CSC_COEFF_MAX_PRECISION		12
>>  #define CHV_CSC_COEFF_MAX_INT			7
>>  #define CHV_CSC_COEFF_MIN_INT			-7
>> +
>> +/* CHV CGM Block */
>> +/* Bit 2 to be enabled in CGM block for CHV */
>> +#define CGM_GAMMA_EN				4
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 053ceb0..a7aaadf 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1453,5 +1453,7 @@ extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
>>  /* intel_color_manager.c */
>>  void intel_color_manager_attach(struct drm_device *dev,
>>  		struct drm_mode_object *mode_obj);
>> +int intel_color_manager_set_gamma(struct drm_device *dev,
>> +		struct drm_mode_object *obj, uint32_t blob_id);
>>  
>>  #endif /* __INTEL_DRV_H__ */
>> -- 
>> 2.4.5
>> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 09/12] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Malladi, Kausal]

Kausal

On Friday 03 July 2015 01:33 PM, Jani Nikula wrote:
> On Thu, 02 Jul 2015, Damien Lespiau <damien.lespiau@intel.com> wrote:
>> On Wed, Jul 01, 2015 at 09:18:19PM +0530, Kausal Malladi wrote:
>>> From: Kausal Malladi <Kausal.Malladi@intel.com>
> I didn't get the series at all, and it's not in the moderation queue
> either. The same happened to the last series from Kausal. What gives?
>
> BR,
> Jani.
Yesterday I realized what was the issue and fixed it. Re-sent patches 
today to the entire list. Now I can see all the patches on the archives too.

--Kausal
>
>>> CHV/BSW platform supports various Gamma correction modes, which are:
>>> 1. Legacy 8-bit mode
>>> 2. 10-bit CGM (Color Gamut Mapping) mode
>>>
>>> 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
>>>
>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_reg.h            |  10 ++
>>>   drivers/gpu/drm/i915/intel_atomic.c        |   6 ++
>>>   drivers/gpu/drm/i915/intel_color_manager.c | 154 +++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/intel_color_manager.h |  12 +++
>>>   drivers/gpu/drm/i915/intel_drv.h           |   2 +
>>>   5 files changed, 184 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 313b1f9..36672e7 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -7900,4 +7900,14 @@ enum skl_disp_power_wells {
>>>   #define _PALETTE_A (dev_priv->info.display_mmio_offset + 0xa000)
>>>   #define _PALETTE_B (dev_priv->info.display_mmio_offset + 0xa800)
>>>   
>>> +/* Color Management */
>>> +#define PIPEA_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x67A00)
>>> +#define PIPEA_CGM_GAMMA_MIN			(VLV_DISPLAY_BASE + 0x67000)
>>> +#define CGM_OFFSET				0x2000
>>> +#define GAMMA_OFFSET				0x2000
>>> +#define _PIPE_CGM_CONTROL(pipe) \
>>> +	(PIPEA_CGM_CONTROL + (pipe * CGM_OFFSET))
>>> +#define _PIPE_GAMMA_BASE(pipe) \
>>> +	(PIPEA_CGM_GAMMA_MIN + (pipe * GAMMA_OFFSET))
>> We use the _PIPE() macro with tha pipe A and B registers for those ,
>> instead of having to defies the offsets.
>>
>>>   #endif /* _I915_REG_H_ */
>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>>> index d2674a6..21f0ac2 100644
>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>>> @@ -473,6 +473,12 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
>>>   				   struct drm_property *property,
>>>   				   uint64_t val)
>>>   {
>>> +	struct drm_device *dev = crtc->dev;
>>> +	struct drm_mode_config *config = &dev->mode_config;
>>> +
>>> +	if (property == config->prop_palette_after_ctm)
>>> +		return intel_color_manager_set_gamma(dev, &crtc->base, val);
>>> +
>>>   	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>>>   	return -EINVAL;
>>>   }
>> You are touching the hardware instead of staging the configuration?
>>
>>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
>>> index 71b4c05..84cc3e47 100644
>>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>>> @@ -27,6 +27,160 @@
>>>   
>>>   #include "intel_color_manager.h"
>>>   
>>> +int chv_set_gamma(struct drm_device *dev, uint32_t blob_id,
>>> +		  struct drm_crtc *crtc)
>>> +{
>>> +	if (num_samples == 0) {
>> [...]
>>
>>> +	} else if (num_samples == CHV_8BIT_GAMMA_MAX_VALS) {
>> [...]
>>> +	} else if (num_samples == CHV_10BIT_GAMMA_MAX_VALS) {
>> [...]
>>
>>> +	} else {
>>> +		DRM_ERROR("Invalid number of samples for Gamma LUT\n");
>>> +		return -EINVAL;
>>> +	}
>> This means you're not accepting 256 values, something we do today with
>> the legacy ioctl() and something we probably want for generic userspace.
>>
>>> +	ret = drm_property_replace_global_blob(dev, &blob, length,
>>> +			(void *) gamma_data, &crtc->base,
>>> +			config->prop_palette_after_ctm);
>>> +
>>> +	if (ret) {
>>> +		DRM_ERROR("Error updating Gamma blob\n");
>>> +		return -EFAULT;
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +int intel_color_manager_set_gamma(struct drm_device *dev,
>>> +		struct drm_mode_object *obj, uint32_t blob_id)
>>> +{
>>> +	struct drm_crtc *crtc = obj_to_crtc(obj);
>>> +
>>> +	if (IS_CHERRYVIEW(dev))
>>> +		return chv_set_gamma(dev, blob_id, crtc);
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>>   int get_chv_pipe_capabilities(struct drm_device *dev,
>>>   		struct drm_color_caps *color_caps, struct drm_crtc *crtc)
>>>   {
>>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
>>> index 32262ac..d83567a 100644
>>> --- a/drivers/gpu/drm/i915/intel_color_manager.h
>>> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
>>> @@ -31,6 +31,8 @@
>>>   #define CHV_PALETTE_STRUCT_VERSION		1
>>>   #define CHV_CTM_STRUCT_VERSION			1
>>>   #define CHV_PLATFORM_STRUCT_VERSION		1
>>> +#define CHV_GAMMA_DATA_STRUCT_VERSION		1
>>> +
>>>   #define CHV_MAX_PALETTE_CAPS_BEFORE_CTM		1
>>>   #define CHV_MAX_PALETTE_CAPS_AFTER_CTM		2
>>>   #define CHV_DEGAMMA_PRECISION			14
>>> @@ -43,6 +45,16 @@
>>>   #define CHV_10BIT_GAMMA_MAX_VALS		257
>>>   #define CHV_8BIT_GAMMA_PRECISION		8
>>>   #define CHV_8BIT_GAMMA_MAX_VALS			256
>>> +#define CHV_8BIT_GAMMA_MSB_SHIFT		8
>>> +#define CHV_8BIT_GAMMA_SHIFT_RED_REG		16
>>> +#define CHV_8BIT_GAMMA_SHIFT_GREEN_REG		8
>>> +#define CHV_10BIT_GAMMA_MSB_SHIFT		6
>>> +#define CHV_GAMMA_SHIFT_GREEN			16
>>> +
>>>   #define CHV_CSC_COEFF_MAX_PRECISION		12
>>>   #define CHV_CSC_COEFF_MAX_INT			7
>>>   #define CHV_CSC_COEFF_MIN_INT			-7
>>> +
>>> +/* CHV CGM Block */
>>> +/* Bit 2 to be enabled in CGM block for CHV */
>>> +#define CGM_GAMMA_EN				4
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 053ceb0..a7aaadf 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -1453,5 +1453,7 @@ extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
>>>   /* intel_color_manager.c */
>>>   void intel_color_manager_attach(struct drm_device *dev,
>>>   		struct drm_mode_object *mode_obj);
>>> +int intel_color_manager_set_gamma(struct drm_device *dev,
>>> +		struct drm_mode_object *obj, uint32_t blob_id);
>>>   
>>>   #endif /* __INTEL_DRV_H__ */
>>> -- 
>>> 2.4.5
>>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Jani Nikula]

On Fri, 03 Jul 2015, "Malladi, Kausal" <Kausal.Malladi@intel.com> wrote:
> On Friday 03 July 2015 01:33 PM, Jani Nikula wrote:
>> I didn't get the series at all, and it's not in the moderation queue
>> either. The same happened to the last series from Kausal. What gives?
>>
>> BR,
>> Jani.
> Yesterday I realized what was the issue and fixed it. Re-sent patches 
> today to the entire list. Now I can see all the patches on the archives too.

Ah okay, sorry for the noise, I wasn't far enough in my inbox yet. :)

Thanks,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/12] drm: Add structures for querying color capabilities

[Damien Lespiau]

On Fri, Jul 03, 2015 at 08:41:31AM +0200, Daniel Vetter wrote:
> > Yeah. My first idea for the gamma stuff was that we'd simply have the
> > blob property for the data, and then a separate enum property which
> > decides how we interpret the blob contents. The default for the enum
> > would be the 8bpc/256 entries thing always, but the other values could
> > be potentially hardware specific. Obviously this would limit the use of
> > the fancier modes to the atomic API since you'd need to configure both
> > the blob and enum at the same time. But I don't see why we shouldn't
> > be allowed to rely on atomic from now on.
> 
> Yeah, enum+blob is what I like too. We can do one gamma table format enum
> in drm core. And drivers can then create it with just the enum values they
> actually support, similar to how we have rotation/mirror defines for
> everything, but the driver doesn't necessarily support it all. And the
> enum would fully encode everything there is to know about a layout, i.e.
> including excat precision and stuff like the 1025th/513th additional entry
> we have in some intel gamma tables.

I'm assuming you're still talking about 3 separate properties though?
pre-csc lut, csc, post-csc lut?

-- 
Damien

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

Re: [Intel-gfx] [PATCH 04/12] drm: Add structures for querying color capabilities

[Ville Syrjälä]

On Fri, Jul 03, 2015 at 11:28:41AM +0100, Damien Lespiau wrote:
> On Fri, Jul 03, 2015 at 08:41:31AM +0200, Daniel Vetter wrote:
> > > Yeah. My first idea for the gamma stuff was that we'd simply have the
> > > blob property for the data, and then a separate enum property which
> > > decides how we interpret the blob contents. The default for the enum
> > > would be the 8bpc/256 entries thing always, but the other values could
> > > be potentially hardware specific. Obviously this would limit the use of
> > > the fancier modes to the atomic API since you'd need to configure both
> > > the blob and enum at the same time. But I don't see why we shouldn't
> > > be allowed to rely on atomic from now on.
> > 
> > Yeah, enum+blob is what I like too. We can do one gamma table format enum
> > in drm core. And drivers can then create it with just the enum values they
> > actually support, similar to how we have rotation/mirror defines for
> > everything, but the driver doesn't necessarily support it all. And the
> > enum would fully encode everything there is to know about a layout, i.e.
> > including excat precision and stuff like the 1025th/513th additional entry
> > we have in some intel gamma tables.
> 
> I'm assuming you're still talking about 3 separate properties though?
> pre-csc lut, csc, post-csc lut?

For CHV that would seem to fit since the pre and post gamma have
different size and precision. Although I suppose we could stuff it
in one blob if needed. Hmm. Actually I think you can even enable both
GGM and the old gamma unit at the same time, but maybe we should just
decice that it's going too far and not provide the option for that.

And we'd need to consider the CGM CSC vs. VLV wide gamut CSC too.
The pipe looks something like this:
"cgm degamma -> cgm csc -> cgm gamma -> wide gamut CSC -> pipe gamma"
So just pre and post csc gammas aren't going to cut it if we want to
expoe it all. But if we decice to not expose the wide gamut csc, then
we should be able to use either the cgm gamma or the pipe gamma as
the post-csc gamma.

Another thing which is unclear at this point is whether the plane
control register gamma bit can be used to bypass the CGM
gamma/degamma on a per plane basis, or if the bit only affects the
pipe gamma.

VLV is easier since it just has the wide gamut csc and pipe gamma
after it, and older gmch platforms have no csc unit.

For ILK+ we can choose to position the gamma before or after the csc,
or we can do the split gamma on IVB+. So from hardware POV it's
just a single table, but I suppose we could expose it as two blobs to
be a bit more compatible with CHV if we use two blobs there.

I tried to enumerate all the different gamma modes we have and came up
with the following list:
- 0.8 x 256				legacy gamma
- 0.10 and 6bit slope x 128		gen2/3 10bit interpolated gamma
- 0.16 x 128 + 1.16 x 1			gen4/g4x/vlv/chv 10bit interpolated gamma
- 0.14 x 65				chv interpolated gcm degamma
- 0.10 x 257				chv interpolated gcm gamma
- 0.10 x 1024				ilk/snb 10bit gamma
- 0.16 x 512 + 1.16 x 1			ilk/snb 12bit interpolated gamma
- 0.10 x 1024 + 3.16 x 1		ivb+ 10bit gamma
- 0.10 x 512  + 3.16 x 1		ivb+ split gamma before csc
- 0.10 x 512				ivb+ split gamma after csc
- 0.16 x 512 + 1.16 x 1 + 3.16 x 1	ivb+ 12bit interpolated gamma

I hope I didn't forget anyting. Also looks like in the future we'll get
yet another extra entry for the higher precision modes.

So if we have two blobs, would we also want two enums? In that case we'd
also need some kind of "bypass" or "disabled" enum value. But with two
enums coming up with supported combinations might become a bit hairy.
If we'd instead go with a single enum I suppose we'd need to have
pre-csc and post-csc variants of all the non-split modes for ilk+.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel