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

[Matt Roper <matthew.d.roper@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
> 	applied on Pipe/plane)

I'm not sure I understand the need for this one...you're getting the
set_property call against a specific DRM object, so I don't think there
should be any confusion at that point about what the values apply to?


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

I feel like the precision would work better as a separate enum property
rather than being part of your blob; I think it would be cleaner if your
blob only held the actual values if possible.

> 
> 	__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)
> };
> 
> 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;

The flags aren't described in your commit message...what are they used
for?  I guess it will become more clear as I read farther through your
patch series.


Matt


> +	__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
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel