Re: [PATCH v2 00/10] Color Manager Implementation

Re: [PATCH v2 00/10] Color Manager Implementation

[Jindal, Sonika]

HI Kausal,

You don't need to send the entire series again .
Just send the updated patch --in-reply-to to the last message.
Otherwise the thread gets lost.

You can send the entire series once the review is complete and you feel that the patches are too nested inside.
Please keep sending the patches using --in-reply-to tag.

Regards,
Sonika

-----Original Message-----
From: Malladi, Kausal 
Sent: Thursday, June 4, 2015 7:13 PM
To: Roper, Matthew D; Barnes, Jesse; Lespiau, Damien; Jindal, Sonika; R, Durgadoss; Purushothaman, Vijay A; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Cc: Vetter, Daniel; Sharma, Shashank; Kamath, Sunil; Mukherjee, Indranil; Matheson, Annie J; R, Dhanya p; Palleti, Avinash Reddy; Malladi, Kausal
Subject: [PATCH v2 00/10] Color Manager Implementation

From: Kausal Malladi <Kausal.Malladi@intel.com>

This patch set adds color manager implementation in drm/i915 layer.
Color Manager is an extension in i915 driver to support color correction/enhancement. Various Intel platforms support several color correction capabilities. Color Manager provides abstraction of these properties and allows a user space UI agent to correct/enhance the display.

The first three patches add code for the framework, which will be common across all the Intel platforms. 
  drm/i915: Initialize Color Manager
  drm/i915: Attach color properties to CRTC
  drm/i915: Add Set property interface for CRTC

In the next patches, we are adding support for Gamma and CSC color properties. Please note that:
1. The current implementation is only limited for CHV/BSW platforms, and the support for
   other platforms will be following up soon.
2. The current patch set implements only the "set" part of the properties, "get" part will
   be following up soon.

The design of color management on linux is done by:
Jesse Barnes
Sirisha Muppavarapu
Shashank Sharma
Susanta Bhattacharjee

The complete design is explained in the design document, which is available at https://docs.google.com/document/d/1jyfNSAStKHEpmIUZd_1Gd68cPuyiyr8wmJXThSDb_2w/edit#

v2: Addressed review comments from Sonika and Daniel Stone.

Kausal Malladi (10):
  drm/i915: Initialize Color Manager
  drm/i915: Attach color properties to CRTC
  drm/i915: Add atomic set property interface for CRTC
  drm: Add Gamma correction structure
  drm: Add a new function for updating color blob
  drm: Avoid atomic commit path for CRTC property (Gamma)
  drm/i915: Add pipe level Gamma correction for CHV/BSW
  drm: Add CSC correction structure
  drm: Avoid atomic commit path for CSC property on CRTC
  drm/i915: Add CSC support for CHV/BSW

 drivers/gpu/drm/drm_atomic_helper.c        |  18 +-
 drivers/gpu/drm/drm_crtc.c                 |  15 ++
 drivers/gpu/drm/i915/Makefile              |   3 +
 drivers/gpu/drm/i915/intel_atomic.c        |  19 +-
 drivers/gpu/drm/i915/intel_color_manager.c | 348 +++++++++++++++++++++++++++++  drivers/gpu/drm/i915/intel_color_manager.h | 122 ++++++++++
 drivers/gpu/drm/i915/intel_display.c       |   8 +
 drivers/gpu/drm/i915/intel_drv.h           |   4 +
 include/drm/drm_crtc.h                     |  12 +
 include/uapi/drm/drm.h                     |  18 ++
 10 files changed, 563 insertions(+), 4 deletions(-)  create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
 create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h

--
2.4.2

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

Re: [PATCH v2 00/10] Color Manager Implementation

[Malladi, Kausal]

Hi Sonika,

Thanks for your suggestion. Will follow that from next time.

Regards
Kausal

On Friday 05 June 2015 09:31 AM, Jindal, Sonika wrote:
> HI Kausal,
>
> You don't need to send the entire series again .
> Just send the updated patch --in-reply-to to the last message.
> Otherwise the thread gets lost.
>
> You can send the entire series once the review is complete and you feel that the patches are too nested inside.
> Please keep sending the patches using --in-reply-to tag.
>
> Regards,
> Sonika
>
> -----Original Message-----
> From: Malladi, Kausal
> Sent: Thursday, June 4, 2015 7:13 PM
> To: Roper, Matthew D; Barnes, Jesse; Lespiau, Damien; Jindal, Sonika; R, Durgadoss; Purushothaman, Vijay A; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Vetter, Daniel; Sharma, Shashank; Kamath, Sunil; Mukherjee, Indranil; Matheson, Annie J; R, Dhanya p; Palleti, Avinash Reddy; Malladi, Kausal
> Subject: [PATCH v2 00/10] Color Manager Implementation
>
> From: Kausal Malladi <Kausal.Malladi@intel.com>
>
> This patch set adds color manager implementation in drm/i915 layer.
> Color Manager is an extension in i915 driver to support color correction/enhancement. Various Intel platforms support several color correction capabilities. Color Manager provides abstraction of these properties and allows a user space UI agent to correct/enhance the display.
>
> The first three patches add code for the framework, which will be common across all the Intel platforms.
>    drm/i915: Initialize Color Manager
>    drm/i915: Attach color properties to CRTC
>    drm/i915: Add Set property interface for CRTC
>
> In the next patches, we are adding support for Gamma and CSC color properties. Please note that:
> 1. The current implementation is only limited for CHV/BSW platforms, and the support for
>     other platforms will be following up soon.
> 2. The current patch set implements only the "set" part of the properties, "get" part will
>     be following up soon.
>
> The design of color management on linux is done by:
> Jesse Barnes
> Sirisha Muppavarapu
> Shashank Sharma
> Susanta Bhattacharjee
>
> The complete design is explained in the design document, which is available at https://docs.google.com/document/d/1jyfNSAStKHEpmIUZd_1Gd68cPuyiyr8wmJXThSDb_2w/edit#
>
> v2: Addressed review comments from Sonika and Daniel Stone.
>
> Kausal Malladi (10):
>    drm/i915: Initialize Color Manager
>    drm/i915: Attach color properties to CRTC
>    drm/i915: Add atomic set property interface for CRTC
>    drm: Add Gamma correction structure
>    drm: Add a new function for updating color blob
>    drm: Avoid atomic commit path for CRTC property (Gamma)
>    drm/i915: Add pipe level Gamma correction for CHV/BSW
>    drm: Add CSC correction structure
>    drm: Avoid atomic commit path for CSC property on CRTC
>    drm/i915: Add CSC support for CHV/BSW
>
>   drivers/gpu/drm/drm_atomic_helper.c        |  18 +-
>   drivers/gpu/drm/drm_crtc.c                 |  15 ++
>   drivers/gpu/drm/i915/Makefile              |   3 +
>   drivers/gpu/drm/i915/intel_atomic.c        |  19 +-
>   drivers/gpu/drm/i915/intel_color_manager.c | 348 +++++++++++++++++++++++++++++  drivers/gpu/drm/i915/intel_color_manager.h | 122 ++++++++++
>   drivers/gpu/drm/i915/intel_display.c       |   8 +
>   drivers/gpu/drm/i915/intel_drv.h           |   4 +
>   include/drm/drm_crtc.h                     |  12 +
>   include/uapi/drm/drm.h                     |  18 ++
>   10 files changed, 563 insertions(+), 4 deletions(-)  create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
>   create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h
>
> --
> 2.4.2
>

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

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

[Jindal, Sonika]

On 6/4/2015 7:12 PM, 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
Do you have any macro defines for these levels? Maybe an enum will be 
better?
> 	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
> 	#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)
>
> 	__u32 num_samples;
> 	(The num_samples indicates the number of Gamma correction
> 	coefficients)
> 	__u32 reserved;
You need this 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;
> +
Just trying to understand, why do we need to store the blob id separately?
> +	/* 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;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Malladi, Kausal]

Hi Sonika,

Please find my responses inline.

Thanks,
Kausal

On Friday 05 June 2015 05:30 PM, Jindal, Sonika wrote:
>
>
> On 6/4/2015 7:12 PM, 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
> Do you have any macro defines for these levels? Maybe an enum will be 
> better?
Yes, we have macro defines for these. There are only two possible levels 
(pipe/plane).
>>     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
>>     #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)
>>
>>     __u32 num_samples;
>>     (The num_samples indicates the number of Gamma correction
>>     coefficients)
>>     __u32 reserved;
> You need this reserved?
In case any future platforms require any additional checks, we can use 
this member. Having said that, yes.. we are currently not using it.
>>     __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;
>> +
> Just trying to understand, why do we need to store the blob id 
> separately?
Saving Blob ID allows us to do a lookup of the blob using ID. This will 
be useful in get_property call.
>> +    /* 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;
>>

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

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

[Emil Velikov]

Hi Kausal Malladi,

On 5 June 2015 at 13:00, Jindal, Sonika <sonika.jindal@intel.com> wrote:
> On 6/4/2015 7:12 PM, Kausal Malladi wrote:
>>
>> From: Kausal Malladi <Kausal.Malladi@intel.com>
>>
...
>> 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/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];
Silly question:
Why use zero sized array ? Afaik it's a construct not covered in
C90/C99, which makes sizeof(struct drm_gamma) act funny. There seems
to be no other instance of a zero-sized array in drm uapi, plus based
of Daniel Vettel's "Botching up IOCTLS" I think that using it here
might be a bad idea.

The commit message mentions that Daniel Stone suggested it, but that
email never made it to the dri-devel mailiing list (and many other
emails, as mentioned previously) :'-(

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

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

[Sharma, Shashank]

Hi, Emil Velikov 

The reason behind a zero sized array is that we want to use the same variable for various color correction possible across various driver . 
Due to current blob implementation, it doesn’t look very efficient to have another pointer in the structure, so we are left with this option only.

I guess as long as we are using gcc (which is for all Linux distributions), we are good. The size of the zero sized array will be zero, so no alignment errors as such. 

Regards
Shashank 
-----Original Message-----
From: Emil Velikov [mailto:emil.l.velikov@gmail.com] 
Sent: Friday, June 12, 2015 10:48 PM
To: Jindal, Sonika
Cc: Malladi, Kausal; Roper, Matthew D; Barnes, Jesse; Lespiau, Damien; R, Durgadoss; Purushothaman, Vijay A; intel-gfx@lists.freedesktop.org; ML dri-devel; Matheson, Annie J; Palleti, Avinash Reddy; Mukherjee, Indranil; R, Dhanya p; Kamath, Sunil; Vetter, Daniel; Sharma, Shashank
Subject: Re: [PATCH v2 04/10] drm: Add Gamma correction structure

Hi Kausal Malladi,

On 5 June 2015 at 13:00, Jindal, Sonika <sonika.jindal@intel.com> wrote:
> On 6/4/2015 7:12 PM, Kausal Malladi wrote:
>>
>> From: Kausal Malladi <Kausal.Malladi@intel.com>
>>
...
>> 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/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];
Silly question:
Why use zero sized array ? Afaik it's a construct not covered in C90/C99, which makes sizeof(struct drm_gamma) act funny. There seems to be no other instance of a zero-sized array in drm uapi, plus based of Daniel Vettel's "Botching up IOCTLS" I think that using it here might be a bad idea.

The commit message mentions that Daniel Stone suggested it, but that email never made it to the dri-devel mailiing list (and many other emails, as mentioned previously) :'-(

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

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

[Emil Velikov]

On 14 June 2015 at 10:02, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> Hi, Emil Velikov
>
> The reason behind a zero sized array is that we want to use the same variable for various color correction possible across various driver .
> Due to current blob implementation, it doesn’t look very efficient to have another pointer in the structure, so we are left with this option only.
>
Can you elaborate (to suggest any reading material) about those inefficiencies ?

> I guess as long as we are using gcc (which is for all Linux distributions), we are good. The size of the zero sized array will be zero, so no alignment errors as such.
>
Note that most of the DRM subsystem code is dual-licensed. As such it
is used in other OSes - Solaris, *BSD, not to mention the work (in
progress) about using clang/LLVM to build the kernel. In the former
case not everyone uses GCC.

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

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

[Matt Roper]

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

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

[Sharma, Shashank]

Regards
Shashank

On 6/6/2015 6:30 AM, Matt Roper wrote:
> 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?
>
Actually that is for the get_property path. If you have a look at the 
design document, there is a provision to query the current applied gamma 
which can be pipe/plane level. Hence keeping this.
>
>>         __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.
>
Again, this would be required for the get_property path. If we create a 
separate property for this, the design might be very complex.

This is the current implementation:
1. Pack the correction values and configurations in blob()
2. call a create_blob() and get the blob_id
3. do a set_porperty() with the blob_id
4. set_property will find this blob, with the blob_id and apply 
corrections.

Adding a separate property for gamma_level will add one more state here, 
which will make it further complex. Do you agree ?
>>
>> 	__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
We are currently using this to define gamma/degamma as such, but yes I 
agree that we have added this for future / undefined last minute usages, 
and can be removed.
>
>
>> +	__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
>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Matt Roper]

On Sat, Jun 06, 2015 at 05:21:41PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> 
> On 6/6/2015 6:30 AM, Matt Roper wrote:
> >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?
> >
> Actually that is for the get_property path. If you have a look at
> the design document, there is a provision to query the current
> applied gamma which can be pipe/plane level. Hence keeping this.

But the get_property path should work the same way...get_property is
issued against a specific DRM object, so the DRM core will call the
appropriate handler to look up the value depending on whether the object
ID passed references a plane or a CRTC.  The crtc get_property handler
will look in crtc->state to get the value to return while the plane
get_property handler will look in plane->state to get the value.  I
don't see anything in the design document that indicates this would be a
problem.  Maybe I'm not understanding your concern?


> >
> >>        __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.
> >
> Again, this would be required for the get_property path. If we
> create a separate property for this, the design might be very
> complex.
> 
> This is the current implementation:
> 1. Pack the correction values and configurations in blob()
> 2. call a create_blob() and get the blob_id
> 3. do a set_porperty() with the blob_id
> 4. set_property will find this blob, with the blob_id and apply
> corrections.

When you call set_property, I'd expect your new blob to replace the old
blob (see drm_atomic_set_mode_for_crtc() for an example of how this
works with mode blobs).  My understanding is that the 'precision' value
here is a distinct piece of information that would be valuable to set
and query separately from the actual gamma values, so there's really no
benefit to smashing them together into a single blob as far as I can
see.  As two separate properties, you'd just push both properties into
the property set submitted to the atomic ioctl by userspace.  Ultimately
everything you set winds up in the DRM object's state pointer and that
state pointer is what is used later to update the hardware programming.

Granted, if your userspace isn't using the atomic ioctl yet and is just
calling individual drmModeSetProperty() calls, then it's a bit uglier
since you need to make multiple ioctl calls.  But atomic is the way
forward, so we're not really trying to design around the legacy
interfaces anymore.

Let me know if I'm overlooking something in your design.

> Adding a separate property for gamma_level will add one more state
> here, which will make it further complex. Do you agree ?

There's only ever one state object for a DRM object (plane->state,
crtc->state, etc.).  That state object (and it's i915-specific subclass)
have multiple fields to store different kinds of information though.  Is
that what you're referring to?


Matt

> >>
> >>	__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
> We are currently using this to define gamma/degamma as such, but yes
> I agree that we have added this for future / undefined last minute
> usages, and can be removed.
> >
> >
> >>+	__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

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

[Damien Lespiau]

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

Re: [PATCH v2 05/10] drm: Add a new function for updating color blob

[Matt Roper]

On Thu, Jun 04, 2015 at 07:12:36PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> This patch adds a new function to update color blob properties
> and exports it.
> 
> v2: Addressing Sonika's comment,
> 1. Moved this function to a separate patch
> 2. Removed one input parameter to the function
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>

This function is basically just a pass-through.  Can we just un-static
drm_property_replace_global_blob() so that it can be called directly
instead?


Matt

> ---
>  drivers/gpu/drm/drm_crtc.c | 15 +++++++++++++++
>  include/drm/drm_crtc.h     |  4 ++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 77f87b2..f6fa147 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4691,6 +4691,21 @@ int drm_mode_connector_set_tile_property(struct drm_connector *connector)
>  }
>  EXPORT_SYMBOL(drm_mode_connector_set_tile_property);
>  
> +int drm_mode_crtc_update_color_property(struct drm_property_blob **blob,
> +					size_t length, const void *color_data,
> +					struct drm_mode_object *obj_holds_id,
> +					struct drm_property *prop_holds_id)
> +{
> +	struct drm_device *dev = prop_holds_id->dev;
> +	int ret;
> +
> +	ret = drm_property_replace_global_blob(dev,
> +			blob, length, color_data, obj_holds_id, prop_holds_id);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_mode_crtc_update_color_property);
> +
>  /**
>   * drm_mode_connector_update_edid_property - update the edid property of a connector
>   * @connector: drm connector
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index bc44f27..31b52cb 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1343,6 +1343,10 @@ extern void drm_mode_config_cleanup(struct drm_device *dev);
>  
>  extern int drm_mode_connector_set_path_property(struct drm_connector *connector,
>  						const char *path);
> +extern int drm_mode_crtc_update_color_property(struct drm_property_blob **blob,
> +			size_t length, const void *color_data,
> +			struct drm_mode_object *obj_holds_id,
> +			struct drm_property *prop_holds_id);
>  int drm_mode_connector_set_tile_property(struct drm_connector *connector);
>  extern int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>  						   const struct edid *edid);
> -- 
> 2.4.2
> 

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

Re: [PATCH v2 05/10] drm: Add a new function for updating color blob

[Sharma, Shashank]

Regards
Shashank

On 6/6/2015 6:30 AM, Matt Roper wrote:
> On Thu, Jun 04, 2015 at 07:12:36PM +0530, Kausal Malladi wrote:
>> From: Kausal Malladi <Kausal.Malladi@intel.com>
>>
>> This patch adds a new function to update color blob properties
>> and exports it.
>>
>> v2: Addressing Sonika's comment,
>> 1. Moved this function to a separate patch
>> 2. Removed one input parameter to the function
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
>
> This function is basically just a pass-through.  Can we just un-static
> drm_property_replace_global_blob() so that it can be called directly
> instead?
>
>
> Matt
Yes, sure. In fact we would like to do that, the same comment was given 
by other reviewers. But when we looked at some other examples present in 
the same file (mode_property, edid_property), they have created a 
wrapper function around this, in the same file. So just followed the 
convention, but if that can be done, we will do it in the next patch set.
>
>> ---
>>   drivers/gpu/drm/drm_crtc.c | 15 +++++++++++++++
>>   include/drm/drm_crtc.h     |  4 ++++
>>   2 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 77f87b2..f6fa147 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -4691,6 +4691,21 @@ int drm_mode_connector_set_tile_property(struct drm_connector *connector)
>>   }
>>   EXPORT_SYMBOL(drm_mode_connector_set_tile_property);
>>
>> +int drm_mode_crtc_update_color_property(struct drm_property_blob **blob,
>> +					size_t length, const void *color_data,
>> +					struct drm_mode_object *obj_holds_id,
>> +					struct drm_property *prop_holds_id)
>> +{
>> +	struct drm_device *dev = prop_holds_id->dev;
>> +	int ret;
>> +
>> +	ret = drm_property_replace_global_blob(dev,
>> +			blob, length, color_data, obj_holds_id, prop_holds_id);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(drm_mode_crtc_update_color_property);
>> +
>>   /**
>>    * drm_mode_connector_update_edid_property - update the edid property of a connector
>>    * @connector: drm connector
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index bc44f27..31b52cb 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -1343,6 +1343,10 @@ extern void drm_mode_config_cleanup(struct drm_device *dev);
>>
>>   extern int drm_mode_connector_set_path_property(struct drm_connector *connector,
>>   						const char *path);
>> +extern int drm_mode_crtc_update_color_property(struct drm_property_blob **blob,
>> +			size_t length, const void *color_data,
>> +			struct drm_mode_object *obj_holds_id,
>> +			struct drm_property *prop_holds_id);
>>   int drm_mode_connector_set_tile_property(struct drm_connector *connector);
>>   extern int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>>   						   const struct edid *edid);
>> --
>> 2.4.2
>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 05/10] drm: Add a new function for updating color blob

[Matt Roper]

On Sat, Jun 06, 2015 at 05:24:46PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> 
> On 6/6/2015 6:30 AM, Matt Roper wrote:
> >On Thu, Jun 04, 2015 at 07:12:36PM +0530, Kausal Malladi wrote:
> >>From: Kausal Malladi <Kausal.Malladi@intel.com>
> >>
> >>This patch adds a new function to update color blob properties
> >>and exports it.
> >>
> >>v2: Addressing Sonika's comment,
> >>1. Moved this function to a separate patch
> >>2. Removed one input parameter to the function
> >>
> >>Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> >
> >This function is basically just a pass-through.  Can we just un-static
> >drm_property_replace_global_blob() so that it can be called directly
> >instead?
> >
> >
> >Matt
> Yes, sure. In fact we would like to do that, the same comment was
> given by other reviewers. But when we looked at some other examples
> present in the same file (mode_property, edid_property), they have
> created a wrapper function around this, in the same file. So just
> followed the convention, but if that can be done, we will do it in
> the next patch set.

I think those other functions started out as somewhat more complicated
functions before drm_property_replace_global_blob() ever existed, then
eventually got whittled down to their current state via refactoring.
Rather than creating new functions today it's probably easiest to just
unstatic the function here so it can be called directly.


Matt

> >
> >>---
> >>  drivers/gpu/drm/drm_crtc.c | 15 +++++++++++++++
> >>  include/drm/drm_crtc.h     |  4 ++++
> >>  2 files changed, 19 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >>index 77f87b2..f6fa147 100644
> >>--- a/drivers/gpu/drm/drm_crtc.c
> >>+++ b/drivers/gpu/drm/drm_crtc.c
> >>@@ -4691,6 +4691,21 @@ int drm_mode_connector_set_tile_property(struct drm_connector *connector)
> >>  }
> >>  EXPORT_SYMBOL(drm_mode_connector_set_tile_property);
> >>
> >>+int drm_mode_crtc_update_color_property(struct drm_property_blob **blob,
> >>+					size_t length, const void *color_data,
> >>+					struct drm_mode_object *obj_holds_id,
> >>+					struct drm_property *prop_holds_id)
> >>+{
> >>+	struct drm_device *dev = prop_holds_id->dev;
> >>+	int ret;
> >>+
> >>+	ret = drm_property_replace_global_blob(dev,
> >>+			blob, length, color_data, obj_holds_id, prop_holds_id);
> >>+
> >>+	return ret;
> >>+}
> >>+EXPORT_SYMBOL(drm_mode_crtc_update_color_property);
> >>+
> >>  /**
> >>   * drm_mode_connector_update_edid_property - update the edid property of a connector
> >>   * @connector: drm connector
> >>diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >>index bc44f27..31b52cb 100644
> >>--- a/include/drm/drm_crtc.h
> >>+++ b/include/drm/drm_crtc.h
> >>@@ -1343,6 +1343,10 @@ extern void drm_mode_config_cleanup(struct drm_device *dev);
> >>
> >>  extern int drm_mode_connector_set_path_property(struct drm_connector *connector,
> >>  						const char *path);
> >>+extern int drm_mode_crtc_update_color_property(struct drm_property_blob **blob,
> >>+			size_t length, const void *color_data,
> >>+			struct drm_mode_object *obj_holds_id,
> >>+			struct drm_property *prop_holds_id);
> >>  int drm_mode_connector_set_tile_property(struct drm_connector *connector);
> >>  extern int drm_mode_connector_update_edid_property(struct drm_connector *connector,
> >>  						   const struct edid *edid);
> >>--
> >>2.4.2
> >>
> >

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

Re: [PATCH v2 06/10] drm: Avoid atomic commit path for CRTC property (Gamma)

[Matt Roper]

On Thu, Jun 04, 2015 at 07:12:37PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> The atomic CRTC set infrastructure is not available yet. So, the atomic
> check path throws error for any non-plane property.
> 
> This patch adds a diversion to avoid commit path for DRM atomic set Gamma
> property, until atomic CRTC infrastructure is ready.

This doesn't look right, but I don't quite understand what you're trying
to do from the commit message.

This function is what will implement legacy set_property ioctl's of a
CRTC on an atomic-based driver.  The idea is that when the ioctl is
issued, we should get (i.e., make a duplicate of) the current CRTC
state, change some of the values in that state (which is what the
.atomic_set_property function takes care of), and then submit that
modified state to the driver for checking and hw-programming.

Okay, I just took a quick peek ahead in your patch set and I think I
understand the confusion now...it looks like you're trying to actually
perform the full hardware update in the .atomic_set_property call chain,
which isn't what we want to be doing in an atomic driver.
.atomic_set_property() is just a matter of updating the state structures
to reflect the changes you *want* to make (but not actually performing
those changes right away).  It isn't until drm_atomic_commit() gets
called that we validate the new state structure and then write it to the
hardware if it looks okay.

Keep in mind that the overall goal behind atomic is that we want to be
able to supply several items to be updated (e.g., mode, CSC, gamma,
etc.) via the atomic ioctl and then have them all accepted (and
programmed) by the driver, or all rejected (so none get programmed) if
any of them are invalid.  Also, if the collection of properties that
you're updating fall within the "nuclear pageflip" subset of
functionality, you'll also get a guarantee that those items all get
updated within the same vblank; updates that straddle a vblank could
cause unwanted flickering or other artifacts.  The helper function
you're updating here only happens to update a single state value at a
time, but the same .atomic_set_property() is also used by the atomic
ioctl, so we need to make sure it's following the expected design.

Finally, keep in mind that the function you're updating here is a DRM
core function.  Even though i915 isn't quite ready for full atomic yet
and might have a bit of brain damage in areas, other vendors' drivers do
have complete atomic modesetting support (I think?), so adding
i915-specific workarounds like this to the core function could actually
hamper them.


Matt

> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index a900858..37dba55 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1696,6 +1696,8 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
>  {
>  	struct drm_atomic_state *state;
>  	struct drm_crtc_state *crtc_state;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
>  	int ret = 0;
>  
>  	state = drm_atomic_state_alloc(crtc->dev);
> @@ -1716,9 +1718,18 @@ retry:
>  	if (ret)
>  		goto fail;
>  
> -	ret = drm_atomic_commit(state);
> -	if (ret != 0)
> -		goto fail;
> +	/**
> +	 * FIXME : This is a hack, to avoid atomic commit
> +	 * for CRTC, because i915 supports only
> +	 * atomic plane operations at the moment
> +	 */
> +	if (property == config->gamma_property)
> +		ret = 0;
> +	else {
> +		ret = drm_atomic_commit(state);
> +		if (ret != 0)
> +			goto fail;
> +	}
>  
>  	/* Driver takes ownership of state on successful commit. */
>  	return 0;
> -- 
> 2.4.2
> 

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

Re: [PATCH v2 06/10] drm: Avoid atomic commit path for CRTC property (Gamma)

[Sharma, Shashank]

Regards
Shashank

On 6/6/2015 6:31 AM, Matt Roper wrote:
> On Thu, Jun 04, 2015 at 07:12:37PM +0530, Kausal Malladi wrote:
>> From: Kausal Malladi <Kausal.Malladi@intel.com>
>>
>> The atomic CRTC set infrastructure is not available yet. So, the atomic
>> check path throws error for any non-plane property.
>>
>> This patch adds a diversion to avoid commit path for DRM atomic set Gamma
>> property, until atomic CRTC infrastructure is ready.
>
> This doesn't look right, but I don't quite understand what you're trying
> to do from the commit message.
>
> This function is what will implement legacy set_property ioctl's of a
> CRTC on an atomic-based driver.  The idea is that when the ioctl is
> issued, we should get (i.e., make a duplicate of) the current CRTC
> state, change some of the values in that state (which is what the
> .atomic_set_property function takes care of), and then submit that
> modified state to the driver for checking and hw-programming.
>
> Okay, I just took a quick peek ahead in your patch set and I think I
> understand the confusion now...it looks like you're trying to actually
> perform the full hardware update in the .atomic_set_property call chain,
> which isn't what we want to be doing in an atomic driver.
> .atomic_set_property() is just a matter of updating the state structures
> to reflect the changes you *want* to make (but not actually performing
> those changes right away).  It isn't until drm_atomic_commit() gets
> called that we validate the new state structure and then write it to the
> hardware if it looks okay.
>
> Keep in mind that the overall goal behind atomic is that we want to be
> able to supply several items to be updated (e.g., mode, CSC, gamma,
> etc.) via the atomic ioctl and then have them all accepted (and
> programmed) by the driver, or all rejected (so none get programmed) if
> any of them are invalid.  Also, if the collection of properties that
> you're updating fall within the "nuclear pageflip" subset of
> functionality, you'll also get a guarantee that those items all get
> updated within the same vblank; updates that straddle a vblank could
> cause unwanted flickering or other artifacts.  The helper function
> you're updating here only happens to update a single state value at a
> time, but the same .atomic_set_property() is also used by the atomic
> ioctl, so we need to make sure it's following the expected design.
>
> Finally, keep in mind that the function you're updating here is a DRM
> core function.  Even though i915 isn't quite ready for full atomic yet
> and might have a bit of brain damage in areas, other vendors' drivers do
> have complete atomic modesetting support (I think?), so adding
> i915-specific workarounds like this to the core function could actually
> hamper them.
>
>
> Matt
>
I understood this point. But in this case, we will be blocked until set 
CRTC implementation comes in. Can you suggest a better way to handle 
this without getting blocked for set_crtc() ?
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
>> ---
>>   drivers/gpu/drm/drm_atomic_helper.c | 17 ++++++++++++++---
>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index a900858..37dba55 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1696,6 +1696,8 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
>>   {
>>   	struct drm_atomic_state *state;
>>   	struct drm_crtc_state *crtc_state;
>> +	struct drm_device *dev = crtc->dev;
>> +	struct drm_mode_config *config = &dev->mode_config;
>>   	int ret = 0;
>>
>>   	state = drm_atomic_state_alloc(crtc->dev);
>> @@ -1716,9 +1718,18 @@ retry:
>>   	if (ret)
>>   		goto fail;
>>
>> -	ret = drm_atomic_commit(state);
>> -	if (ret != 0)
>> -		goto fail;
>> +	/**
>> +	 * FIXME : This is a hack, to avoid atomic commit
>> +	 * for CRTC, because i915 supports only
>> +	 * atomic plane operations at the moment
>> +	 */
>> +	if (property == config->gamma_property)
>> +		ret = 0;
>> +	else {
>> +		ret = drm_atomic_commit(state);
>> +		if (ret != 0)
>> +			goto fail;
>> +	}
>>
>>   	/* Driver takes ownership of state on successful commit. */
>>   	return 0;
>> --
>> 2.4.2
>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 06/10] drm: Avoid atomic commit path for CRTC property (Gamma)

[Matt Roper]

On Sat, Jun 06, 2015 at 05:34:45PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> 
> On 6/6/2015 6:31 AM, Matt Roper wrote:
> >On Thu, Jun 04, 2015 at 07:12:37PM +0530, Kausal Malladi wrote:
> >>From: Kausal Malladi <Kausal.Malladi@intel.com>
> >>
> >>The atomic CRTC set infrastructure is not available yet. So, the atomic
> >>check path throws error for any non-plane property.
> >>
> >>This patch adds a diversion to avoid commit path for DRM atomic set Gamma
> >>property, until atomic CRTC infrastructure is ready.
> >
> >This doesn't look right, but I don't quite understand what you're trying
> >to do from the commit message.
> >
> >This function is what will implement legacy set_property ioctl's of a
> >CRTC on an atomic-based driver.  The idea is that when the ioctl is
> >issued, we should get (i.e., make a duplicate of) the current CRTC
> >state, change some of the values in that state (which is what the
> >.atomic_set_property function takes care of), and then submit that
> >modified state to the driver for checking and hw-programming.
> >
> >Okay, I just took a quick peek ahead in your patch set and I think I
> >understand the confusion now...it looks like you're trying to actually
> >perform the full hardware update in the .atomic_set_property call chain,
> >which isn't what we want to be doing in an atomic driver.
> >.atomic_set_property() is just a matter of updating the state structures
> >to reflect the changes you *want* to make (but not actually performing
> >those changes right away).  It isn't until drm_atomic_commit() gets
> >called that we validate the new state structure and then write it to the
> >hardware if it looks okay.
> >
> >Keep in mind that the overall goal behind atomic is that we want to be
> >able to supply several items to be updated (e.g., mode, CSC, gamma,
> >etc.) via the atomic ioctl and then have them all accepted (and
> >programmed) by the driver, or all rejected (so none get programmed) if
> >any of them are invalid.  Also, if the collection of properties that
> >you're updating fall within the "nuclear pageflip" subset of
> >functionality, you'll also get a guarantee that those items all get
> >updated within the same vblank; updates that straddle a vblank could
> >cause unwanted flickering or other artifacts.  The helper function
> >you're updating here only happens to update a single state value at a
> >time, but the same .atomic_set_property() is also used by the atomic
> >ioctl, so we need to make sure it's following the expected design.
> >
> >Finally, keep in mind that the function you're updating here is a DRM
> >core function.  Even though i915 isn't quite ready for full atomic yet
> >and might have a bit of brain damage in areas, other vendors' drivers do
> >have complete atomic modesetting support (I think?), so adding
> >i915-specific workarounds like this to the core function could actually
> >hamper them.
> >
> >
> >Matt
> >
> I understood this point. But in this case, we will be blocked until
> set CRTC implementation comes in. Can you suggest a better way to
> handle this without getting blocked for set_crtc() ?

Is the functionality you need still lacking with Maarten Lankhorst's
"convert to atomic, part 2" series that recently landed upstream?  If
that series isn't sufficient, them I'm pretty sure that his "part 3"
series on the mailing list will fix it; that will hopefully be ready to
merge as soon as I get a chance to do a review of it.

I'll add Maarten to the Cc here in case he wants to comment...


Matt

> >>
> >>Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> >>---
> >>  drivers/gpu/drm/drm_atomic_helper.c | 17 ++++++++++++++---
> >>  1 file changed, 14 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>index a900858..37dba55 100644
> >>--- a/drivers/gpu/drm/drm_atomic_helper.c
> >>+++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>@@ -1696,6 +1696,8 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
> >>  {
> >>  	struct drm_atomic_state *state;
> >>  	struct drm_crtc_state *crtc_state;
> >>+	struct drm_device *dev = crtc->dev;
> >>+	struct drm_mode_config *config = &dev->mode_config;
> >>  	int ret = 0;
> >>
> >>  	state = drm_atomic_state_alloc(crtc->dev);
> >>@@ -1716,9 +1718,18 @@ retry:
> >>  	if (ret)
> >>  		goto fail;
> >>
> >>-	ret = drm_atomic_commit(state);
> >>-	if (ret != 0)
> >>-		goto fail;
> >>+	/**
> >>+	 * FIXME : This is a hack, to avoid atomic commit
> >>+	 * for CRTC, because i915 supports only
> >>+	 * atomic plane operations at the moment
> >>+	 */
> >>+	if (property == config->gamma_property)
> >>+		ret = 0;
> >>+	else {
> >>+		ret = drm_atomic_commit(state);
> >>+		if (ret != 0)
> >>+			goto fail;
> >>+	}
> >>
> >>  	/* Driver takes ownership of state on successful commit. */
> >>  	return 0;
> >>--
> >>2.4.2
> >>
> >

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

Re: [Intel-gfx] [PATCH v2 06/10] drm: Avoid atomic commit path for CRTC property (Gamma)

[Matt Roper]

On Mon, Jun 08, 2015 at 05:58:23PM -0700, Matt Roper wrote:
> On Sat, Jun 06, 2015 at 05:34:45PM +0530, Sharma, Shashank wrote:
> > Regards
> > Shashank
> > 
> > On 6/6/2015 6:31 AM, Matt Roper wrote:
> > >On Thu, Jun 04, 2015 at 07:12:37PM +0530, Kausal Malladi wrote:
> > >>From: Kausal Malladi <Kausal.Malladi@intel.com>
> > >>
> > >>The atomic CRTC set infrastructure is not available yet. So, the atomic
> > >>check path throws error for any non-plane property.
> > >>
> > >>This patch adds a diversion to avoid commit path for DRM atomic set Gamma
> > >>property, until atomic CRTC infrastructure is ready.
> > >
> > >This doesn't look right, but I don't quite understand what you're trying
> > >to do from the commit message.
> > >
> > >This function is what will implement legacy set_property ioctl's of a
> > >CRTC on an atomic-based driver.  The idea is that when the ioctl is
> > >issued, we should get (i.e., make a duplicate of) the current CRTC
> > >state, change some of the values in that state (which is what the
> > >.atomic_set_property function takes care of), and then submit that
> > >modified state to the driver for checking and hw-programming.
> > >
> > >Okay, I just took a quick peek ahead in your patch set and I think I
> > >understand the confusion now...it looks like you're trying to actually
> > >perform the full hardware update in the .atomic_set_property call chain,
> > >which isn't what we want to be doing in an atomic driver.
> > >.atomic_set_property() is just a matter of updating the state structures
> > >to reflect the changes you *want* to make (but not actually performing
> > >those changes right away).  It isn't until drm_atomic_commit() gets
> > >called that we validate the new state structure and then write it to the
> > >hardware if it looks okay.
> > >
> > >Keep in mind that the overall goal behind atomic is that we want to be
> > >able to supply several items to be updated (e.g., mode, CSC, gamma,
> > >etc.) via the atomic ioctl and then have them all accepted (and
> > >programmed) by the driver, or all rejected (so none get programmed) if
> > >any of them are invalid.  Also, if the collection of properties that
> > >you're updating fall within the "nuclear pageflip" subset of
> > >functionality, you'll also get a guarantee that those items all get
> > >updated within the same vblank; updates that straddle a vblank could
> > >cause unwanted flickering or other artifacts.  The helper function
> > >you're updating here only happens to update a single state value at a
> > >time, but the same .atomic_set_property() is also used by the atomic
> > >ioctl, so we need to make sure it's following the expected design.
> > >
> > >Finally, keep in mind that the function you're updating here is a DRM
> > >core function.  Even though i915 isn't quite ready for full atomic yet
> > >and might have a bit of brain damage in areas, other vendors' drivers do
> > >have complete atomic modesetting support (I think?), so adding
> > >i915-specific workarounds like this to the core function could actually
> > >hamper them.
> > >
> > >
> > >Matt
> > >
> > I understood this point. But in this case, we will be blocked until
> > set CRTC implementation comes in. Can you suggest a better way to
> > handle this without getting blocked for set_crtc() ?
> 
> Is the functionality you need still lacking with Maarten Lankhorst's
> "convert to atomic, part 2" series that recently landed upstream?  If
> that series isn't sufficient, them I'm pretty sure that his "part 3"
> series on the mailing list will fix it; that will hopefully be ready to
> merge as soon as I get a chance to do a review of it.
> 
> I'll add Maarten to the Cc here in case he wants to comment...
> 
> 
> Matt

Kausal/Shashank, have you guys had a chance to check out Maarten's
latest i915 atomic work?  His "part 2" series is already merged and
"part 3" should be merged soon and those really help clean up a lot of
the Intel-specific pain points caused by the atomic transition.  I think
we need to make sure you can handle properties properly in an atomic
manner, so if you find you're still blocked on the current code, we
should look into how to address that so that we don't have to bring any
temporary Intel hackiness into the shared DRM core code.


Matt

> 
> > >>
> > >>Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > >>Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> > >>---
> > >>  drivers/gpu/drm/drm_atomic_helper.c | 17 ++++++++++++++---
> > >>  1 file changed, 14 insertions(+), 3 deletions(-)
> > >>
> > >>diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > >>index a900858..37dba55 100644
> > >>--- a/drivers/gpu/drm/drm_atomic_helper.c
> > >>+++ b/drivers/gpu/drm/drm_atomic_helper.c
> > >>@@ -1696,6 +1696,8 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
> > >>  {
> > >>  	struct drm_atomic_state *state;
> > >>  	struct drm_crtc_state *crtc_state;
> > >>+	struct drm_device *dev = crtc->dev;
> > >>+	struct drm_mode_config *config = &dev->mode_config;
> > >>  	int ret = 0;
> > >>
> > >>  	state = drm_atomic_state_alloc(crtc->dev);
> > >>@@ -1716,9 +1718,18 @@ retry:
> > >>  	if (ret)
> > >>  		goto fail;
> > >>
> > >>-	ret = drm_atomic_commit(state);
> > >>-	if (ret != 0)
> > >>-		goto fail;
> > >>+	/**
> > >>+	 * FIXME : This is a hack, to avoid atomic commit
> > >>+	 * for CRTC, because i915 supports only
> > >>+	 * atomic plane operations at the moment
> > >>+	 */
> > >>+	if (property == config->gamma_property)
> > >>+		ret = 0;
> > >>+	else {
> > >>+		ret = drm_atomic_commit(state);
> > >>+		if (ret != 0)
> > >>+			goto fail;
> > >>+	}
> > >>
> > >>  	/* Driver takes ownership of state on successful commit. */
> > >>  	return 0;
> > >>--
> > >>2.4.2
> > >>
> > >
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [PATCH v2 06/10] drm: Avoid atomic commit path for CRTC property (Gamma)

[Malladi, Kausal]

On Saturday 20 June 2015 04:20 AM, Matt Roper wrote:
> On Mon, Jun 08, 2015 at 05:58:23PM -0700, Matt Roper wrote:
>> On Sat, Jun 06, 2015 at 05:34:45PM +0530, Sharma, Shashank wrote:
>>> Regards
>>> Shashank
>>>
>>> On 6/6/2015 6:31 AM, Matt Roper wrote:
>>>> On Thu, Jun 04, 2015 at 07:12:37PM +0530, Kausal Malladi wrote:
>>>>> From: Kausal Malladi <Kausal.Malladi@intel.com>
>>>>>
>>>>> The atomic CRTC set infrastructure is not available yet. So, the atomic
>>>>> check path throws error for any non-plane property.
>>>>>
>>>>> This patch adds a diversion to avoid commit path for DRM atomic set Gamma
>>>>> property, until atomic CRTC infrastructure is ready.
>>>> This doesn't look right, but I don't quite understand what you're trying
>>>> to do from the commit message.
>>>>
>>>> This function is what will implement legacy set_property ioctl's of a
>>>> CRTC on an atomic-based driver.  The idea is that when the ioctl is
>>>> issued, we should get (i.e., make a duplicate of) the current CRTC
>>>> state, change some of the values in that state (which is what the
>>>> .atomic_set_property function takes care of), and then submit that
>>>> modified state to the driver for checking and hw-programming.
>>>>
>>>> Okay, I just took a quick peek ahead in your patch set and I think I
>>>> understand the confusion now...it looks like you're trying to actually
>>>> perform the full hardware update in the .atomic_set_property call chain,
>>>> which isn't what we want to be doing in an atomic driver.
>>>> .atomic_set_property() is just a matter of updating the state structures
>>>> to reflect the changes you *want* to make (but not actually performing
>>>> those changes right away).  It isn't until drm_atomic_commit() gets
>>>> called that we validate the new state structure and then write it to the
>>>> hardware if it looks okay.
>>>>
>>>> Keep in mind that the overall goal behind atomic is that we want to be
>>>> able to supply several items to be updated (e.g., mode, CSC, gamma,
>>>> etc.) via the atomic ioctl and then have them all accepted (and
>>>> programmed) by the driver, or all rejected (so none get programmed) if
>>>> any of them are invalid.  Also, if the collection of properties that
>>>> you're updating fall within the "nuclear pageflip" subset of
>>>> functionality, you'll also get a guarantee that those items all get
>>>> updated within the same vblank; updates that straddle a vblank could
>>>> cause unwanted flickering or other artifacts.  The helper function
>>>> you're updating here only happens to update a single state value at a
>>>> time, but the same .atomic_set_property() is also used by the atomic
>>>> ioctl, so we need to make sure it's following the expected design.
>>>>
>>>> Finally, keep in mind that the function you're updating here is a DRM
>>>> core function.  Even though i915 isn't quite ready for full atomic yet
>>>> and might have a bit of brain damage in areas, other vendors' drivers do
>>>> have complete atomic modesetting support (I think?), so adding
>>>> i915-specific workarounds like this to the core function could actually
>>>> hamper them.
>>>>
>>>>
>>>> Matt
>>>>
>>> I understood this point. But in this case, we will be blocked until
>>> set CRTC implementation comes in. Can you suggest a better way to
>>> handle this without getting blocked for set_crtc() ?
>> Is the functionality you need still lacking with Maarten Lankhorst's
>> "convert to atomic, part 2" series that recently landed upstream?  If
>> that series isn't sufficient, them I'm pretty sure that his "part 3"
>> series on the mailing list will fix it; that will hopefully be ready to
>> merge as soon as I get a chance to do a review of it.
>>
>> I'll add Maarten to the Cc here in case he wants to comment...
>>
>>
>> Matt
> Kausal/Shashank, have you guys had a chance to check out Maarten's
> latest i915 atomic work?  His "part 2" series is already merged and
> "part 3" should be merged soon and those really help clean up a lot of
> the Intel-specific pain points caused by the atomic transition.  I think
> we need to make sure you can handle properties properly in an atomic
> manner, so if you find you're still blocked on the current code, we
> should look into how to address that so that we don't have to bring any
> temporary Intel hackiness into the shared DRM core code.
>
>
> Matt

Thanks Matt, for helping with a fix for this issue. As suggested by you, 
we will push this fix along with the other set of patches for color 
management.

Kausal
>
>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>>> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/drm_atomic_helper.c | 17 ++++++++++++++---
>>>>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>>> index a900858..37dba55 100644
>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>>> @@ -1696,6 +1696,8 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
>>>>>   {
>>>>>   	struct drm_atomic_state *state;
>>>>>   	struct drm_crtc_state *crtc_state;
>>>>> +	struct drm_device *dev = crtc->dev;
>>>>> +	struct drm_mode_config *config = &dev->mode_config;
>>>>>   	int ret = 0;
>>>>>
>>>>>   	state = drm_atomic_state_alloc(crtc->dev);
>>>>> @@ -1716,9 +1718,18 @@ retry:
>>>>>   	if (ret)
>>>>>   		goto fail;
>>>>>
>>>>> -	ret = drm_atomic_commit(state);
>>>>> -	if (ret != 0)
>>>>> -		goto fail;
>>>>> +	/**
>>>>> +	 * FIXME : This is a hack, to avoid atomic commit
>>>>> +	 * for CRTC, because i915 supports only
>>>>> +	 * atomic plane operations at the moment
>>>>> +	 */
>>>>> +	if (property == config->gamma_property)
>>>>> +		ret = 0;
>>>>> +	else {
>>>>> +		ret = drm_atomic_commit(state);
>>>>> +		if (ret != 0)
>>>>> +			goto fail;
>>>>> +	}
>>>>>
>>>>>   	/* Driver takes ownership of state on successful commit. */
>>>>>   	return 0;
>>>>> --
>>>>> 2.4.2
>>>>>
>> -- 
>> Matt Roper
>> Graphics Software Engineer
>> IoTG Platform Enabling & Development
>> Intel Corporation
>> (916) 356-2795
>> _______________________________________________
>> 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 v2 06/10] drm: Avoid atomic commit path for CRTC property (Gamma)

[Matheson, Annie J]

Thank you guys!

Annie Matheson
Intel Corporation
Phone: (503) 712-0586 
Email: annie.j.matheson@intel.com

-----Original Message-----
From: Malladi, Kausal 
Sent: Wednesday, June 24, 2015 8:41 AM
To: Roper, Matthew D; Sharma, Shashank
Cc: Matheson, Annie J; R, Dhanya p; dri-devel@lists.freedesktop.org; Purushothaman, Vijay A; Barnes, Jesse; Vetter, Daniel; intel-gfx@lists.freedesktop.org; Maarten Lankhorst
Subject: Re: [Intel-gfx] [PATCH v2 06/10] drm: Avoid atomic commit path for CRTC property (Gamma)

On Saturday 20 June 2015 04:20 AM, Matt Roper wrote:
> On Mon, Jun 08, 2015 at 05:58:23PM -0700, Matt Roper wrote:
>> On Sat, Jun 06, 2015 at 05:34:45PM +0530, Sharma, Shashank wrote:
>>> Regards
>>> Shashank
>>>
>>> On 6/6/2015 6:31 AM, Matt Roper wrote:
>>>> On Thu, Jun 04, 2015 at 07:12:37PM +0530, Kausal Malladi wrote:
>>>>> From: Kausal Malladi <Kausal.Malladi@intel.com>
>>>>>
>>>>> The atomic CRTC set infrastructure is not available yet. So, the atomic
>>>>> check path throws error for any non-plane property.
>>>>>
>>>>> This patch adds a diversion to avoid commit path for DRM atomic set Gamma
>>>>> property, until atomic CRTC infrastructure is ready.
>>>> This doesn't look right, but I don't quite understand what you're trying
>>>> to do from the commit message.
>>>>
>>>> This function is what will implement legacy set_property ioctl's of a
>>>> CRTC on an atomic-based driver.  The idea is that when the ioctl is
>>>> issued, we should get (i.e., make a duplicate of) the current CRTC
>>>> state, change some of the values in that state (which is what the
>>>> .atomic_set_property function takes care of), and then submit that
>>>> modified state to the driver for checking and hw-programming.
>>>>
>>>> Okay, I just took a quick peek ahead in your patch set and I think I
>>>> understand the confusion now...it looks like you're trying to actually
>>>> perform the full hardware update in the .atomic_set_property call chain,
>>>> which isn't what we want to be doing in an atomic driver.
>>>> .atomic_set_property() is just a matter of updating the state structures
>>>> to reflect the changes you *want* to make (but not actually performing
>>>> those changes right away).  It isn't until drm_atomic_commit() gets
>>>> called that we validate the new state structure and then write it to the
>>>> hardware if it looks okay.
>>>>
>>>> Keep in mind that the overall goal behind atomic is that we want to be
>>>> able to supply several items to be updated (e.g., mode, CSC, gamma,
>>>> etc.) via the atomic ioctl and then have them all accepted (and
>>>> programmed) by the driver, or all rejected (so none get programmed) if
>>>> any of them are invalid.  Also, if the collection of properties that
>>>> you're updating fall within the "nuclear pageflip" subset of
>>>> functionality, you'll also get a guarantee that those items all get
>>>> updated within the same vblank; updates that straddle a vblank could
>>>> cause unwanted flickering or other artifacts.  The helper function
>>>> you're updating here only happens to update a single state value at a
>>>> time, but the same .atomic_set_property() is also used by the atomic
>>>> ioctl, so we need to make sure it's following the expected design.
>>>>
>>>> Finally, keep in mind that the function you're updating here is a DRM
>>>> core function.  Even though i915 isn't quite ready for full atomic yet
>>>> and might have a bit of brain damage in areas, other vendors' drivers do
>>>> have complete atomic modesetting support (I think?), so adding
>>>> i915-specific workarounds like this to the core function could actually
>>>> hamper them.
>>>>
>>>>
>>>> Matt
>>>>
>>> I understood this point. But in this case, we will be blocked until
>>> set CRTC implementation comes in. Can you suggest a better way to
>>> handle this without getting blocked for set_crtc() ?
>> Is the functionality you need still lacking with Maarten Lankhorst's
>> "convert to atomic, part 2" series that recently landed upstream?  If
>> that series isn't sufficient, them I'm pretty sure that his "part 3"
>> series on the mailing list will fix it; that will hopefully be ready to
>> merge as soon as I get a chance to do a review of it.
>>
>> I'll add Maarten to the Cc here in case he wants to comment...
>>
>>
>> Matt
> Kausal/Shashank, have you guys had a chance to check out Maarten's
> latest i915 atomic work?  His "part 2" series is already merged and
> "part 3" should be merged soon and those really help clean up a lot of
> the Intel-specific pain points caused by the atomic transition.  I think
> we need to make sure you can handle properties properly in an atomic
> manner, so if you find you're still blocked on the current code, we
> should look into how to address that so that we don't have to bring any
> temporary Intel hackiness into the shared DRM core code.
>
>
> Matt

Thanks Matt, for helping with a fix for this issue. As suggested by you, 
we will push this fix along with the other set of patches for color 
management.

Kausal
>
>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>>> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/drm_atomic_helper.c | 17 ++++++++++++++---
>>>>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>>> index a900858..37dba55 100644
>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>>> @@ -1696,6 +1696,8 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
>>>>>   {
>>>>>   	struct drm_atomic_state *state;
>>>>>   	struct drm_crtc_state *crtc_state;
>>>>> +	struct drm_device *dev = crtc->dev;
>>>>> +	struct drm_mode_config *config = &dev->mode_config;
>>>>>   	int ret = 0;
>>>>>
>>>>>   	state = drm_atomic_state_alloc(crtc->dev);
>>>>> @@ -1716,9 +1718,18 @@ retry:
>>>>>   	if (ret)
>>>>>   		goto fail;
>>>>>
>>>>> -	ret = drm_atomic_commit(state);
>>>>> -	if (ret != 0)
>>>>> -		goto fail;
>>>>> +	/**
>>>>> +	 * FIXME : This is a hack, to avoid atomic commit
>>>>> +	 * for CRTC, because i915 supports only
>>>>> +	 * atomic plane operations at the moment
>>>>> +	 */
>>>>> +	if (property == config->gamma_property)
>>>>> +		ret = 0;
>>>>> +	else {
>>>>> +		ret = drm_atomic_commit(state);
>>>>> +		if (ret != 0)
>>>>> +			goto fail;
>>>>> +	}
>>>>>
>>>>>   	/* Driver takes ownership of state on successful commit. */
>>>>>   	return 0;
>>>>> --
>>>>> 2.4.2
>>>>>
>> -- 
>> Matt Roper
>> Graphics Software Engineer
>> IoTG Platform Enabling & Development
>> Intel Corporation
>> (916) 356-2795
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 00/10] Color Manager Implementation

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> This patch set adds color manager implementation in drm/i915 layer.
> Color Manager is an extension in i915 driver to support color 
> correction/enhancement. Various Intel platforms support several
> color correction capabilities. Color Manager provides abstraction
> of these properties and allows a user space UI agent to 
> correct/enhance the display.

For some reason, this patch set doesn't appear to have reached the
mailing lists: people are saying they haven't received the emails and the
mailing list archives[1][2] don't show them. Out of curiosity, how did
you send those emails?

Thanks,

-- 
Damien

[1] http://lists.freedesktop.org/archives/intel-gfx/2015-June/thread.html
[2] http://lists.freedesktop.org/archives/dri-devel/2015-June/thread.html
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH v2 00/10] Color Manager Implementation

[Malladi, Kausal]

I am guessing, the mails were not received because I wasn't subscribed to the these two mailing lists. Now I am subscribed and shouldn't have any problem hopefully.

Thanks Damien for bringing it to my notice.

Regards
Kausal

-----Original Message-----
From: Lespiau, Damien 
Sent: Tuesday, June 9, 2015 3:41 PM
To: Malladi, Kausal
Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 00/10] Color Manager Implementation

On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> This patch set adds color manager implementation in drm/i915 layer.
> Color Manager is an extension in i915 driver to support color 
> correction/enhancement. Various Intel platforms support several color 
> correction capabilities. Color Manager provides abstraction of these 
> properties and allows a user space UI agent to correct/enhance the 
> display.

For some reason, this patch set doesn't appear to have reached the mailing lists: people are saying they haven't received the emails and the mailing list archives[1][2] don't show them. Out of curiosity, how did you send those emails?

Thanks,

--
Damien

[1] http://lists.freedesktop.org/archives/intel-gfx/2015-June/thread.html
[2] http://lists.freedesktop.org/archives/dri-devel/2015-June/thread.html
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 00/10] Color Manager Implementation

[Jani Nikula]

On Thu, 11 Jun 2015, "Malladi, Kausal" <kausal.malladi@intel.com> wrote:
> I am guessing, the mails were not received because I wasn't subscribed
> to the these two mailing lists. Now I am subscribed and shouldn't have
> any problem hopefully.

In that case they should've popped up in the intel-gfx moderation list,
but for some reason didn't. *shrug*. Hopefully it works now.

BR,
Jani.


>
> Thanks Damien for bringing it to my notice.
>
> Regards
> Kausal
>
> -----Original Message-----
> From: Lespiau, Damien 
> Sent: Tuesday, June 9, 2015 3:41 PM
> To: Malladi, Kausal
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH v2 00/10] Color Manager Implementation
>
> On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
>> From: Kausal Malladi <Kausal.Malladi@intel.com>
>> 
>> This patch set adds color manager implementation in drm/i915 layer.
>> Color Manager is an extension in i915 driver to support color 
>> correction/enhancement. Various Intel platforms support several color 
>> correction capabilities. Color Manager provides abstraction of these 
>> properties and allows a user space UI agent to correct/enhance the 
>> display.
>
> For some reason, this patch set doesn't appear to have reached the mailing lists: people are saying they haven't received the emails and the mailing list archives[1][2] don't show them. Out of curiosity, how did you send those emails?
>
> Thanks,
>
> --
> Damien
>
> [1] http://lists.freedesktop.org/archives/intel-gfx/2015-June/thread.html
> [2] http://lists.freedesktop.org/archives/dri-devel/2015-June/thread.html
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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 v2 01/10] drm/i915: Initialize Color Manager

[Matt Roper]

On Thu, Jun 04, 2015 at 07:12:32PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> Color Manager is an extension in i915 driver to handle color correction
> and enhancements across various Intel platforms.
> 
> This patch initializes color manager framework by :
> 1. Adding two new files, intel_color_manager(.c/.h)
> 2. Introducing new pointers in DRM mode_config structure to
>    carry CSC and Gamma color correction properties.
> 3. Creating these DRM properties in Color Manager initialization
>    sequence.
> 
> v2: Addressing Sonika's review comment.
> 1. Made intel_color_manager_init void
> 2. Moved init after NUM_PIPES check
> 
> 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 | 48 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color_manager.h | 32 ++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c       |  3 ++
>  include/drm/drm_crtc.h                     |  4 +++
>  5 files changed, 90 insertions(+)
>  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 b7ddf48..c62d048 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -89,6 +89,9 @@ i915-y += i915_vgpu.o
>  # legacy horrors
>  i915-y += i915_dma.o
>  
> +# Color Management
> +i915-y += intel_color_manager.o
> +
>  obj-$(CONFIG_DRM_I915)  += i915.o
>  
>  CFLAGS_i915_trace_points.o := -I$(src)
> 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..f7e2380
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -0,0 +1,48 @@
> +/*
> + * 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_init(struct drm_device *dev)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	/* Create Gamma and CSC properties */
> +	config->gamma_property = drm_property_create(dev,
> +		DRM_MODE_PROP_BLOB, "gamma_property", 0);
> +	if (!config->gamma_property)
> +		DRM_ERROR("Gamma property creation failed\n");
> +	else
> +		DRM_DEBUG_DRIVER("Created Gamma property\n");
> +
> +	config->csc_property = drm_property_create(dev,
> +			DRM_MODE_PROP_BLOB, "csc_property", 0);
> +	if (!config->csc_property)
> +		DRM_ERROR("CSC property creation failed\n");
> +	else
> +		DRM_DEBUG_DRIVER("Created CSC property\n");
> +}
> 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..154bf16
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -0,0 +1,32 @@
> +/*
> + * 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>
> +
> +/* Generic Function prototypes */
> +void intel_color_manager_init(struct drm_device *dev);

While I personally don't mind creating a new header for prototypes, most
of our other KMS-related stuff just gets thrown in intel_drv.h under a
comment like "/* intel_foobar.c */" so this is a little inconsistent.
Maybe we should keep these prototypes there as well since that's the
file most developers are going to look for these in out of habit?


> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 067b1de..2322dee 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -44,6 +44,7 @@
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_rect.h>
>  #include <linux/dma_remapping.h>
> +#include "intel_color_manager.h"
>  
>  /* Primary plane formats for gen <= 3 */
>  static const uint32_t i8xx_primary_formats[] = {
> @@ -14673,6 +14674,8 @@ void intel_modeset_init(struct drm_device *dev)
>  	if (INTEL_INFO(dev)->num_pipes == 0)
>  		return;
>  
> +	intel_color_manager_init(dev);
> +
>  	intel_init_display(dev);
>  	intel_init_audio(dev);
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3b4d8a4..2a75d7d 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1176,6 +1176,10 @@ struct drm_mode_config {
>  	struct drm_property *suggested_x_property;
>  	struct drm_property *suggested_y_property;
>  
> +	/* Color Management Properties */
> +	struct drm_property *gamma_property;
> +	struct drm_property *csc_property;
> +

I notice you're adding these to a core DRM structure; is the expectation
that other drivers will want to re-use these properties for their own
color correction functionality?

If the answer is yes, you'll definitely need to add documentation for
the property to Documentation/DocBook/drm.tmpl which will get compiled
into documentation like you see at
http://people.freedesktop.org/~danvet/drm/drm-kms-properties.html#idp59563120
(actually you'll want to add that documentation even if it's an
i915-specific property, but it's especially important for anything we
expect to be reusable by other drivers).

If the answer is no, and you expect this to be i915-specific for the
foreseeable future, then stashing the property in dev_priv might be a
better choice to avoid growing the driver-independent structures.


Matt

>  	/* dumb ioctl parameters */
>  	uint32_t preferred_depth, prefer_shadow;
>  
> -- 
> 2.4.2
> 

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

Re: [PATCH v2 01/10] drm/i915: Initialize Color Manager

[Sharma, Shashank]

Thanks for your time and review Matt.
Please find my comments inline

Regards
Shashank
On 6/6/2015 6:30 AM, Matt Roper wrote:
> On Thu, Jun 04, 2015 at 07:12:32PM +0530, Kausal Malladi wrote:
>> From: Kausal Malladi <Kausal.Malladi@intel.com>
>>
>> Color Manager is an extension in i915 driver to handle color correction
>> and enhancements across various Intel platforms.
>>
>> This patch initializes color manager framework by :
>> 1. Adding two new files, intel_color_manager(.c/.h)
>> 2. Introducing new pointers in DRM mode_config structure to
>>     carry CSC and Gamma color correction properties.
>> 3. Creating these DRM properties in Color Manager initialization
>>     sequence.
>>
>> v2: Addressing Sonika's review comment.
>> 1. Made intel_color_manager_init void
>> 2. Moved init after NUM_PIPES check
>>
>> 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 | 48 ++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_color_manager.h | 32 ++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_display.c       |  3 ++
>>   include/drm/drm_crtc.h                     |  4 +++
>>   5 files changed, 90 insertions(+)
>>   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 b7ddf48..c62d048 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -89,6 +89,9 @@ i915-y += i915_vgpu.o
>>   # legacy horrors
>>   i915-y += i915_dma.o
>>
>> +# Color Management
>> +i915-y += intel_color_manager.o
>> +
>>   obj-$(CONFIG_DRM_I915)  += i915.o
>>
>>   CFLAGS_i915_trace_points.o := -I$(src)
>> 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..f7e2380
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -0,0 +1,48 @@
>> +/*
>> + * 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_init(struct drm_device *dev)
>> +{
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +
>> +	/* Create Gamma and CSC properties */
>> +	config->gamma_property = drm_property_create(dev,
>> +		DRM_MODE_PROP_BLOB, "gamma_property", 0);
>> +	if (!config->gamma_property)
>> +		DRM_ERROR("Gamma property creation failed\n");
>> +	else
>> +		DRM_DEBUG_DRIVER("Created Gamma property\n");
>> +
>> +	config->csc_property = drm_property_create(dev,
>> +			DRM_MODE_PROP_BLOB, "csc_property", 0);
>> +	if (!config->csc_property)
>> +		DRM_ERROR("CSC property creation failed\n");
>> +	else
>> +		DRM_DEBUG_DRIVER("Created CSC property\n");
>> +}
>> 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..154bf16
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
>> @@ -0,0 +1,32 @@
>> +/*
>> + * 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>
>> +
>> +/* Generic Function prototypes */
>> +void intel_color_manager_init(struct drm_device *dev);
>
> While I personally don't mind creating a new header for prototypes, most
> of our other KMS-related stuff just gets thrown in intel_drv.h under a
> comment like "/* intel_foobar.c */" so this is a little inconsistent.
> Maybe we should keep these prototypes there as well since that's the
> file most developers are going to look for these in out of habit?
>
Yes sure. Actually there are a lot of macros coming up in the next set 
of patches, which are only specific to color management (CSC gamma hue, 
saturation, brightness and contrast), which creates a necessity of a new 
header file, so we though why don't we put
the prototypes also here. But if you think that we should move it, we 
can very well do that.
>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 067b1de..2322dee 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -44,6 +44,7 @@
>>   #include <drm/drm_plane_helper.h>
>>   #include <drm/drm_rect.h>
>>   #include <linux/dma_remapping.h>
>> +#include "intel_color_manager.h"
>>
>>   /* Primary plane formats for gen <= 3 */
>>   static const uint32_t i8xx_primary_formats[] = {
>> @@ -14673,6 +14674,8 @@ void intel_modeset_init(struct drm_device *dev)
>>   	if (INTEL_INFO(dev)->num_pipes == 0)
>>   		return;
>>
>> +	intel_color_manager_init(dev);
>> +
>>   	intel_init_display(dev);
>>   	intel_init_audio(dev);
>>
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 3b4d8a4..2a75d7d 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -1176,6 +1176,10 @@ struct drm_mode_config {
>>   	struct drm_property *suggested_x_property;
>>   	struct drm_property *suggested_y_property;
>>
>> +	/* Color Management Properties */
>> +	struct drm_property *gamma_property;
>> +	struct drm_property *csc_property;
>> +
>
> I notice you're adding these to a core DRM structure; is the expectation
> that other drivers will want to re-use these properties for their own
> color correction functionality?
>
> If the answer is yes, you'll definitely need to add documentation for
> the property to Documentation/DocBook/drm.tmpl which will get compiled
> into documentation like you see at
> http://people.freedesktop.org/~danvet/drm/drm-kms-properties.html#idp59563120
> (actually you'll want to add that documentation even if it's an
> i915-specific property, but it's especially important for anything we
> expect to be reusable by other drivers).
>
> If the answer is no, and you expect this to be i915-specific for the
> foreseeable future, then stashing the property in dev_priv might be a
> better choice to avoid growing the driver-independent structures.
>
Well, actually the intention is to have a generic core property which 
can encourage other drivers also, to use this interface. We will update 
the documentation accordingly.
>
> Matt
>
>>   	/* dumb ioctl parameters */
>>   	uint32_t preferred_depth, prefer_shadow;
>>
>> --
>> 2.4.2
>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 01/10] drm/i915: Initialize Color Manager

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:32PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> Color Manager is an extension in i915 driver to handle color correction
> and enhancements across various Intel platforms.
> 
> This patch initializes color manager framework by :
> 1. Adding two new files, intel_color_manager(.c/.h)
> 2. Introducing new pointers in DRM mode_config structure to
>    carry CSC and Gamma color correction properties.
> 3. Creating these DRM properties in Color Manager initialization
>    sequence.
> 
> v2: Addressing Sonika's review comment.
> 1. Made intel_color_manager_init void
> 2. Moved init after NUM_PIPES check
> 
> 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 | 48 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color_manager.h | 32 ++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c       |  3 ++
>  include/drm/drm_crtc.h                     |  4 +++
>  5 files changed, 90 insertions(+)
>  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 b7ddf48..c62d048 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -89,6 +89,9 @@ i915-y += i915_vgpu.o
>  # legacy horrors
>  i915-y += i915_dma.o
>  
> +# Color Management
> +i915-y += intel_color_manager.o
> +

FWIW, I'd put this in the "# modesetting core code" section. The objects
are somewhat sorted by big categories, having a category saying that
"intel_color_manager.c" is for "Color Management" is, well, not that
useful :)

>  obj-$(CONFIG_DRM_I915)  += i915.o
>  
>  CFLAGS_i915_trace_points.o := -I$(src)
> 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..f7e2380
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -0,0 +1,48 @@
> +/*
> + * 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_init(struct drm_device *dev)

This function create "generic" properties (more on that in the cover
letter) in drm_mode_config. It looks like it should be part of the DRM
core, not i915?

> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	/* Create Gamma and CSC properties */
> +	config->gamma_property = drm_property_create(dev,
> +		DRM_MODE_PROP_BLOB, "gamma_property", 0);

You are creating properties with the names "gamma_property",
"csc_property". While we do have (and that's quite unfortunate) a
variety of naming schemes for properties none of them include the
_property suffix. Just "gamma" and "csc" would be more appropriate. See
the current list of properties:

https://www.kernel.org/doc/htmldocs/drm/drm-kms-properties.html#idp13803344

Speaking of which, you also need to add the property to the
documentation (Documentation/DocBook/drm.xml):

> +	if (!config->gamma_property)
> +		DRM_ERROR("Gamma property creation failed\n");
> +	else
> +		DRM_DEBUG_DRIVER("Created Gamma property\n");

That's really too verbose on the logs, I don't think you need any of
them. It's easy enough to check if the property was created, it's part
of the API.
> +
> +	config->csc_property = drm_property_create(dev,
> +			DRM_MODE_PROP_BLOB, "csc_property", 0);

"CSC" is quite specific to how Intel calls the unit doing the 3x3 matrix
transform on colors, maybe we could be more generic there?
"color-matrix"? This also depends if we want to try to create somewhat
generic properties or not (if not, maybe we should prefix the properties
with "i915-")

> +	if (!config->csc_property)
> +		DRM_ERROR("CSC property creation failed\n");
> +	else
> +		DRM_DEBUG_DRIVER("Created CSC property\n");
> +}
> 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..154bf16
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -0,0 +1,32 @@
> +/*
> + * 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>
> +
> +/* Generic Function prototypes */
> +void intel_color_manager_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 067b1de..2322dee 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -44,6 +44,7 @@
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_rect.h>
>  #include <linux/dma_remapping.h>
> +#include "intel_color_manager.h"
>  
>  /* Primary plane formats for gen <= 3 */
>  static const uint32_t i8xx_primary_formats[] = {
> @@ -14673,6 +14674,8 @@ void intel_modeset_init(struct drm_device *dev)
>  	if (INTEL_INFO(dev)->num_pipes == 0)
>  		return;
>  
> +	intel_color_manager_init(dev);
> +
>  	intel_init_display(dev);
>  	intel_init_audio(dev);
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3b4d8a4..2a75d7d 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1176,6 +1176,10 @@ struct drm_mode_config {
>  	struct drm_property *suggested_x_property;
>  	struct drm_property *suggested_y_property;
>  
> +	/* Color Management Properties */
> +	struct drm_property *gamma_property;
> +	struct drm_property *csc_property;
> +
>  	/* dumb ioctl parameters */
>  	uint32_t preferred_depth, prefer_shadow;
>  
> -- 
> 2.4.2
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 01/10] drm/i915: Initialize Color Manager

[Damien Lespiau]

On Tue, Jun 09, 2015 at 11:34:44AM +0100, Damien Lespiau wrote:
> "CSC" is quite specific to how Intel calls the unit doing the 3x3 matrix
> transform on colors, maybe we could be more generic there?
> "color-matrix"? This also depends if we want to try to create somewhat
> generic properties or not (if not, maybe we should prefix the properties
> with "i915-")

Actually, given the list of current properties "color-matrix" would look
out of place (but seems like a good property name if you're doing a bit
of CSS). People seem to go with either:

  - "COLOR_MATRIX", please don't :(
  - "color_matrix"
  - "color matrix"
  - "Color matrix"

Oh well, naming rat hole...

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

Re: [PATCH v2 02/10] drm/i915: Attach color properties to CRTC

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:33PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> Every CRTC would be carrying its own instances of CSC and Gamma color
> correction values. This patch adds a new function to attach color
> properties to respective CRTCs while initialization.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color_manager.c | 24 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color_manager.h |  2 ++
>  drivers/gpu/drm/i915/intel_display.c       |  3 +++
>  3 files changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> index f7e2380..8d4ee8f 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,6 +27,30 @@
>  
>  #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;
> +
> +	/* Attach all properties generic to crtc and plane */
> +	if (config->gamma_property) {
> +		drm_object_attach_property(mode_obj,
> +				config->gamma_property, 0);

No platform check? I doubt all of our platforms have LUTs on all planes.
Ah but we don't attach this property to any plane just yet, so the
comment is misleading.

> +
> +		DRM_DEBUG_DRIVER("Initialized gamma property\n");

That looks too verbose to me.

> +	}
> +
> +	/* Attach properties specific to crtc only */
> +	if (mode_obj->type == DRM_MODE_OBJECT_CRTC) {
> +		if (config->csc_property) {
> +			drm_object_attach_property(mode_obj,
> +					config->csc_property, 0);
> +
> +			DRM_DEBUG_DRIVER("Initialized CSC property\n");
> +		}
> +	}
> +}
> +
>  void intel_color_manager_init(struct drm_device *dev)
>  {
>  	struct drm_mode_config *config = &dev->mode_config;
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
> index 154bf16..a55ce23 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.h
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -30,3 +30,5 @@
>  
>  /* Generic Function prototypes */
>  void intel_color_manager_init(struct drm_device *dev);
> +void intel_color_manager_attach(struct drm_device *dev,
> +		struct drm_mode_object *mode_obj);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2322dee..d4e9aa3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13794,6 +13794,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	intel_crtc->cursor_cntl = ~0;
>  	intel_crtc->cursor_size = ~0;
>  
> +	/* Attaching color properties to the CRTC */
> +	intel_color_manager_attach(dev, &intel_crtc->base.base);
> +
>  	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;
> -- 
> 2.4.2
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Jindal, Sonika]

On 6/4/2015 7:12 PM, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
>
> 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
> 3. Adds drm_mode_crtc_update_color_property function, which replaces the
>     old blob for the property with the new one
> 4. Adds a pointer to hold blob for Gamma property in drm_crtc
>
> v2: Addressed all review comments from Sonika and Daniel Stone.
Instead you can mention the changes briefly.

>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_atomic.c        |   6 ++
>   drivers/gpu/drm/i915/intel_color_manager.c | 161 +++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_color_manager.h |  62 +++++++++++
>   3 files changed, 229 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index b5c78d8..4726847 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -428,6 +428,12 @@ 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->gamma_property)
> +		return intel_color_manager_set_gamma(dev, &crtc->base, val);
> +
>   	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>   	return -EINVAL;
>   }
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> index 8d4ee8f..421c267 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,6 +27,167 @@
>
>   #include "intel_color_manager.h"
>
> +int chv_set_gamma(struct drm_device *dev, uint32_t blob_id,
> +		struct drm_crtc *crtc)
> +{
> +	struct drm_gamma *gamma_data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_property_blob *blob;
> +	struct drm_mode_config *config = &dev->mode_config;
> +	u32 cgm_control_reg = 0;
> +	u32 cgm_gamma_reg = 0;
> +	u32 reg, val;
> +	enum pipe pipe;
> +	u16 red, green, blue;
> +	struct rgb_pixel correct_rgb;
> +	u32 count = 0;
> +	struct rgb_pixel *correction_values = NULL;
> +	u32 num_samples;
> +	u32 word;
> +	u32 palette;
> +	int ret = 0, length;
> +
> +	blob = drm_property_lookup_blob(dev, blob_id);
> +	if (!blob) {
> +		DRM_ERROR("Invalid Blob ID\n");
> +		return -EINVAL;
> +	}
> +
> +	gamma_data = (struct drm_gamma *)blob->data;
> +	pipe = to_intel_crtc(crtc)->pipe;
> +	num_samples = gamma_data->num_samples;
> +	length = num_samples * sizeof(struct rgb_pixel);
> +
> +	if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_UNKNOWN) {
Switch/case instead?

Also, is UNKNOWN for disabling? Why not rename it to DISABLE then?
> +
> +		/* Disable Gamma functionality on Pipe - CGM Block */
> +		cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
> +		cgm_control_reg &= ~CGM_GAMMA_EN;
> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
> +
> +		DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
> +				pipe_name(pipe));
> +		ret = 0;
> +	} else if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_LEGACY) {
> +
> +		if (num_samples != CHV_8BIT_GAMMA_MAX_VALS) {
> +			DRM_ERROR("Incorrect number of samples received\n");
> +			return -EINVAL;
> +		}
> +
> +		/* First, disable CGM Gamma, if already set */
> +		cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
> +		cgm_control_reg &= ~CGM_GAMMA_EN;
> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
> +
> +		/* Enable (Legacy) Gamma on Pipe */
> +		palette = _PIPE_GAMMA_BASE(pipe);
> +
> +		count = 0;
> +		correction_values = (struct rgb_pixel *)&gamma_data->values;
> +		while (count < num_samples) {
> +			correct_rgb = correction_values[count];
> +			blue = correction_values[count].blue;
> +			green = correction_values[count].green;
> +			red = correction_values[count].red;
> +
> +			blue = blue >> CHV_8BIT_GAMMA_MSB_SHIFT;
> +			green = green >> CHV_8BIT_GAMMA_MSB_SHIFT;
> +			red = red >> CHV_8BIT_GAMMA_MSB_SHIFT;
> +
> +			/* Red (23:16), Green (15:8), Blue (7:0) */
> +			word = (red << CHV_8BIT_GAMMA_SHIFT_RED_REG) |
> +				(green <<
> +				 CHV_8BIT_GAMMA_SHIFT_GREEN_REG) |
> +				blue;
> +			I915_WRITE(palette, word);
> +
> +			palette += 4;
> +			count++;
> +		}
> +		reg = PIPECONF(pipe);
> +		val = I915_READ(reg) | PIPECONF_GAMMA;
> +		I915_WRITE(reg, val);
> +
> +		DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n",
> +			pipe_name(pipe));
> +		ret = 0;
> +	} else if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_10BIT) {
> +
> +		if (num_samples != CHV_10BIT_GAMMA_MAX_VALS) {
> +			DRM_ERROR("Incorrect number of samples received\n");
> +			return -EINVAL;
> +		}
> +
> +		/* Enable (CGM) Gamma on Pipe */
> +		cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
> +
> +		count = 0;
> +		correction_values = (struct rgb_pixel *)&gamma_data->values;
> +		while (count < num_samples) {
> +			correct_rgb = correction_values[count];
> +			blue = correction_values[count].blue;
> +			green = correction_values[count].green;
> +			red = correction_values[count].red;
> +
> +			blue = blue >> CHV_10BIT_GAMMA_MSB_SHIFT;
> +			green = green >> CHV_10BIT_GAMMA_MSB_SHIFT;
> +			red = red >> CHV_10BIT_GAMMA_MSB_SHIFT;
> +
> +			/* Green (25:16) and Blue (9:0) to be written */
> +			word = (green << CHV_GAMMA_SHIFT_GREEN) | blue;
> +			I915_WRITE(cgm_gamma_reg, word);
> +			cgm_gamma_reg += 4;
> +
> +			/* Red (9:0) to be written */
> +			word = red;
> +			I915_WRITE(cgm_gamma_reg, word);
> +
> +			cgm_gamma_reg += 4;
> +			count++;
> +		}
> +
> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe),
> +			I915_READ(_PIPE_CGM_CONTROL(pipe))
> +			| CGM_GAMMA_EN);
> +
> +		DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n",
> +			pipe_name(pipe));
> +
> +		ret = 0;
> +	} else {
> +		DRM_ERROR("Invalid gamma_level received\n");
> +		return -EFAULT;
> +	}
> +
> +	ret = drm_mode_crtc_update_color_property(&blob, length,
> +			(void *) gamma_data, &crtc->base,
> +			config->gamma_property);
> +
> +	if (ret) {
> +		DRM_ERROR("Error updating Gamma blob\n");
> +		crtc->gamma_blob_id = INVALID_BLOB_ID;
> +		return -EFAULT;
> +	}
> +
> +	/* Save blob ID for future use */
> +	crtc->gamma_blob_id = blob->base.id;
Do you have a patch which uses this blob_id. Still not sure why will it 
be used in get_property, when the user will send the blob property and 
from that we can get the blob_id (blob->base.id)

> +	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);
> +
> +	DRM_DEBUG_DRIVER("\n");
> +
> +	if (IS_CHERRYVIEW(dev))
> +		return chv_set_gamma(dev, blob_id, crtc);
> +
> +	return -EINVAL;
> +}
> +
>   void intel_color_manager_attach(struct drm_device *dev,
>   		struct drm_mode_object *mode_obj)
>   {
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
> index a55ce23..0acf8e9 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.h
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -27,8 +27,70 @@
>
>   #include <drm/drmP.h>
>   #include <drm/drm_crtc_helper.h>
> +#include "i915_drv.h"
> +
> +/* Color Management macros for Gamma */
> +#define I915_GAMMA_FLAG_DEGAMMA			(1 << 0)
> +#define I915_PIPE_GAMMA				(1 << 0)
> +#define I915_PLANE_GAMMA			(1 << 1)
> +#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)
> +
> +#define CHV_MAX_PIPES				3
I915_MAX_PIPES ?
> +#define CHV_DISPLAY_BASE			0x180000
Use VLV_DISPLAY_BASE

> +#define INVALID_BLOB_ID				9999
> +
> +struct rgb_pixel {
> +	u16 red;
> +	u16 green;
> +	u16 blue;
> +};
> +
> +/* CHV CGM Block */
> +/* Bit 2 to be enabled in CGM block for CHV */
> +#define CGM_GAMMA_EN				4
> +
> +/* Gamma */
> +#define CHV_GAMMA_VALS				257
> +#define CHV_10BIT_GAMMA_MAX_INDEX		256
> +#define CHV_8BIT_GAMMA_MAX_INDEX		255
> +#define CHV_10BIT_GAMMA_MSB_SHIFT		6
> +#define CHV_GAMMA_EVEN_MASK			0xFF
> +#define CHV_GAMMA_SHIFT_BLUE			0
> +#define CHV_GAMMA_SHIFT_GREEN			16
> +#define CHV_GAMMA_SHIFT_RED			0
> +#define CHV_GAMMA_ODD_SHIFT			8
> +#define CHV_CLRMGR_GAMMA_GCMAX_SHIFT		17
> +#define CHV_GAMMA_GCMAX_MASK			0x1FFFF
> +#define CHV_GAMMA_GCMAX_MAX			0x400
> +#define CHV_10BIT_GAMMA_MAX_VALS		(CHV_10BIT_GAMMA_MAX_INDEX + 1)
> +#define CHV_8BIT_GAMMA_MAX_VALS			(CHV_8BIT_GAMMA_MAX_INDEX + 1)
> +#define CHV_8BIT_GAMMA_MSB_SHIFT		8
> +#define CHV_8BIT_GAMMA_SHIFT_GREEN_REG		8
> +#define CHV_8BIT_GAMMA_SHIFT_RED_REG		16
> +
> +/* CGM Registers */
> +#define CGM_OFFSET				0x2000
> +#define GAMMA_OFFSET				0x2000
> +#define PIPEA_CGM_CONTROL			(CHV_DISPLAY_BASE + 0x67A00)
> +#define PIPEA_CGM_GAMMA_MIN			(CHV_DISPLAY_BASE + 0x67000)
> +#define _PIPE_CGM_CONTROL(pipe) \
> +	(PIPEA_CGM_CONTROL + (pipe * CGM_OFFSET))
> +#define _PIPE_GAMMA_BASE(pipe) \
> +	(PIPEA_CGM_GAMMA_MIN + (pipe * GAMMA_OFFSET))
>
I think it makes more sense to put them in i915_reg.h.

>   /* Generic Function prototypes */
>   void intel_color_manager_init(struct drm_device *dev);
>   void intel_color_manager_attach(struct drm_device *dev,
>   		struct drm_mode_object *mode_obj);
> +extern int intel_color_manager_set_gamma(struct drm_device *dev,
> +		struct drm_mode_object *obj, uint32_t blob_id);
> +
> +/* Platform specific function prototypes */
> +extern int chv_set_gamma(struct drm_device *dev,
> +		uint32_t blob_id, struct drm_crtc *crtc);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Sharma, Shashank]

Regards
Shashank

On 6/6/2015 11:03 AM, Jindal, Sonika wrote:
>
>
> On 6/4/2015 7:12 PM, Kausal Malladi wrote:
>> From: Kausal Malladi <Kausal.Malladi@intel.com>
>>
>> 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
>> 3. Adds drm_mode_crtc_update_color_property function, which replaces the
>>     old blob for the property with the new one
>> 4. Adds a pointer to hold blob for Gamma property in drm_crtc
>>
>> v2: Addressed all review comments from Sonika and Daniel Stone.
> Instead you can mention the changes briefly.
>
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_atomic.c        |   6 ++
>>   drivers/gpu/drm/i915/intel_color_manager.c | 161
>> +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_color_manager.h |  62 +++++++++++
>>   3 files changed, 229 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index b5c78d8..4726847 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -428,6 +428,12 @@ 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->gamma_property)
>> +        return intel_color_manager_set_gamma(dev, &crtc->base, val);
>> +
>>       DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>>       return -EINVAL;
>>   }
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c
>> b/drivers/gpu/drm/i915/intel_color_manager.c
>> index 8d4ee8f..421c267 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -27,6 +27,167 @@
>>
>>   #include "intel_color_manager.h"
>>
>> +int chv_set_gamma(struct drm_device *dev, uint32_t blob_id,
>> +        struct drm_crtc *crtc)
>> +{
>> +    struct drm_gamma *gamma_data;
>> +    struct drm_i915_private *dev_priv = dev->dev_private;
>> +    struct drm_property_blob *blob;
>> +    struct drm_mode_config *config = &dev->mode_config;
>> +    u32 cgm_control_reg = 0;
>> +    u32 cgm_gamma_reg = 0;
>> +    u32 reg, val;
>> +    enum pipe pipe;
>> +    u16 red, green, blue;
>> +    struct rgb_pixel correct_rgb;
>> +    u32 count = 0;
>> +    struct rgb_pixel *correction_values = NULL;
>> +    u32 num_samples;
>> +    u32 word;
>> +    u32 palette;
>> +    int ret = 0, length;
>> +
>> +    blob = drm_property_lookup_blob(dev, blob_id);
>> +    if (!blob) {
>> +        DRM_ERROR("Invalid Blob ID\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    gamma_data = (struct drm_gamma *)blob->data;
>> +    pipe = to_intel_crtc(crtc)->pipe;
>> +    num_samples = gamma_data->num_samples;
>> +    length = num_samples * sizeof(struct rgb_pixel);
>> +
>> +    if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_UNKNOWN) {
> Switch/case instead?
Yep, got it.
>
> Also, is UNKNOWN for disabling? Why not rename it to DISABLE then?
Actually unknown is valid in case of get_property() when we want to 
query about the capabilities, just want to reuse the same, to avoid need 
for another one. Else we have to handle one extra case in each get_prop 
(disable) and set_prop(unknown)
>> +
>> +        /* Disable Gamma functionality on Pipe - CGM Block */
>> +        cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
>> +        cgm_control_reg &= ~CGM_GAMMA_EN;
>> +        I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
>> +
>> +        DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
>> +                pipe_name(pipe));
>> +        ret = 0;
>> +    } else if (gamma_data->gamma_precision ==
>> I915_GAMMA_PRECISION_LEGACY) {
>> +
>> +        if (num_samples != CHV_8BIT_GAMMA_MAX_VALS) {
>> +            DRM_ERROR("Incorrect number of samples received\n");
>> +            return -EINVAL;
>> +        }
>> +
>> +        /* First, disable CGM Gamma, if already set */
>> +        cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
>> +        cgm_control_reg &= ~CGM_GAMMA_EN;
>> +        I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
>> +
>> +        /* Enable (Legacy) Gamma on Pipe */
>> +        palette = _PIPE_GAMMA_BASE(pipe);
>> +
>> +        count = 0;
>> +        correction_values = (struct rgb_pixel *)&gamma_data->values;
>> +        while (count < num_samples) {
>> +            correct_rgb = correction_values[count];
>> +            blue = correction_values[count].blue;
>> +            green = correction_values[count].green;
>> +            red = correction_values[count].red;
>> +
>> +            blue = blue >> CHV_8BIT_GAMMA_MSB_SHIFT;
>> +            green = green >> CHV_8BIT_GAMMA_MSB_SHIFT;
>> +            red = red >> CHV_8BIT_GAMMA_MSB_SHIFT;
>> +
>> +            /* Red (23:16), Green (15:8), Blue (7:0) */
>> +            word = (red << CHV_8BIT_GAMMA_SHIFT_RED_REG) |
>> +                (green <<
>> +                 CHV_8BIT_GAMMA_SHIFT_GREEN_REG) |
>> +                blue;
>> +            I915_WRITE(palette, word);
>> +
>> +            palette += 4;
>> +            count++;
>> +        }
>> +        reg = PIPECONF(pipe);
>> +        val = I915_READ(reg) | PIPECONF_GAMMA;
>> +        I915_WRITE(reg, val);
>> +
>> +        DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n",
>> +            pipe_name(pipe));
>> +        ret = 0;
>> +    } else if (gamma_data->gamma_precision ==
>> I915_GAMMA_PRECISION_10BIT) {
>> +
>> +        if (num_samples != CHV_10BIT_GAMMA_MAX_VALS) {
>> +            DRM_ERROR("Incorrect number of samples received\n");
>> +            return -EINVAL;
>> +        }
>> +
>> +        /* Enable (CGM) Gamma on Pipe */
>> +        cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
>> +
>> +        count = 0;
>> +        correction_values = (struct rgb_pixel *)&gamma_data->values;
>> +        while (count < num_samples) {
>> +            correct_rgb = correction_values[count];
>> +            blue = correction_values[count].blue;
>> +            green = correction_values[count].green;
>> +            red = correction_values[count].red;
>> +
>> +            blue = blue >> CHV_10BIT_GAMMA_MSB_SHIFT;
>> +            green = green >> CHV_10BIT_GAMMA_MSB_SHIFT;
>> +            red = red >> CHV_10BIT_GAMMA_MSB_SHIFT;
>> +
>> +            /* Green (25:16) and Blue (9:0) to be written */
>> +            word = (green << CHV_GAMMA_SHIFT_GREEN) | blue;
>> +            I915_WRITE(cgm_gamma_reg, word);
>> +            cgm_gamma_reg += 4;
>> +
>> +            /* Red (9:0) to be written */
>> +            word = red;
>> +            I915_WRITE(cgm_gamma_reg, word);
>> +
>> +            cgm_gamma_reg += 4;
>> +            count++;
>> +        }
>> +
>> +        I915_WRITE(_PIPE_CGM_CONTROL(pipe),
>> +            I915_READ(_PIPE_CGM_CONTROL(pipe))
>> +            | CGM_GAMMA_EN);
>> +
>> +        DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n",
>> +            pipe_name(pipe));
>> +
>> +        ret = 0;
>> +    } else {
>> +        DRM_ERROR("Invalid gamma_level received\n");
>> +        return -EFAULT;
>> +    }
>> +
>> +    ret = drm_mode_crtc_update_color_property(&blob, length,
>> +            (void *) gamma_data, &crtc->base,
>> +            config->gamma_property);
>> +
>> +    if (ret) {
>> +        DRM_ERROR("Error updating Gamma blob\n");
>> +        crtc->gamma_blob_id = INVALID_BLOB_ID;
>> +        return -EFAULT;
>> +    }
>> +
>> +    /* Save blob ID for future use */
>> +    crtc->gamma_blob_id = blob->base.id;
> Do you have a patch which uses this blob_id. Still not sure why will it
> be used in get_property, when the user will send the blob property and
> from that we can get the blob_id (blob->base.id)
>
Yes, the blob id will be used for any future reference of the blob, one 
of which is get_prop()
>> +    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);
>> +
>> +    DRM_DEBUG_DRIVER("\n");
>> +
>> +    if (IS_CHERRYVIEW(dev))
>> +        return chv_set_gamma(dev, blob_id, crtc);
>> +
>> +    return -EINVAL;
>> +}
>> +
>>   void intel_color_manager_attach(struct drm_device *dev,
>>           struct drm_mode_object *mode_obj)
>>   {
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h
>> b/drivers/gpu/drm/i915/intel_color_manager.h
>> index a55ce23..0acf8e9 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.h
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
>> @@ -27,8 +27,70 @@
>>
>>   #include <drm/drmP.h>
>>   #include <drm/drm_crtc_helper.h>
>> +#include "i915_drv.h"
>> +
>> +/* Color Management macros for Gamma */
>> +#define I915_GAMMA_FLAG_DEGAMMA            (1 << 0)
>> +#define I915_PIPE_GAMMA                (1 << 0)
>> +#define I915_PLANE_GAMMA            (1 << 1)
>> +#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)
>> +
>> +#define CHV_MAX_PIPES                3
> I915_MAX_PIPES ?
>> +#define CHV_DISPLAY_BASE            0x180000
> Use VLV_DISPLAY_BASE
Got it.
>
>> +#define INVALID_BLOB_ID                9999
>> +
>> +struct rgb_pixel {
>> +    u16 red;
>> +    u16 green;
>> +    u16 blue;
>> +};
>> +
>> +/* CHV CGM Block */
>> +/* Bit 2 to be enabled in CGM block for CHV */
>> +#define CGM_GAMMA_EN                4
>> +
>> +/* Gamma */
>> +#define CHV_GAMMA_VALS                257
>> +#define CHV_10BIT_GAMMA_MAX_INDEX        256
>> +#define CHV_8BIT_GAMMA_MAX_INDEX        255
>> +#define CHV_10BIT_GAMMA_MSB_SHIFT        6
>> +#define CHV_GAMMA_EVEN_MASK            0xFF
>> +#define CHV_GAMMA_SHIFT_BLUE            0
>> +#define CHV_GAMMA_SHIFT_GREEN            16
>> +#define CHV_GAMMA_SHIFT_RED            0
>> +#define CHV_GAMMA_ODD_SHIFT            8
>> +#define CHV_CLRMGR_GAMMA_GCMAX_SHIFT        17
>> +#define CHV_GAMMA_GCMAX_MASK            0x1FFFF
>> +#define CHV_GAMMA_GCMAX_MAX            0x400
>> +#define CHV_10BIT_GAMMA_MAX_VALS        (CHV_10BIT_GAMMA_MAX_INDEX + 1)
>> +#define CHV_8BIT_GAMMA_MAX_VALS            (CHV_8BIT_GAMMA_MAX_INDEX
>> + 1)
>> +#define CHV_8BIT_GAMMA_MSB_SHIFT        8
>> +#define CHV_8BIT_GAMMA_SHIFT_GREEN_REG        8
>> +#define CHV_8BIT_GAMMA_SHIFT_RED_REG        16
>> +
>> +/* CGM Registers */
>> +#define CGM_OFFSET                0x2000
>> +#define GAMMA_OFFSET                0x2000
>> +#define PIPEA_CGM_CONTROL            (CHV_DISPLAY_BASE + 0x67A00)
>> +#define PIPEA_CGM_GAMMA_MIN            (CHV_DISPLAY_BASE + 0x67000)
>> +#define _PIPE_CGM_CONTROL(pipe) \
>> +    (PIPEA_CGM_CONTROL + (pipe * CGM_OFFSET))
>> +#define _PIPE_GAMMA_BASE(pipe) \
>> +    (PIPEA_CGM_GAMMA_MIN + (pipe * GAMMA_OFFSET))
>>
> I think it makes more sense to put them in i915_reg.h.
Yes, will move this.
>
>>   /* Generic Function prototypes */
>>   void intel_color_manager_init(struct drm_device *dev);
>>   void intel_color_manager_attach(struct drm_device *dev,
>>           struct drm_mode_object *mode_obj);
>> +extern int intel_color_manager_set_gamma(struct drm_device *dev,
>> +        struct drm_mode_object *obj, uint32_t blob_id);
>> +
>> +/* Platform specific function prototypes */
>> +extern int chv_set_gamma(struct drm_device *dev,
>> +        uint32_t blob_id, struct drm_crtc *crtc);
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Damien Lespiau]

On Sat, Jun 06, 2015 at 05:39:23PM +0530, Sharma, Shashank wrote:
> >>+    if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_UNKNOWN) {
> >Switch/case instead?
> Yep, got it.
> >
> >Also, is UNKNOWN for disabling? Why not rename it to DISABLE then?
> Actually unknown is valid in case of get_property() when we want to query
> about the capabilities, just want to reuse the same, to avoid need for
> another one. Else we have to handle one extra case in each get_prop
> (disable) and set_prop(unknown)

Is it? the code isn't telling that story, at least in the current form.

get_property() should primarily be for retrieving the current value of
that property, so I'd suggest having a precision field of 0 means
what is CURRENT today (that'd be the default expected behaviour of
get_property()).

Then, 2 other "needs":

 - Query interface: how useful is the query interface in this present
   form? a query for the precision only isn't that useful as we need
   per-platform knowledge beyond that anyway (say we can't encode things
   like split gamma configurations sharing the same LUT storage)

 - Some other value to disable the function (set_property() with
   precision = 0 looks fine to me.

The get_property() path is missing from this patch set AFAICT.

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

Re: [PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Damien Lespiau]

(Note I haven't actually looked at the CHV specific details just yet,
that'll be for another pass).

On Thu, Jun 04, 2015 at 07:12:38PM +0530, Kausal Malladi wrote:
> +int chv_set_gamma(struct drm_device *dev, uint32_t blob_id,
> +		struct drm_crtc *crtc)
> +{
> +	struct drm_gamma *gamma_data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_property_blob *blob;
> +	struct drm_mode_config *config = &dev->mode_config;
> +	u32 cgm_control_reg = 0;
> +	u32 cgm_gamma_reg = 0;
> +	u32 reg, val;
> +	enum pipe pipe;
> +	u16 red, green, blue;
> +	struct rgb_pixel correct_rgb;
> +	u32 count = 0;
> +	struct rgb_pixel *correction_values = NULL;
> +	u32 num_samples;
> +	u32 word;
> +	u32 palette;
> +	int ret = 0, length;
> +
> +	blob = drm_property_lookup_blob(dev, blob_id);
> +	if (!blob) {
> +		DRM_ERROR("Invalid Blob ID\n");

Not DRM_ERROR. We do give -EINVAL hint for people developping user
space, but with DRM_DEBUG_KMS().

> +		return -EINVAL;
> +	}
> +
> +	gamma_data = (struct drm_gamma *)blob->data;
> +	pipe = to_intel_crtc(crtc)->pipe;
> +	num_samples = gamma_data->num_samples;
> +	length = num_samples * sizeof(struct rgb_pixel);
> +
> +	if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_UNKNOWN) {
> +
> +		/* Disable Gamma functionality on Pipe - CGM Block */
> +		cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
> +		cgm_control_reg &= ~CGM_GAMMA_EN;
> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
> +
> +		DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
> +				pipe_name(pipe));
> +		ret = 0;
> +	} else if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_LEGACY) {
> +
> +		if (num_samples != CHV_8BIT_GAMMA_MAX_VALS) {
> +			DRM_ERROR("Incorrect number of samples received\n");

Not DRM_ERROR. We do give -EINVAL hint for people developping user
space, but with DRM_DEBUG_KMS().

> +			return -EINVAL;
> +		}

This means the current code doesn't allow us to load LUTs with 256
values (like today). That's not what we want.

Have you looked at the interactions between this property and the legacy
gamma ioctl()?

> +
> +		/* First, disable CGM Gamma, if already set */
> +		cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
> +		cgm_control_reg &= ~CGM_GAMMA_EN;
> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
> +
> +		/* Enable (Legacy) Gamma on Pipe */
> +		palette = _PIPE_GAMMA_BASE(pipe);
> +
> +		count = 0;
> +		correction_values = (struct rgb_pixel *)&gamma_data->values;
> +		while (count < num_samples) {
> +			correct_rgb = correction_values[count];
> +			blue = correction_values[count].blue;
> +			green = correction_values[count].green;
> +			red = correction_values[count].red;
> +
> +			blue = blue >> CHV_8BIT_GAMMA_MSB_SHIFT;
> +			green = green >> CHV_8BIT_GAMMA_MSB_SHIFT;
> +			red = red >> CHV_8BIT_GAMMA_MSB_SHIFT;
> +
> +			/* Red (23:16), Green (15:8), Blue (7:0) */
> +			word = (red << CHV_8BIT_GAMMA_SHIFT_RED_REG) |
> +				(green <<
> +				 CHV_8BIT_GAMMA_SHIFT_GREEN_REG) |
> +				blue;
> +			I915_WRITE(palette, word);
> +
> +			palette += 4;
> +			count++;
> +		}
> +		reg = PIPECONF(pipe);
> +		val = I915_READ(reg) | PIPECONF_GAMMA;
> +		I915_WRITE(reg, val);
> +
> +		DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n",
> +			pipe_name(pipe));
> +		ret = 0;
> +	} else if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_10BIT) {
> +
> +		if (num_samples != CHV_10BIT_GAMMA_MAX_VALS) {
> +			DRM_ERROR("Incorrect number of samples received\n");

Not DRM_ERROR. We do give -EINVAL hint for people developping user
space, but with DRM_DEBUG_KMS().

> +			return -EINVAL;
> +		}
> +
> +		/* Enable (CGM) Gamma on Pipe */
> +		cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
> +
> +		count = 0;
> +		correction_values = (struct rgb_pixel *)&gamma_data->values;
> +		while (count < num_samples) {
> +			correct_rgb = correction_values[count];
> +			blue = correction_values[count].blue;
> +			green = correction_values[count].green;
> +			red = correction_values[count].red;
> +
> +			blue = blue >> CHV_10BIT_GAMMA_MSB_SHIFT;
> +			green = green >> CHV_10BIT_GAMMA_MSB_SHIFT;
> +			red = red >> CHV_10BIT_GAMMA_MSB_SHIFT;
> +
> +			/* Green (25:16) and Blue (9:0) to be written */
> +			word = (green << CHV_GAMMA_SHIFT_GREEN) | blue;
> +			I915_WRITE(cgm_gamma_reg, word);
> +			cgm_gamma_reg += 4;
> +
> +			/* Red (9:0) to be written */
> +			word = red;
> +			I915_WRITE(cgm_gamma_reg, word);
> +
> +			cgm_gamma_reg += 4;
> +			count++;
> +		}
> +
> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe),
> +			I915_READ(_PIPE_CGM_CONTROL(pipe))
> +			| CGM_GAMMA_EN);
> +
> +		DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n",
> +			pipe_name(pipe));
> +
> +		ret = 0;
> +	} else {
> +		DRM_ERROR("Invalid gamma_level received\n");

Not DRM_ERROR. We do give -EINVAL hint for people developping user
space, but with DRM_DEBUG_KMS().

> +		return -EFAULT;
> +	}
> +
> +	ret = drm_mode_crtc_update_color_property(&blob, length,
> +			(void *) gamma_data, &crtc->base,
> +			config->gamma_property);
> +
> +	if (ret) {
> +		DRM_ERROR("Error updating Gamma blob\n");

Not DRM_ERROR. We do give -EINVAL hint for people developping user
space, but with DRM_DEBUG_KMS().

> +		crtc->gamma_blob_id = INVALID_BLOB_ID;
> +		return -EFAULT;
> +	}
> +
> +	/* Save blob ID for future use */
> +	crtc->gamma_blob_id = blob->base.id;
> +	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);
> +
> +	DRM_DEBUG_DRIVER("\n");

Too verbose.

> +
> +	if (IS_CHERRYVIEW(dev))
> +		return chv_set_gamma(dev, blob_id, crtc);
> +
> +	return -EINVAL;
> +}
> +
>  void intel_color_manager_attach(struct drm_device *dev,
>  		struct drm_mode_object *mode_obj)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
> index a55ce23..0acf8e9 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.h
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -27,8 +27,70 @@
>  
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc_helper.h>
> +#include "i915_drv.h"
> +
> +/* Color Management macros for Gamma */
> +#define I915_GAMMA_FLAG_DEGAMMA			(1 << 0)
> +#define I915_PIPE_GAMMA				(1 << 0)
> +#define I915_PLANE_GAMMA			(1 << 1)
> +#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)
> +
> +#define CHV_MAX_PIPES				3

we have the number of pipes in the device info in dev_priv.

> +#define CHV_DISPLAY_BASE			0x180000

We also have the display base ther.

> +#define INVALID_BLOB_ID				9999

This looks weird. If really needed, it should be part of the generic
blob interface.

> +struct rgb_pixel {
> +	u16 red;
> +	u16 green;
> +	u16 blue;
> +};

That shouldn't be exposed in this header. If this is how userspace will
give us values, how about including it in the interface itself? Can't be
as generic as "rgb_pixel" though, struct drm_lut_entry could be a
contender.

> +
> +/* CHV CGM Block */
> +/* Bit 2 to be enabled in CGM block for CHV */
> +#define CGM_GAMMA_EN				4

This should be part of the corresponding register defines (we group
defines per-register)

> +
> +/* Gamma */
> +#define CHV_GAMMA_VALS				257

This define is unused in this patch and duplicates with
CHV_8BIT_GAMMA_MAX_VALS.

> +#define CHV_10BIT_GAMMA_MAX_INDEX		256
> +#define CHV_8BIT_GAMMA_MAX_INDEX		255
> +#define CHV_10BIT_GAMMA_MSB_SHIFT		6
> +#define CHV_GAMMA_EVEN_MASK			0xFF

Not used in this patch.

> +#define CHV_GAMMA_SHIFT_BLUE			0
> +#define CHV_GAMMA_SHIFT_GREEN			16
> +#define CHV_GAMMA_SHIFT_RED			0
> +#define CHV_GAMMA_ODD_SHIFT			8

Not used in this patch.

> +#define CHV_CLRMGR_GAMMA_GCMAX_SHIFT		17

Not used in this patch.

> +#define CHV_GAMMA_GCMAX_MASK			0x1FFFF

Not used in this patch.

> +#define CHV_GAMMA_GCMAX_MAX			0x400

Not used in this patch.

> +#define CHV_10BIT_GAMMA_MAX_VALS		(CHV_10BIT_GAMMA_MAX_INDEX + 1)
> +#define CHV_8BIT_GAMMA_MAX_VALS			(CHV_8BIT_GAMMA_MAX_INDEX + 1)
> +#define CHV_8BIT_GAMMA_MSB_SHIFT		8
> +#define CHV_8BIT_GAMMA_SHIFT_GREEN_REG		8
> +#define CHV_8BIT_GAMMA_SHIFT_RED_REG		16
> +
> +/* CGM Registers */
> +#define CGM_OFFSET				0x2000
> +#define GAMMA_OFFSET				0x2000
> +#define PIPEA_CGM_CONTROL			(CHV_DISPLAY_BASE + 0x67A00)
> +#define PIPEA_CGM_GAMMA_MIN			(CHV_DISPLAY_BASE + 0x67000)
> +#define _PIPE_CGM_CONTROL(pipe) \
> +	(PIPEA_CGM_CONTROL + (pipe * CGM_OFFSET))
> +#define _PIPE_GAMMA_BASE(pipe) \
> +	(PIPEA_CGM_GAMMA_MIN + (pipe * GAMMA_OFFSET))

Use the _PIPE() macro. The '_' is, by convention, for symbols considered
private, so the PIPEA_* defines should have it, not the define used in
code.

>  
>  /* Generic Function prototypes */
>  void intel_color_manager_init(struct drm_device *dev);
>  void intel_color_manager_attach(struct drm_device *dev,
>  		struct drm_mode_object *mode_obj);
> +extern int intel_color_manager_set_gamma(struct drm_device *dev,
> +		struct drm_mode_object *obj, uint32_t blob_id);

extern or not? one must choose (we go without extern these days).

> +
> +/* Platform specific function prototypes */
> +extern int chv_set_gamma(struct drm_device *dev,
> +		uint32_t blob_id, struct drm_crtc *crtc);
> -- 
> 2.4.2
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:38PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> This patch does the following:
> 1. Adds the core function to program Gamma correction values for CHV/BSW
>    platform

A couple of things I forgot (they are probably others):
  - This patch doesn't actually add anything for CHV despite the claim
  - The properties are exposed everywhere, but return -EINVAL on
    !IS_CHERRYVIEW. We only expose the properties on a platform when we
    do have code that can make this property work for that platform.
    This way userspace can easily know if the feature is supported or
    not by querying if the property exists.

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

Re: [PATCH v2 08/10] drm: Add CSC correction structure

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:39PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> This patch adds a new structure in DRM layer for CSC color correction.
> This structure will be used by all user space agents to configure
> appropriate CSC format and CSC level.
> 
> /* Color Management structure for CSC */
> struct drm_intel_csc {
> 	__u32 csc_level;
> 	(The csc_level indicates whether the CSC to be applied at
> 	pipe/plane level)

Same as before, I don't believe we need this.

> 	__u32 csc_format;
> 	(The csc_format indicates the supported correction value \
> 	formats by underlying hardware)

Well, as with this whole series, leaking hw specific details in generic
interfaces.

> 	__u32 reserved;
> 	__s32 csc_matrix[9];
> 	(Raw CSC correction matrix)

Well, here again, hw specific, how about going for 16.16 fixed point?

> };

> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>  include/drm/drm_crtc.h | 1 +
>  include/uapi/drm/drm.h | 8 ++++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 31b52cb..bcba1e7 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -486,6 +486,7 @@ struct drm_crtc {
>  
>  	/* Color Management Blob IDs */
>  	u32 gamma_blob_id;
> +	u32 csc_blob_id;
>  };
>  
>  /**
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index fc2661c..2458b6c 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -839,6 +839,14 @@ struct drm_gamma {
>  	__u16 values[0];
>  };
>  
> +/* Color Management structure for CSC */
> +struct drm_csc {
> +	__u32 csc_level;
> +	__u32 csc_format;
> +	__u32 reserved;
> +	__s32 csc_matrix[9];
> +};
> +
>  /* 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

Re: [PATCH v2 08/10] drm: Add CSC correction structure

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:39PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> This patch adds a new structure in DRM layer for CSC color correction.
> This structure will be used by all user space agents to configure
> appropriate CSC format and CSC level.

And another little one:

> /* Color Management structure for CSC */
> struct drm_intel_csc {
> 	__u32 csc_level;
> 	(The csc_level indicates whether the CSC to be applied at
> 	pipe/plane level)
> 	__u32 csc_format;
> 	(The csc_format indicates the supported correction value \
> 	formats by underlying hardware)
> 	__u32 reserved;
> 	__s32 csc_matrix[9];
> 	(Raw CSC correction matrix)

You haven't defined if that data represents a row-major or column-major
matrix.

> };

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

Re: [PATCH v2 10/10] drm/i915: Add CSC support for CHV/BSW

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:41PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> This patch does the following:
> 1. Adds the core function to program CSC correction values for
>    CHV/BSW platform
> 2. Adds CSC correction macros/defines
> 3. Adds a pointer to hold blob for CSC property in drm_crtc
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c        |   3 +-
>  drivers/gpu/drm/i915/intel_color_manager.c | 115 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color_manager.h |  26 +++++++
>  3 files changed, 143 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 4726847..dcf4694 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -433,7 +433,8 @@ intel_crtc_atomic_set_property(struct drm_crtc *crtc,
>  
>  	if (property == config->gamma_property)
>  		return intel_color_manager_set_gamma(dev, &crtc->base, val);
> +	if (property == config->csc_property)
> +		return intel_color_manager_set_csc(dev, &crtc->base, val);
>  
> -	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>  	return -EINVAL;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> index 421c267..d904050 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,6 +27,108 @@
>  
>  #include "intel_color_manager.h"
>  
> +int chv_set_csc(struct drm_device *dev, uint64_t blob_id,
> +		struct drm_crtc *crtc)
> +{
> +	struct drm_csc *csc_data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_property_blob *blob;
> +	struct drm_mode_config *config = &dev->mode_config;
> +	u32 reg;
> +	enum pipe pipe;
> +	s16 csc_value;
> +	s32 word, temp;
> +	int ret, count = 0;
> +
> +	blob = drm_property_lookup_blob(dev, blob_id);
> +	if (!blob) {
> +		DRM_ERROR("Invalid Blob ID\n");
> +		return -EINVAL;
> +	}
> +
> +	csc_data = (struct drm_csc *)blob->data;
> +	pipe = to_intel_crtc(crtc)->pipe;
> +
> +	if (csc_data->csc_format == I915_CSC_COEFF_FORMAT_UNKNOWN) {
> +
> +		/* Disable CSC functionality */
> +		reg = _PIPE_CGM_CONTROL(pipe);
> +		I915_WRITE(reg, I915_READ(reg) & (~CGM_CSC_EN));
> +
> +		DRM_DEBUG_DRIVER("Disabled CSC Functionality on Pipe %c\n",
> +				pipe_name(pipe));
> +		ret = 0;

Same remarks here I did for the gamma property. i915 specific defines
for an interface that looks like it want to be generic, UNKNOWN Vs
DISABLED, DRM_ERROR, ...

And I haven't actually looked at the CHV details in this pass.

> +	} else if (csc_data->csc_format == I915_CSC_COEFF_FORMAT_S2_29) {
> +
> +		/* Disable CSC functionality in case it was set earlier */
> +		reg = _PIPE_CGM_CONTROL(pipe);
> +		I915_WRITE(reg, I915_READ(reg) & (~CGM_CSC_EN));
> +
> +		DRM_DEBUG_DRIVER("Disabled CSC Functionality on Pipe %c\n",
> +				pipe_name(pipe));
> +
> +		reg = _PIPE_CSC_BASE(pipe);
> +		while (count < CSC_MAX_VALS) {
> +
> +			/* Rounding off, to decrease loss of precision */
> +			if (csc_data->csc_matrix[count] < 0) {
> +				temp = csc_data->csc_matrix[count];
> +				temp -= CHV_CSC_ROUNDOFF;
> +				if (temp < CHV_CSC_MIN)
> +					temp = CHV_CSC_MIN;
> +			} else {
> +				temp = csc_data->csc_matrix[count];
> +				temp += CHV_CSC_ROUNDOFF;
> +				if (temp > CHV_CSC_MAX)
> +					temp = CHV_CSC_MIN;
> +			}
> +			csc_value = temp >> S2_29_CSC_COEFF_SHIFT;
> +			word = csc_value;
> +
> +			/*
> +			 * Last value to be written in 1 register.
> +			 * Otherwise, each pair of CSC values go
> +			 * into 1 register
> +			 */
> +			if (count != (CSC_MAX_VALS - 1)) {
> +				count++;
> +				csc_value = temp >> S2_29_CSC_COEFF_SHIFT;
> +				temp = csc_value;
> +				temp <<= CHV_CSC_SHIFT;
> +				word |= temp;
> +			}
> +			I915_WRITE(reg, word);
> +			reg += 4;
> +			count++;
> +		}
> +
> +		DRM_DEBUG_DRIVER("All CSC values written to registers\n");
> +
> +		/* Enable CSC functionality */
> +		reg = _PIPE_CGM_CONTROL(pipe);
> +		I915_WRITE(reg, I915_READ(reg) | CGM_CSC_EN);
> +		DRM_DEBUG_DRIVER("CSC enabled on Pipe %c\n",
> +				pipe_name(pipe));
> +		ret = 0;
> +	} else {
> +		DRM_ERROR("Invalid CSC COEFF Format\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = drm_mode_crtc_update_color_property(&blob,
> +			sizeof(struct drm_csc), (void *) csc_data,
> +			&crtc->base, config->csc_property);
> +	if (ret) {
> +		DRM_ERROR("Error updating CSC blob\n");
> +		crtc->csc_blob_id = INVALID_BLOB_ID;
> +		return -EFAULT;
> +	}
> +
> +	/* Save blob ID for future reference */
> +	crtc->csc_blob_id = blob->base.id;
> +	return ret;
> +}
> +
>  int chv_set_gamma(struct drm_device *dev, uint32_t blob_id,
>  		struct drm_crtc *crtc)
>  {
> @@ -175,6 +277,19 @@ int chv_set_gamma(struct drm_device *dev, uint32_t blob_id,
>  	return ret;
>  }
>  
> +int intel_color_manager_set_csc(struct drm_device *dev,
> +		struct drm_mode_object *obj, uint64_t blob_id)
> +{
> +	struct drm_crtc *crtc = obj_to_crtc(obj);
> +
> +	DRM_DEBUG_DRIVER("\n");
> +
> +	if (IS_CHERRYVIEW(dev))
> +		return chv_set_csc(dev, blob_id, crtc);
> +
> +	return -EINVAL;
> +}
> +
>  int intel_color_manager_set_gamma(struct drm_device *dev,
>  		struct drm_mode_object *obj, uint32_t blob_id)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
> index 0acf8e9..8a3e76a 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.h
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -41,6 +41,14 @@
>  #define I915_GAMMA_PRECISION_14BIT		(1 << 3)
>  #define I915_GAMMA_PRECISION_16BIT		(1 << 4)
>  
> +/* Color Management macros for CSC */
> +#define I915_PIPE_CSC				(1 << 0)
> +#define I915_PLANE_CSC				(1 << 1)
> +#define I915_CSC_COEFF_FORMAT_UNKNOWN		0
> +#define I915_CSC_COEFF_FORMAT_CURRENT		0xFFFFFFFF
> +#define I915_CSC_COEFF_FORMAT_S1_30		(1 << 0)
> +#define I915_CSC_COEFF_FORMAT_S2_29		(1 << 1)
> +
>  #define CHV_MAX_PIPES				3
>  #define CHV_DISPLAY_BASE			0x180000
>  #define INVALID_BLOB_ID				9999
> @@ -52,6 +60,8 @@ struct rgb_pixel {
>  };
>  
>  /* CHV CGM Block */
> +/* Bit 1 to be enabled */
> +#define CGM_CSC_EN				2
>  /* Bit 2 to be enabled in CGM block for CHV */
>  #define CGM_GAMMA_EN				4
>  
> @@ -74,15 +84,27 @@ struct rgb_pixel {
>  #define CHV_8BIT_GAMMA_SHIFT_GREEN_REG		8
>  #define CHV_8BIT_GAMMA_SHIFT_RED_REG		16
>  
> +/* CSC */
> +#define CSC_MAX_VALS				9
> +#define CHV_CSC_SHIFT				16
> +#define CHV_CSC_ROUNDOFF			(1 << 15)
> +#define CHV_CSC_MAX				0x7FFF
> +#define CHV_CSC_MIN				0x8000
> +#define S2_29_CSC_COEFF_SHIFT			16
> +
>  /* CGM Registers */
>  #define CGM_OFFSET				0x2000
>  #define GAMMA_OFFSET				0x2000
> +#define CGM_CSC_OFFSET				0x2000
>  #define PIPEA_CGM_CONTROL			(CHV_DISPLAY_BASE + 0x67A00)
>  #define PIPEA_CGM_GAMMA_MIN			(CHV_DISPLAY_BASE + 0x67000)
> +#define PIPEA_CGM_CSC_MIN			(CHV_DISPLAY_BASE + 0x67900)
>  #define _PIPE_CGM_CONTROL(pipe) \
>  	(PIPEA_CGM_CONTROL + (pipe * CGM_OFFSET))
>  #define _PIPE_GAMMA_BASE(pipe) \
>  	(PIPEA_CGM_GAMMA_MIN + (pipe * GAMMA_OFFSET))
> +#define _PIPE_CSC_BASE(pipe) \
> +	(PIPEA_CGM_CSC_MIN + (pipe * CGM_CSC_OFFSET))
>  
>  /* Generic Function prototypes */
>  void intel_color_manager_init(struct drm_device *dev);
> @@ -90,7 +112,11 @@ void intel_color_manager_attach(struct drm_device *dev,
>  		struct drm_mode_object *mode_obj);
>  extern int intel_color_manager_set_gamma(struct drm_device *dev,
>  		struct drm_mode_object *obj, uint32_t blob_id);
> +extern int intel_color_manager_set_csc(struct drm_device *dev,
> +		struct drm_mode_object *obj, uint64_t blob_id);
>  
>  /* Platform specific function prototypes */
>  extern int chv_set_gamma(struct drm_device *dev,
>  		uint32_t blob_id, struct drm_crtc *crtc);
> +extern int chv_set_csc(struct drm_device *dev,
> +		uint64_t blob_id, struct drm_crtc *crtc);
> -- 
> 2.4.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 00/10] Color Manager Implementation

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> This patch set adds color manager implementation in drm/i915 layer.
> Color Manager is an extension in i915 driver to support color 
> correction/enhancement. Various Intel platforms support several
> color correction capabilities. Color Manager provides abstraction
> of these properties and allows a user space UI agent to 
> correct/enhance the display.

So I did a first rough pass on the API itself. The big question that
isn't solved at the moment is: do we want to try to do generic KMS
properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:

  1/ Generic for all KMS drivers
  2/ Generic for i915 supported platfoms
  3/ Specific to each platform

At this point, I'm quite tempted to say we should give 1/ a shot. We
should be able to have pre-LUT + matrix + post-LUT on CRTC objects and
guarantee that, when the drivers expose such properties, user space can
at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.

It may be possible to use the "try" version of the atomic ioctl to
explore the space of possibilities from a generic user space to use
bigger LUTs as well. A HAL layer (which is already there in some but not
all OSes) would still be able to use those generic properties to load
"precision optimized" LUTs with some knowledge of the hardware.

Option 3/ is, IMHO, a no-go, we should really try hard to limit the work
we need to do per-platform, which means defining a common format for the
values we give to the kernel. As stated in various places, 16.16 seems
the format of choice, even for the LUTs as we have wide gamut support in
some of the LUTs where we can map values > 1.0 to other values > 1.0.

Another thing, the documentation of the interface needs to be a bit more
crisp. For instance, we don't currently define the order in which the
CSC and LUT transforms of this patch set are applied: is this a de-gamma
LUT to do the CSC in linear space? but then that means the display is
linear, oops. So it must be a post-CSC lut, but then we don't de-gamma
sRGB (not technically a single gamma power curve for sRGB, but details,
details) before applying a linear transform. So with this interface, we
have to enforce the fbs are linear, losing dynamic range. I'm sure later
patches would expose more properties, but as a stand-alone patch set, it
would seem we can't do anything useful?

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

Re: [Intel-gfx] [PATCH v2 00/10] Color Manager Implementation

[Daniel Vetter]

On Tue, Jun 09, 2015 at 01:50:48PM +0100, Damien Lespiau wrote:
> On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
> > From: Kausal Malladi <Kausal.Malladi@intel.com>
> > 
> > This patch set adds color manager implementation in drm/i915 layer.
> > Color Manager is an extension in i915 driver to support color 
> > correction/enhancement. Various Intel platforms support several
> > color correction capabilities. Color Manager provides abstraction
> > of these properties and allows a user space UI agent to 
> > correct/enhance the display.
> 
> So I did a first rough pass on the API itself. The big question that
> isn't solved at the moment is: do we want to try to do generic KMS
> properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:
> 
>   1/ Generic for all KMS drivers
>   2/ Generic for i915 supported platfoms
>   3/ Specific to each platform
> 
> At this point, I'm quite tempted to say we should give 1/ a shot. We
> should be able to have pre-LUT + matrix + post-LUT on CRTC objects and
> guarantee that, when the drivers expose such properties, user space can
> at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.
> 
> It may be possible to use the "try" version of the atomic ioctl to
> explore the space of possibilities from a generic user space to use
> bigger LUTs as well. A HAL layer (which is already there in some but not
> all OSes) would still be able to use those generic properties to load
> "precision optimized" LUTs with some knowledge of the hardware.

Yeah, imo 1/ should be doable. For the matrix we should be able to be
fully generic with a 16.16 format. For gamma one option would be to have
an enum property listing all the supported gamma table formats, of which
8bit 256 entry (the current standard) would be a one. This enum space
would need to be drm-wide ofc. Then the gamma blob would just contain the
table. This way we can allow funky stuff like the 1025th entry for 1.0+
values some intel tables have, and similar things.

Wrt pre-post and plan/crtc I guess we'd just add the properties to all the
objects where they're possible on a given platform and then the driver
must check if there's constraints (e.g. post-lut gamma only on 1 plane or
the crtc or similar stuff).

Also there's the legacy gamma ioctl. That should forward to the crtc gamma
(and there probably pick post lut and pre-lut only if there's no post
lut). For names I'd suggest

"pre-gamma-type", "pre-gamma-data", "post-gamma-type" and
"post-gamma-data" but I don't care terrible much about them.
-Daniel

> 
> Option 3/ is, IMHO, a no-go, we should really try hard to limit the work
> we need to do per-platform, which means defining a common format for the
> values we give to the kernel. As stated in various places, 16.16 seems
> the format of choice, even for the LUTs as we have wide gamut support in
> some of the LUTs where we can map values > 1.0 to other values > 1.0.
> 
> Another thing, the documentation of the interface needs to be a bit more
> crisp. For instance, we don't currently define the order in which the
> CSC and LUT transforms of this patch set are applied: is this a de-gamma
> LUT to do the CSC in linear space? but then that means the display is
> linear, oops. So it must be a post-CSC lut, but then we don't de-gamma
> sRGB (not technically a single gamma power curve for sRGB, but details,
> details) before applying a linear transform. So with this interface, we
> have to enforce the fbs are linear, losing dynamic range. I'm sure later
> patches would expose more properties, but as a stand-alone patch set, it
> would seem we can't do anything useful?
> 
> -- 
> Damien
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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 v2 00/10] Color Manager Implementation

[Matheson, Annie J]

+Susanta/Shashank

How does this review from Daniel sound to you guys? I know you've ask for the Display team to review the latest design doc before you start the external communication and there's been some discussion below...

Thanks. 

Annie Matheson
Intel Corporation
Phone: (503) 712-0586 
Email: annie.j.matheson@intel.com

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Sunday, June 14, 2015 11:53 PM
To: Lespiau, Damien
Cc: Malladi, Kausal; Matheson, Annie J; R, Dhanya p; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Purushothaman, Vijay A; Barnes, Jesse; Vetter, Daniel
Subject: Re: [Intel-gfx] [PATCH v2 00/10] Color Manager Implementation

On Tue, Jun 09, 2015 at 01:50:48PM +0100, Damien Lespiau wrote:
> On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
> > From: Kausal Malladi <Kausal.Malladi@intel.com>
> > 
> > This patch set adds color manager implementation in drm/i915 layer.
> > Color Manager is an extension in i915 driver to support color 
> > correction/enhancement. Various Intel platforms support several
> > color correction capabilities. Color Manager provides abstraction
> > of these properties and allows a user space UI agent to 
> > correct/enhance the display.
> 
> So I did a first rough pass on the API itself. The big question that
> isn't solved at the moment is: do we want to try to do generic KMS
> properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:
> 
>   1/ Generic for all KMS drivers
>   2/ Generic for i915 supported platfoms
>   3/ Specific to each platform
> 
> At this point, I'm quite tempted to say we should give 1/ a shot. We
> should be able to have pre-LUT + matrix + post-LUT on CRTC objects and
> guarantee that, when the drivers expose such properties, user space can
> at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.
> 
> It may be possible to use the "try" version of the atomic ioctl to
> explore the space of possibilities from a generic user space to use
> bigger LUTs as well. A HAL layer (which is already there in some but not
> all OSes) would still be able to use those generic properties to load
> "precision optimized" LUTs with some knowledge of the hardware.

Yeah, imo 1/ should be doable. For the matrix we should be able to be
fully generic with a 16.16 format. For gamma one option would be to have
an enum property listing all the supported gamma table formats, of which
8bit 256 entry (the current standard) would be a one. This enum space
would need to be drm-wide ofc. Then the gamma blob would just contain the
table. This way we can allow funky stuff like the 1025th entry for 1.0+
values some intel tables have, and similar things.

Wrt pre-post and plan/crtc I guess we'd just add the properties to all the
objects where they're possible on a given platform and then the driver
must check if there's constraints (e.g. post-lut gamma only on 1 plane or
the crtc or similar stuff).

Also there's the legacy gamma ioctl. That should forward to the crtc gamma
(and there probably pick post lut and pre-lut only if there's no post
lut). For names I'd suggest

"pre-gamma-type", "pre-gamma-data", "post-gamma-type" and
"post-gamma-data" but I don't care terrible much about them.
-Daniel

> 
> Option 3/ is, IMHO, a no-go, we should really try hard to limit the work
> we need to do per-platform, which means defining a common format for the
> values we give to the kernel. As stated in various places, 16.16 seems
> the format of choice, even for the LUTs as we have wide gamut support in
> some of the LUTs where we can map values > 1.0 to other values > 1.0.
> 
> Another thing, the documentation of the interface needs to be a bit more
> crisp. For instance, we don't currently define the order in which the
> CSC and LUT transforms of this patch set are applied: is this a de-gamma
> LUT to do the CSC in linear space? but then that means the display is
> linear, oops. So it must be a post-CSC lut, but then we don't de-gamma
> sRGB (not technically a single gamma power curve for sRGB, but details,
> details) before applying a linear transform. So with this interface, we
> have to enforce the fbs are linear, losing dynamic range. I'm sure later
> patches would expose more properties, but as a stand-alone patch set, it
> would seem we can't do anything useful?
> 
> -- 
> Damien
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 00/10] Color Manager Implementation

[Sharma, Shashank]

Hi Annie, 

I missed this comment (I was missing from to/cc list, so my filter did the trick :))
Overall, comments from Daniel looks good, and is pretty much aligned to what we are trying to do (1. Generic for all KMS drivers).

If you check the latest design, we are also planning to expose a read-only property, to userspace, to expose all the color capabilities. 
It's similar to what Danvet suggested,  we are just doing it for all color properties, instead of just for gamma. 
Userspace can read this blob property, and extract all the color capabilities of the platform (gamma, CSC, degamma) and decide which one to opt for.

I would again recommend all to go through the document (only the how to query / get / set section would be enough), and let us know if we need a change at this level.

Regards
Shashank
-----Original Message-----
From: Matheson, Annie J 
Sent: Tuesday, June 16, 2015 2:00 AM
To: Daniel Vetter; Lespiau, Damien; Sharma, Shashank; Bhattacharjee, Susanta
Cc: Malladi, Kausal; R, Dhanya p; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Purushothaman, Vijay A; Barnes, Jesse; Vetter, Daniel
Subject: RE: [Intel-gfx] [PATCH v2 00/10] Color Manager Implementation

+Susanta/Shashank

How does this review from Daniel sound to you guys? I know you've ask for the Display team to review the latest design doc before you start the external communication and there's been some discussion below...

Thanks. 

Annie Matheson
Intel Corporation
Phone: (503) 712-0586
Email: annie.j.matheson@intel.com

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Sunday, June 14, 2015 11:53 PM
To: Lespiau, Damien
Cc: Malladi, Kausal; Matheson, Annie J; R, Dhanya p; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Purushothaman, Vijay A; Barnes, Jesse; Vetter, Daniel
Subject: Re: [Intel-gfx] [PATCH v2 00/10] Color Manager Implementation

On Tue, Jun 09, 2015 at 01:50:48PM +0100, Damien Lespiau wrote:
> On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
> > From: Kausal Malladi <Kausal.Malladi@intel.com>
> > 
> > This patch set adds color manager implementation in drm/i915 layer.
> > Color Manager is an extension in i915 driver to support color 
> > correction/enhancement. Various Intel platforms support several 
> > color correction capabilities. Color Manager provides abstraction of 
> > these properties and allows a user space UI agent to correct/enhance 
> > the display.
> 
> So I did a first rough pass on the API itself. The big question that 
> isn't solved at the moment is: do we want to try to do generic KMS 
> properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:
> 
>   1/ Generic for all KMS drivers
>   2/ Generic for i915 supported platfoms
>   3/ Specific to each platform
> 
> At this point, I'm quite tempted to say we should give 1/ a shot. We 
> should be able to have pre-LUT + matrix + post-LUT on CRTC objects and 
> guarantee that, when the drivers expose such properties, user space 
> can at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.
> 
> It may be possible to use the "try" version of the atomic ioctl to 
> explore the space of possibilities from a generic user space to use 
> bigger LUTs as well. A HAL layer (which is already there in some but 
> not all OSes) would still be able to use those generic properties to 
> load "precision optimized" LUTs with some knowledge of the hardware.

Yeah, imo 1/ should be doable. For the matrix we should be able to be fully generic with a 16.16 format. For gamma one option would be to have an enum property listing all the supported gamma table formats, of which 8bit 256 entry (the current standard) would be a one. This enum space would need to be drm-wide ofc. Then the gamma blob would just contain the table. This way we can allow funky stuff like the 1025th entry for 1.0+ values some intel tables have, and similar things.

Wrt pre-post and plan/crtc I guess we'd just add the properties to all the objects where they're possible on a given platform and then the driver must check if there's constraints (e.g. post-lut gamma only on 1 plane or the crtc or similar stuff).

Also there's the legacy gamma ioctl. That should forward to the crtc gamma (and there probably pick post lut and pre-lut only if there's no post lut). For names I'd suggest

"pre-gamma-type", "pre-gamma-data", "post-gamma-type" and "post-gamma-data" but I don't care terrible much about them.
-Daniel

> 
> Option 3/ is, IMHO, a no-go, we should really try hard to limit the 
> work we need to do per-platform, which means defining a common format 
> for the values we give to the kernel. As stated in various places, 
> 16.16 seems the format of choice, even for the LUTs as we have wide 
> gamut support in some of the LUTs where we can map values > 1.0 to other values > 1.0.
> 
> Another thing, the documentation of the interface needs to be a bit 
> more crisp. For instance, we don't currently define the order in which 
> the CSC and LUT transforms of this patch set are applied: is this a 
> de-gamma LUT to do the CSC in linear space? but then that means the 
> display is linear, oops. So it must be a post-CSC lut, but then we 
> don't de-gamma sRGB (not technically a single gamma power curve for 
> sRGB, but details,
> details) before applying a linear transform. So with this interface, 
> we have to enforce the fbs are linear, losing dynamic range. I'm sure 
> later patches would expose more properties, but as a stand-alone patch 
> set, it would seem we can't do anything useful?
> 
> --
> Damien
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 00/10] Color Manager Implementation

[Matheson, Annie J]

Jesse-Daniel:

Can you take a look at this and let us know your thoughts please?

Thanks. 

Annie Matheson
Intel Corporation
Phone: (503) 712-0586 
Email: annie.j.matheson@intel.com

-----Original Message-----
From: Sharma, Shashank 
Sent: Monday, June 15, 2015 8:12 PM
To: Matheson, Annie J; Daniel Vetter; Lespiau, Damien; Bhattacharjee, Susanta
Cc: Malladi, Kausal; R, Dhanya p; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Purushothaman, Vijay A; Barnes, Jesse; Vetter, Daniel
Subject: RE: [Intel-gfx] [PATCH v2 00/10] Color Manager Implementation

Hi Annie, 

I missed this comment (I was missing from to/cc list, so my filter did the trick :))
Overall, comments from Daniel looks good, and is pretty much aligned to what we are trying to do (1. Generic for all KMS drivers).

If you check the latest design, we are also planning to expose a read-only property, to userspace, to expose all the color capabilities. 
It's similar to what Danvet suggested,  we are just doing it for all color properties, instead of just for gamma. 
Userspace can read this blob property, and extract all the color capabilities of the platform (gamma, CSC, degamma) and decide which one to opt for.

I would again recommend all to go through the document (only the how to query / get / set section would be enough), and let us know if we need a change at this level.

Regards
Shashank
-----Original Message-----
From: Matheson, Annie J 
Sent: Tuesday, June 16, 2015 2:00 AM
To: Daniel Vetter; Lespiau, Damien; Sharma, Shashank; Bhattacharjee, Susanta
Cc: Malladi, Kausal; R, Dhanya p; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Purushothaman, Vijay A; Barnes, Jesse; Vetter, Daniel
Subject: RE: [Intel-gfx] [PATCH v2 00/10] Color Manager Implementation

+Susanta/Shashank

How does this review from Daniel sound to you guys? I know you've ask for the Display team to review the latest design doc before you start the external communication and there's been some discussion below...

Thanks. 

Annie Matheson
Intel Corporation
Phone: (503) 712-0586
Email: annie.j.matheson@intel.com

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Sunday, June 14, 2015 11:53 PM
To: Lespiau, Damien
Cc: Malladi, Kausal; Matheson, Annie J; R, Dhanya p; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Purushothaman, Vijay A; Barnes, Jesse; Vetter, Daniel
Subject: Re: [Intel-gfx] [PATCH v2 00/10] Color Manager Implementation

On Tue, Jun 09, 2015 at 01:50:48PM +0100, Damien Lespiau wrote:
> On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
> > From: Kausal Malladi <Kausal.Malladi@intel.com>
> > 
> > This patch set adds color manager implementation in drm/i915 layer.
> > Color Manager is an extension in i915 driver to support color 
> > correction/enhancement. Various Intel platforms support several 
> > color correction capabilities. Color Manager provides abstraction of 
> > these properties and allows a user space UI agent to correct/enhance 
> > the display.
> 
> So I did a first rough pass on the API itself. The big question that 
> isn't solved at the moment is: do we want to try to do generic KMS 
> properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:
> 
>   1/ Generic for all KMS drivers
>   2/ Generic for i915 supported platfoms
>   3/ Specific to each platform
> 
> At this point, I'm quite tempted to say we should give 1/ a shot. We 
> should be able to have pre-LUT + matrix + post-LUT on CRTC objects and 
> guarantee that, when the drivers expose such properties, user space 
> can at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.
> 
> It may be possible to use the "try" version of the atomic ioctl to 
> explore the space of possibilities from a generic user space to use 
> bigger LUTs as well. A HAL layer (which is already there in some but 
> not all OSes) would still be able to use those generic properties to 
> load "precision optimized" LUTs with some knowledge of the hardware.

Yeah, imo 1/ should be doable. For the matrix we should be able to be fully generic with a 16.16 format. For gamma one option would be to have an enum property listing all the supported gamma table formats, of which 8bit 256 entry (the current standard) would be a one. This enum space would need to be drm-wide ofc. Then the gamma blob would just contain the table. This way we can allow funky stuff like the 1025th entry for 1.0+ values some intel tables have, and similar things.

Wrt pre-post and plan/crtc I guess we'd just add the properties to all the objects where they're possible on a given platform and then the driver must check if there's constraints (e.g. post-lut gamma only on 1 plane or the crtc or similar stuff).

Also there's the legacy gamma ioctl. That should forward to the crtc gamma (and there probably pick post lut and pre-lut only if there's no post lut). For names I'd suggest

"pre-gamma-type", "pre-gamma-data", "post-gamma-type" and "post-gamma-data" but I don't care terrible much about them.
-Daniel

> 
> Option 3/ is, IMHO, a no-go, we should really try hard to limit the 
> work we need to do per-platform, which means defining a common format 
> for the values we give to the kernel. As stated in various places, 
> 16.16 seems the format of choice, even for the LUTs as we have wide 
> gamut support in some of the LUTs where we can map values > 1.0 to other values > 1.0.
> 
> Another thing, the documentation of the interface needs to be a bit 
> more crisp. For instance, we don't currently define the order in which 
> the CSC and LUT transforms of this patch set are applied: is this a 
> de-gamma LUT to do the CSC in linear space? but then that means the 
> display is linear, oops. So it must be a post-CSC lut, but then we 
> don't de-gamma sRGB (not technically a single gamma power curve for 
> sRGB, but details,
> details) before applying a linear transform. So with this interface, 
> we have to enforce the fbs are linear, losing dynamic range. I'm sure 
> later patches would expose more properties, but as a stand-alone patch 
> set, it would seem we can't do anything useful?
> 
> --
> Damien
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 00/10] Color Manager Implementation

[Hans Verkuil]

On 06/15/2015 08:53 AM, Daniel Vetter wrote:
> On Tue, Jun 09, 2015 at 01:50:48PM +0100, Damien Lespiau wrote:
>> On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
>>> From: Kausal Malladi <Kausal.Malladi@intel.com>
>>>
>>> This patch set adds color manager implementation in drm/i915 layer.
>>> Color Manager is an extension in i915 driver to support color 
>>> correction/enhancement. Various Intel platforms support several
>>> color correction capabilities. Color Manager provides abstraction
>>> of these properties and allows a user space UI agent to 
>>> correct/enhance the display.
>>
>> So I did a first rough pass on the API itself. The big question that
>> isn't solved at the moment is: do we want to try to do generic KMS
>> properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:
>>
>>   1/ Generic for all KMS drivers
>>   2/ Generic for i915 supported platfoms
>>   3/ Specific to each platform
>>
>> At this point, I'm quite tempted to say we should give 1/ a shot. We
>> should be able to have pre-LUT + matrix + post-LUT on CRTC objects and
>> guarantee that, when the drivers expose such properties, user space can
>> at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.
>>
>> It may be possible to use the "try" version of the atomic ioctl to
>> explore the space of possibilities from a generic user space to use
>> bigger LUTs as well. A HAL layer (which is already there in some but not
>> all OSes) would still be able to use those generic properties to load
>> "precision optimized" LUTs with some knowledge of the hardware.
> 
> Yeah, imo 1/ should be doable. For the matrix we should be able to be
> fully generic with a 16.16 format. For gamma one option would be to have

I know I am late replying, apologies for that.

I've been working on CSC support for V4L2 as well (still work in progress)
and I would like to at least end up with the same low-level fixed point
format as DRM so we can share matrix/vector calculations.

Based on my experiences I have concerns about the 16.16 format: the precision
is quite low which can be a problem when such values are used in matrix
multiplications.

In addition, while the precision may be sufficient for 8 bit color component
values, I'm pretty sure it will be insufficient when dealing with 12 or 16 bit
color components.

In earlier versions of my CSC code I used a 12.20 format, but in the latest I
switched to 32.32. This fits nicely in a u64 and it's easy to extract the
integer and fractional parts.

If this is going to be a generic and future proof API, then my suggestion
would be to increase the precision of the underlying data type.

Regards,

	Hans

> an enum property listing all the supported gamma table formats, of which
> 8bit 256 entry (the current standard) would be a one. This enum space
> would need to be drm-wide ofc. Then the gamma blob would just contain the
> table. This way we can allow funky stuff like the 1025th entry for 1.0+
> values some intel tables have, and similar things.
> 
> Wrt pre-post and plan/crtc I guess we'd just add the properties to all the
> objects where they're possible on a given platform and then the driver
> must check if there's constraints (e.g. post-lut gamma only on 1 plane or
> the crtc or similar stuff).
> 
> Also there's the legacy gamma ioctl. That should forward to the crtc gamma
> (and there probably pick post lut and pre-lut only if there's no post
> lut). For names I'd suggest
> 
> "pre-gamma-type", "pre-gamma-data", "post-gamma-type" and
> "post-gamma-data" but I don't care terrible much about them.
> -Daniel
> 
>>
>> Option 3/ is, IMHO, a no-go, we should really try hard to limit the work
>> we need to do per-platform, which means defining a common format for the
>> values we give to the kernel. As stated in various places, 16.16 seems
>> the format of choice, even for the LUTs as we have wide gamut support in
>> some of the LUTs where we can map values > 1.0 to other values > 1.0.
>>
>> Another thing, the documentation of the interface needs to be a bit more
>> crisp. For instance, we don't currently define the order in which the
>> CSC and LUT transforms of this patch set are applied: is this a de-gamma
>> LUT to do the CSC in linear space? but then that means the display is
>> linear, oops. So it must be a post-CSC lut, but then we don't de-gamma
>> sRGB (not technically a single gamma power curve for sRGB, but details,
>> details) before applying a linear transform. So with this interface, we
>> have to enforce the fbs are linear, losing dynamic range. I'm sure later
>> patches would expose more properties, but as a stand-alone patch set, it
>> would seem we can't do anything useful?
>>
>> -- 
>> Damien
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

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

Re: [PATCH v2 00/10] Color Manager Implementation

[Daniel Vetter]

On Mon, Jul 13, 2015 at 10:29:32AM +0200, Hans Verkuil wrote:
> On 06/15/2015 08:53 AM, Daniel Vetter wrote:
> > On Tue, Jun 09, 2015 at 01:50:48PM +0100, Damien Lespiau wrote:
> >> On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
> >>> From: Kausal Malladi <Kausal.Malladi@intel.com>
> >>>
> >>> This patch set adds color manager implementation in drm/i915 layer.
> >>> Color Manager is an extension in i915 driver to support color 
> >>> correction/enhancement. Various Intel platforms support several
> >>> color correction capabilities. Color Manager provides abstraction
> >>> of these properties and allows a user space UI agent to 
> >>> correct/enhance the display.
> >>
> >> So I did a first rough pass on the API itself. The big question that
> >> isn't solved at the moment is: do we want to try to do generic KMS
> >> properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:
> >>
> >>   1/ Generic for all KMS drivers
> >>   2/ Generic for i915 supported platfoms
> >>   3/ Specific to each platform
> >>
> >> At this point, I'm quite tempted to say we should give 1/ a shot. We
> >> should be able to have pre-LUT + matrix + post-LUT on CRTC objects and
> >> guarantee that, when the drivers expose such properties, user space can
> >> at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.
> >>
> >> It may be possible to use the "try" version of the atomic ioctl to
> >> explore the space of possibilities from a generic user space to use
> >> bigger LUTs as well. A HAL layer (which is already there in some but not
> >> all OSes) would still be able to use those generic properties to load
> >> "precision optimized" LUTs with some knowledge of the hardware.
> > 
> > Yeah, imo 1/ should be doable. For the matrix we should be able to be
> > fully generic with a 16.16 format. For gamma one option would be to have
> 
> I know I am late replying, apologies for that.
> 
> I've been working on CSC support for V4L2 as well (still work in progress)
> and I would like to at least end up with the same low-level fixed point
> format as DRM so we can share matrix/vector calculations.
> 
> Based on my experiences I have concerns about the 16.16 format: the precision
> is quite low which can be a problem when such values are used in matrix
> multiplications.
> 
> In addition, while the precision may be sufficient for 8 bit color component
> values, I'm pretty sure it will be insufficient when dealing with 12 or 16 bit
> color components.
> 
> In earlier versions of my CSC code I used a 12.20 format, but in the latest I
> switched to 32.32. This fits nicely in a u64 and it's easy to extract the
> integer and fractional parts.
> 
> If this is going to be a generic and future proof API, then my suggestion
> would be to increase the precision of the underlying data type.

We discussed this a bit more internally and figured it would be nice to have the same
fixed point for both CSC matrix and LUT/gamma tables. Current consensus
seems to be to go with 8.24 for both. Since LUTs are fairly big I think it
makes sense if we try to be not too wasteful (while still future-proof
ofc).

But yeah agreeing on the underlying layout would be good so that we could
share in-kernel code. We're aiming to not have any LUT interpolation in
the kernel (just dropping samples at most if e.g. the hw table doesn't
have linear sample positions). But with the LUT we might need to mutliply
it with an in-kernel one (we need the CSC unit on some platforms to
compress the color output range for hdmi). And maybe compress the LUTs
too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 00/10] Color Manager Implementation

[Hans Verkuil]

On 07/13/2015 11:18 AM, Daniel Vetter wrote:
> On Mon, Jul 13, 2015 at 10:29:32AM +0200, Hans Verkuil wrote:
>> On 06/15/2015 08:53 AM, Daniel Vetter wrote:
>>> On Tue, Jun 09, 2015 at 01:50:48PM +0100, Damien Lespiau wrote:
>>>> On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
>>>>> From: Kausal Malladi <Kausal.Malladi@intel.com>
>>>>>
>>>>> This patch set adds color manager implementation in drm/i915 layer.
>>>>> Color Manager is an extension in i915 driver to support color 
>>>>> correction/enhancement. Various Intel platforms support several
>>>>> color correction capabilities. Color Manager provides abstraction
>>>>> of these properties and allows a user space UI agent to 
>>>>> correct/enhance the display.
>>>>
>>>> So I did a first rough pass on the API itself. The big question that
>>>> isn't solved at the moment is: do we want to try to do generic KMS
>>>> properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:
>>>>
>>>>   1/ Generic for all KMS drivers
>>>>   2/ Generic for i915 supported platfoms
>>>>   3/ Specific to each platform
>>>>
>>>> At this point, I'm quite tempted to say we should give 1/ a shot. We
>>>> should be able to have pre-LUT + matrix + post-LUT on CRTC objects and
>>>> guarantee that, when the drivers expose such properties, user space can
>>>> at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.
>>>>
>>>> It may be possible to use the "try" version of the atomic ioctl to
>>>> explore the space of possibilities from a generic user space to use
>>>> bigger LUTs as well. A HAL layer (which is already there in some but not
>>>> all OSes) would still be able to use those generic properties to load
>>>> "precision optimized" LUTs with some knowledge of the hardware.
>>>
>>> Yeah, imo 1/ should be doable. For the matrix we should be able to be
>>> fully generic with a 16.16 format. For gamma one option would be to have
>>
>> I know I am late replying, apologies for that.
>>
>> I've been working on CSC support for V4L2 as well (still work in progress)
>> and I would like to at least end up with the same low-level fixed point
>> format as DRM so we can share matrix/vector calculations.
>>
>> Based on my experiences I have concerns about the 16.16 format: the precision
>> is quite low which can be a problem when such values are used in matrix
>> multiplications.
>>
>> In addition, while the precision may be sufficient for 8 bit color component
>> values, I'm pretty sure it will be insufficient when dealing with 12 or 16 bit
>> color components.
>>
>> In earlier versions of my CSC code I used a 12.20 format, but in the latest I
>> switched to 32.32. This fits nicely in a u64 and it's easy to extract the
>> integer and fractional parts.
>>
>> If this is going to be a generic and future proof API, then my suggestion
>> would be to increase the precision of the underlying data type.
> 
> We discussed this a bit more internally and figured it would be nice to have the same
> fixed point for both CSC matrix and LUT/gamma tables. Current consensus
> seems to be to go with 8.24 for both. Since LUTs are fairly big I think it
> makes sense if we try to be not too wasteful (while still future-proof
> ofc).

The .24 should have enough precision, but I am worried about the 8: while
this works for 8 bit components, you can't use it to represent values
>255, which might be needed (now or in the future) for 10, 12 or 16 bit
color components.

It's why I ended up with 32.32: it's very generic so usable for other
things besides CSC.

Note that 8.24 is really 7.24 + one sign bit. So 255 can't be represented
in this format.

That said, all values I'm working with in my current code are small integers
(say between -4 and 4 worst case), so 8.24 would work. But I am not at all
confident that this is future proof. My gut feeling is that you need to be
able to represent at least the max component value + a sign bit + 7 decimals
precision. Which makes 17.24.

Regards,

	Hans

> 
> But yeah agreeing on the underlying layout would be good so that we could
> share in-kernel code. We're aiming to not have any LUT interpolation in
> the kernel (just dropping samples at most if e.g. the hw table doesn't
> have linear sample positions). But with the LUT we might need to mutliply
> it with an in-kernel one (we need the CSC unit on some platforms to
> compress the color output range for hdmi). And maybe compress the LUTs
> too.
> -Daniel
> 

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

Re: [PATCH v2 00/10] Color Manager Implementation

[Daniel Vetter]

On Mon, Jul 13, 2015 at 11:43:31AM +0200, Hans Verkuil wrote:
> On 07/13/2015 11:18 AM, Daniel Vetter wrote:
> > On Mon, Jul 13, 2015 at 10:29:32AM +0200, Hans Verkuil wrote:
> >> On 06/15/2015 08:53 AM, Daniel Vetter wrote:
> >>> On Tue, Jun 09, 2015 at 01:50:48PM +0100, Damien Lespiau wrote:
> >>>> On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
> >>>>> From: Kausal Malladi <Kausal.Malladi@intel.com>
> >>>>>
> >>>>> This patch set adds color manager implementation in drm/i915 layer.
> >>>>> Color Manager is an extension in i915 driver to support color 
> >>>>> correction/enhancement. Various Intel platforms support several
> >>>>> color correction capabilities. Color Manager provides abstraction
> >>>>> of these properties and allows a user space UI agent to 
> >>>>> correct/enhance the display.
> >>>>
> >>>> So I did a first rough pass on the API itself. The big question that
> >>>> isn't solved at the moment is: do we want to try to do generic KMS
> >>>> properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:
> >>>>
> >>>>   1/ Generic for all KMS drivers
> >>>>   2/ Generic for i915 supported platfoms
> >>>>   3/ Specific to each platform
> >>>>
> >>>> At this point, I'm quite tempted to say we should give 1/ a shot. We
> >>>> should be able to have pre-LUT + matrix + post-LUT on CRTC objects and
> >>>> guarantee that, when the drivers expose such properties, user space can
> >>>> at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.
> >>>>
> >>>> It may be possible to use the "try" version of the atomic ioctl to
> >>>> explore the space of possibilities from a generic user space to use
> >>>> bigger LUTs as well. A HAL layer (which is already there in some but not
> >>>> all OSes) would still be able to use those generic properties to load
> >>>> "precision optimized" LUTs with some knowledge of the hardware.
> >>>
> >>> Yeah, imo 1/ should be doable. For the matrix we should be able to be
> >>> fully generic with a 16.16 format. For gamma one option would be to have
> >>
> >> I know I am late replying, apologies for that.
> >>
> >> I've been working on CSC support for V4L2 as well (still work in progress)
> >> and I would like to at least end up with the same low-level fixed point
> >> format as DRM so we can share matrix/vector calculations.
> >>
> >> Based on my experiences I have concerns about the 16.16 format: the precision
> >> is quite low which can be a problem when such values are used in matrix
> >> multiplications.
> >>
> >> In addition, while the precision may be sufficient for 8 bit color component
> >> values, I'm pretty sure it will be insufficient when dealing with 12 or 16 bit
> >> color components.
> >>
> >> In earlier versions of my CSC code I used a 12.20 format, but in the latest I
> >> switched to 32.32. This fits nicely in a u64 and it's easy to extract the
> >> integer and fractional parts.
> >>
> >> If this is going to be a generic and future proof API, then my suggestion
> >> would be to increase the precision of the underlying data type.
> > 
> > We discussed this a bit more internally and figured it would be nice to have the same
> > fixed point for both CSC matrix and LUT/gamma tables. Current consensus
> > seems to be to go with 8.24 for both. Since LUTs are fairly big I think it
> > makes sense if we try to be not too wasteful (while still future-proof
> > ofc).
> 
> The .24 should have enough precision, but I am worried about the 8: while
> this works for 8 bit components, you can't use it to represent values
> >255, which might be needed (now or in the future) for 10, 12 or 16 bit
> color components.
> 
> It's why I ended up with 32.32: it's very generic so usable for other
> things besides CSC.
> 
> Note that 8.24 is really 7.24 + one sign bit. So 255 can't be represented
> in this format.
> 
> That said, all values I'm working with in my current code are small integers
> (say between -4 and 4 worst case), so 8.24 would work. But I am not at all
> confident that this is future proof. My gut feeling is that you need to be
> able to represent at least the max component value + a sign bit + 7 decimals
> precision. Which makes 17.24.

The idea is to steal from GL and always normalize everything to [0.0,
1.0], irrespective of the source color format. We need that in drm since
if you blend together planes with different formats it's completely
undefined which one you should pick. 8 bits of precision for values out of
range should be enough ;-)

Oh and we might need those since for CSC and at least some LUTs you can do
this. It's probably needed if your destination color space is much smaller
than the source and you need to expand it. Will result in some clamping
ofc.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 00/10] Color Manager Implementation

[Hans Verkuil]

On 07/13/2015 11:54 AM, Daniel Vetter wrote:
> On Mon, Jul 13, 2015 at 11:43:31AM +0200, Hans Verkuil wrote:
>> On 07/13/2015 11:18 AM, Daniel Vetter wrote:
>>> On Mon, Jul 13, 2015 at 10:29:32AM +0200, Hans Verkuil wrote:
>>>> On 06/15/2015 08:53 AM, Daniel Vetter wrote:
>>>>> On Tue, Jun 09, 2015 at 01:50:48PM +0100, Damien Lespiau wrote:
>>>>>> On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
>>>>>>> From: Kausal Malladi <Kausal.Malladi@intel.com>
>>>>>>>
>>>>>>> This patch set adds color manager implementation in drm/i915 layer.
>>>>>>> Color Manager is an extension in i915 driver to support color 
>>>>>>> correction/enhancement. Various Intel platforms support several
>>>>>>> color correction capabilities. Color Manager provides abstraction
>>>>>>> of these properties and allows a user space UI agent to 
>>>>>>> correct/enhance the display.
>>>>>>
>>>>>> So I did a first rough pass on the API itself. The big question that
>>>>>> isn't solved at the moment is: do we want to try to do generic KMS
>>>>>> properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:
>>>>>>
>>>>>>   1/ Generic for all KMS drivers
>>>>>>   2/ Generic for i915 supported platfoms
>>>>>>   3/ Specific to each platform
>>>>>>
>>>>>> At this point, I'm quite tempted to say we should give 1/ a shot. We
>>>>>> should be able to have pre-LUT + matrix + post-LUT on CRTC objects and
>>>>>> guarantee that, when the drivers expose such properties, user space can
>>>>>> at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.
>>>>>>
>>>>>> It may be possible to use the "try" version of the atomic ioctl to
>>>>>> explore the space of possibilities from a generic user space to use
>>>>>> bigger LUTs as well. A HAL layer (which is already there in some but not
>>>>>> all OSes) would still be able to use those generic properties to load
>>>>>> "precision optimized" LUTs with some knowledge of the hardware.
>>>>>
>>>>> Yeah, imo 1/ should be doable. For the matrix we should be able to be
>>>>> fully generic with a 16.16 format. For gamma one option would be to have
>>>>
>>>> I know I am late replying, apologies for that.
>>>>
>>>> I've been working on CSC support for V4L2 as well (still work in progress)
>>>> and I would like to at least end up with the same low-level fixed point
>>>> format as DRM so we can share matrix/vector calculations.
>>>>
>>>> Based on my experiences I have concerns about the 16.16 format: the precision
>>>> is quite low which can be a problem when such values are used in matrix
>>>> multiplications.
>>>>
>>>> In addition, while the precision may be sufficient for 8 bit color component
>>>> values, I'm pretty sure it will be insufficient when dealing with 12 or 16 bit
>>>> color components.
>>>>
>>>> In earlier versions of my CSC code I used a 12.20 format, but in the latest I
>>>> switched to 32.32. This fits nicely in a u64 and it's easy to extract the
>>>> integer and fractional parts.
>>>>
>>>> If this is going to be a generic and future proof API, then my suggestion
>>>> would be to increase the precision of the underlying data type.
>>>
>>> We discussed this a bit more internally and figured it would be nice to have the same
>>> fixed point for both CSC matrix and LUT/gamma tables. Current consensus
>>> seems to be to go with 8.24 for both. Since LUTs are fairly big I think it
>>> makes sense if we try to be not too wasteful (while still future-proof
>>> ofc).
>>
>> The .24 should have enough precision, but I am worried about the 8: while
>> this works for 8 bit components, you can't use it to represent values
>>> 255, which might be needed (now or in the future) for 10, 12 or 16 bit
>> color components.
>>
>> It's why I ended up with 32.32: it's very generic so usable for other
>> things besides CSC.
>>
>> Note that 8.24 is really 7.24 + one sign bit. So 255 can't be represented
>> in this format.
>>
>> That said, all values I'm working with in my current code are small integers
>> (say between -4 and 4 worst case), so 8.24 would work. But I am not at all
>> confident that this is future proof. My gut feeling is that you need to be
>> able to represent at least the max component value + a sign bit + 7 decimals
>> precision. Which makes 17.24.
> 
> The idea is to steal from GL and always normalize everything to [0.0,
> 1.0], irrespective of the source color format. We need that in drm since
> if you blend together planes with different formats it's completely
> undefined which one you should pick. 8 bits of precision for values out of
> range should be enough ;-)

That doesn't really help much, using a [0-1] range just means that you need
more precision for the fraction since the integer precision is now added to
the fractional precision.

So for 16-bit color components the 8.24 format will leave you with only 8 bits
precision if you scale each component to the [0-1] range. That's slightly more
than 2 decimals. I don't believe that is enough. If you do a gamma table lookup
and then feed the result to a CSC matrix you need more precision if you want
to get accurate results.

> Oh and we might need those since for CSC and at least some LUTs you can do
> this.

Sorry, I don't understand this sentence. What does 'those' and 'this' refer to?

> It's probably needed if your destination color space is much smaller
> than the source and you need to expand it. Will result in some clamping
> ofc.
> -Daniel
> 

Regards,

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

Re: [Intel-gfx] [PATCH v2 00/10] Color Manager Implementation

[Daniel Vetter]

On Mon, Jul 13, 2015 at 12:11:08PM +0200, Hans Verkuil wrote:
> On 07/13/2015 11:54 AM, Daniel Vetter wrote:
> > On Mon, Jul 13, 2015 at 11:43:31AM +0200, Hans Verkuil wrote:
> >> On 07/13/2015 11:18 AM, Daniel Vetter wrote:
> >>> On Mon, Jul 13, 2015 at 10:29:32AM +0200, Hans Verkuil wrote:
> >>>> On 06/15/2015 08:53 AM, Daniel Vetter wrote:
> >>>>> On Tue, Jun 09, 2015 at 01:50:48PM +0100, Damien Lespiau wrote:
> >>>>>> On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
> >>>>>>> From: Kausal Malladi <Kausal.Malladi@intel.com>
> >>>>>>>
> >>>>>>> This patch set adds color manager implementation in drm/i915 layer.
> >>>>>>> Color Manager is an extension in i915 driver to support color 
> >>>>>>> correction/enhancement. Various Intel platforms support several
> >>>>>>> color correction capabilities. Color Manager provides abstraction
> >>>>>>> of these properties and allows a user space UI agent to 
> >>>>>>> correct/enhance the display.
> >>>>>>
> >>>>>> So I did a first rough pass on the API itself. The big question that
> >>>>>> isn't solved at the moment is: do we want to try to do generic KMS
> >>>>>> properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:
> >>>>>>
> >>>>>>   1/ Generic for all KMS drivers
> >>>>>>   2/ Generic for i915 supported platfoms
> >>>>>>   3/ Specific to each platform
> >>>>>>
> >>>>>> At this point, I'm quite tempted to say we should give 1/ a shot. We
> >>>>>> should be able to have pre-LUT + matrix + post-LUT on CRTC objects and
> >>>>>> guarantee that, when the drivers expose such properties, user space can
> >>>>>> at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.
> >>>>>>
> >>>>>> It may be possible to use the "try" version of the atomic ioctl to
> >>>>>> explore the space of possibilities from a generic user space to use
> >>>>>> bigger LUTs as well. A HAL layer (which is already there in some but not
> >>>>>> all OSes) would still be able to use those generic properties to load
> >>>>>> "precision optimized" LUTs with some knowledge of the hardware.
> >>>>>
> >>>>> Yeah, imo 1/ should be doable. For the matrix we should be able to be
> >>>>> fully generic with a 16.16 format. For gamma one option would be to have
> >>>>
> >>>> I know I am late replying, apologies for that.
> >>>>
> >>>> I've been working on CSC support for V4L2 as well (still work in progress)
> >>>> and I would like to at least end up with the same low-level fixed point
> >>>> format as DRM so we can share matrix/vector calculations.
> >>>>
> >>>> Based on my experiences I have concerns about the 16.16 format: the precision
> >>>> is quite low which can be a problem when such values are used in matrix
> >>>> multiplications.
> >>>>
> >>>> In addition, while the precision may be sufficient for 8 bit color component
> >>>> values, I'm pretty sure it will be insufficient when dealing with 12 or 16 bit
> >>>> color components.
> >>>>
> >>>> In earlier versions of my CSC code I used a 12.20 format, but in the latest I
> >>>> switched to 32.32. This fits nicely in a u64 and it's easy to extract the
> >>>> integer and fractional parts.
> >>>>
> >>>> If this is going to be a generic and future proof API, then my suggestion
> >>>> would be to increase the precision of the underlying data type.
> >>>
> >>> We discussed this a bit more internally and figured it would be nice to have the same
> >>> fixed point for both CSC matrix and LUT/gamma tables. Current consensus
> >>> seems to be to go with 8.24 for both. Since LUTs are fairly big I think it
> >>> makes sense if we try to be not too wasteful (while still future-proof
> >>> ofc).
> >>
> >> The .24 should have enough precision, but I am worried about the 8: while
> >> this works for 8 bit components, you can't use it to represent values
> >>> 255, which might be needed (now or in the future) for 10, 12 or 16 bit
> >> color components.
> >>
> >> It's why I ended up with 32.32: it's very generic so usable for other
> >> things besides CSC.
> >>
> >> Note that 8.24 is really 7.24 + one sign bit. So 255 can't be represented
> >> in this format.
> >>
> >> That said, all values I'm working with in my current code are small integers
> >> (say between -4 and 4 worst case), so 8.24 would work. But I am not at all
> >> confident that this is future proof. My gut feeling is that you need to be
> >> able to represent at least the max component value + a sign bit + 7 decimals
> >> precision. Which makes 17.24.
> > 
> > The idea is to steal from GL and always normalize everything to [0.0,
> > 1.0], irrespective of the source color format. We need that in drm since
> > if you blend together planes with different formats it's completely
> > undefined which one you should pick. 8 bits of precision for values out of
> > range should be enough ;-)
> 
> That doesn't really help much, using a [0-1] range just means that you need
> more precision for the fraction since the integer precision is now added to
> the fractional precision.
> 
> So for 16-bit color components the 8.24 format will leave you with only 8 bits
> precision if you scale each component to the [0-1] range. That's slightly more
> than 2 decimals. I don't believe that is enough. If you do a gamma table lookup
> and then feed the result to a CSC matrix you need more precision if you want
> to get accurate results.

Hm, why do we need 8 bits more precision than source data? At least in the
intel hw I've seen the most bits we can stuff into the hw is 0.12 (again
for rescaled range to 0.0-1.0). 24 bits means as-is we'll throw 12 bits
away. What would you want to use these bits for?

> > Oh and we might need those since for CSC and at least some LUTs you can do
> > this.
> 
> Sorry, I don't understand this sentence. What does 'those' and 'this' refer to?

I meant that the higher bits before the decimal are needed in some cases
by the hw since it allows values > logical 1.0. At least some hw supports
16bit (half) floats as scanout sources too.
-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: [Intel-gfx] [PATCH v2 00/10] Color Manager Implementation

[Hans Verkuil]

On 07/13/15 16:07, Daniel Vetter wrote:
> On Mon, Jul 13, 2015 at 12:11:08PM +0200, Hans Verkuil wrote:
>> On 07/13/2015 11:54 AM, Daniel Vetter wrote:
>>> On Mon, Jul 13, 2015 at 11:43:31AM +0200, Hans Verkuil wrote:
>>>> On 07/13/2015 11:18 AM, Daniel Vetter wrote:
>>>>> On Mon, Jul 13, 2015 at 10:29:32AM +0200, Hans Verkuil wrote:
>>>>>> On 06/15/2015 08:53 AM, Daniel Vetter wrote:
>>>>>>> On Tue, Jun 09, 2015 at 01:50:48PM +0100, Damien Lespiau wrote:
>>>>>>>> On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
>>>>>>>>> From: Kausal Malladi <Kausal.Malladi@intel.com>
>>>>>>>>>
>>>>>>>>> This patch set adds color manager implementation in drm/i915 layer.
>>>>>>>>> Color Manager is an extension in i915 driver to support color 
>>>>>>>>> correction/enhancement. Various Intel platforms support several
>>>>>>>>> color correction capabilities. Color Manager provides abstraction
>>>>>>>>> of these properties and allows a user space UI agent to 
>>>>>>>>> correct/enhance the display.
>>>>>>>>
>>>>>>>> So I did a first rough pass on the API itself. The big question that
>>>>>>>> isn't solved at the moment is: do we want to try to do generic KMS
>>>>>>>> properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:
>>>>>>>>
>>>>>>>>   1/ Generic for all KMS drivers
>>>>>>>>   2/ Generic for i915 supported platfoms
>>>>>>>>   3/ Specific to each platform
>>>>>>>>
>>>>>>>> At this point, I'm quite tempted to say we should give 1/ a shot. We
>>>>>>>> should be able to have pre-LUT + matrix + post-LUT on CRTC objects and
>>>>>>>> guarantee that, when the drivers expose such properties, user space can
>>>>>>>> at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.
>>>>>>>>
>>>>>>>> It may be possible to use the "try" version of the atomic ioctl to
>>>>>>>> explore the space of possibilities from a generic user space to use
>>>>>>>> bigger LUTs as well. A HAL layer (which is already there in some but not
>>>>>>>> all OSes) would still be able to use those generic properties to load
>>>>>>>> "precision optimized" LUTs with some knowledge of the hardware.
>>>>>>>
>>>>>>> Yeah, imo 1/ should be doable. For the matrix we should be able to be
>>>>>>> fully generic with a 16.16 format. For gamma one option would be to have
>>>>>>
>>>>>> I know I am late replying, apologies for that.
>>>>>>
>>>>>> I've been working on CSC support for V4L2 as well (still work in progress)
>>>>>> and I would like to at least end up with the same low-level fixed point
>>>>>> format as DRM so we can share matrix/vector calculations.
>>>>>>
>>>>>> Based on my experiences I have concerns about the 16.16 format: the precision
>>>>>> is quite low which can be a problem when such values are used in matrix
>>>>>> multiplications.
>>>>>>
>>>>>> In addition, while the precision may be sufficient for 8 bit color component
>>>>>> values, I'm pretty sure it will be insufficient when dealing with 12 or 16 bit
>>>>>> color components.
>>>>>>
>>>>>> In earlier versions of my CSC code I used a 12.20 format, but in the latest I
>>>>>> switched to 32.32. This fits nicely in a u64 and it's easy to extract the
>>>>>> integer and fractional parts.
>>>>>>
>>>>>> If this is going to be a generic and future proof API, then my suggestion
>>>>>> would be to increase the precision of the underlying data type.
>>>>>
>>>>> We discussed this a bit more internally and figured it would be nice to have the same
>>>>> fixed point for both CSC matrix and LUT/gamma tables. Current consensus
>>>>> seems to be to go with 8.24 for both. Since LUTs are fairly big I think it
>>>>> makes sense if we try to be not too wasteful (while still future-proof
>>>>> ofc).
>>>>
>>>> The .24 should have enough precision, but I am worried about the 8: while
>>>> this works for 8 bit components, you can't use it to represent values
>>>>> 255, which might be needed (now or in the future) for 10, 12 or 16 bit
>>>> color components.
>>>>
>>>> It's why I ended up with 32.32: it's very generic so usable for other
>>>> things besides CSC.
>>>>
>>>> Note that 8.24 is really 7.24 + one sign bit. So 255 can't be represented
>>>> in this format.
>>>>
>>>> That said, all values I'm working with in my current code are small integers
>>>> (say between -4 and 4 worst case), so 8.24 would work. But I am not at all
>>>> confident that this is future proof. My gut feeling is that you need to be
>>>> able to represent at least the max component value + a sign bit + 7 decimals
>>>> precision. Which makes 17.24.
>>>
>>> The idea is to steal from GL and always normalize everything to [0.0,
>>> 1.0], irrespective of the source color format. We need that in drm since
>>> if you blend together planes with different formats it's completely
>>> undefined which one you should pick. 8 bits of precision for values out of
>>> range should be enough ;-)
>>
>> That doesn't really help much, using a [0-1] range just means that you need
>> more precision for the fraction since the integer precision is now added to
>> the fractional precision.
>>
>> So for 16-bit color components the 8.24 format will leave you with only 8 bits
>> precision if you scale each component to the [0-1] range. That's slightly more
>> than 2 decimals. I don't believe that is enough. If you do a gamma table lookup
>> and then feed the result to a CSC matrix you need more precision if you want
>> to get accurate results.
> 
> Hm, why do we need 8 bits more precision than source data? At least in the
> intel hw I've seen the most bits we can stuff into the hw is 0.12 (again
> for rescaled range to 0.0-1.0). 24 bits means as-is we'll throw 12 bits
> away. What would you want to use these bits for?

The intel hardware uses 12 bits today, but what about the next-gen? If you are
defining an API and data type just for the hardware the kernel supports today,
then 12 bits might be enough precision. If you want to be future proof then you
need to be prepared for more capable future hardware.

So 0.12 will obviously not be enough if you want to support 16 bit color components
in the future.

In addition, to fully support HW colorspace conversion (e.g. sRGB to Rec.709) where
lookup tables are used for implementing the transfer functions (normal and inverse),
then you need more precision then just the number of bits per component or you will
get quite large errors in the calculation.

It all depends how a LUT is used: if the value from the LUT is the 'final' value,
then you don't need more precision than the number of bits of a color component. But
if it is used in other calculations (3x3 matrices, full/limited range scaling, etc),
then the LUT should provide more bits precision.

Which seems to be the case with Intel hardware: 12 bits is 4 bits more than the 8 bits
per component it probably uses.

I would guess that a LUT supporting 16 bit color components would need a precision
of 0.20 or so (assuming the resulting values are used in further calculations).

High dynamic range video will be an important driving force towards higher bit depths
and accurate color handling, so you can expect to see this become much more important
in the coming years.

And as I mentioned another consideration is that this fixed point data type might
be useful elsewhere in the kernel where you need to do some precision arithmetic.
So using a standard type that anyone can use with functions in lib/ to do basic
operations can be very useful indeed beyond just DRM and V4L2.

> 
>>> Oh and we might need those since for CSC and at least some LUTs you can do
>>> this.
>>
>> Sorry, I don't understand this sentence. What does 'those' and 'this' refer to?
> 
> I meant that the higher bits before the decimal are needed in some cases
> by the hw since it allows values > logical 1.0. At least some hw supports
> 16bit (half) floats as scanout sources too.

Ah, OK. Thanks for the clarification.

Regards,

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

Re: [Intel-gfx] [PATCH v2 00/10] Color Manager Implementation

[Daniel Vetter]

On Tue, Jul 14, 2015 at 10:17:09AM +0200, Hans Verkuil wrote:
> On 07/13/15 16:07, Daniel Vetter wrote:
> > On Mon, Jul 13, 2015 at 12:11:08PM +0200, Hans Verkuil wrote:
> >> On 07/13/2015 11:54 AM, Daniel Vetter wrote:
> >>> On Mon, Jul 13, 2015 at 11:43:31AM +0200, Hans Verkuil wrote:
> >>>> On 07/13/2015 11:18 AM, Daniel Vetter wrote:
> >>>>> On Mon, Jul 13, 2015 at 10:29:32AM +0200, Hans Verkuil wrote:
> >>>>>> On 06/15/2015 08:53 AM, Daniel Vetter wrote:
> >>>>>>> On Tue, Jun 09, 2015 at 01:50:48PM +0100, Damien Lespiau wrote:
> >>>>>>>> On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
> >>>>>>>>> From: Kausal Malladi <Kausal.Malladi@intel.com>
> >>>>>>>>>
> >>>>>>>>> This patch set adds color manager implementation in drm/i915 layer.
> >>>>>>>>> Color Manager is an extension in i915 driver to support color 
> >>>>>>>>> correction/enhancement. Various Intel platforms support several
> >>>>>>>>> color correction capabilities. Color Manager provides abstraction
> >>>>>>>>> of these properties and allows a user space UI agent to 
> >>>>>>>>> correct/enhance the display.
> >>>>>>>>
> >>>>>>>> So I did a first rough pass on the API itself. The big question that
> >>>>>>>> isn't solved at the moment is: do we want to try to do generic KMS
> >>>>>>>> properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:
> >>>>>>>>
> >>>>>>>>   1/ Generic for all KMS drivers
> >>>>>>>>   2/ Generic for i915 supported platfoms
> >>>>>>>>   3/ Specific to each platform
> >>>>>>>>
> >>>>>>>> At this point, I'm quite tempted to say we should give 1/ a shot. We
> >>>>>>>> should be able to have pre-LUT + matrix + post-LUT on CRTC objects and
> >>>>>>>> guarantee that, when the drivers expose such properties, user space can
> >>>>>>>> at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.
> >>>>>>>>
> >>>>>>>> It may be possible to use the "try" version of the atomic ioctl to
> >>>>>>>> explore the space of possibilities from a generic user space to use
> >>>>>>>> bigger LUTs as well. A HAL layer (which is already there in some but not
> >>>>>>>> all OSes) would still be able to use those generic properties to load
> >>>>>>>> "precision optimized" LUTs with some knowledge of the hardware.
> >>>>>>>
> >>>>>>> Yeah, imo 1/ should be doable. For the matrix we should be able to be
> >>>>>>> fully generic with a 16.16 format. For gamma one option would be to have
> >>>>>>
> >>>>>> I know I am late replying, apologies for that.
> >>>>>>
> >>>>>> I've been working on CSC support for V4L2 as well (still work in progress)
> >>>>>> and I would like to at least end up with the same low-level fixed point
> >>>>>> format as DRM so we can share matrix/vector calculations.
> >>>>>>
> >>>>>> Based on my experiences I have concerns about the 16.16 format: the precision
> >>>>>> is quite low which can be a problem when such values are used in matrix
> >>>>>> multiplications.
> >>>>>>
> >>>>>> In addition, while the precision may be sufficient for 8 bit color component
> >>>>>> values, I'm pretty sure it will be insufficient when dealing with 12 or 16 bit
> >>>>>> color components.
> >>>>>>
> >>>>>> In earlier versions of my CSC code I used a 12.20 format, but in the latest I
> >>>>>> switched to 32.32. This fits nicely in a u64 and it's easy to extract the
> >>>>>> integer and fractional parts.
> >>>>>>
> >>>>>> If this is going to be a generic and future proof API, then my suggestion
> >>>>>> would be to increase the precision of the underlying data type.
> >>>>>
> >>>>> We discussed this a bit more internally and figured it would be nice to have the same
> >>>>> fixed point for both CSC matrix and LUT/gamma tables. Current consensus
> >>>>> seems to be to go with 8.24 for both. Since LUTs are fairly big I think it
> >>>>> makes sense if we try to be not too wasteful (while still future-proof
> >>>>> ofc).
> >>>>
> >>>> The .24 should have enough precision, but I am worried about the 8: while
> >>>> this works for 8 bit components, you can't use it to represent values
> >>>>> 255, which might be needed (now or in the future) for 10, 12 or 16 bit
> >>>> color components.
> >>>>
> >>>> It's why I ended up with 32.32: it's very generic so usable for other
> >>>> things besides CSC.
> >>>>
> >>>> Note that 8.24 is really 7.24 + one sign bit. So 255 can't be represented
> >>>> in this format.
> >>>>
> >>>> That said, all values I'm working with in my current code are small integers
> >>>> (say between -4 and 4 worst case), so 8.24 would work. But I am not at all
> >>>> confident that this is future proof. My gut feeling is that you need to be
> >>>> able to represent at least the max component value + a sign bit + 7 decimals
> >>>> precision. Which makes 17.24.
> >>>
> >>> The idea is to steal from GL and always normalize everything to [0.0,
> >>> 1.0], irrespective of the source color format. We need that in drm since
> >>> if you blend together planes with different formats it's completely
> >>> undefined which one you should pick. 8 bits of precision for values out of
> >>> range should be enough ;-)
> >>
> >> That doesn't really help much, using a [0-1] range just means that you need
> >> more precision for the fraction since the integer precision is now added to
> >> the fractional precision.
> >>
> >> So for 16-bit color components the 8.24 format will leave you with only 8 bits
> >> precision if you scale each component to the [0-1] range. That's slightly more
> >> than 2 decimals. I don't believe that is enough. If you do a gamma table lookup
> >> and then feed the result to a CSC matrix you need more precision if you want
> >> to get accurate results.
> > 
> > Hm, why do we need 8 bits more precision than source data? At least in the
> > intel hw I've seen the most bits we can stuff into the hw is 0.12 (again
> > for rescaled range to 0.0-1.0). 24 bits means as-is we'll throw 12 bits
> > away. What would you want to use these bits for?
> 
> The intel hardware uses 12 bits today, but what about the next-gen? If you are
> defining an API and data type just for the hardware the kernel supports today,
> then 12 bits might be enough precision. If you want to be future proof then you
> need to be prepared for more capable future hardware.
> 
> So 0.12 will obviously not be enough if you want to support 16 bit color components
> in the future.
> 
> In addition, to fully support HW colorspace conversion (e.g. sRGB to Rec.709) where
> lookup tables are used for implementing the transfer functions (normal and inverse),
> then you need more precision then just the number of bits per component or you will
> get quite large errors in the calculation.
> 
> It all depends how a LUT is used: if the value from the LUT is the 'final' value,
> then you don't need more precision than the number of bits of a color component. But
> if it is used in other calculations (3x3 matrices, full/limited range scaling, etc),
> then the LUT should provide more bits precision.
> 
> Which seems to be the case with Intel hardware: 12 bits is 4 bits more than the 8 bits
> per component it probably uses.

Intel hw supports a 12bpp pixel pipeline. They didnt add _any_ additional
precision at all afaik. Which is why I wonder why we need it. I'm also not
aware of any plans for pushing past 12bpp of data sent to the sink, but I
honestly don't have much clue really.

I guess input is a different story, todays cmos already easily to 14bit
with more to come I guess with all the noise about HDR. We probably need
more headroom on v4l input side than we ever need on drm display side.
Still 24bits is an awful lot of headroom, at least for the next few years.
Or do you expect to hit that already soonish on v4l side?

> I would guess that a LUT supporting 16 bit color components would need a precision
> of 0.20 or so (assuming the resulting values are used in further calculations).
> 
> High dynamic range video will be an important driving force towards higher bit depths
> and accurate color handling, so you can expect to see this become much more important
> in the coming years.
> 
> And as I mentioned another consideration is that this fixed point data type might
> be useful elsewhere in the kernel where you need to do some precision arithmetic.
> So using a standard type that anyone can use with functions in lib/ to do basic
> operations can be very useful indeed beyond just DRM and V4L2.

0.20 would still comfortably fit into 8.24. And yeah worst-case (in 10
years or so) we need to add a high-bpp variant if it really comes to it.
But for the next few years (and that's the pace at which we completely
rewrite gfx drivers anyway) I don't see anything going past .24. .16 was
indeed a bit too little I think, which is why we decided to move the fixed
point a bit.
-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 v2 00/10] Color Manager Implementation

[Hans Verkuil]

On 07/14/15 11:11, Daniel Vetter wrote:
> On Tue, Jul 14, 2015 at 10:17:09AM +0200, Hans Verkuil wrote:
>> On 07/13/15 16:07, Daniel Vetter wrote:
>>> On Mon, Jul 13, 2015 at 12:11:08PM +0200, Hans Verkuil wrote:
>>>> On 07/13/2015 11:54 AM, Daniel Vetter wrote:
>>>>> On Mon, Jul 13, 2015 at 11:43:31AM +0200, Hans Verkuil wrote:
>>>>>> On 07/13/2015 11:18 AM, Daniel Vetter wrote:
>>>>>>> On Mon, Jul 13, 2015 at 10:29:32AM +0200, Hans Verkuil wrote:
>>>>>>>> On 06/15/2015 08:53 AM, Daniel Vetter wrote:
>>>>>>>>> On Tue, Jun 09, 2015 at 01:50:48PM +0100, Damien Lespiau wrote:
>>>>>>>>>> On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
>>>>>>>>>>> From: Kausal Malladi <Kausal.Malladi@intel.com>
>>>>>>>>>>>
>>>>>>>>>>> This patch set adds color manager implementation in drm/i915 layer.
>>>>>>>>>>> Color Manager is an extension in i915 driver to support color 
>>>>>>>>>>> correction/enhancement. Various Intel platforms support several
>>>>>>>>>>> color correction capabilities. Color Manager provides abstraction
>>>>>>>>>>> of these properties and allows a user space UI agent to 
>>>>>>>>>>> correct/enhance the display.
>>>>>>>>>>
>>>>>>>>>> So I did a first rough pass on the API itself. The big question that
>>>>>>>>>> isn't solved at the moment is: do we want to try to do generic KMS
>>>>>>>>>> properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:
>>>>>>>>>>
>>>>>>>>>>   1/ Generic for all KMS drivers
>>>>>>>>>>   2/ Generic for i915 supported platfoms
>>>>>>>>>>   3/ Specific to each platform
>>>>>>>>>>
>>>>>>>>>> At this point, I'm quite tempted to say we should give 1/ a shot. We
>>>>>>>>>> should be able to have pre-LUT + matrix + post-LUT on CRTC objects and
>>>>>>>>>> guarantee that, when the drivers expose such properties, user space can
>>>>>>>>>> at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.
>>>>>>>>>>
>>>>>>>>>> It may be possible to use the "try" version of the atomic ioctl to
>>>>>>>>>> explore the space of possibilities from a generic user space to use
>>>>>>>>>> bigger LUTs as well. A HAL layer (which is already there in some but not
>>>>>>>>>> all OSes) would still be able to use those generic properties to load
>>>>>>>>>> "precision optimized" LUTs with some knowledge of the hardware.
>>>>>>>>>
>>>>>>>>> Yeah, imo 1/ should be doable. For the matrix we should be able to be
>>>>>>>>> fully generic with a 16.16 format. For gamma one option would be to have
>>>>>>>>
>>>>>>>> I know I am late replying, apologies for that.
>>>>>>>>
>>>>>>>> I've been working on CSC support for V4L2 as well (still work in progress)
>>>>>>>> and I would like to at least end up with the same low-level fixed point
>>>>>>>> format as DRM so we can share matrix/vector calculations.
>>>>>>>>
>>>>>>>> Based on my experiences I have concerns about the 16.16 format: the precision
>>>>>>>> is quite low which can be a problem when such values are used in matrix
>>>>>>>> multiplications.
>>>>>>>>
>>>>>>>> In addition, while the precision may be sufficient for 8 bit color component
>>>>>>>> values, I'm pretty sure it will be insufficient when dealing with 12 or 16 bit
>>>>>>>> color components.
>>>>>>>>
>>>>>>>> In earlier versions of my CSC code I used a 12.20 format, but in the latest I
>>>>>>>> switched to 32.32. This fits nicely in a u64 and it's easy to extract the
>>>>>>>> integer and fractional parts.
>>>>>>>>
>>>>>>>> If this is going to be a generic and future proof API, then my suggestion
>>>>>>>> would be to increase the precision of the underlying data type.
>>>>>>>
>>>>>>> We discussed this a bit more internally and figured it would be nice to have the same
>>>>>>> fixed point for both CSC matrix and LUT/gamma tables. Current consensus
>>>>>>> seems to be to go with 8.24 for both. Since LUTs are fairly big I think it
>>>>>>> makes sense if we try to be not too wasteful (while still future-proof
>>>>>>> ofc).
>>>>>>
>>>>>> The .24 should have enough precision, but I am worried about the 8: while
>>>>>> this works for 8 bit components, you can't use it to represent values
>>>>>>> 255, which might be needed (now or in the future) for 10, 12 or 16 bit
>>>>>> color components.
>>>>>>
>>>>>> It's why I ended up with 32.32: it's very generic so usable for other
>>>>>> things besides CSC.
>>>>>>
>>>>>> Note that 8.24 is really 7.24 + one sign bit. So 255 can't be represented
>>>>>> in this format.
>>>>>>
>>>>>> That said, all values I'm working with in my current code are small integers
>>>>>> (say between -4 and 4 worst case), so 8.24 would work. But I am not at all
>>>>>> confident that this is future proof. My gut feeling is that you need to be
>>>>>> able to represent at least the max component value + a sign bit + 7 decimals
>>>>>> precision. Which makes 17.24.
>>>>>
>>>>> The idea is to steal from GL and always normalize everything to [0.0,
>>>>> 1.0], irrespective of the source color format. We need that in drm since
>>>>> if you blend together planes with different formats it's completely
>>>>> undefined which one you should pick. 8 bits of precision for values out of
>>>>> range should be enough ;-)
>>>>
>>>> That doesn't really help much, using a [0-1] range just means that you need
>>>> more precision for the fraction since the integer precision is now added to
>>>> the fractional precision.
>>>>
>>>> So for 16-bit color components the 8.24 format will leave you with only 8 bits
>>>> precision if you scale each component to the [0-1] range. That's slightly more
>>>> than 2 decimals. I don't believe that is enough. If you do a gamma table lookup
>>>> and then feed the result to a CSC matrix you need more precision if you want
>>>> to get accurate results.
>>>
>>> Hm, why do we need 8 bits more precision than source data? At least in the
>>> intel hw I've seen the most bits we can stuff into the hw is 0.12 (again
>>> for rescaled range to 0.0-1.0). 24 bits means as-is we'll throw 12 bits
>>> away. What would you want to use these bits for?
>>
>> The intel hardware uses 12 bits today, but what about the next-gen? If you are
>> defining an API and data type just for the hardware the kernel supports today,
>> then 12 bits might be enough precision. If you want to be future proof then you
>> need to be prepared for more capable future hardware.
>>
>> So 0.12 will obviously not be enough if you want to support 16 bit color components
>> in the future.
>>
>> In addition, to fully support HW colorspace conversion (e.g. sRGB to Rec.709) where
>> lookup tables are used for implementing the transfer functions (normal and inverse),
>> then you need more precision then just the number of bits per component or you will
>> get quite large errors in the calculation.
>>
>> It all depends how a LUT is used: if the value from the LUT is the 'final' value,
>> then you don't need more precision than the number of bits of a color component. But
>> if it is used in other calculations (3x3 matrices, full/limited range scaling, etc),
>> then the LUT should provide more bits precision.
>>
>> Which seems to be the case with Intel hardware: 12 bits is 4 bits more than the 8 bits
>> per component it probably uses.
> 
> Intel hw supports a 12bpp pixel pipeline. They didnt add _any_ additional
> precision at all afaik. Which is why I wonder why we need it. I'm also not
> aware of any plans for pushing past 12bpp of data sent to the sink, but I
> honestly don't have much clue really.
> 
> I guess input is a different story, todays cmos already easily to 14bit
> with more to come I guess with all the noise about HDR. We probably need
> more headroom on v4l input side than we ever need on drm display side.
> Still 24bits is an awful lot of headroom, at least for the next few years.
> Or do you expect to hit that already soonish on v4l side?

I think 24 bits precision is enough, but that assumes that the integer part
will be between -128 and 127. And I am not so sure that that is a valid assumption.

It's true today, but what if you have a HW LUT that maps integer values and expects
16.0 or perhaps 12.4?

BTW, I am assuming that the proposed 8.24 format is a signed format: the CSC
3x3 matrices contain negative values, so any fixed point data type has to be signed.

I'm just wondering: is it really such a big deal to use a 32.32 format? Yes, the
amount of data doubles, but it's quite rare that you need to configure a LUT, right?

For a 12 bit LUT it's 16 kB vs 32 kB. Yes, it's more data, but the advantage is that
the data type is future proof (well, probably :-) ) and much more likely to be usable
in other subsystems.

>> I would guess that a LUT supporting 16 bit color components would need a precision
>> of 0.20 or so (assuming the resulting values are used in further calculations).
>>
>> High dynamic range video will be an important driving force towards higher bit depths
>> and accurate color handling, so you can expect to see this become much more important
>> in the coming years.
>>
>> And as I mentioned another consideration is that this fixed point data type might
>> be useful elsewhere in the kernel where you need to do some precision arithmetic.
>> So using a standard type that anyone can use with functions in lib/ to do basic
>> operations can be very useful indeed beyond just DRM and V4L2.
> 
> 0.20 would still comfortably fit into 8.24. And yeah worst-case (in 10
> years or so) we need to add a high-bpp variant if it really comes to it.

I think this is much closer than you think. I agree that you are not likely to see
this soon for consumer graphics cards, but for professional equipment and high-end
consumer electronics this is another story.

And if it is being done for input, then output will need it as well: after all,
what's the point of 16-bit color components if you can't display it? Whether Intel
will support it is another matter, but there are other vendors, you know... :-)

Regards,

	Hans

> But for the next few years (and that's the pace at which we completely
> rewrite gfx drivers anyway) I don't see anything going past .24. .16 was
> indeed a bit too little I think, which is why we decided to move the fixed
> point a bit.
> -Daniel
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 00/10] Color Manager Implementation

[Daniel Vetter]

On Tue, Jul 14, 2015 at 11:35:30AM +0200, Hans Verkuil wrote:
> On 07/14/15 11:11, Daniel Vetter wrote:
> > On Tue, Jul 14, 2015 at 10:17:09AM +0200, Hans Verkuil wrote:
> >> On 07/13/15 16:07, Daniel Vetter wrote:
> >>> On Mon, Jul 13, 2015 at 12:11:08PM +0200, Hans Verkuil wrote:
> >>>> On 07/13/2015 11:54 AM, Daniel Vetter wrote:
> >>>>> On Mon, Jul 13, 2015 at 11:43:31AM +0200, Hans Verkuil wrote:
> >>>>>> On 07/13/2015 11:18 AM, Daniel Vetter wrote:
> >>>>>>> On Mon, Jul 13, 2015 at 10:29:32AM +0200, Hans Verkuil wrote:
> >>>>>>>> On 06/15/2015 08:53 AM, Daniel Vetter wrote:
> >>>>>>>>> On Tue, Jun 09, 2015 at 01:50:48PM +0100, Damien Lespiau wrote:
> >>>>>>>>>> On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
> >>>>>>>>>>> From: Kausal Malladi <Kausal.Malladi@intel.com>
> >>>>>>>>>>>
> >>>>>>>>>>> This patch set adds color manager implementation in drm/i915 layer.
> >>>>>>>>>>> Color Manager is an extension in i915 driver to support color 
> >>>>>>>>>>> correction/enhancement. Various Intel platforms support several
> >>>>>>>>>>> color correction capabilities. Color Manager provides abstraction
> >>>>>>>>>>> of these properties and allows a user space UI agent to 
> >>>>>>>>>>> correct/enhance the display.
> >>>>>>>>>>
> >>>>>>>>>> So I did a first rough pass on the API itself. The big question that
> >>>>>>>>>> isn't solved at the moment is: do we want to try to do generic KMS
> >>>>>>>>>> properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:
> >>>>>>>>>>
> >>>>>>>>>>   1/ Generic for all KMS drivers
> >>>>>>>>>>   2/ Generic for i915 supported platfoms
> >>>>>>>>>>   3/ Specific to each platform
> >>>>>>>>>>
> >>>>>>>>>> At this point, I'm quite tempted to say we should give 1/ a shot. We
> >>>>>>>>>> should be able to have pre-LUT + matrix + post-LUT on CRTC objects and
> >>>>>>>>>> guarantee that, when the drivers expose such properties, user space can
> >>>>>>>>>> at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.
> >>>>>>>>>>
> >>>>>>>>>> It may be possible to use the "try" version of the atomic ioctl to
> >>>>>>>>>> explore the space of possibilities from a generic user space to use
> >>>>>>>>>> bigger LUTs as well. A HAL layer (which is already there in some but not
> >>>>>>>>>> all OSes) would still be able to use those generic properties to load
> >>>>>>>>>> "precision optimized" LUTs with some knowledge of the hardware.
> >>>>>>>>>
> >>>>>>>>> Yeah, imo 1/ should be doable. For the matrix we should be able to be
> >>>>>>>>> fully generic with a 16.16 format. For gamma one option would be to have
> >>>>>>>>
> >>>>>>>> I know I am late replying, apologies for that.
> >>>>>>>>
> >>>>>>>> I've been working on CSC support for V4L2 as well (still work in progress)
> >>>>>>>> and I would like to at least end up with the same low-level fixed point
> >>>>>>>> format as DRM so we can share matrix/vector calculations.
> >>>>>>>>
> >>>>>>>> Based on my experiences I have concerns about the 16.16 format: the precision
> >>>>>>>> is quite low which can be a problem when such values are used in matrix
> >>>>>>>> multiplications.
> >>>>>>>>
> >>>>>>>> In addition, while the precision may be sufficient for 8 bit color component
> >>>>>>>> values, I'm pretty sure it will be insufficient when dealing with 12 or 16 bit
> >>>>>>>> color components.
> >>>>>>>>
> >>>>>>>> In earlier versions of my CSC code I used a 12.20 format, but in the latest I
> >>>>>>>> switched to 32.32. This fits nicely in a u64 and it's easy to extract the
> >>>>>>>> integer and fractional parts.
> >>>>>>>>
> >>>>>>>> If this is going to be a generic and future proof API, then my suggestion
> >>>>>>>> would be to increase the precision of the underlying data type.
> >>>>>>>
> >>>>>>> We discussed this a bit more internally and figured it would be nice to have the same
> >>>>>>> fixed point for both CSC matrix and LUT/gamma tables. Current consensus
> >>>>>>> seems to be to go with 8.24 for both. Since LUTs are fairly big I think it
> >>>>>>> makes sense if we try to be not too wasteful (while still future-proof
> >>>>>>> ofc).
> >>>>>>
> >>>>>> The .24 should have enough precision, but I am worried about the 8: while
> >>>>>> this works for 8 bit components, you can't use it to represent values
> >>>>>>> 255, which might be needed (now or in the future) for 10, 12 or 16 bit
> >>>>>> color components.
> >>>>>>
> >>>>>> It's why I ended up with 32.32: it's very generic so usable for other
> >>>>>> things besides CSC.
> >>>>>>
> >>>>>> Note that 8.24 is really 7.24 + one sign bit. So 255 can't be represented
> >>>>>> in this format.
> >>>>>>
> >>>>>> That said, all values I'm working with in my current code are small integers
> >>>>>> (say between -4 and 4 worst case), so 8.24 would work. But I am not at all
> >>>>>> confident that this is future proof. My gut feeling is that you need to be
> >>>>>> able to represent at least the max component value + a sign bit + 7 decimals
> >>>>>> precision. Which makes 17.24.
> >>>>>
> >>>>> The idea is to steal from GL and always normalize everything to [0.0,
> >>>>> 1.0], irrespective of the source color format. We need that in drm since
> >>>>> if you blend together planes with different formats it's completely
> >>>>> undefined which one you should pick. 8 bits of precision for values out of
> >>>>> range should be enough ;-)
> >>>>
> >>>> That doesn't really help much, using a [0-1] range just means that you need
> >>>> more precision for the fraction since the integer precision is now added to
> >>>> the fractional precision.
> >>>>
> >>>> So for 16-bit color components the 8.24 format will leave you with only 8 bits
> >>>> precision if you scale each component to the [0-1] range. That's slightly more
> >>>> than 2 decimals. I don't believe that is enough. If you do a gamma table lookup
> >>>> and then feed the result to a CSC matrix you need more precision if you want
> >>>> to get accurate results.
> >>>
> >>> Hm, why do we need 8 bits more precision than source data? At least in the
> >>> intel hw I've seen the most bits we can stuff into the hw is 0.12 (again
> >>> for rescaled range to 0.0-1.0). 24 bits means as-is we'll throw 12 bits
> >>> away. What would you want to use these bits for?
> >>
> >> The intel hardware uses 12 bits today, but what about the next-gen? If you are
> >> defining an API and data type just for the hardware the kernel supports today,
> >> then 12 bits might be enough precision. If you want to be future proof then you
> >> need to be prepared for more capable future hardware.
> >>
> >> So 0.12 will obviously not be enough if you want to support 16 bit color components
> >> in the future.
> >>
> >> In addition, to fully support HW colorspace conversion (e.g. sRGB to Rec.709) where
> >> lookup tables are used for implementing the transfer functions (normal and inverse),
> >> then you need more precision then just the number of bits per component or you will
> >> get quite large errors in the calculation.
> >>
> >> It all depends how a LUT is used: if the value from the LUT is the 'final' value,
> >> then you don't need more precision than the number of bits of a color component. But
> >> if it is used in other calculations (3x3 matrices, full/limited range scaling, etc),
> >> then the LUT should provide more bits precision.
> >>
> >> Which seems to be the case with Intel hardware: 12 bits is 4 bits more than the 8 bits
> >> per component it probably uses.
> > 
> > Intel hw supports a 12bpp pixel pipeline. They didnt add _any_ additional
> > precision at all afaik. Which is why I wonder why we need it. I'm also not
> > aware of any plans for pushing past 12bpp of data sent to the sink, but I
> > honestly don't have much clue really.
> > 
> > I guess input is a different story, todays cmos already easily to 14bit
> > with more to come I guess with all the noise about HDR. We probably need
> > more headroom on v4l input side than we ever need on drm display side.
> > Still 24bits is an awful lot of headroom, at least for the next few years.
> > Or do you expect to hit that already soonish on v4l side?
> 
> I think 24 bits precision is enough, but that assumes that the integer part
> will be between -128 and 127. And I am not so sure that that is a valid assumption.

The idea is always that you'd normalize to 0.0-1.0 of the range going over
the wire to the sink. The 7 bits of headroom is just for smoother clamping
when your colorspaces don't match up. The most I've seen in intel hw is 3
additional bits used there.
> 
> It's true today, but what if you have a HW LUT that maps integer values and expects
> 16.0 or perhaps 12.4?
> 
> BTW, I am assuming that the proposed 8.24 format is a signed format: the CSC
> 3x3 matrices contain negative values, so any fixed point data type has to be signed.

Yeah, it's s8.24 really. We definitely need a signed integer part, agreed
on that.

> I'm just wondering: is it really such a big deal to use a 32.32 format? Yes, the
> amount of data doubles, but it's quite rare that you need to configure a LUT, right?
> 
> For a 12 bit LUT it's 16 kB vs 32 kB. Yes, it's more data, but the advantage is that
> the data type is future proof (well, probably :-) ) and much more likely to be usable
> in other subsystems.

We need to bash this stuff into hw under spin_lock_irqsave in i915. Yeah
props to our hw engineers for screwing things up, but I'd like to not be
too wasteful. But otoh the mmios will totally swamp any kind of memory
loads we're doing.

The other bit is that our android folks are a bit over the top with
reducing overhead sometimes, e.g. they don't like keeping around
metadata-only drm_framebuffer objects for xrgb vs. argb because it takes
away a few bytes ;-)

> >> I would guess that a LUT supporting 16 bit color components would need a precision
> >> of 0.20 or so (assuming the resulting values are used in further calculations).
> >>
> >> High dynamic range video will be an important driving force towards higher bit depths
> >> and accurate color handling, so you can expect to see this become much more important
> >> in the coming years.
> >>
> >> And as I mentioned another consideration is that this fixed point data type might
> >> be useful elsewhere in the kernel where you need to do some precision arithmetic.
> >> So using a standard type that anyone can use with functions in lib/ to do basic
> >> operations can be very useful indeed beyond just DRM and V4L2.
> > 
> > 0.20 would still comfortably fit into 8.24. And yeah worst-case (in 10
> > years or so) we need to add a high-bpp variant if it really comes to it.
> 
> I think this is much closer than you think. I agree that you are not likely to see
> this soon for consumer graphics cards, but for professional equipment and high-end
> consumer electronics this is another story.
> 
> And if it is being done for input, then output will need it as well: after all,
> what's the point of 16-bit color components if you can't display it? Whether Intel
> will support it is another matter, but there are other vendors, you know... :-)

Input is different because of post-processing - you need that much depth
to be able to get useful data out of the dark areas, without the risk for
the highlights to clip. While processing you need that depth to avoid
banding (because integer math sucks). But tbh I haven't seen anything but
12bpc (and those usually use dithered 10bpc panels internally) anywhere
and the common screens top out at 10bpc.

So from my pov of drm s8.24 will be enough for a long time, but if you're
convinced that the input side needs this soon I guess it makes sense to go
with the bit more overhead and 32.32. Otoh we'll never need 32 of integer
part if we normalize to 0.0-1.0, and that normalization is really
something I think we want.
-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 v2 00/10] Color Manager Implementation

[Hans Verkuil]

On 07/14/15 12:16, Daniel Vetter wrote:

<cut away old quotes>

>>>> I would guess that a LUT supporting 16 bit color components would need a precision
>>>> of 0.20 or so (assuming the resulting values are used in further calculations).
>>>>
>>>> High dynamic range video will be an important driving force towards higher bit depths
>>>> and accurate color handling, so you can expect to see this become much more important
>>>> in the coming years.
>>>>
>>>> And as I mentioned another consideration is that this fixed point data type might
>>>> be useful elsewhere in the kernel where you need to do some precision arithmetic.
>>>> So using a standard type that anyone can use with functions in lib/ to do basic
>>>> operations can be very useful indeed beyond just DRM and V4L2.
>>>
>>> 0.20 would still comfortably fit into 8.24. And yeah worst-case (in 10
>>> years or so) we need to add a high-bpp variant if it really comes to it.
>>
>> I think this is much closer than you think. I agree that you are not likely to see
>> this soon for consumer graphics cards, but for professional equipment and high-end
>> consumer electronics this is another story.
>>
>> And if it is being done for input, then output will need it as well: after all,
>> what's the point of 16-bit color components if you can't display it? Whether Intel
>> will support it is another matter, but there are other vendors, you know... :-)
> 
> Input is different because of post-processing - you need that much depth
> to be able to get useful data out of the dark areas, without the risk for
> the highlights to clip. While processing you need that depth to avoid
> banding (because integer math sucks). But tbh I haven't seen anything but
> 12bpc (and those usually use dithered 10bpc panels internally) anywhere
> and the common screens top out at 10bpc.
> 
> So from my pov of drm s8.24 will be enough for a long time, but if you're
> convinced that the input side needs this soon I guess it makes sense to go
> with the bit more overhead and 32.32. Otoh we'll never need 32 of integer
> part if we normalize to 0.0-1.0, and that normalization is really
> something I think we want.

I think 32.32 is primarily important as a standard data type for public APIs
and as the standard data type for math operations. How it is stored in the
driver is driver (or possibly subsystem) specific. Is it a problem for you to
store it as 8.24 in the driver (specifically for LUTs) to reduce memory usage?
Converting from 8.24 to 32.32 and vice versa is trivial, so this might be the
best of both worlds.

Regards,

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

Re: [Intel-gfx] [PATCH v2 00/10] Color Manager Implementation

[Hans Verkuil]

On 07/15/15 14:35, Hans Verkuil wrote:
> On 07/14/15 12:16, Daniel Vetter wrote:
> 
> <cut away old quotes>
> 
>>>>> I would guess that a LUT supporting 16 bit color components would need a precision
>>>>> of 0.20 or so (assuming the resulting values are used in further calculations).
>>>>>
>>>>> High dynamic range video will be an important driving force towards higher bit depths
>>>>> and accurate color handling, so you can expect to see this become much more important
>>>>> in the coming years.
>>>>>
>>>>> And as I mentioned another consideration is that this fixed point data type might
>>>>> be useful elsewhere in the kernel where you need to do some precision arithmetic.
>>>>> So using a standard type that anyone can use with functions in lib/ to do basic
>>>>> operations can be very useful indeed beyond just DRM and V4L2.
>>>>
>>>> 0.20 would still comfortably fit into 8.24. And yeah worst-case (in 10
>>>> years or so) we need to add a high-bpp variant if it really comes to it.
>>>
>>> I think this is much closer than you think. I agree that you are not likely to see
>>> this soon for consumer graphics cards, but for professional equipment and high-end
>>> consumer electronics this is another story.
>>>
>>> And if it is being done for input, then output will need it as well: after all,
>>> what's the point of 16-bit color components if you can't display it? Whether Intel
>>> will support it is another matter, but there are other vendors, you know... :-)
>>
>> Input is different because of post-processing - you need that much depth
>> to be able to get useful data out of the dark areas, without the risk for
>> the highlights to clip. While processing you need that depth to avoid
>> banding (because integer math sucks). But tbh I haven't seen anything but
>> 12bpc (and those usually use dithered 10bpc panels internally) anywhere
>> and the common screens top out at 10bpc.
>>
>> So from my pov of drm s8.24 will be enough for a long time, but if you're
>> convinced that the input side needs this soon I guess it makes sense to go
>> with the bit more overhead and 32.32. Otoh we'll never need 32 of integer
>> part if we normalize to 0.0-1.0, and that normalization is really
>> something I think we want.
> 
> I think 32.32 is primarily important as a standard data type for public APIs
> and as the standard data type for math operations. How it is stored in the
> driver is driver (or possibly subsystem) specific. Is it a problem for you to
> store it as 8.24 in the driver (specifically for LUTs) to reduce memory usage?
> Converting from 8.24 to 32.32 and vice versa is trivial, so this might be the
> best of both worlds.

Follow-up: I just read through the latest Color Manager Implementation patch
series and there the LUTs use U8.24 and CSC use S31.32.

Since DRM will always use LUT values in the range [0-1] and given the fact that
these values are just stored and not used in further calculations, I think that
this is OK. V4L2 might still use 32.32 for a LUT (currently we don't support
that yet, but we likely will in the near future), but that's OK.

As long as any fixed point math functions we create in the kernel all use S31.32
so they can be shared between DRM and V4L2 (and quite possibly other subsystems)
I have no problems with this scheme.

Regards,

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