Re: [PATCH v2 04/10] drm: Add Gamma correction structure

From: Damien Lespiau <damien.lespiau@intel.com>
To: Kausal Malladi <Kausal.Malladi@intel.com>
Cc: annie.j.matheson@intel.com, dhanya.p.r@intel.com,
	avinash.reddy.palleti@intel.com, intel-gfx@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org, vijay.a.purushothaman@intel.com,
	indranil.mukherjee@intel.com, jesse.barnes@intel.com,
	daniel.vetter@intel.com, sunil.kamath@intel.com,
	shashank.sharma@intel.com
Subject: Re: [PATCH v2 04/10] drm: Add Gamma correction structure
Date: Tue, 9 Jun 2015 12:06:16 +0100	[thread overview]
Message-ID: <20150609110616.GD1671@strange.amr.corp.intel.com> (raw)
In-Reply-To: <1433425361-17864-5-git-send-email-Kausal.Malladi@intel.com>

On Thu, Jun 04, 2015 at 07:12:35PM +0530, 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 gamma_level;
> 	(The gamma_level variable indicates if the Gamma correction is to be

Why do we need this? The properties are installed on pipe and plane
objects and the set_property() interface already tells us which object
we are talking about.

> 	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

UNKNOWN is used to disable the function, why not call it GAMMA_DISABLE?

> 	#define I915_GAMMA_PRECISION_CURRENT	0xFFFFFFFF

Current isn't used in this patch set.

> 	#define I915_GAMMA_PRECISION_LEGACY	(1 << 0)

That looks like a name leaking hw specific knowledge to an interface
that want to be generic. _8BITS?

> 	#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)

I915_ prefix but the structures are in drm.h. Here again, there's an
impedance mismatch between generic properties and driver (or worse
platform) specific interface.

> 
> 	__u32 num_samples;
> 	(The num_samples indicates the number of Gamma correction
> 	coefficients)
> 	__u32 reserved;
> 	__u16 values[0];
> 	(An array of size 0, to accommodate the "num_samples" number of
> 	R16G16B16 pixels, dynamically)

That doesn't seem enough. We can have values > 1.0 for wide gamut
pipelines, I'd suggest 16.16 fixed point values.

> };

The defines and documentation of that fields should be in the code, not
in the commit message. It'd also be handy to have all the corresponding
defines in this patch so we have a good view of the API.

> 
> v2: Addressing Daniel Stone's comment, added a variable sized array to
> carry Gamma correction values as blob property.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>  include/drm/drm_crtc.h |  3 +++
>  include/uapi/drm/drm.h | 10 ++++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 2a75d7d..bc44f27 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 Blob IDs */
> +	u32 gamma_blob_id;
>  };
>  
>  /**
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 3801584..fc2661c 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -829,6 +829,16 @@ struct drm_event_vblank {
>  	__u32 reserved;
>  };
>  
> +/* Color Management structure for Gamma */
> +struct drm_gamma {
> +	__u32 flags;
> +	__u32 gamma_level;
> +	__u32 gamma_precision;
> +	__u32 num_samples;
> +	__u32 reserved;
> +	__u16 values[0];
> +};
> +
>  /* typedef area */
>  #ifndef __KERNEL__
>  typedef struct drm_clip_rect drm_clip_rect_t;
> -- 
> 2.4.2
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel