[PATCH v2] drm/color: Document CTM eqations

[PATCH v2] drm/color: Document CTM eqations

[Brian Starkey]

Explicitly state the expected CTM equations in the kerneldoc for the CTM
property, and the form of the matrix on struct drm_color_ctm.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/drm_color_mgmt.c |   13 +++++++++++++
 include/uapi/drm/drm_mode.h      |    8 +++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 789b4c65cd69..7573ca4b6ea6 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -62,6 +62,19 @@
  *	unit/pass-thru matrix should be used. This is generally the driver
  *	boot-up state too.
  *
+ *	The output vector is related to the input vector as below:
+ *
+ *	| ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];``
+ *	| ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];``
+ *	| ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];``
+ *
+ *	The component order in the input/output vectors is assumed to be
+ *	{ R, G, B }.
+ *
+ *	The color-space of the input vector must not be confused with the
+ *	color-space implied by a framebuffer pixel format, which may be the same
+ *	or different.
+ *
  * “GAMMA_LUT”:
  *	Blob property to set the gamma lookup table (LUT) mapping pixel data
  *	after the transformation matrix to data sent to the connector. The
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index ce7efe2e8a5e..3401637caf8e 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
 };
 
 struct drm_color_ctm {
-	/* Conversion matrix in S31.32 format. */
+	/*
+	 * Conversion matrix in S31.32 format, in row-major form:
+	 *
+	 * | matrix[0] matrix[1] matrix[2] |
+	 * | matrix[3] matrix[4] matrix[5] |
+	 * | matrix[6] matrix[7] matrix[8] |
+	 */
 	__s64 matrix[9];
 };
 
-- 
1.7.9.5

[PATCH v2] drm/color: Document CTM eqations

[Brian Starkey]

Explicitly state the expected CTM equations in the kerneldoc for the CTM
property, and the form of the matrix on struct drm_color_ctm.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/drm_color_mgmt.c |   13 +++++++++++++
 include/uapi/drm/drm_mode.h      |    8 +++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 789b4c65cd69..7573ca4b6ea6 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -62,6 +62,19 @@
  *	unit/pass-thru matrix should be used. This is generally the driver
  *	boot-up state too.
  *
+ *	The output vector is related to the input vector as below:
+ *
+ *	| ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];``
+ *	| ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];``
+ *	| ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];``
+ *
+ *	The component order in the input/output vectors is assumed to be
+ *	{ R, G, B }.
+ *
+ *	The color-space of the input vector must not be confused with the
+ *	color-space implied by a framebuffer pixel format, which may be the same
+ *	or different.
+ *
  * “GAMMA_LUT”:
  *	Blob property to set the gamma lookup table (LUT) mapping pixel data
  *	after the transformation matrix to data sent to the connector. The
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index ce7efe2e8a5e..3401637caf8e 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
 };
 
 struct drm_color_ctm {
-	/* Conversion matrix in S31.32 format. */
+	/*
+	 * Conversion matrix in S31.32 format, in row-major form:
+	 *
+	 * | matrix[0] matrix[1] matrix[2] |
+	 * | matrix[3] matrix[4] matrix[5] |
+	 * | matrix[6] matrix[7] matrix[8] |
+	 */
 	__s64 matrix[9];
 };
 
-- 
1.7.9.5

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

Re: [PATCH v2] drm/color: Document CTM eqations

[Jani Nikula]

On Tue, 31 Jan 2017, Brian Starkey <brian.starkey@arm.com> wrote:
> Explicitly state the expected CTM equations in the kerneldoc for the CTM
> property, and the form of the matrix on struct drm_color_ctm.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
>  drivers/gpu/drm/drm_color_mgmt.c |   13 +++++++++++++
>  include/uapi/drm/drm_mode.h      |    8 +++++++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 789b4c65cd69..7573ca4b6ea6 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -62,6 +62,19 @@
>   *	unit/pass-thru matrix should be used. This is generally the driver
>   *	boot-up state too.
>   *
> + *	The output vector is related to the input vector as below:
> + *
> + *	| ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];``
> + *	| ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];``
> + *	| ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];``

Would that not work better with a preformatted block? Replace the colon
in the preceding paragraph with the double colon ::, and indent the
block.

> + *
> + *	The component order in the input/output vectors is assumed to be
> + *	{ R, G, B }.
> + *
> + *	The color-space of the input vector must not be confused with the
> + *	color-space implied by a framebuffer pixel format, which may be the same
> + *	or different.
> + *
>   * “GAMMA_LUT”:
>   *	Blob property to set the gamma lookup table (LUT) mapping pixel data
>   *	after the transformation matrix to data sent to the connector. The
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index ce7efe2e8a5e..3401637caf8e 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
>  };
>  
>  struct drm_color_ctm {
> -	/* Conversion matrix in S31.32 format. */
> +	/*
> +	 * Conversion matrix in S31.32 format, in row-major form:
> +	 *
> +	 * | matrix[0] matrix[1] matrix[2] |
> +	 * | matrix[3] matrix[4] matrix[5] |
> +	 * | matrix[6] matrix[7] matrix[8] |
> +	 */

Same here.

>  	__s64 matrix[9];
>  };

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2] drm/color: Document CTM eqations

[Jani Nikula]

On Tue, 31 Jan 2017, Brian Starkey <brian.starkey@arm.com> wrote:
> Explicitly state the expected CTM equations in the kerneldoc for the CTM
> property, and the form of the matrix on struct drm_color_ctm.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
>  drivers/gpu/drm/drm_color_mgmt.c |   13 +++++++++++++
>  include/uapi/drm/drm_mode.h      |    8 +++++++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 789b4c65cd69..7573ca4b6ea6 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -62,6 +62,19 @@
>   *	unit/pass-thru matrix should be used. This is generally the driver
>   *	boot-up state too.
>   *
> + *	The output vector is related to the input vector as below:
> + *
> + *	| ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];``
> + *	| ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];``
> + *	| ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];``

Would that not work better with a preformatted block? Replace the colon
in the preceding paragraph with the double colon ::, and indent the
block.

> + *
> + *	The component order in the input/output vectors is assumed to be
> + *	{ R, G, B }.
> + *
> + *	The color-space of the input vector must not be confused with the
> + *	color-space implied by a framebuffer pixel format, which may be the same
> + *	or different.
> + *
>   * “GAMMA_LUT”:
>   *	Blob property to set the gamma lookup table (LUT) mapping pixel data
>   *	after the transformation matrix to data sent to the connector. The
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index ce7efe2e8a5e..3401637caf8e 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
>  };
>  
>  struct drm_color_ctm {
> -	/* Conversion matrix in S31.32 format. */
> +	/*
> +	 * Conversion matrix in S31.32 format, in row-major form:
> +	 *
> +	 * | matrix[0] matrix[1] matrix[2] |
> +	 * | matrix[3] matrix[4] matrix[5] |
> +	 * | matrix[6] matrix[7] matrix[8] |
> +	 */

Same here.

>  	__s64 matrix[9];
>  };

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/color: Document CTM eqations

[Brian Starkey]

Hi Jani,

On Tue, Jan 31, 2017 at 01:30:41PM +0200, Jani Nikula wrote:
>On Tue, 31 Jan 2017, Brian Starkey <brian.starkey@arm.com> wrote:
>> Explicitly state the expected CTM equations in the kerneldoc for the CTM
>> property, and the form of the matrix on struct drm_color_ctm.
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>> ---
>>  drivers/gpu/drm/drm_color_mgmt.c |   13 +++++++++++++
>>  include/uapi/drm/drm_mode.h      |    8 +++++++-
>>  2 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
>> index 789b4c65cd69..7573ca4b6ea6 100644
>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
>> @@ -62,6 +62,19 @@
>>   *	unit/pass-thru matrix should be used. This is generally the driver
>>   *	boot-up state too.
>>   *
>> + *	The output vector is related to the input vector as below:
>> + *
>> + *	| ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];``
>> + *	| ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];``
>> + *	| ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];``
>
>Would that not work better with a preformatted block? Replace the colon
>in the preceding paragraph with the double colon ::, and indent the
>block.
>

Ah thanks for the tip, I couldn't get it to work but it looks like my
syntax was a bit off. I'll resend with that change.

>> + *
>> + *	The component order in the input/output vectors is assumed to be
>> + *	{ R, G, B }.
>> + *
>> + *	The color-space of the input vector must not be confused with the
>> + *	color-space implied by a framebuffer pixel format, which may be the same
>> + *	or different.
>> + *
>>   * “GAMMA_LUT”:
>>   *	Blob property to set the gamma lookup table (LUT) mapping pixel data
>>   *	after the transformation matrix to data sent to the connector. The
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index ce7efe2e8a5e..3401637caf8e 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
>>  };
>>
>>  struct drm_color_ctm {
>> -	/* Conversion matrix in S31.32 format. */
>> +	/*
>> +	 * Conversion matrix in S31.32 format, in row-major form:
>> +	 *
>> +	 * | matrix[0] matrix[1] matrix[2] |
>> +	 * | matrix[3] matrix[4] matrix[5] |
>> +	 * | matrix[6] matrix[7] matrix[8] |
>> +	 */
>
>Same here.

This comment isn't actually kerneldoc, so I guess not rst either.
I can include the markup if you like, but the |s here were to indicate
it's a matrix rather than for rst.

Cheers,
-Brian

>
>>  	__s64 matrix[9];
>>  };
>
>-- 
>Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2] drm/color: Document CTM eqations

[Brian Starkey]

Hi Jani,

On Tue, Jan 31, 2017 at 01:30:41PM +0200, Jani Nikula wrote:
>On Tue, 31 Jan 2017, Brian Starkey <brian.starkey@arm.com> wrote:
>> Explicitly state the expected CTM equations in the kerneldoc for the CTM
>> property, and the form of the matrix on struct drm_color_ctm.
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>> ---
>>  drivers/gpu/drm/drm_color_mgmt.c |   13 +++++++++++++
>>  include/uapi/drm/drm_mode.h      |    8 +++++++-
>>  2 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
>> index 789b4c65cd69..7573ca4b6ea6 100644
>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
>> @@ -62,6 +62,19 @@
>>   *	unit/pass-thru matrix should be used. This is generally the driver
>>   *	boot-up state too.
>>   *
>> + *	The output vector is related to the input vector as below:
>> + *
>> + *	| ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];``
>> + *	| ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];``
>> + *	| ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];``
>
>Would that not work better with a preformatted block? Replace the colon
>in the preceding paragraph with the double colon ::, and indent the
>block.
>

Ah thanks for the tip, I couldn't get it to work but it looks like my
syntax was a bit off. I'll resend with that change.

>> + *
>> + *	The component order in the input/output vectors is assumed to be
>> + *	{ R, G, B }.
>> + *
>> + *	The color-space of the input vector must not be confused with the
>> + *	color-space implied by a framebuffer pixel format, which may be the same
>> + *	or different.
>> + *
>>   * “GAMMA_LUT”:
>>   *	Blob property to set the gamma lookup table (LUT) mapping pixel data
>>   *	after the transformation matrix to data sent to the connector. The
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index ce7efe2e8a5e..3401637caf8e 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
>>  };
>>
>>  struct drm_color_ctm {
>> -	/* Conversion matrix in S31.32 format. */
>> +	/*
>> +	 * Conversion matrix in S31.32 format, in row-major form:
>> +	 *
>> +	 * | matrix[0] matrix[1] matrix[2] |
>> +	 * | matrix[3] matrix[4] matrix[5] |
>> +	 * | matrix[6] matrix[7] matrix[8] |
>> +	 */
>
>Same here.

This comment isn't actually kerneldoc, so I guess not rst either.
I can include the markup if you like, but the |s here were to indicate
it's a matrix rather than for rst.

Cheers,
-Brian

>
>>  	__s64 matrix[9];
>>  };
>
>-- 
>Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/color: Document CTM eqations

[Jani Nikula]

On Tue, 31 Jan 2017, Brian Starkey <brian.starkey@arm.com> wrote:
> Hi Jani,
>
> On Tue, Jan 31, 2017 at 01:30:41PM +0200, Jani Nikula wrote:
>>On Tue, 31 Jan 2017, Brian Starkey <brian.starkey@arm.com> wrote:
>>> Explicitly state the expected CTM equations in the kerneldoc for the CTM
>>> property, and the form of the matrix on struct drm_color_ctm.
>>>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>>> ---
>>>  drivers/gpu/drm/drm_color_mgmt.c |   13 +++++++++++++
>>>  include/uapi/drm/drm_mode.h      |    8 +++++++-
>>>  2 files changed, 20 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
>>> index 789b4c65cd69..7573ca4b6ea6 100644
>>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
>>> @@ -62,6 +62,19 @@
>>>   *	unit/pass-thru matrix should be used. This is generally the driver
>>>   *	boot-up state too.
>>>   *
>>> + *	The output vector is related to the input vector as below:
>>> + *
>>> + *	| ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];``
>>> + *	| ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];``
>>> + *	| ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];``
>>
>>Would that not work better with a preformatted block? Replace the colon
>>in the preceding paragraph with the double colon ::, and indent the
>>block.
>>
>
> Ah thanks for the tip, I couldn't get it to work but it looks like my
> syntax was a bit off. I'll resend with that change.
>
>>> + *
>>> + *	The component order in the input/output vectors is assumed to be
>>> + *	{ R, G, B }.
>>> + *
>>> + *	The color-space of the input vector must not be confused with the
>>> + *	color-space implied by a framebuffer pixel format, which may be the same
>>> + *	or different.
>>> + *
>>>   * “GAMMA_LUT”:
>>>   *	Blob property to set the gamma lookup table (LUT) mapping pixel data
>>>   *	after the transformation matrix to data sent to the connector. The
>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>>> index ce7efe2e8a5e..3401637caf8e 100644
>>> --- a/include/uapi/drm/drm_mode.h
>>> +++ b/include/uapi/drm/drm_mode.h
>>> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
>>>  };
>>>
>>>  struct drm_color_ctm {
>>> -	/* Conversion matrix in S31.32 format. */
>>> +	/*
>>> +	 * Conversion matrix in S31.32 format, in row-major form:
>>> +	 *
>>> +	 * | matrix[0] matrix[1] matrix[2] |
>>> +	 * | matrix[3] matrix[4] matrix[5] |
>>> +	 * | matrix[6] matrix[7] matrix[8] |
>>> +	 */
>>
>>Same here.
>
> This comment isn't actually kerneldoc, so I guess not rst either.
> I can include the markup if you like, but the |s here were to indicate
> it's a matrix rather than for rst.

Oh, right. Up to you.

>
> Cheers,
> -Brian
>
>>
>>>  	__s64 matrix[9];
>>>  };
>>
>>-- 
>>Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2] drm/color: Document CTM eqations

[Jani Nikula]

On Tue, 31 Jan 2017, Brian Starkey <brian.starkey@arm.com> wrote:
> Hi Jani,
>
> On Tue, Jan 31, 2017 at 01:30:41PM +0200, Jani Nikula wrote:
>>On Tue, 31 Jan 2017, Brian Starkey <brian.starkey@arm.com> wrote:
>>> Explicitly state the expected CTM equations in the kerneldoc for the CTM
>>> property, and the form of the matrix on struct drm_color_ctm.
>>>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>>> ---
>>>  drivers/gpu/drm/drm_color_mgmt.c |   13 +++++++++++++
>>>  include/uapi/drm/drm_mode.h      |    8 +++++++-
>>>  2 files changed, 20 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
>>> index 789b4c65cd69..7573ca4b6ea6 100644
>>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
>>> @@ -62,6 +62,19 @@
>>>   *	unit/pass-thru matrix should be used. This is generally the driver
>>>   *	boot-up state too.
>>>   *
>>> + *	The output vector is related to the input vector as below:
>>> + *
>>> + *	| ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];``
>>> + *	| ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];``
>>> + *	| ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];``
>>
>>Would that not work better with a preformatted block? Replace the colon
>>in the preceding paragraph with the double colon ::, and indent the
>>block.
>>
>
> Ah thanks for the tip, I couldn't get it to work but it looks like my
> syntax was a bit off. I'll resend with that change.
>
>>> + *
>>> + *	The component order in the input/output vectors is assumed to be
>>> + *	{ R, G, B }.
>>> + *
>>> + *	The color-space of the input vector must not be confused with the
>>> + *	color-space implied by a framebuffer pixel format, which may be the same
>>> + *	or different.
>>> + *
>>>   * “GAMMA_LUT”:
>>>   *	Blob property to set the gamma lookup table (LUT) mapping pixel data
>>>   *	after the transformation matrix to data sent to the connector. The
>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>>> index ce7efe2e8a5e..3401637caf8e 100644
>>> --- a/include/uapi/drm/drm_mode.h
>>> +++ b/include/uapi/drm/drm_mode.h
>>> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
>>>  };
>>>
>>>  struct drm_color_ctm {
>>> -	/* Conversion matrix in S31.32 format. */
>>> +	/*
>>> +	 * Conversion matrix in S31.32 format, in row-major form:
>>> +	 *
>>> +	 * | matrix[0] matrix[1] matrix[2] |
>>> +	 * | matrix[3] matrix[4] matrix[5] |
>>> +	 * | matrix[6] matrix[7] matrix[8] |
>>> +	 */
>>
>>Same here.
>
> This comment isn't actually kerneldoc, so I guess not rst either.
> I can include the markup if you like, but the |s here were to indicate
> it's a matrix rather than for rst.

Oh, right. Up to you.

>
> Cheers,
> -Brian
>
>>
>>>  	__s64 matrix[9];
>>>  };
>>
>>-- 
>>Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/color: Document CTM eqations

[Ville Syrjälä]

On Tue, Jan 31, 2017 at 10:48:34AM +0000, Brian Starkey wrote:
> Explicitly state the expected CTM equations in the kerneldoc for the CTM
> property, and the form of the matrix on struct drm_color_ctm.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
>  drivers/gpu/drm/drm_color_mgmt.c |   13 +++++++++++++
>  include/uapi/drm/drm_mode.h      |    8 +++++++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 789b4c65cd69..7573ca4b6ea6 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -62,6 +62,19 @@
>   *	unit/pass-thru matrix should be used. This is generally the driver
>   *	boot-up state too.
>   *
> + *	The output vector is related to the input vector as below:
> + *
> + *	| ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];``
> + *	| ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];``
> + *	| ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];``
> + *
> + *	The component order in the input/output vectors is assumed to be
> + *	{ R, G, B }.
> + *
> + *	The color-space of the input vector must not be confused with the
> + *	color-space implied by a framebuffer pixel format, which may be the same
> + *	or different.
> + *
>   * “GAMMA_LUT”:
>   *	Blob property to set the gamma lookup table (LUT) mapping pixel data
>   *	after the transformation matrix to data sent to the connector. The
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index ce7efe2e8a5e..3401637caf8e 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
>  };
>  
>  struct drm_color_ctm {
> -	/* Conversion matrix in S31.32 format. */
> +	/*
> +	 * Conversion matrix in S31.32 format, in row-major form:

s32.32 is how I'd state that (to match the regular s32 and whatnot
types).

> +	 *
> +	 * | matrix[0] matrix[1] matrix[2] |
> +	 * | matrix[3] matrix[4] matrix[5] |
> +	 * | matrix[6] matrix[7] matrix[8] |
> +	 */
>  	__s64 matrix[9];
>  };
>  
> -- 
> 1.7.9.5

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2] drm/color: Document CTM eqations

[Ville Syrjälä]

On Tue, Jan 31, 2017 at 10:48:34AM +0000, Brian Starkey wrote:
> Explicitly state the expected CTM equations in the kerneldoc for the CTM
> property, and the form of the matrix on struct drm_color_ctm.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
>  drivers/gpu/drm/drm_color_mgmt.c |   13 +++++++++++++
>  include/uapi/drm/drm_mode.h      |    8 +++++++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 789b4c65cd69..7573ca4b6ea6 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -62,6 +62,19 @@
>   *	unit/pass-thru matrix should be used. This is generally the driver
>   *	boot-up state too.
>   *
> + *	The output vector is related to the input vector as below:
> + *
> + *	| ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];``
> + *	| ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];``
> + *	| ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];``
> + *
> + *	The component order in the input/output vectors is assumed to be
> + *	{ R, G, B }.
> + *
> + *	The color-space of the input vector must not be confused with the
> + *	color-space implied by a framebuffer pixel format, which may be the same
> + *	or different.
> + *
>   * “GAMMA_LUT”:
>   *	Blob property to set the gamma lookup table (LUT) mapping pixel data
>   *	after the transformation matrix to data sent to the connector. The
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index ce7efe2e8a5e..3401637caf8e 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
>  };
>  
>  struct drm_color_ctm {
> -	/* Conversion matrix in S31.32 format. */
> +	/*
> +	 * Conversion matrix in S31.32 format, in row-major form:

s32.32 is how I'd state that (to match the regular s32 and whatnot
types).

> +	 *
> +	 * | matrix[0] matrix[1] matrix[2] |
> +	 * | matrix[3] matrix[4] matrix[5] |
> +	 * | matrix[6] matrix[7] matrix[8] |
> +	 */
>  	__s64 matrix[9];
>  };
>  
> -- 
> 1.7.9.5

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/color: Document CTM eqations

[Brian Starkey]

Hi Ville,

On Tue, Jan 31, 2017 at 05:18:28PM +0200, Ville Syrjälä wrote:
>On Tue, Jan 31, 2017 at 10:48:34AM +0000, Brian Starkey wrote:
>> Explicitly state the expected CTM equations in the kerneldoc for the CTM
>> property, and the form of the matrix on struct drm_color_ctm.
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>> ---
>>  drivers/gpu/drm/drm_color_mgmt.c |   13 +++++++++++++
>>  include/uapi/drm/drm_mode.h      |    8 +++++++-
>>  2 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
>> index 789b4c65cd69..7573ca4b6ea6 100644
>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
>> @@ -62,6 +62,19 @@
>>   *	unit/pass-thru matrix should be used. This is generally the driver
>>   *	boot-up state too.
>>   *
>> + *	The output vector is related to the input vector as below:
>> + *
>> + *	| ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];``
>> + *	| ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];``
>> + *	| ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];``
>> + *
>> + *	The component order in the input/output vectors is assumed to be
>> + *	{ R, G, B }.
>> + *
>> + *	The color-space of the input vector must not be confused with the
>> + *	color-space implied by a framebuffer pixel format, which may be the same
>> + *	or different.
>> + *
>>   * “GAMMA_LUT”:
>>   *	Blob property to set the gamma lookup table (LUT) mapping pixel data
>>   *	after the transformation matrix to data sent to the connector. The
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index ce7efe2e8a5e..3401637caf8e 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
>>  };
>>
>>  struct drm_color_ctm {
>> -	/* Conversion matrix in S31.32 format. */
>> +	/*
>> +	 * Conversion matrix in S31.32 format, in row-major form:
>
>s32.32 is how I'd state that (to match the regular s32 and whatnot
>types).
>

Can you explain a bit more what exactly you mean by s32.32? e.g. what
would be the bitfield representing the most negative number?

I understand the S31.32 here as a sign + magnitude format (which makes
it rather odd to store it in a signed variable, but never mind). This
also appears to be what igt does in set_ctm() in kms_pipe_color.c:

	for (i = 0; i < ARRAY_SIZE(ctm.matrix); i++) {
		if (coefficients[i] < 0) {
			ctm.matrix[i] =
				(int64_t) (-coefficients[i] * ((int64_t) 1L << 32));
			ctm.matrix[i] |= 1ULL << 63;
		} else
			ctm.matrix[i] =
				(int64_t) (coefficients[i] * ((int64_t) 1L << 32));
	}

If that's what you meant as well, then I don't think s32.32 is a good
way to describe it, because the integer part has only 31 bits
available.

If you meant a regular two's-complement fixed-point number, where the
most negative number would be 0x10000000.00000000, then yeah that's
what I thought it meant too originally. Clarifying the docs here
sounds like a great plan.

I guess the igt implementation means that it's a sign + magnitude
number, and the fact that it's stored in an s64 is a bizarre quirk
that we just live with.

Cheers,
Brian

>> +	 *
>> +	 * | matrix[0] matrix[1] matrix[2] |
>> +	 * | matrix[3] matrix[4] matrix[5] |
>> +	 * | matrix[6] matrix[7] matrix[8] |
>> +	 */
>>  	__s64 matrix[9];
>>  };
>>
>> --
>> 1.7.9.5
>
>-- 
>Ville Syrjälä
>Intel OTC

Re: [PATCH v2] drm/color: Document CTM eqations

[Brian Starkey]

Hi Ville,

On Tue, Jan 31, 2017 at 05:18:28PM +0200, Ville Syrjälä wrote:
>On Tue, Jan 31, 2017 at 10:48:34AM +0000, Brian Starkey wrote:
>> Explicitly state the expected CTM equations in the kerneldoc for the CTM
>> property, and the form of the matrix on struct drm_color_ctm.
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>> ---
>>  drivers/gpu/drm/drm_color_mgmt.c |   13 +++++++++++++
>>  include/uapi/drm/drm_mode.h      |    8 +++++++-
>>  2 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
>> index 789b4c65cd69..7573ca4b6ea6 100644
>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
>> @@ -62,6 +62,19 @@
>>   *	unit/pass-thru matrix should be used. This is generally the driver
>>   *	boot-up state too.
>>   *
>> + *	The output vector is related to the input vector as below:
>> + *
>> + *	| ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];``
>> + *	| ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];``
>> + *	| ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];``
>> + *
>> + *	The component order in the input/output vectors is assumed to be
>> + *	{ R, G, B }.
>> + *
>> + *	The color-space of the input vector must not be confused with the
>> + *	color-space implied by a framebuffer pixel format, which may be the same
>> + *	or different.
>> + *
>>   * “GAMMA_LUT”:
>>   *	Blob property to set the gamma lookup table (LUT) mapping pixel data
>>   *	after the transformation matrix to data sent to the connector. The
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index ce7efe2e8a5e..3401637caf8e 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
>>  };
>>
>>  struct drm_color_ctm {
>> -	/* Conversion matrix in S31.32 format. */
>> +	/*
>> +	 * Conversion matrix in S31.32 format, in row-major form:
>
>s32.32 is how I'd state that (to match the regular s32 and whatnot
>types).
>

Can you explain a bit more what exactly you mean by s32.32? e.g. what
would be the bitfield representing the most negative number?

I understand the S31.32 here as a sign + magnitude format (which makes
it rather odd to store it in a signed variable, but never mind). This
also appears to be what igt does in set_ctm() in kms_pipe_color.c:

	for (i = 0; i < ARRAY_SIZE(ctm.matrix); i++) {
		if (coefficients[i] < 0) {
			ctm.matrix[i] =
				(int64_t) (-coefficients[i] * ((int64_t) 1L << 32));
			ctm.matrix[i] |= 1ULL << 63;
		} else
			ctm.matrix[i] =
				(int64_t) (coefficients[i] * ((int64_t) 1L << 32));
	}

If that's what you meant as well, then I don't think s32.32 is a good
way to describe it, because the integer part has only 31 bits
available.

If you meant a regular two's-complement fixed-point number, where the
most negative number would be 0x10000000.00000000, then yeah that's
what I thought it meant too originally. Clarifying the docs here
sounds like a great plan.

I guess the igt implementation means that it's a sign + magnitude
number, and the fact that it's stored in an s64 is a bizarre quirk
that we just live with.

Cheers,
Brian

>> +	 *
>> +	 * | matrix[0] matrix[1] matrix[2] |
>> +	 * | matrix[3] matrix[4] matrix[5] |
>> +	 * | matrix[6] matrix[7] matrix[8] |
>> +	 */
>>  	__s64 matrix[9];
>>  };
>>
>> --
>> 1.7.9.5
>
>-- 
>Ville Syrjälä
>Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/color: Document CTM eqations

[Ville Syrjälä]

On Tue, Jan 31, 2017 at 03:39:29PM +0000, Brian Starkey wrote:
> Hi Ville,
> 
> On Tue, Jan 31, 2017 at 05:18:28PM +0200, Ville Syrjälä wrote:
> >On Tue, Jan 31, 2017 at 10:48:34AM +0000, Brian Starkey wrote:
> >> Explicitly state the expected CTM equations in the kerneldoc for the CTM
> >> property, and the form of the matrix on struct drm_color_ctm.
> >>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> >> ---
> >>  drivers/gpu/drm/drm_color_mgmt.c |   13 +++++++++++++
> >>  include/uapi/drm/drm_mode.h      |    8 +++++++-
> >>  2 files changed, 20 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> >> index 789b4c65cd69..7573ca4b6ea6 100644
> >> --- a/drivers/gpu/drm/drm_color_mgmt.c
> >> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> >> @@ -62,6 +62,19 @@
> >>   *	unit/pass-thru matrix should be used. This is generally the driver
> >>   *	boot-up state too.
> >>   *
> >> + *	The output vector is related to the input vector as below:
> >> + *
> >> + *	| ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];``
> >> + *	| ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];``
> >> + *	| ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];``
> >> + *
> >> + *	The component order in the input/output vectors is assumed to be
> >> + *	{ R, G, B }.
> >> + *
> >> + *	The color-space of the input vector must not be confused with the
> >> + *	color-space implied by a framebuffer pixel format, which may be the same
> >> + *	or different.
> >> + *
> >>   * “GAMMA_LUT”:
> >>   *	Blob property to set the gamma lookup table (LUT) mapping pixel data
> >>   *	after the transformation matrix to data sent to the connector. The
> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >> index ce7efe2e8a5e..3401637caf8e 100644
> >> --- a/include/uapi/drm/drm_mode.h
> >> +++ b/include/uapi/drm/drm_mode.h
> >> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
> >>  };
> >>
> >>  struct drm_color_ctm {
> >> -	/* Conversion matrix in S31.32 format. */
> >> +	/*
> >> +	 * Conversion matrix in S31.32 format, in row-major form:
> >
> >s32.32 is how I'd state that (to match the regular s32 and whatnot
> >types).
> >
> 
> Can you explain a bit more what exactly you mean by s32.32? e.g. what
> would be the bitfield representing the most negative number?
> 
> I understand the S31.32 here as a sign + magnitude format (which makes
> it rather odd to store it in a signed variable, but never mind). This
> also appears to be what igt does in set_ctm() in kms_pipe_color.c:
> 
> 	for (i = 0; i < ARRAY_SIZE(ctm.matrix); i++) {
> 		if (coefficients[i] < 0) {
> 			ctm.matrix[i] =
> 				(int64_t) (-coefficients[i] * ((int64_t) 1L << 32));
> 			ctm.matrix[i] |= 1ULL << 63;
> 		} else
> 			ctm.matrix[i] =
> 				(int64_t) (coefficients[i] * ((int64_t) 1L << 32));
> 	}
> 
> If that's what you meant as well, then I don't think s32.32 is a good
> way to describe it, because the integer part has only 31 bits
> available.
> 
> If you meant a regular two's-complement fixed-point number, where the
> most negative number would be 0x10000000.00000000, then yeah that's
> what I thought it meant too originally. Clarifying the docs here
> sounds like a great plan.
> 
> I guess the igt implementation means that it's a sign + magnitude
> number, and the fact that it's stored in an s64 is a bizarre quirk
> that we just live with.

Hmm. Two's complement is what I was thinking it is. Which shows that
I never managed to read the code in any detail. Definitely needs to
be documented properly.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2] drm/color: Document CTM eqations

[Ville Syrjälä]

On Tue, Jan 31, 2017 at 03:39:29PM +0000, Brian Starkey wrote:
> Hi Ville,
> 
> On Tue, Jan 31, 2017 at 05:18:28PM +0200, Ville Syrjälä wrote:
> >On Tue, Jan 31, 2017 at 10:48:34AM +0000, Brian Starkey wrote:
> >> Explicitly state the expected CTM equations in the kerneldoc for the CTM
> >> property, and the form of the matrix on struct drm_color_ctm.
> >>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> >> ---
> >>  drivers/gpu/drm/drm_color_mgmt.c |   13 +++++++++++++
> >>  include/uapi/drm/drm_mode.h      |    8 +++++++-
> >>  2 files changed, 20 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> >> index 789b4c65cd69..7573ca4b6ea6 100644
> >> --- a/drivers/gpu/drm/drm_color_mgmt.c
> >> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> >> @@ -62,6 +62,19 @@
> >>   *	unit/pass-thru matrix should be used. This is generally the driver
> >>   *	boot-up state too.
> >>   *
> >> + *	The output vector is related to the input vector as below:
> >> + *
> >> + *	| ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];``
> >> + *	| ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];``
> >> + *	| ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];``
> >> + *
> >> + *	The component order in the input/output vectors is assumed to be
> >> + *	{ R, G, B }.
> >> + *
> >> + *	The color-space of the input vector must not be confused with the
> >> + *	color-space implied by a framebuffer pixel format, which may be the same
> >> + *	or different.
> >> + *
> >>   * “GAMMA_LUT”:
> >>   *	Blob property to set the gamma lookup table (LUT) mapping pixel data
> >>   *	after the transformation matrix to data sent to the connector. The
> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >> index ce7efe2e8a5e..3401637caf8e 100644
> >> --- a/include/uapi/drm/drm_mode.h
> >> +++ b/include/uapi/drm/drm_mode.h
> >> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
> >>  };
> >>
> >>  struct drm_color_ctm {
> >> -	/* Conversion matrix in S31.32 format. */
> >> +	/*
> >> +	 * Conversion matrix in S31.32 format, in row-major form:
> >
> >s32.32 is how I'd state that (to match the regular s32 and whatnot
> >types).
> >
> 
> Can you explain a bit more what exactly you mean by s32.32? e.g. what
> would be the bitfield representing the most negative number?
> 
> I understand the S31.32 here as a sign + magnitude format (which makes
> it rather odd to store it in a signed variable, but never mind). This
> also appears to be what igt does in set_ctm() in kms_pipe_color.c:
> 
> 	for (i = 0; i < ARRAY_SIZE(ctm.matrix); i++) {
> 		if (coefficients[i] < 0) {
> 			ctm.matrix[i] =
> 				(int64_t) (-coefficients[i] * ((int64_t) 1L << 32));
> 			ctm.matrix[i] |= 1ULL << 63;
> 		} else
> 			ctm.matrix[i] =
> 				(int64_t) (coefficients[i] * ((int64_t) 1L << 32));
> 	}
> 
> If that's what you meant as well, then I don't think s32.32 is a good
> way to describe it, because the integer part has only 31 bits
> available.
> 
> If you meant a regular two's-complement fixed-point number, where the
> most negative number would be 0x10000000.00000000, then yeah that's
> what I thought it meant too originally. Clarifying the docs here
> sounds like a great plan.
> 
> I guess the igt implementation means that it's a sign + magnitude
> number, and the fact that it's stored in an s64 is a bizarre quirk
> that we just live with.

Hmm. Two's complement is what I was thinking it is. Which shows that
I never managed to read the code in any detail. Definitely needs to
be documented properly.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/color: Document CTM eqations

[Daniel Vetter]

On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> >> index ce7efe2e8a5e..3401637caf8e 100644
>> >> --- a/include/uapi/drm/drm_mode.h
>> >> +++ b/include/uapi/drm/drm_mode.h
>> >> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
>> >>  };
>> >>
>> >>  struct drm_color_ctm {
>> >> -  /* Conversion matrix in S31.32 format. */
>> >> +  /*
>> >> +   * Conversion matrix in S31.32 format, in row-major form:
>> >
>> >s32.32 is how I'd state that (to match the regular s32 and whatnot
>> >types).
>> >
>>
>> Can you explain a bit more what exactly you mean by s32.32? e.g. what
>> would be the bitfield representing the most negative number?
>>
>> I understand the S31.32 here as a sign + magnitude format (which makes
>> it rather odd to store it in a signed variable, but never mind). This
>> also appears to be what igt does in set_ctm() in kms_pipe_color.c:
>>
>>       for (i = 0; i < ARRAY_SIZE(ctm.matrix); i++) {
>>               if (coefficients[i] < 0) {
>>                       ctm.matrix[i] =
>>                               (int64_t) (-coefficients[i] * ((int64_t) 1L << 32));
>>                       ctm.matrix[i] |= 1ULL << 63;
>>               } else
>>                       ctm.matrix[i] =
>>                               (int64_t) (coefficients[i] * ((int64_t) 1L << 32));
>>       }
>>
>> If that's what you meant as well, then I don't think s32.32 is a good
>> way to describe it, because the integer part has only 31 bits
>> available.
>>
>> If you meant a regular two's-complement fixed-point number, where the
>> most negative number would be 0x10000000.00000000, then yeah that's
>> what I thought it meant too originally. Clarifying the docs here
>> sounds like a great plan.
>>
>> I guess the igt implementation means that it's a sign + magnitude
>> number, and the fact that it's stored in an s64 is a bizarre quirk
>> that we just live with.
>
> Hmm. Two's complement is what I was thinking it is. Which shows that
> I never managed to read the code in any detail. Definitely needs to
> be documented properly.

That sounds supremely backwards. I guess we can't fix this anymore?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2] drm/color: Document CTM eqations

[Daniel Vetter]

On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> >> index ce7efe2e8a5e..3401637caf8e 100644
>> >> --- a/include/uapi/drm/drm_mode.h
>> >> +++ b/include/uapi/drm/drm_mode.h
>> >> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
>> >>  };
>> >>
>> >>  struct drm_color_ctm {
>> >> -  /* Conversion matrix in S31.32 format. */
>> >> +  /*
>> >> +   * Conversion matrix in S31.32 format, in row-major form:
>> >
>> >s32.32 is how I'd state that (to match the regular s32 and whatnot
>> >types).
>> >
>>
>> Can you explain a bit more what exactly you mean by s32.32? e.g. what
>> would be the bitfield representing the most negative number?
>>
>> I understand the S31.32 here as a sign + magnitude format (which makes
>> it rather odd to store it in a signed variable, but never mind). This
>> also appears to be what igt does in set_ctm() in kms_pipe_color.c:
>>
>>       for (i = 0; i < ARRAY_SIZE(ctm.matrix); i++) {
>>               if (coefficients[i] < 0) {
>>                       ctm.matrix[i] =
>>                               (int64_t) (-coefficients[i] * ((int64_t) 1L << 32));
>>                       ctm.matrix[i] |= 1ULL << 63;
>>               } else
>>                       ctm.matrix[i] =
>>                               (int64_t) (coefficients[i] * ((int64_t) 1L << 32));
>>       }
>>
>> If that's what you meant as well, then I don't think s32.32 is a good
>> way to describe it, because the integer part has only 31 bits
>> available.
>>
>> If you meant a regular two's-complement fixed-point number, where the
>> most negative number would be 0x10000000.00000000, then yeah that's
>> what I thought it meant too originally. Clarifying the docs here
>> sounds like a great plan.
>>
>> I guess the igt implementation means that it's a sign + magnitude
>> number, and the fact that it's stored in an s64 is a bizarre quirk
>> that we just live with.
>
> Hmm. Two's complement is what I was thinking it is. Which shows that
> I never managed to read the code in any detail. Definitely needs to
> be documented properly.

That sounds supremely backwards. I guess we can't fix this anymore?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/color: Document CTM eqations

[Ville Syrjälä]

On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
> On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >> >> index ce7efe2e8a5e..3401637caf8e 100644
> >> >> --- a/include/uapi/drm/drm_mode.h
> >> >> +++ b/include/uapi/drm/drm_mode.h
> >> >> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
> >> >>  };
> >> >>
> >> >>  struct drm_color_ctm {
> >> >> -  /* Conversion matrix in S31.32 format. */
> >> >> +  /*
> >> >> +   * Conversion matrix in S31.32 format, in row-major form:
> >> >
> >> >s32.32 is how I'd state that (to match the regular s32 and whatnot
> >> >types).
> >> >
> >>
> >> Can you explain a bit more what exactly you mean by s32.32? e.g. what
> >> would be the bitfield representing the most negative number?
> >>
> >> I understand the S31.32 here as a sign + magnitude format (which makes
> >> it rather odd to store it in a signed variable, but never mind). This
> >> also appears to be what igt does in set_ctm() in kms_pipe_color.c:
> >>
> >>       for (i = 0; i < ARRAY_SIZE(ctm.matrix); i++) {
> >>               if (coefficients[i] < 0) {
> >>                       ctm.matrix[i] =
> >>                               (int64_t) (-coefficients[i] * ((int64_t) 1L << 32));
> >>                       ctm.matrix[i] |= 1ULL << 63;
> >>               } else
> >>                       ctm.matrix[i] =
> >>                               (int64_t) (coefficients[i] * ((int64_t) 1L << 32));
> >>       }
> >>
> >> If that's what you meant as well, then I don't think s32.32 is a good
> >> way to describe it, because the integer part has only 31 bits
> >> available.
> >>
> >> If you meant a regular two's-complement fixed-point number, where the
> >> most negative number would be 0x10000000.00000000, then yeah that's
> >> what I thought it meant too originally. Clarifying the docs here
> >> sounds like a great plan.
> >>
> >> I guess the igt implementation means that it's a sign + magnitude
> >> number, and the fact that it's stored in an s64 is a bizarre quirk
> >> that we just live with.
> >
> > Hmm. Two's complement is what I was thinking it is. Which shows that
> > I never managed to read the code in any detail. Definitely needs to
> > be documented properly.
> 
> That sounds supremely backwards. I guess we can't fix this anymore?

I have no idea. Anyone else?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2] drm/color: Document CTM eqations

[Ville Syrjälä]

On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
> On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >> >> index ce7efe2e8a5e..3401637caf8e 100644
> >> >> --- a/include/uapi/drm/drm_mode.h
> >> >> +++ b/include/uapi/drm/drm_mode.h
> >> >> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
> >> >>  };
> >> >>
> >> >>  struct drm_color_ctm {
> >> >> -  /* Conversion matrix in S31.32 format. */
> >> >> +  /*
> >> >> +   * Conversion matrix in S31.32 format, in row-major form:
> >> >
> >> >s32.32 is how I'd state that (to match the regular s32 and whatnot
> >> >types).
> >> >
> >>
> >> Can you explain a bit more what exactly you mean by s32.32? e.g. what
> >> would be the bitfield representing the most negative number?
> >>
> >> I understand the S31.32 here as a sign + magnitude format (which makes
> >> it rather odd to store it in a signed variable, but never mind). This
> >> also appears to be what igt does in set_ctm() in kms_pipe_color.c:
> >>
> >>       for (i = 0; i < ARRAY_SIZE(ctm.matrix); i++) {
> >>               if (coefficients[i] < 0) {
> >>                       ctm.matrix[i] =
> >>                               (int64_t) (-coefficients[i] * ((int64_t) 1L << 32));
> >>                       ctm.matrix[i] |= 1ULL << 63;
> >>               } else
> >>                       ctm.matrix[i] =
> >>                               (int64_t) (coefficients[i] * ((int64_t) 1L << 32));
> >>       }
> >>
> >> If that's what you meant as well, then I don't think s32.32 is a good
> >> way to describe it, because the integer part has only 31 bits
> >> available.
> >>
> >> If you meant a regular two's-complement fixed-point number, where the
> >> most negative number would be 0x10000000.00000000, then yeah that's
> >> what I thought it meant too originally. Clarifying the docs here
> >> sounds like a great plan.
> >>
> >> I guess the igt implementation means that it's a sign + magnitude
> >> number, and the fact that it's stored in an s64 is a bizarre quirk
> >> that we just live with.
> >
> > Hmm. Two's complement is what I was thinking it is. Which shows that
> > I never managed to read the code in any detail. Definitely needs to
> > be documented properly.
> 
> That sounds supremely backwards. I guess we can't fix this anymore?

I have no idea. Anyone else?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/color: Document CTM eqations

[Daniel Stone]

Hi,

On 15 February 2017 at 11:39, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
>> On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > Hmm. Two's complement is what I was thinking it is. Which shows that
>> > I never managed to read the code in any detail. Definitely needs to
>> > be documented properly.
>>
>> That sounds supremely backwards. I guess we can't fix this anymore?
>
> I have no idea. Anyone else?

I don't know of any implementation using this; maybe closed Intel
Android stuff? Certainly GitHub showed no-one using it, and neither X
nor Weston/Mutter are using it yet.

Cheers,
Daniel

Re: [PATCH v2] drm/color: Document CTM eqations

[Daniel Stone]

Hi,

On 15 February 2017 at 11:39, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
>> On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > Hmm. Two's complement is what I was thinking it is. Which shows that
>> > I never managed to read the code in any detail. Definitely needs to
>> > be documented properly.
>>
>> That sounds supremely backwards. I guess we can't fix this anymore?
>
> I have no idea. Anyone else?

I don't know of any implementation using this; maybe closed Intel
Android stuff? Certainly GitHub showed no-one using it, and neither X
nor Weston/Mutter are using it yet.

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/color: Document CTM eqations

[Brian Starkey]

What's the verdict? We've got [1] which is about to become another
(driver) implementation - better to change before that merges than
after I guess.

-Brian

[1] https://lkml.org/lkml/2017/2/13/304

On Wed, Feb 15, 2017 at 11:56:55AM +0000, Daniel Stone wrote:
>Hi,
>
>On 15 February 2017 at 11:39, Ville Syrjälä
><ville.syrjala@linux.intel.com> wrote:
>> On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
>>> On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
>>> <ville.syrjala@linux.intel.com> wrote:
>>> > Hmm. Two's complement is what I was thinking it is. Which shows that
>>> > I never managed to read the code in any detail. Definitely needs to
>>> > be documented properly.
>>>
>>> That sounds supremely backwards. I guess we can't fix this anymore?
>>
>> I have no idea. Anyone else?
>
>I don't know of any implementation using this; maybe closed Intel
>Android stuff? Certainly GitHub showed no-one using it, and neither X
>nor Weston/Mutter are using it yet.
>
>Cheers,
>Daniel

Re: [PATCH v2] drm/color: Document CTM eqations

[Brian Starkey]

What's the verdict? We've got [1] which is about to become another
(driver) implementation - better to change before that merges than
after I guess.

-Brian

[1] https://lkml.org/lkml/2017/2/13/304

On Wed, Feb 15, 2017 at 11:56:55AM +0000, Daniel Stone wrote:
>Hi,
>
>On 15 February 2017 at 11:39, Ville Syrjälä
><ville.syrjala@linux.intel.com> wrote:
>> On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
>>> On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
>>> <ville.syrjala@linux.intel.com> wrote:
>>> > Hmm. Two's complement is what I was thinking it is. Which shows that
>>> > I never managed to read the code in any detail. Definitely needs to
>>> > be documented properly.
>>>
>>> That sounds supremely backwards. I guess we can't fix this anymore?
>>
>> I have no idea. Anyone else?
>
>I don't know of any implementation using this; maybe closed Intel
>Android stuff? Certainly GitHub showed no-one using it, and neither X
>nor Weston/Mutter are using it yet.
>
>Cheers,
>Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/color: Document CTM eqations

[Lionel Landwerlin]

On 17/02/17 13:54, Brian Starkey wrote:
> What's the verdict? We've got [1] which is about to become another
> (driver) implementation - better to change before that merges than
> after I guess.
>
> -Brian
>
> [1] https://lkml.org/lkml/2017/2/13/304
>
> On Wed, Feb 15, 2017 at 11:56:55AM +0000, Daniel Stone wrote:
>> Hi,
>>
>> On 15 February 2017 at 11:39, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>>> On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
>>>> On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
>>>> <ville.syrjala@linux.intel.com> wrote:
>>>> > Hmm. Two's complement is what I was thinking it is. Which shows that
>>>> > I never managed to read the code in any detail. Definitely needs to
>>>> > be documented properly.
>>>>
>>>> That sounds supremely backwards. I guess we can't fix this anymore?
>>>
>>> I have no idea. Anyone else?
>>
>> I don't know of any implementation using this; maybe closed Intel
>> Android stuff? Certainly GitHub showed no-one using it, and neither X
>> nor Weston/Mutter are using it yet.
>>
>> Cheers,
>> Daniel
>
If we're talking fixed point reprsentation, ChromeOS is using this :

https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?q=DrmDevice&l=209

-
Lionel

Re: [PATCH v2] drm/color: Document CTM eqations

[Lionel Landwerlin]

On 17/02/17 13:54, Brian Starkey wrote:
> What's the verdict? We've got [1] which is about to become another
> (driver) implementation - better to change before that merges than
> after I guess.
>
> -Brian
>
> [1] https://lkml.org/lkml/2017/2/13/304
>
> On Wed, Feb 15, 2017 at 11:56:55AM +0000, Daniel Stone wrote:
>> Hi,
>>
>> On 15 February 2017 at 11:39, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>>> On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
>>>> On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
>>>> <ville.syrjala@linux.intel.com> wrote:
>>>> > Hmm. Two's complement is what I was thinking it is. Which shows that
>>>> > I never managed to read the code in any detail. Definitely needs to
>>>> > be documented properly.
>>>>
>>>> That sounds supremely backwards. I guess we can't fix this anymore?
>>>
>>> I have no idea. Anyone else?
>>
>> I don't know of any implementation using this; maybe closed Intel
>> Android stuff? Certainly GitHub showed no-one using it, and neither X
>> nor Weston/Mutter are using it yet.
>>
>> Cheers,
>> Daniel
>
If we're talking fixed point reprsentation, ChromeOS is using this :

https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?q=DrmDevice&l=209

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

Re: [PATCH v2] drm/color: Document CTM eqations

[Ville Syrjälä]

On Fri, Feb 17, 2017 at 02:42:26PM +0000, Lionel Landwerlin wrote:
> On 17/02/17 13:54, Brian Starkey wrote:
> > What's the verdict? We've got [1] which is about to become another
> > (driver) implementation - better to change before that merges than
> > after I guess.
> >
> > -Brian
> >
> > [1] https://lkml.org/lkml/2017/2/13/304
> >
> > On Wed, Feb 15, 2017 at 11:56:55AM +0000, Daniel Stone wrote:
> >> Hi,
> >>
> >> On 15 February 2017 at 11:39, Ville Syrjälä
> >> <ville.syrjala@linux.intel.com> wrote:
> >>> On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
> >>>> On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
> >>>> <ville.syrjala@linux.intel.com> wrote:
> >>>> > Hmm. Two's complement is what I was thinking it is. Which shows that
> >>>> > I never managed to read the code in any detail. Definitely needs to
> >>>> > be documented properly.
> >>>>
> >>>> That sounds supremely backwards. I guess we can't fix this anymore?
> >>>
> >>> I have no idea. Anyone else?
> >>
> >> I don't know of any implementation using this; maybe closed Intel
> >> Android stuff? Certainly GitHub showed no-one using it, and neither X
> >> nor Weston/Mutter are using it yet.
> >>
> >> Cheers,
> >> Daniel
> >
> If we're talking fixed point reprsentation, ChromeOS is using this :
> 
> https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?q=DrmDevice&l=209

So it's already using the sign+magnitude stuff. Which presumably
means we can't change it to two's complement anymore :( Maybe we add a
CTM2 property ;)

Using sign+magnitude definitely looks rather inefficient since there's
a branch inside the loop. With two's complement you wouldn't need that
thing slowing you down.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2] drm/color: Document CTM eqations

[Lionel Landwerlin]

On 17/02/17 14:56, Ville Syrjälä wrote:
> On Fri, Feb 17, 2017 at 02:42:26PM +0000, Lionel Landwerlin wrote:
>> On 17/02/17 13:54, Brian Starkey wrote:
>>> What's the verdict? We've got [1] which is about to become another
>>> (driver) implementation - better to change before that merges than
>>> after I guess.
>>>
>>> -Brian
>>>
>>> [1] https://lkml.org/lkml/2017/2/13/304
>>>
>>> On Wed, Feb 15, 2017 at 11:56:55AM +0000, Daniel Stone wrote:
>>>> Hi,
>>>>
>>>> On 15 February 2017 at 11:39, Ville Syrjälä
>>>> <ville.syrjala@linux.intel.com> wrote:
>>>>> On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
>>>>>> On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
>>>>>> <ville.syrjala@linux.intel.com> wrote:
>>>>>>> Hmm. Two's complement is what I was thinking it is. Which shows that
>>>>>>> I never managed to read the code in any detail. Definitely needs to
>>>>>>> be documented properly.
>>>>>> That sounds supremely backwards. I guess we can't fix this anymore?
>>>>> I have no idea. Anyone else?
>>>> I don't know of any implementation using this; maybe closed Intel
>>>> Android stuff? Certainly GitHub showed no-one using it, and neither X
>>>> nor Weston/Mutter are using it yet.
>>>>
>>>> Cheers,
>>>> Daniel
>> If we're talking fixed point reprsentation, ChromeOS is using this :
>>
>> https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?q=DrmDevice&l=209
> So it's already using the sign+magnitude stuff. Which presumably
> means we can't change it to two's complement anymore :( Maybe we add a
> CTM2 property ;)
>
> Using sign+magnitude definitely looks rather inefficient since there's
> a branch inside the loop. With two's complement you wouldn't need that
> thing slowing you down.
>
If you're seriously considering that, you might also want to bump struct 
drm_color_lut to use 32bits fields.
It seems some people have concerned about HDR.

Re: [PATCH v2] drm/color: Document CTM eqations

[Lionel Landwerlin]

On 17/02/17 14:56, Ville Syrjälä wrote:
> On Fri, Feb 17, 2017 at 02:42:26PM +0000, Lionel Landwerlin wrote:
>> On 17/02/17 13:54, Brian Starkey wrote:
>>> What's the verdict? We've got [1] which is about to become another
>>> (driver) implementation - better to change before that merges than
>>> after I guess.
>>>
>>> -Brian
>>>
>>> [1] https://lkml.org/lkml/2017/2/13/304
>>>
>>> On Wed, Feb 15, 2017 at 11:56:55AM +0000, Daniel Stone wrote:
>>>> Hi,
>>>>
>>>> On 15 February 2017 at 11:39, Ville Syrjälä
>>>> <ville.syrjala@linux.intel.com> wrote:
>>>>> On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
>>>>>> On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
>>>>>> <ville.syrjala@linux.intel.com> wrote:
>>>>>>> Hmm. Two's complement is what I was thinking it is. Which shows that
>>>>>>> I never managed to read the code in any detail. Definitely needs to
>>>>>>> be documented properly.
>>>>>> That sounds supremely backwards. I guess we can't fix this anymore?
>>>>> I have no idea. Anyone else?
>>>> I don't know of any implementation using this; maybe closed Intel
>>>> Android stuff? Certainly GitHub showed no-one using it, and neither X
>>>> nor Weston/Mutter are using it yet.
>>>>
>>>> Cheers,
>>>> Daniel
>> If we're talking fixed point reprsentation, ChromeOS is using this :
>>
>> https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?q=DrmDevice&l=209
> So it's already using the sign+magnitude stuff. Which presumably
> means we can't change it to two's complement anymore :( Maybe we add a
> CTM2 property ;)
>
> Using sign+magnitude definitely looks rather inefficient since there's
> a branch inside the loop. With two's complement you wouldn't need that
> thing slowing you down.
>
If you're seriously considering that, you might also want to bump struct 
drm_color_lut to use 32bits fields.
It seems some people have concerned about HDR.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/color: Document CTM eqations

[Daniel Stone]

Hi,

On 17 February 2017 at 14:56, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Feb 17, 2017 at 02:42:26PM +0000, Lionel Landwerlin wrote:
>> If we're talking fixed point reprsentation, ChromeOS is using this :
>>
>> https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?q=DrmDevice&l=209
>
> So it's already using the sign+magnitude stuff. Which presumably
> means we can't change it to two's complement anymore :( Maybe we add a
> CTM2 property ;)

I wouldn't be so sure; AFAIK it only ships on platforms where the
kernel is also built from the same tree, and generally where the
support is backported anyway. So it would be possible to atomically
land a change to CrOS such that the kernels and Chrome move together
to a changed representation.

Cheers,
Daniel

Re: [PATCH v2] drm/color: Document CTM eqations

[Daniel Stone]

Hi,

On 17 February 2017 at 14:56, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Feb 17, 2017 at 02:42:26PM +0000, Lionel Landwerlin wrote:
>> If we're talking fixed point reprsentation, ChromeOS is using this :
>>
>> https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?q=DrmDevice&l=209
>
> So it's already using the sign+magnitude stuff. Which presumably
> means we can't change it to two's complement anymore :( Maybe we add a
> CTM2 property ;)

I wouldn't be so sure; AFAIK it only ships on platforms where the
kernel is also built from the same tree, and generally where the
support is backported anyway. So it would be possible to atomically
land a change to CrOS such that the kernels and Chrome move together
to a changed representation.

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/color: Document CTM eqations

[Ville Syrjälä]

On Fri, Feb 17, 2017 at 03:05:28PM +0000, Lionel Landwerlin wrote:
> On 17/02/17 14:56, Ville Syrjälä wrote:
> > On Fri, Feb 17, 2017 at 02:42:26PM +0000, Lionel Landwerlin wrote:
> >> On 17/02/17 13:54, Brian Starkey wrote:
> >>> What's the verdict? We've got [1] which is about to become another
> >>> (driver) implementation - better to change before that merges than
> >>> after I guess.
> >>>
> >>> -Brian
> >>>
> >>> [1] https://lkml.org/lkml/2017/2/13/304
> >>>
> >>> On Wed, Feb 15, 2017 at 11:56:55AM +0000, Daniel Stone wrote:
> >>>> Hi,
> >>>>
> >>>> On 15 February 2017 at 11:39, Ville Syrjälä
> >>>> <ville.syrjala@linux.intel.com> wrote:
> >>>>> On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
> >>>>>> On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
> >>>>>> <ville.syrjala@linux.intel.com> wrote:
> >>>>>>> Hmm. Two's complement is what I was thinking it is. Which shows that
> >>>>>>> I never managed to read the code in any detail. Definitely needs to
> >>>>>>> be documented properly.
> >>>>>> That sounds supremely backwards. I guess we can't fix this anymore?
> >>>>> I have no idea. Anyone else?
> >>>> I don't know of any implementation using this; maybe closed Intel
> >>>> Android stuff? Certainly GitHub showed no-one using it, and neither X
> >>>> nor Weston/Mutter are using it yet.
> >>>>
> >>>> Cheers,
> >>>> Daniel
> >> If we're talking fixed point reprsentation, ChromeOS is using this :
> >>
> >> https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?q=DrmDevice&l=209
> > So it's already using the sign+magnitude stuff. Which presumably
> > means we can't change it to two's complement anymore :( Maybe we add a
> > CTM2 property ;)
> >
> > Using sign+magnitude definitely looks rather inefficient since there's
> > a branch inside the loop. With two's complement you wouldn't need that
> > thing slowing you down.
> >
> If you're seriously considering that, you might also want to bump struct 
> drm_color_lut to use 32bits fields.

Which is what I thought we had already agreed to do when we started
planning this color management stuff. But I guess that plan got
somehow scrapped after I was no longer part of the discussions.

> It seems some people have concerned about HDR.

HDR is definitely something I'd like to have a look at.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2] drm/color: Document CTM eqations

[Ville Syrjälä]

On Fri, Feb 17, 2017 at 03:05:28PM +0000, Lionel Landwerlin wrote:
> On 17/02/17 14:56, Ville Syrjälä wrote:
> > On Fri, Feb 17, 2017 at 02:42:26PM +0000, Lionel Landwerlin wrote:
> >> On 17/02/17 13:54, Brian Starkey wrote:
> >>> What's the verdict? We've got [1] which is about to become another
> >>> (driver) implementation - better to change before that merges than
> >>> after I guess.
> >>>
> >>> -Brian
> >>>
> >>> [1] https://lkml.org/lkml/2017/2/13/304
> >>>
> >>> On Wed, Feb 15, 2017 at 11:56:55AM +0000, Daniel Stone wrote:
> >>>> Hi,
> >>>>
> >>>> On 15 February 2017 at 11:39, Ville Syrjälä
> >>>> <ville.syrjala@linux.intel.com> wrote:
> >>>>> On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
> >>>>>> On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
> >>>>>> <ville.syrjala@linux.intel.com> wrote:
> >>>>>>> Hmm. Two's complement is what I was thinking it is. Which shows that
> >>>>>>> I never managed to read the code in any detail. Definitely needs to
> >>>>>>> be documented properly.
> >>>>>> That sounds supremely backwards. I guess we can't fix this anymore?
> >>>>> I have no idea. Anyone else?
> >>>> I don't know of any implementation using this; maybe closed Intel
> >>>> Android stuff? Certainly GitHub showed no-one using it, and neither X
> >>>> nor Weston/Mutter are using it yet.
> >>>>
> >>>> Cheers,
> >>>> Daniel
> >> If we're talking fixed point reprsentation, ChromeOS is using this :
> >>
> >> https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?q=DrmDevice&l=209
> > So it's already using the sign+magnitude stuff. Which presumably
> > means we can't change it to two's complement anymore :( Maybe we add a
> > CTM2 property ;)
> >
> > Using sign+magnitude definitely looks rather inefficient since there's
> > a branch inside the loop. With two's complement you wouldn't need that
> > thing slowing you down.
> >
> If you're seriously considering that, you might also want to bump struct 
> drm_color_lut to use 32bits fields.

Which is what I thought we had already agreed to do when we started
planning this color management stuff. But I guess that plan got
somehow scrapped after I was no longer part of the discussions.

> It seems some people have concerned about HDR.

HDR is definitely something I'd like to have a look at.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/color: Document CTM eqations

[Daniel Vetter]

On Fri, Feb 17, 2017 at 01:54:52PM +0000, Brian Starkey wrote:
> What's the verdict? We've got [1] which is about to become another
> (driver) implementation - better to change before that merges than
> after I guess.

We could also just switch the internal representation to something more
reasonable, and add glue code in the atomic core to remap between whatever
the properties are and the internal representation. Same thing we
essentially already do with the display mode.

That would then also extend to CTM2 or whatever.

For sure not really a good reason to stall a driver imo.
-Daniel

> 
> -Brian
> 
> [1] https://lkml.org/lkml/2017/2/13/304
> 
> On Wed, Feb 15, 2017 at 11:56:55AM +0000, Daniel Stone wrote:
> > Hi,
> > 
> > On 15 February 2017 at 11:39, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > > On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
> > > > On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
> > > > <ville.syrjala@linux.intel.com> wrote:
> > > > > Hmm. Two's complement is what I was thinking it is. Which shows that
> > > > > I never managed to read the code in any detail. Definitely needs to
> > > > > be documented properly.
> > > > 
> > > > That sounds supremely backwards. I guess we can't fix this anymore?
> > > 
> > > I have no idea. Anyone else?
> > 
> > I don't know of any implementation using this; maybe closed Intel
> > Android stuff? Certainly GitHub showed no-one using it, and neither X
> > nor Weston/Mutter are using it yet.
> > 
> > Cheers,
> > Daniel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch