[Intel-gfx] [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 at 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 at intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi at 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
>