Re: [PATCH 05/23] drm: Add structure to set/get a CTM color property

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Shashank Sharma <shashank.sharma@intel.com>
Cc: annie.j.matheson@intel.com, intel-gfx@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org, kausalmalladi@gmail.com,
	daniel.vetter@intel.com
Subject: Re: [PATCH 05/23] drm: Add structure to set/get a CTM color property
Date: Tue, 22 Sep 2015 18:22:08 +0300	[thread overview]
Message-ID: <20150922152208.GR26517@intel.com> (raw)
In-Reply-To: <1442425040-32185-6-git-send-email-shashank.sharma@intel.com>

On Wed, Sep 16, 2015 at 11:07:02PM +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@gmail.com>
> 
> Color Manager framework defines a color correction property for color
> space transformation and Gamut mapping. This property is called CTM (Color
> Transformation Matrix).
> 
> This patch adds a new structure in DRM layer for CTM.
> This structure can be used by all user space agents to
> configure CTM coefficients for color correction.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.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 f72b916..9580772 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -867,6 +867,18 @@ struct drm_palette {
>  	struct drm_r32g32b32 lut[0];
>  };
>  
> +struct drm_ctm {
> +	/* Structure version. Should be 1 currently */
> +	__u32 version;
> +	/*
> +	 * Each value is in S31.32 format.
> +	 * This is 3x3 matrix in row major format.

I'm not sure that exlains things. Maybe sketch the math equation here
(something like what I put in i915_reg.h for the CHV sprite CSC). Which
reminds me: what are we going to do about the pre/post offset stuff? I
was sort of expecting to see it here, but maybe you put those into
separate properties?

> +	 * Integer part will be clipped to nearest
> +	 * max/min boundary as supported by the HW platform.
> +	 */
> +	__s64 ctm_coeff[9];

ctm_ prefix seems superfluous. Also could maybe use some comment
explaining what ctm is?

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

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