[PATCH v5 0/4] drm/komeda: Enable CRTC color-mgmt

[PATCH v5 0/4] drm/komeda: Enable CRTC color-mgmt

[james qian wang (Arm Technology China)]

This series enable CRTC color-mgmt for komeda driver, for current komeda HW
which only supports color conversion and forward gamma for CRTC.

This series actually are regrouped from:
- drm/komeda: Enable layer/plane color-mgmt:
  https://patchwork.freedesktop.org/series/60893/

- drm/komeda: Enable CRTC color-mgmt
  https://patchwork.freedesktop.org/series/61370/

For removing the dependence on:
- https://patchwork.freedesktop.org/series/30876/

Lowry Li (Arm Technology China) (1):
  drm/komeda: Adds gamma and color-transform support for DOU-IPS

james qian wang (Arm Technology China) (3):
  drm/komeda: Add a new helper drm_color_ctm_s31_32_to_qm_n()
  drm/komeda: Add drm_lut_to_fgamma_coeffs()
  drm/komeda: Add drm_ctm_to_coeffs()

v2:
  Move the fixpoint conversion function s31_32_to_q2_12() to drm core
  as a shared helper.

v4:
  Address review comments from Mihai, Daniel and Ilia.

V5:
- Includes the sign bit in the value of m (Qm.n).
- Rebase with drm-misc-next

Lowry Li (Arm Technology China) (1):
  drm/komeda: Adds gamma and color-transform support for DOU-IPS

james qian wang (Arm Technology China) (3):
  drm: Add a new helper drm_color_ctm_s31_32_to_qm_n()
  drm/komeda: Add drm_lut_to_fgamma_coeffs()
  drm/komeda: Add drm_ctm_to_coeffs()

 .../arm/display/komeda/d71/d71_component.c    | 20 ++++++
 .../arm/display/komeda/komeda_color_mgmt.c    | 66 +++++++++++++++++++
 .../arm/display/komeda/komeda_color_mgmt.h    | 10 ++-
 .../gpu/drm/arm/display/komeda/komeda_crtc.c  |  2 +
 .../drm/arm/display/komeda/komeda_pipeline.h  |  3 +
 .../display/komeda/komeda_pipeline_state.c    |  6 ++
 drivers/gpu/drm/drm_color_mgmt.c              | 27 ++++++++
 include/drm/drm_color_mgmt.h                  |  2 +
 8 files changed, 135 insertions(+), 1 deletion(-)

--
2.20.1

[PATCH v5 0/4] drm/komeda: Enable CRTC color-mgmt

[james qian wang (Arm Technology China)]

This series enable CRTC color-mgmt for komeda driver, for current komeda HW
which only supports color conversion and forward gamma for CRTC.

This series actually are regrouped from:
- drm/komeda: Enable layer/plane color-mgmt:
  https://patchwork.freedesktop.org/series/60893/

- drm/komeda: Enable CRTC color-mgmt
  https://patchwork.freedesktop.org/series/61370/

For removing the dependence on:
- https://patchwork.freedesktop.org/series/30876/

Lowry Li (Arm Technology China) (1):
  drm/komeda: Adds gamma and color-transform support for DOU-IPS

james qian wang (Arm Technology China) (3):
  drm/komeda: Add a new helper drm_color_ctm_s31_32_to_qm_n()
  drm/komeda: Add drm_lut_to_fgamma_coeffs()
  drm/komeda: Add drm_ctm_to_coeffs()

v2:
  Move the fixpoint conversion function s31_32_to_q2_12() to drm core
  as a shared helper.

v4:
  Address review comments from Mihai, Daniel and Ilia.

V5:
- Includes the sign bit in the value of m (Qm.n).
- Rebase with drm-misc-next

Lowry Li (Arm Technology China) (1):
  drm/komeda: Adds gamma and color-transform support for DOU-IPS

james qian wang (Arm Technology China) (3):
  drm: Add a new helper drm_color_ctm_s31_32_to_qm_n()
  drm/komeda: Add drm_lut_to_fgamma_coeffs()
  drm/komeda: Add drm_ctm_to_coeffs()

 .../arm/display/komeda/d71/d71_component.c    | 20 ++++++
 .../arm/display/komeda/komeda_color_mgmt.c    | 66 +++++++++++++++++++
 .../arm/display/komeda/komeda_color_mgmt.h    | 10 ++-
 .../gpu/drm/arm/display/komeda/komeda_crtc.c  |  2 +
 .../drm/arm/display/komeda/komeda_pipeline.h  |  3 +
 .../display/komeda/komeda_pipeline_state.c    |  6 ++
 drivers/gpu/drm/drm_color_mgmt.c              | 27 ++++++++
 include/drm/drm_color_mgmt.h                  |  2 +
 8 files changed, 135 insertions(+), 1 deletion(-)

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

[PATCH v5 1/4] drm: Add a new helper drm_color_ctm_s31_32_to_qm_n()

[james qian wang (Arm Technology China)]

Add a new helper function drm_color_ctm_s31_32_to_qm_n() for driver to
convert S31.32 sign-magnitude to Qm.n 2's complement that supported by
hardware.

V4: Address Mihai, Daniel and Ilia's review comments.
V5: Includes the sign bit in the value of m (Qm.n).

Signed-off-by: james qian wang (Arm Technology China) <james.qian.wang@arm.com>
Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_color_mgmt.c | 27 +++++++++++++++++++++++++++
 include/drm/drm_color_mgmt.h     |  2 ++
 2 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 4ce5c6d8de99..d313f194f1ec 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -132,6 +132,33 @@ uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision)
 }
 EXPORT_SYMBOL(drm_color_lut_extract);
 
+/**
+ * drm_color_ctm_s31_32_to_qm_n
+ *
+ * @user_input: input value
+ * @m: number of integer bits, include the sign-bit, support range is [1, 32]
+ * @n: number of fractional bits, only support n <= 32
+ *
+ * Convert and clamp S31.32 sign-magnitude to Qm.n (signed 2's complement). The
+ * higher bits that above m + n are cleared or equal to sign-bit BIT(m+n).
+ */
+uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
+				      uint32_t m, uint32_t n)
+{
+	u64 mag = (user_input & ~BIT_ULL(63)) >> (32 - n);
+	bool negative = !!(user_input & BIT_ULL(63));
+	s64 val;
+
+	WARN_ON(m < 1 || m > 32 || n > 32);
+
+	/* the range of signed 2's complement is [-2^(m-1), 2^(m-1) - 2^-n] */
+	val = clamp_val(mag, 0, negative ?
+				BIT_ULL(n + m - 1) : BIT_ULL(n + m - 1) - 1);
+
+	return negative ? -val : val;
+}
+EXPORT_SYMBOL(drm_color_ctm_s31_32_to_qm_n);
+
 /**
  * drm_crtc_enable_color_mgmt - enable color management properties
  * @crtc: DRM CRTC
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index d1c662d92ab7..60fea5501886 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -30,6 +30,8 @@ struct drm_crtc;
 struct drm_plane;
 
 uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
+uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
+				      uint32_t m, uint32_t n);
 
 void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
 				uint degamma_lut_size,
-- 
2.20.1

[PATCH v5 1/4] drm: Add a new helper drm_color_ctm_s31_32_to_qm_n()

[james qian wang (Arm Technology China)]

Add a new helper function drm_color_ctm_s31_32_to_qm_n() for driver to
convert S31.32 sign-magnitude to Qm.n 2's complement that supported by
hardware.

V4: Address Mihai, Daniel and Ilia's review comments.
V5: Includes the sign bit in the value of m (Qm.n).

Signed-off-by: james qian wang (Arm Technology China) <james.qian.wang@arm.com>
Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_color_mgmt.c | 27 +++++++++++++++++++++++++++
 include/drm/drm_color_mgmt.h     |  2 ++
 2 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 4ce5c6d8de99..d313f194f1ec 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -132,6 +132,33 @@ uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision)
 }
 EXPORT_SYMBOL(drm_color_lut_extract);
 
+/**
+ * drm_color_ctm_s31_32_to_qm_n
+ *
+ * @user_input: input value
+ * @m: number of integer bits, include the sign-bit, support range is [1, 32]
+ * @n: number of fractional bits, only support n <= 32
+ *
+ * Convert and clamp S31.32 sign-magnitude to Qm.n (signed 2's complement). The
+ * higher bits that above m + n are cleared or equal to sign-bit BIT(m+n).
+ */
+uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
+				      uint32_t m, uint32_t n)
+{
+	u64 mag = (user_input & ~BIT_ULL(63)) >> (32 - n);
+	bool negative = !!(user_input & BIT_ULL(63));
+	s64 val;
+
+	WARN_ON(m < 1 || m > 32 || n > 32);
+
+	/* the range of signed 2's complement is [-2^(m-1), 2^(m-1) - 2^-n] */
+	val = clamp_val(mag, 0, negative ?
+				BIT_ULL(n + m - 1) : BIT_ULL(n + m - 1) - 1);
+
+	return negative ? -val : val;
+}
+EXPORT_SYMBOL(drm_color_ctm_s31_32_to_qm_n);
+
 /**
  * drm_crtc_enable_color_mgmt - enable color management properties
  * @crtc: DRM CRTC
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index d1c662d92ab7..60fea5501886 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -30,6 +30,8 @@ struct drm_crtc;
 struct drm_plane;
 
 uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
+uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
+				      uint32_t m, uint32_t n);
 
 void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
 				uint degamma_lut_size,
-- 
2.20.1

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

Re: [PATCH v5 1/4] drm: Add a new helper drm_color_ctm_s31_32_to_qm_n()

[Mihail Atanassov]

On Wednesday, 16 October 2019 11:34:08 BST james qian wang (Arm Technology China) wrote:
> Add a new helper function drm_color_ctm_s31_32_to_qm_n() for driver to
> convert S31.32 sign-magnitude to Qm.n 2's complement that supported by
> hardware.
> 
> V4: Address Mihai, Daniel and Ilia's review comments.
> V5: Includes the sign bit in the value of m (Qm.n).
> 
> Signed-off-by: james qian wang (Arm Technology China) <james.qian.wang@arm.com>
> Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_color_mgmt.c | 27 +++++++++++++++++++++++++++
>  include/drm/drm_color_mgmt.h     |  2 ++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 4ce5c6d8de99..d313f194f1ec 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -132,6 +132,33 @@ uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision)
>  }
>  EXPORT_SYMBOL(drm_color_lut_extract);
>  
> +/**
> + * drm_color_ctm_s31_32_to_qm_n
> + *
> + * @user_input: input value
> + * @m: number of integer bits, include the sign-bit, support range is [1, 32]

Any reason why numbers like Q0.32 are disallowed? In those cases, the
'sign' bit and the first fractional bit just happen to be the same bit.
The longer I look at it, the more I think mentioning a 'sign-bit' here
might confuse people more, since 2's complement doesn't have a
dedicated bit just for the sign. How about reducing it simply to:

 * @m: number of integer bits, m <= 32.

> + * @n: number of fractional bits, only support n <= 32
> + *
> + * Convert and clamp S31.32 sign-magnitude to Qm.n (signed 2's complement). The
> + * higher bits that above m + n are cleared or equal to sign-bit BIT(m+n).

[nit] BIT(m + n - 1) if we count from 0.

> + */
> +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> +				      uint32_t m, uint32_t n)
> +{
> +	u64 mag = (user_input & ~BIT_ULL(63)) >> (32 - n);
> +	bool negative = !!(user_input & BIT_ULL(63));
> +	s64 val;
> +
> +	WARN_ON(m < 1 || m > 32 || n > 32);
> +
> +	/* the range of signed 2's complement is [-2^(m-1), 2^(m-1) - 2^-n] */
> +	val = clamp_val(mag, 0, negative ?
> +				BIT_ULL(n + m - 1) : BIT_ULL(n + m - 1) - 1);
> +
> +	return negative ? -val : val;
> +}
> +EXPORT_SYMBOL(drm_color_ctm_s31_32_to_qm_n);
> +
>  /**
>   * drm_crtc_enable_color_mgmt - enable color management properties
>   * @crtc: DRM CRTC
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index d1c662d92ab7..60fea5501886 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -30,6 +30,8 @@ struct drm_crtc;
>  struct drm_plane;
>  
>  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
> +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> +				      uint32_t m, uint32_t n);
>  
>  void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>  				uint degamma_lut_size,
> 


-- 
Mihail

Re: [PATCH v5 1/4] drm: Add a new helper drm_color_ctm_s31_32_to_qm_n()

[Mihail Atanassov]

On Wednesday, 16 October 2019 11:34:08 BST james qian wang (Arm Technology China) wrote:
> Add a new helper function drm_color_ctm_s31_32_to_qm_n() for driver to
> convert S31.32 sign-magnitude to Qm.n 2's complement that supported by
> hardware.
> 
> V4: Address Mihai, Daniel and Ilia's review comments.
> V5: Includes the sign bit in the value of m (Qm.n).
> 
> Signed-off-by: james qian wang (Arm Technology China) <james.qian.wang@arm.com>
> Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_color_mgmt.c | 27 +++++++++++++++++++++++++++
>  include/drm/drm_color_mgmt.h     |  2 ++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 4ce5c6d8de99..d313f194f1ec 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -132,6 +132,33 @@ uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision)
>  }
>  EXPORT_SYMBOL(drm_color_lut_extract);
>  
> +/**
> + * drm_color_ctm_s31_32_to_qm_n
> + *
> + * @user_input: input value
> + * @m: number of integer bits, include the sign-bit, support range is [1, 32]

Any reason why numbers like Q0.32 are disallowed? In those cases, the
'sign' bit and the first fractional bit just happen to be the same bit.
The longer I look at it, the more I think mentioning a 'sign-bit' here
might confuse people more, since 2's complement doesn't have a
dedicated bit just for the sign. How about reducing it simply to:

 * @m: number of integer bits, m <= 32.

> + * @n: number of fractional bits, only support n <= 32
> + *
> + * Convert and clamp S31.32 sign-magnitude to Qm.n (signed 2's complement). The
> + * higher bits that above m + n are cleared or equal to sign-bit BIT(m+n).

[nit] BIT(m + n - 1) if we count from 0.

> + */
> +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> +				      uint32_t m, uint32_t n)
> +{
> +	u64 mag = (user_input & ~BIT_ULL(63)) >> (32 - n);
> +	bool negative = !!(user_input & BIT_ULL(63));
> +	s64 val;
> +
> +	WARN_ON(m < 1 || m > 32 || n > 32);
> +
> +	/* the range of signed 2's complement is [-2^(m-1), 2^(m-1) - 2^-n] */
> +	val = clamp_val(mag, 0, negative ?
> +				BIT_ULL(n + m - 1) : BIT_ULL(n + m - 1) - 1);
> +
> +	return negative ? -val : val;
> +}
> +EXPORT_SYMBOL(drm_color_ctm_s31_32_to_qm_n);
> +
>  /**
>   * drm_crtc_enable_color_mgmt - enable color management properties
>   * @crtc: DRM CRTC
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index d1c662d92ab7..60fea5501886 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -30,6 +30,8 @@ struct drm_crtc;
>  struct drm_plane;
>  
>  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
> +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> +				      uint32_t m, uint32_t n);
>  
>  void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>  				uint degamma_lut_size,
> 


-- 
Mihail



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

Re: [PATCH v5 1/4] drm: Add a new helper drm_color_ctm_s31_32_to_qm_n()

[james qian wang (Arm Technology China)]

On Wed, Oct 16, 2019 at 11:02:03AM +0000, Mihail Atanassov wrote:
> On Wednesday, 16 October 2019 11:34:08 BST james qian wang (Arm Technology China) wrote:
> > Add a new helper function drm_color_ctm_s31_32_to_qm_n() for driver to
> > convert S31.32 sign-magnitude to Qm.n 2's complement that supported by
> > hardware.
> > 
> > V4: Address Mihai, Daniel and Ilia's review comments.
> > V5: Includes the sign bit in the value of m (Qm.n).
> > 
> > Signed-off-by: james qian wang (Arm Technology China) <james.qian.wang@arm.com>
> > Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_color_mgmt.c | 27 +++++++++++++++++++++++++++
> >  include/drm/drm_color_mgmt.h     |  2 ++
> >  2 files changed, 29 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > index 4ce5c6d8de99..d313f194f1ec 100644
> > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > @@ -132,6 +132,33 @@ uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision)
> >  }
> >  EXPORT_SYMBOL(drm_color_lut_extract);
> >  
> > +/**
> > + * drm_color_ctm_s31_32_to_qm_n
> > + *
> > + * @user_input: input value
> > + * @m: number of integer bits, include the sign-bit, support range is [1, 32]
> 
> Any reason why numbers like Q0.32 are disallowed? In those cases, the
> 'sign' bit and the first fractional bit just happen to be the same bit.
> The longer I look at it, the more I think mentioning a 'sign-bit' here
> might confuse people more, since 2's complement doesn't have a
> dedicated bit just for the sign. How about reducing it simply to:

No, since the value is signed there must be dedicated sign-bit.

consider very simple 2 bit signed, Q1.1

 0.5  is 01
 0    is 00
-0.5  is 11
-1.0  is 10, sign-bit and value share same bit, but it is integer part.

See the wiki:

One convention includes the sign bit in the value of m,[1][2] and the other convention
does not. The choice of convention can be determined by summing m+n. If the value is equal
to the register size, then the sign bit is included in the value of m. If it is one
less than the register size, the sign bit is not included in the value of m.

So for the 32bit value, all fractional:

a) M include sign-bit: Q1.31
b) M doesn't include sign-bit: Q0.31

> 
>  * @m: number of integer bits, m <= 32.
> 
> > + * @n: number of fractional bits, only support n <= 32
> > + *
> > + * Convert and clamp S31.32 sign-magnitude to Qm.n (signed 2's complement). The
> > + * higher bits that above m + n are cleared or equal to sign-bit BIT(m+n).
> 
> [nit] BIT(m + n - 1) if we count from 0.

do we real need to consider this, convert to (Q1.0) :)
I think it can be easily caught by review.
> 
> > + */
> > +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> > +				      uint32_t m, uint32_t n)
> > +{
> > +	u64 mag = (user_input & ~BIT_ULL(63)) >> (32 - n);
> > +	bool negative = !!(user_input & BIT_ULL(63));
> > +	s64 val;
> > +
> > +	WARN_ON(m < 1 || m > 32 || n > 32);
> > +
> > +	/* the range of signed 2's complement is [-2^(m-1), 2^(m-1) - 2^-n] */
> > +	val = clamp_val(mag, 0, negative ?
> > +				BIT_ULL(n + m - 1) : BIT_ULL(n + m - 1) - 1);
> > +
> > +	return negative ? -val : val;
> > +}
> > +EXPORT_SYMBOL(drm_color_ctm_s31_32_to_qm_n);
> > +
> >  /**
> >   * drm_crtc_enable_color_mgmt - enable color management properties
> >   * @crtc: DRM CRTC
> > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > index d1c662d92ab7..60fea5501886 100644
> > --- a/include/drm/drm_color_mgmt.h
> > +++ b/include/drm/drm_color_mgmt.h
> > @@ -30,6 +30,8 @@ struct drm_crtc;
> >  struct drm_plane;
> >  
> >  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
> > +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> > +				      uint32_t m, uint32_t n);
> >  
> >  void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> >  				uint degamma_lut_size,
> > 
> 
> 
> -- 
> Mihail
> 
> 

Re: [PATCH v5 1/4] drm: Add a new helper drm_color_ctm_s31_32_to_qm_n()

[james qian wang (Arm Technology China)]

On Wed, Oct 16, 2019 at 11:02:03AM +0000, Mihail Atanassov wrote:
> On Wednesday, 16 October 2019 11:34:08 BST james qian wang (Arm Technology China) wrote:
> > Add a new helper function drm_color_ctm_s31_32_to_qm_n() for driver to
> > convert S31.32 sign-magnitude to Qm.n 2's complement that supported by
> > hardware.
> > 
> > V4: Address Mihai, Daniel and Ilia's review comments.
> > V5: Includes the sign bit in the value of m (Qm.n).
> > 
> > Signed-off-by: james qian wang (Arm Technology China) <james.qian.wang@arm.com>
> > Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_color_mgmt.c | 27 +++++++++++++++++++++++++++
> >  include/drm/drm_color_mgmt.h     |  2 ++
> >  2 files changed, 29 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > index 4ce5c6d8de99..d313f194f1ec 100644
> > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > @@ -132,6 +132,33 @@ uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision)
> >  }
> >  EXPORT_SYMBOL(drm_color_lut_extract);
> >  
> > +/**
> > + * drm_color_ctm_s31_32_to_qm_n
> > + *
> > + * @user_input: input value
> > + * @m: number of integer bits, include the sign-bit, support range is [1, 32]
> 
> Any reason why numbers like Q0.32 are disallowed? In those cases, the
> 'sign' bit and the first fractional bit just happen to be the same bit.
> The longer I look at it, the more I think mentioning a 'sign-bit' here
> might confuse people more, since 2's complement doesn't have a
> dedicated bit just for the sign. How about reducing it simply to:

No, since the value is signed there must be dedicated sign-bit.

consider very simple 2 bit signed, Q1.1

 0.5  is 01
 0    is 00
-0.5  is 11
-1.0  is 10, sign-bit and value share same bit, but it is integer part.

See the wiki:

One convention includes the sign bit in the value of m,[1][2] and the other convention
does not. The choice of convention can be determined by summing m+n. If the value is equal
to the register size, then the sign bit is included in the value of m. If it is one
less than the register size, the sign bit is not included in the value of m.

So for the 32bit value, all fractional:

a) M include sign-bit: Q1.31
b) M doesn't include sign-bit: Q0.31

> 
>  * @m: number of integer bits, m <= 32.
> 
> > + * @n: number of fractional bits, only support n <= 32
> > + *
> > + * Convert and clamp S31.32 sign-magnitude to Qm.n (signed 2's complement). The
> > + * higher bits that above m + n are cleared or equal to sign-bit BIT(m+n).
> 
> [nit] BIT(m + n - 1) if we count from 0.

do we real need to consider this, convert to (Q1.0) :)
I think it can be easily caught by review.
> 
> > + */
> > +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> > +				      uint32_t m, uint32_t n)
> > +{
> > +	u64 mag = (user_input & ~BIT_ULL(63)) >> (32 - n);
> > +	bool negative = !!(user_input & BIT_ULL(63));
> > +	s64 val;
> > +
> > +	WARN_ON(m < 1 || m > 32 || n > 32);
> > +
> > +	/* the range of signed 2's complement is [-2^(m-1), 2^(m-1) - 2^-n] */
> > +	val = clamp_val(mag, 0, negative ?
> > +				BIT_ULL(n + m - 1) : BIT_ULL(n + m - 1) - 1);
> > +
> > +	return negative ? -val : val;
> > +}
> > +EXPORT_SYMBOL(drm_color_ctm_s31_32_to_qm_n);
> > +
> >  /**
> >   * drm_crtc_enable_color_mgmt - enable color management properties
> >   * @crtc: DRM CRTC
> > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > index d1c662d92ab7..60fea5501886 100644
> > --- a/include/drm/drm_color_mgmt.h
> > +++ b/include/drm/drm_color_mgmt.h
> > @@ -30,6 +30,8 @@ struct drm_crtc;
> >  struct drm_plane;
> >  
> >  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
> > +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> > +				      uint32_t m, uint32_t n);
> >  
> >  void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> >  				uint degamma_lut_size,
> > 
> 
> 
> -- 
> Mihail
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 1/4] drm: Add a new helper drm_color_ctm_s31_32_to_qm_n()

[Mihail Atanassov]

On Friday, 18 October 2019 08:51:09 BST james qian wang (Arm Technology China) wrote:
> On Wed, Oct 16, 2019 at 11:02:03AM +0000, Mihail Atanassov wrote:
> > On Wednesday, 16 October 2019 11:34:08 BST james qian wang (Arm Technology China) wrote:
> > > Add a new helper function drm_color_ctm_s31_32_to_qm_n() for driver to
> > > convert S31.32 sign-magnitude to Qm.n 2's complement that supported by
> > > hardware.
> > > 
> > > V4: Address Mihai, Daniel and Ilia's review comments.
> > > V5: Includes the sign bit in the value of m (Qm.n).
> > > 
> > > Signed-off-by: james qian wang (Arm Technology China) <james.qian.wang@arm.com>
> > > Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/drm_color_mgmt.c | 27 +++++++++++++++++++++++++++
> > >  include/drm/drm_color_mgmt.h     |  2 ++
> > >  2 files changed, 29 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > > index 4ce5c6d8de99..d313f194f1ec 100644
> > > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > > @@ -132,6 +132,33 @@ uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision)
> > >  }
> > >  EXPORT_SYMBOL(drm_color_lut_extract);
> > >  
> > > +/**
> > > + * drm_color_ctm_s31_32_to_qm_n
> > > + *
> > > + * @user_input: input value
> > > + * @m: number of integer bits, include the sign-bit, support range is [1, 32]
> > 
> > Any reason why numbers like Q0.32 are disallowed? In those cases, the
> > 'sign' bit and the first fractional bit just happen to be the same bit.
> > The longer I look at it, the more I think mentioning a 'sign-bit' here
> > might confuse people more, since 2's complement doesn't have a
> > dedicated bit just for the sign. How about reducing it simply to:
> 
> No, since the value is signed there must be dedicated sign-bit.

As I've said a few times before in this review, 2's complement has no
dedicated sign bit, that's the whole reason 2's complement exists in
the first place. The sign is implemented by negating the weight of
the most significant bit. This isn't a dedicated +/- field!

> 
> consider very simple 2 bit signed, Q1.1
> 
>  0.5  is 01
>  0    is 00
> -0.5  is 11
> -1.0  is 10, sign-bit and value share same bit, but it is integer part.

And a very simple 2-bit signed Q0.2 would look like this:

weights: (-2^-1)*b1 + (2^-2)*b0
          ^
          L-> note negative weight at most significant bit position

+-------------+---------------+
/ bit pattern | decimal value |
+-------------+---------------+
\     00      |     0.0       |
/     01      |     0.25      |
\     10      |    -0.5       |
/     11      |    -0.25      |
+-------------+---------------+

(Apologies for the ragged left border on the table :/)

I genuinely don't see why you really want to have that integer part be
strictly non-zero in size, it can very well be all fractional.

> 
> See the wiki:
> 
> One convention includes the sign bit in the value of m,[1][2] and the other convention
> does not. The choice of convention can be determined by summing m+n. If the value is equal
> to the register size, then the sign bit is included in the value of m. If it is one
> less than the register size, the sign bit is not included in the value of m.

This is very much off the mark. See above for my sign bit in 2's
complement rant. With that caveat, what you refer to as the sign
bit is simply the top bit. If m+n is 16, then what you refer to as
the sign bit is in bit position 15 with a weight of -2^(m-1). If
m+n is instead 13, then all that changes is that the bit with the
weight of -2^(m-1) is at position 12.

Most importantly, there is nothing special about m + n == regsize,
the lack of sign-extension aside.

What I think is the source of confusion is that when you expand, say,
Q4.4 into a Q8.8, you need to do sign extension, so bit position 7
in the original Q4.4 needs to be replicated into positions 12-15 in
addition to moving it to position 11 in the destination format. But
then those are no longer sign bits, the signedness is encoded in bit
15 as a weight of -2^7 :).

> 
> So for the 32bit value, all fractional:
> 
> a) M include sign-bit: Q1.31

This is a 32-bit number with range [-1, 1 - 2^-31] and precision 2^-31.
The weight of bit 31 is simply -2^0 (i.e. -1). This has 1 integer bit.

> b) M doesn't include sign-bit: Q0.31

This is a 31-bit number with range [-0.5, 1 - 2^-31]. Same precision as
above but smaller range. This is all fractional but not a 32-bit value.
I think you're looking for Q0.32, which has almost the same range but
double the precision.

> 
> > 
> >  * @m: number of integer bits, m <= 32.
> > 
> > > + * @n: number of fractional bits, only support n <= 32
> > > + *
> > > + * Convert and clamp S31.32 sign-magnitude to Qm.n (signed 2's complement). The
> > > + * higher bits that above m + n are cleared or equal to sign-bit BIT(m+n).
> > 
> > [nit] BIT(m + n - 1) if we count from 0.
> 
> do we real need to consider this, convert to (Q1.0) :)
> I think it can be easily caught by review.

Q1.0 has a range of [-1, 0] and precision of 1, but I don't get how
this is relevant. I was just referring to convention that bits get
counted from 0, so the most significant bit is simply at position
m + n - 1 and not m + n.
m + n in, say, Q16.16 would be bit 32, which is just past the valid
[0..31] bits.

I was hoping that by explicitly tagging the comment with '[nit]' would
help convey its low importance :).

> > 
> > > + */
> > > +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> > > +				      uint32_t m, uint32_t n)
> > > +{
> > > +	u64 mag = (user_input & ~BIT_ULL(63)) >> (32 - n);
> > > +	bool negative = !!(user_input & BIT_ULL(63));
> > > +	s64 val;
> > > +
> > > +	WARN_ON(m < 1 || m > 32 || n > 32);
> > > +
> > > +	/* the range of signed 2's complement is [-2^(m-1), 2^(m-1) - 2^-n] */
> > > +	val = clamp_val(mag, 0, negative ?
> > > +				BIT_ULL(n + m - 1) : BIT_ULL(n + m - 1) - 1);
> > > +
> > > +	return negative ? -val : val;
> > > +}
> > > +EXPORT_SYMBOL(drm_color_ctm_s31_32_to_qm_n);
> > > +
> > >  /**
> > >   * drm_crtc_enable_color_mgmt - enable color management properties
> > >   * @crtc: DRM CRTC
> > > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > > index d1c662d92ab7..60fea5501886 100644
> > > --- a/include/drm/drm_color_mgmt.h
> > > +++ b/include/drm/drm_color_mgmt.h
> > > @@ -30,6 +30,8 @@ struct drm_crtc;
> > >  struct drm_plane;
> > >  
> > >  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
> > > +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> > > +				      uint32_t m, uint32_t n);
> > >  
> > >  void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> > >  				uint degamma_lut_size,
> > > 
> > 
> > 
> 


-- 
Mihail

Re: [PATCH v5 1/4] drm: Add a new helper drm_color_ctm_s31_32_to_qm_n()

[Mihail Atanassov]

On Friday, 18 October 2019 08:51:09 BST james qian wang (Arm Technology China) wrote:
> On Wed, Oct 16, 2019 at 11:02:03AM +0000, Mihail Atanassov wrote:
> > On Wednesday, 16 October 2019 11:34:08 BST james qian wang (Arm Technology China) wrote:
> > > Add a new helper function drm_color_ctm_s31_32_to_qm_n() for driver to
> > > convert S31.32 sign-magnitude to Qm.n 2's complement that supported by
> > > hardware.
> > > 
> > > V4: Address Mihai, Daniel and Ilia's review comments.
> > > V5: Includes the sign bit in the value of m (Qm.n).
> > > 
> > > Signed-off-by: james qian wang (Arm Technology China) <james.qian.wang@arm.com>
> > > Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/drm_color_mgmt.c | 27 +++++++++++++++++++++++++++
> > >  include/drm/drm_color_mgmt.h     |  2 ++
> > >  2 files changed, 29 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > > index 4ce5c6d8de99..d313f194f1ec 100644
> > > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > > @@ -132,6 +132,33 @@ uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision)
> > >  }
> > >  EXPORT_SYMBOL(drm_color_lut_extract);
> > >  
> > > +/**
> > > + * drm_color_ctm_s31_32_to_qm_n
> > > + *
> > > + * @user_input: input value
> > > + * @m: number of integer bits, include the sign-bit, support range is [1, 32]
> > 
> > Any reason why numbers like Q0.32 are disallowed? In those cases, the
> > 'sign' bit and the first fractional bit just happen to be the same bit.
> > The longer I look at it, the more I think mentioning a 'sign-bit' here
> > might confuse people more, since 2's complement doesn't have a
> > dedicated bit just for the sign. How about reducing it simply to:
> 
> No, since the value is signed there must be dedicated sign-bit.

As I've said a few times before in this review, 2's complement has no
dedicated sign bit, that's the whole reason 2's complement exists in
the first place. The sign is implemented by negating the weight of
the most significant bit. This isn't a dedicated +/- field!

> 
> consider very simple 2 bit signed, Q1.1
> 
>  0.5  is 01
>  0    is 00
> -0.5  is 11
> -1.0  is 10, sign-bit and value share same bit, but it is integer part.

And a very simple 2-bit signed Q0.2 would look like this:

weights: (-2^-1)*b1 + (2^-2)*b0
          ^
          L-> note negative weight at most significant bit position

+-------------+---------------+
/ bit pattern | decimal value |
+-------------+---------------+
\     00      |     0.0       |
/     01      |     0.25      |
\     10      |    -0.5       |
/     11      |    -0.25      |
+-------------+---------------+

(Apologies for the ragged left border on the table :/)

I genuinely don't see why you really want to have that integer part be
strictly non-zero in size, it can very well be all fractional.

> 
> See the wiki:
> 
> One convention includes the sign bit in the value of m,[1][2] and the other convention
> does not. The choice of convention can be determined by summing m+n. If the value is equal
> to the register size, then the sign bit is included in the value of m. If it is one
> less than the register size, the sign bit is not included in the value of m.

This is very much off the mark. See above for my sign bit in 2's
complement rant. With that caveat, what you refer to as the sign
bit is simply the top bit. If m+n is 16, then what you refer to as
the sign bit is in bit position 15 with a weight of -2^(m-1). If
m+n is instead 13, then all that changes is that the bit with the
weight of -2^(m-1) is at position 12.

Most importantly, there is nothing special about m + n == regsize,
the lack of sign-extension aside.

What I think is the source of confusion is that when you expand, say,
Q4.4 into a Q8.8, you need to do sign extension, so bit position 7
in the original Q4.4 needs to be replicated into positions 12-15 in
addition to moving it to position 11 in the destination format. But
then those are no longer sign bits, the signedness is encoded in bit
15 as a weight of -2^7 :).

> 
> So for the 32bit value, all fractional:
> 
> a) M include sign-bit: Q1.31

This is a 32-bit number with range [-1, 1 - 2^-31] and precision 2^-31.
The weight of bit 31 is simply -2^0 (i.e. -1). This has 1 integer bit.

> b) M doesn't include sign-bit: Q0.31

This is a 31-bit number with range [-0.5, 1 - 2^-31]. Same precision as
above but smaller range. This is all fractional but not a 32-bit value.
I think you're looking for Q0.32, which has almost the same range but
double the precision.

> 
> > 
> >  * @m: number of integer bits, m <= 32.
> > 
> > > + * @n: number of fractional bits, only support n <= 32
> > > + *
> > > + * Convert and clamp S31.32 sign-magnitude to Qm.n (signed 2's complement). The
> > > + * higher bits that above m + n are cleared or equal to sign-bit BIT(m+n).
> > 
> > [nit] BIT(m + n - 1) if we count from 0.
> 
> do we real need to consider this, convert to (Q1.0) :)
> I think it can be easily caught by review.

Q1.0 has a range of [-1, 0] and precision of 1, but I don't get how
this is relevant. I was just referring to convention that bits get
counted from 0, so the most significant bit is simply at position
m + n - 1 and not m + n.
m + n in, say, Q16.16 would be bit 32, which is just past the valid
[0..31] bits.

I was hoping that by explicitly tagging the comment with '[nit]' would
help convey its low importance :).

> > 
> > > + */
> > > +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> > > +				      uint32_t m, uint32_t n)
> > > +{
> > > +	u64 mag = (user_input & ~BIT_ULL(63)) >> (32 - n);
> > > +	bool negative = !!(user_input & BIT_ULL(63));
> > > +	s64 val;
> > > +
> > > +	WARN_ON(m < 1 || m > 32 || n > 32);
> > > +
> > > +	/* the range of signed 2's complement is [-2^(m-1), 2^(m-1) - 2^-n] */
> > > +	val = clamp_val(mag, 0, negative ?
> > > +				BIT_ULL(n + m - 1) : BIT_ULL(n + m - 1) - 1);
> > > +
> > > +	return negative ? -val : val;
> > > +}
> > > +EXPORT_SYMBOL(drm_color_ctm_s31_32_to_qm_n);
> > > +
> > >  /**
> > >   * drm_crtc_enable_color_mgmt - enable color management properties
> > >   * @crtc: DRM CRTC
> > > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > > index d1c662d92ab7..60fea5501886 100644
> > > --- a/include/drm/drm_color_mgmt.h
> > > +++ b/include/drm/drm_color_mgmt.h
> > > @@ -30,6 +30,8 @@ struct drm_crtc;
> > >  struct drm_plane;
> > >  
> > >  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
> > > +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> > > +				      uint32_t m, uint32_t n);
> > >  
> > >  void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> > >  				uint degamma_lut_size,
> > > 
> > 
> > 
> 


-- 
Mihail



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

Re: [PATCH v5 1/4] drm: Add a new helper drm_color_ctm_s31_32_to_qm_n()

[james qian wang (Arm Technology China)]

On Fri, Oct 18, 2019 at 09:30:11AM +0000, Mihail Atanassov wrote:
> On Friday, 18 October 2019 08:51:09 BST james qian wang (Arm Technology China) wrote:
> > On Wed, Oct 16, 2019 at 11:02:03AM +0000, Mihail Atanassov wrote:
> > > On Wednesday, 16 October 2019 11:34:08 BST james qian wang (Arm Technology China) wrote:
> > > > Add a new helper function drm_color_ctm_s31_32_to_qm_n() for driver to
> > > > convert S31.32 sign-magnitude to Qm.n 2's complement that supported by
> > > > hardware.
> > > > 
> > > > V4: Address Mihai, Daniel and Ilia's review comments.
> > > > V5: Includes the sign bit in the value of m (Qm.n).
> > > > 
> > > > Signed-off-by: james qian wang (Arm Technology China) <james.qian.wang@arm.com>
> > > > Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
> > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > ---
> > > >  drivers/gpu/drm/drm_color_mgmt.c | 27 +++++++++++++++++++++++++++
> > > >  include/drm/drm_color_mgmt.h     |  2 ++
> > > >  2 files changed, 29 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > > > index 4ce5c6d8de99..d313f194f1ec 100644
> > > > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > > > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > > > @@ -132,6 +132,33 @@ uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision)
> > > >  }
> > > >  EXPORT_SYMBOL(drm_color_lut_extract);
> > > >  
> > > > +/**
> > > > + * drm_color_ctm_s31_32_to_qm_n
> > > > + *
> > > > + * @user_input: input value
> > > > + * @m: number of integer bits, include the sign-bit, support range is [1, 32]
> > > 
> > > Any reason why numbers like Q0.32 are disallowed? In those cases, the
> > > 'sign' bit and the first fractional bit just happen to be the same bit.
> > > The longer I look at it, the more I think mentioning a 'sign-bit' here
> > > might confuse people more, since 2's complement doesn't have a
> > > dedicated bit just for the sign. How about reducing it simply to:
> > 
> > No, since the value is signed there must be dedicated sign-bit.
> 
> As I've said a few times before in this review, 2's complement has no
> dedicated sign bit, that's the whole reason 2's complement exists in
> the first place. The sign is implemented by negating the weight of
> the most significant bit. This isn't a dedicated +/- field!
> 
> > 
> > consider very simple 2 bit signed, Q1.1
> > 
> >  0.5  is 01
> >  0    is 00
> > -0.5  is 11
> > -1.0  is 10, sign-bit and value share same bit, but it is integer part.
> 
> And a very simple 2-bit signed Q0.2 would look like this:
> 
> weights: (-2^-1)*b1 + (2^-2)*b0
>           ^
>           L-> note negative weight at most significant bit position
> 
> +-------------+---------------+
> / bit pattern | decimal value |
> +-------------+---------------+
> \     00      |     0.0       |
> /     01      |     0.25      |
> \     10      |    -0.5       |
> /     11      |    -0.25      |
> +-------------+---------------+
> 
> (Apologies for the ragged left border on the table :/)
> 
> I genuinely don't see why you really want to have that integer part be
> strictly non-zero in size, it can very well be all fractional.
> 
> > 
> > See the wiki:
> > 
> > One convention includes the sign bit in the value of m,[1][2] and the other convention
> > does not. The choice of convention can be determined by summing m+n. If the value is equal
> > to the register size, then the sign bit is included in the value of m. If it is one
> > less than the register size, the sign bit is not included in the value of m.
> 
> This is very much off the mark. See above for my sign bit in 2's
> complement rant. With that caveat, what you refer to as the sign
> bit is simply the top bit. If m+n is 16, then what you refer to as
> the sign bit is in bit position 15 with a weight of -2^(m-1). If
> m+n is instead 13, then all that changes is that the bit with the
> weight of -2^(m-1) is at position 12.
> 
> Most importantly, there is nothing special about m + n == regsize,
> the lack of sign-extension aside.
> 
> What I think is the source of confusion is that when you expand, say,
> Q4.4 into a Q8.8, you need to do sign extension, so bit position 7
> in the original Q4.4 needs to be replicated into positions 12-15 in
> addition to moving it to position 11 in the destination format. But
> then those are no longer sign bits, the signedness is encoded in bit
> 15 as a weight of -2^7 :).
>

Thank you very much.

finial I got it, will update the patch in the next version 

> > 
> > So for the 32bit value, all fractional:
> > 
> > a) M include sign-bit: Q1.31
> 
> This is a 32-bit number with range [-1, 1 - 2^-31] and precision 2^-31.
> The weight of bit 31 is simply -2^0 (i.e. -1). This has 1 integer bit.
> 
> > b) M doesn't include sign-bit: Q0.31
> 
> This is a 31-bit number with range [-0.5, 1 - 2^-31]. Same precision as
> above but smaller range. This is all fractional but not a 32-bit value.
> I think you're looking for Q0.32, which has almost the same range but
> double the precision.
> 
> > 
> > > 
> > >  * @m: number of integer bits, m <= 32.
> > > 
> > > > + * @n: number of fractional bits, only support n <= 32
> > > > + *
> > > > + * Convert and clamp S31.32 sign-magnitude to Qm.n (signed 2's complement). The
> > > > + * higher bits that above m + n are cleared or equal to sign-bit BIT(m+n).
> > > 
> > > [nit] BIT(m + n - 1) if we count from 0.
> > 
> > do we real need to consider this, convert to (Q1.0) :)
> > I think it can be easily caught by review.
> 
> Q1.0 has a range of [-1, 0] and precision of 1, but I don't get how
> this is relevant. I was just referring to convention that bits get
> counted from 0, so the most significant bit is simply at position
> m + n - 1 and not m + n.
> m + n in, say, Q16.16 would be bit 32, which is just past the valid
> [0..31] bits.
> 
> I was hoping that by explicitly tagging the comment with '[nit]' would
> help convey its low importance :).
> 
> > > 
> > > > + */
> > > > +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> > > > +				      uint32_t m, uint32_t n)
> > > > +{
> > > > +	u64 mag = (user_input & ~BIT_ULL(63)) >> (32 - n);
> > > > +	bool negative = !!(user_input & BIT_ULL(63));
> > > > +	s64 val;
> > > > +
> > > > +	WARN_ON(m < 1 || m > 32 || n > 32);
> > > > +
> > > > +	/* the range of signed 2's complement is [-2^(m-1), 2^(m-1) - 2^-n] */
> > > > +	val = clamp_val(mag, 0, negative ?
> > > > +				BIT_ULL(n + m - 1) : BIT_ULL(n + m - 1) - 1);
> > > > +
> > > > +	return negative ? -val : val;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_color_ctm_s31_32_to_qm_n);
> > > > +
> > > >  /**
> > > >   * drm_crtc_enable_color_mgmt - enable color management properties
> > > >   * @crtc: DRM CRTC
> > > > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > > > index d1c662d92ab7..60fea5501886 100644
> > > > --- a/include/drm/drm_color_mgmt.h
> > > > +++ b/include/drm/drm_color_mgmt.h
> > > > @@ -30,6 +30,8 @@ struct drm_crtc;
> > > >  struct drm_plane;
> > > >  
> > > >  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
> > > > +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> > > > +				      uint32_t m, uint32_t n);
> > > >  
> > > >  void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> > > >  				uint degamma_lut_size,
> > > > 
> > > 
> > > 
> > 
> 
> 
> -- 
> Mihail
> 
> 

Re: [PATCH v5 1/4] drm: Add a new helper drm_color_ctm_s31_32_to_qm_n()

[james qian wang (Arm Technology China)]

On Fri, Oct 18, 2019 at 09:30:11AM +0000, Mihail Atanassov wrote:
> On Friday, 18 October 2019 08:51:09 BST james qian wang (Arm Technology China) wrote:
> > On Wed, Oct 16, 2019 at 11:02:03AM +0000, Mihail Atanassov wrote:
> > > On Wednesday, 16 October 2019 11:34:08 BST james qian wang (Arm Technology China) wrote:
> > > > Add a new helper function drm_color_ctm_s31_32_to_qm_n() for driver to
> > > > convert S31.32 sign-magnitude to Qm.n 2's complement that supported by
> > > > hardware.
> > > > 
> > > > V4: Address Mihai, Daniel and Ilia's review comments.
> > > > V5: Includes the sign bit in the value of m (Qm.n).
> > > > 
> > > > Signed-off-by: james qian wang (Arm Technology China) <james.qian.wang@arm.com>
> > > > Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
> > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > ---
> > > >  drivers/gpu/drm/drm_color_mgmt.c | 27 +++++++++++++++++++++++++++
> > > >  include/drm/drm_color_mgmt.h     |  2 ++
> > > >  2 files changed, 29 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > > > index 4ce5c6d8de99..d313f194f1ec 100644
> > > > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > > > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > > > @@ -132,6 +132,33 @@ uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision)
> > > >  }
> > > >  EXPORT_SYMBOL(drm_color_lut_extract);
> > > >  
> > > > +/**
> > > > + * drm_color_ctm_s31_32_to_qm_n
> > > > + *
> > > > + * @user_input: input value
> > > > + * @m: number of integer bits, include the sign-bit, support range is [1, 32]
> > > 
> > > Any reason why numbers like Q0.32 are disallowed? In those cases, the
> > > 'sign' bit and the first fractional bit just happen to be the same bit.
> > > The longer I look at it, the more I think mentioning a 'sign-bit' here
> > > might confuse people more, since 2's complement doesn't have a
> > > dedicated bit just for the sign. How about reducing it simply to:
> > 
> > No, since the value is signed there must be dedicated sign-bit.
> 
> As I've said a few times before in this review, 2's complement has no
> dedicated sign bit, that's the whole reason 2's complement exists in
> the first place. The sign is implemented by negating the weight of
> the most significant bit. This isn't a dedicated +/- field!
> 
> > 
> > consider very simple 2 bit signed, Q1.1
> > 
> >  0.5  is 01
> >  0    is 00
> > -0.5  is 11
> > -1.0  is 10, sign-bit and value share same bit, but it is integer part.
> 
> And a very simple 2-bit signed Q0.2 would look like this:
> 
> weights: (-2^-1)*b1 + (2^-2)*b0
>           ^
>           L-> note negative weight at most significant bit position
> 
> +-------------+---------------+
> / bit pattern | decimal value |
> +-------------+---------------+
> \     00      |     0.0       |
> /     01      |     0.25      |
> \     10      |    -0.5       |
> /     11      |    -0.25      |
> +-------------+---------------+
> 
> (Apologies for the ragged left border on the table :/)
> 
> I genuinely don't see why you really want to have that integer part be
> strictly non-zero in size, it can very well be all fractional.
> 
> > 
> > See the wiki:
> > 
> > One convention includes the sign bit in the value of m,[1][2] and the other convention
> > does not. The choice of convention can be determined by summing m+n. If the value is equal
> > to the register size, then the sign bit is included in the value of m. If it is one
> > less than the register size, the sign bit is not included in the value of m.
> 
> This is very much off the mark. See above for my sign bit in 2's
> complement rant. With that caveat, what you refer to as the sign
> bit is simply the top bit. If m+n is 16, then what you refer to as
> the sign bit is in bit position 15 with a weight of -2^(m-1). If
> m+n is instead 13, then all that changes is that the bit with the
> weight of -2^(m-1) is at position 12.
> 
> Most importantly, there is nothing special about m + n == regsize,
> the lack of sign-extension aside.
> 
> What I think is the source of confusion is that when you expand, say,
> Q4.4 into a Q8.8, you need to do sign extension, so bit position 7
> in the original Q4.4 needs to be replicated into positions 12-15 in
> addition to moving it to position 11 in the destination format. But
> then those are no longer sign bits, the signedness is encoded in bit
> 15 as a weight of -2^7 :).
>

Thank you very much.

finial I got it, will update the patch in the next version 

> > 
> > So for the 32bit value, all fractional:
> > 
> > a) M include sign-bit: Q1.31
> 
> This is a 32-bit number with range [-1, 1 - 2^-31] and precision 2^-31.
> The weight of bit 31 is simply -2^0 (i.e. -1). This has 1 integer bit.
> 
> > b) M doesn't include sign-bit: Q0.31
> 
> This is a 31-bit number with range [-0.5, 1 - 2^-31]. Same precision as
> above but smaller range. This is all fractional but not a 32-bit value.
> I think you're looking for Q0.32, which has almost the same range but
> double the precision.
> 
> > 
> > > 
> > >  * @m: number of integer bits, m <= 32.
> > > 
> > > > + * @n: number of fractional bits, only support n <= 32
> > > > + *
> > > > + * Convert and clamp S31.32 sign-magnitude to Qm.n (signed 2's complement). The
> > > > + * higher bits that above m + n are cleared or equal to sign-bit BIT(m+n).
> > > 
> > > [nit] BIT(m + n - 1) if we count from 0.
> > 
> > do we real need to consider this, convert to (Q1.0) :)
> > I think it can be easily caught by review.
> 
> Q1.0 has a range of [-1, 0] and precision of 1, but I don't get how
> this is relevant. I was just referring to convention that bits get
> counted from 0, so the most significant bit is simply at position
> m + n - 1 and not m + n.
> m + n in, say, Q16.16 would be bit 32, which is just past the valid
> [0..31] bits.
> 
> I was hoping that by explicitly tagging the comment with '[nit]' would
> help convey its low importance :).
> 
> > > 
> > > > + */
> > > > +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> > > > +				      uint32_t m, uint32_t n)
> > > > +{
> > > > +	u64 mag = (user_input & ~BIT_ULL(63)) >> (32 - n);
> > > > +	bool negative = !!(user_input & BIT_ULL(63));
> > > > +	s64 val;
> > > > +
> > > > +	WARN_ON(m < 1 || m > 32 || n > 32);
> > > > +
> > > > +	/* the range of signed 2's complement is [-2^(m-1), 2^(m-1) - 2^-n] */
> > > > +	val = clamp_val(mag, 0, negative ?
> > > > +				BIT_ULL(n + m - 1) : BIT_ULL(n + m - 1) - 1);
> > > > +
> > > > +	return negative ? -val : val;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_color_ctm_s31_32_to_qm_n);
> > > > +
> > > >  /**
> > > >   * drm_crtc_enable_color_mgmt - enable color management properties
> > > >   * @crtc: DRM CRTC
> > > > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > > > index d1c662d92ab7..60fea5501886 100644
> > > > --- a/include/drm/drm_color_mgmt.h
> > > > +++ b/include/drm/drm_color_mgmt.h
> > > > @@ -30,6 +30,8 @@ struct drm_crtc;
> > > >  struct drm_plane;
> > > >  
> > > >  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
> > > > +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> > > > +				      uint32_t m, uint32_t n);
> > > >  
> > > >  void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> > > >  				uint degamma_lut_size,
> > > > 
> > > 
> > > 
> > 
> 
> 
> -- 
> Mihail
> 
> 

[PATCH v5 2/4] drm/komeda: Add drm_lut_to_fgamma_coeffs()

[james qian wang (Arm Technology China)]

This function is used to convert drm color lut to komeda HW required curve
coeffs values.

Signed-off-by: james qian wang (Arm Technology China) <james.qian.wang@arm.com>
Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 .../arm/display/komeda/komeda_color_mgmt.c    | 52 +++++++++++++++++++
 .../arm/display/komeda/komeda_color_mgmt.h    |  9 +++-
 2 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
index 9d14a92dbb17..c180ce70c26c 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
@@ -65,3 +65,55 @@ const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range)
 
 	return coeffs;
 }
+
+struct gamma_curve_sector {
+	u32 boundary_start;
+	u32 num_of_segments;
+	u32 segment_width;
+};
+
+struct gamma_curve_segment {
+	u32 start;
+	u32 end;
+};
+
+static struct gamma_curve_sector sector_tbl[] = {
+	{ 0,    4,  4   },
+	{ 16,   4,  4   },
+	{ 32,   4,  8   },
+	{ 64,   4,  16  },
+	{ 128,  4,  32  },
+	{ 256,  4,  64  },
+	{ 512,  16, 32  },
+	{ 1024, 24, 128 },
+};
+
+static void
+drm_lut_to_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs,
+		  struct gamma_curve_sector *sector_tbl, u32 num_sectors)
+{
+	struct drm_color_lut *lut;
+	u32 i, j, in, num = 0;
+
+	if (!lut_blob)
+		return;
+
+	lut = lut_blob->data;
+
+	for (i = 0; i < num_sectors; i++) {
+		for (j = 0; j < sector_tbl[i].num_of_segments; j++) {
+			in = sector_tbl[i].boundary_start +
+			     j * sector_tbl[i].segment_width;
+
+			coeffs[num++] = drm_color_lut_extract(lut[in].red,
+						KOMEDA_COLOR_PRECISION);
+		}
+	}
+
+	coeffs[num] = BIT(KOMEDA_COLOR_PRECISION);
+}
+
+void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
+{
+	drm_lut_to_coeffs(lut_blob, coeffs, sector_tbl, ARRAY_SIZE(sector_tbl));
+}
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
index a2df218f58e7..08ab69281648 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
@@ -11,7 +11,14 @@
 #include <drm/drm_color_mgmt.h>
 
 #define KOMEDA_N_YUV2RGB_COEFFS		12
+#define KOMEDA_N_RGB2YUV_COEFFS		12
+#define KOMEDA_COLOR_PRECISION		12
+#define KOMEDA_N_GAMMA_COEFFS		65
+#define KOMEDA_COLOR_LUT_SIZE		BIT(KOMEDA_COLOR_PRECISION)
+#define KOMEDA_N_CTM_COEFFS		9
+
+void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
 
 const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range);
 
-#endif
+#endif /*_KOMEDA_COLOR_MGMT_H_*/
-- 
2.20.1

[PATCH v5 2/4] drm/komeda: Add drm_lut_to_fgamma_coeffs()

[james qian wang (Arm Technology China)]

This function is used to convert drm color lut to komeda HW required curve
coeffs values.

Signed-off-by: james qian wang (Arm Technology China) <james.qian.wang@arm.com>
Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 .../arm/display/komeda/komeda_color_mgmt.c    | 52 +++++++++++++++++++
 .../arm/display/komeda/komeda_color_mgmt.h    |  9 +++-
 2 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
index 9d14a92dbb17..c180ce70c26c 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
@@ -65,3 +65,55 @@ const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range)
 
 	return coeffs;
 }
+
+struct gamma_curve_sector {
+	u32 boundary_start;
+	u32 num_of_segments;
+	u32 segment_width;
+};
+
+struct gamma_curve_segment {
+	u32 start;
+	u32 end;
+};
+
+static struct gamma_curve_sector sector_tbl[] = {
+	{ 0,    4,  4   },
+	{ 16,   4,  4   },
+	{ 32,   4,  8   },
+	{ 64,   4,  16  },
+	{ 128,  4,  32  },
+	{ 256,  4,  64  },
+	{ 512,  16, 32  },
+	{ 1024, 24, 128 },
+};
+
+static void
+drm_lut_to_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs,
+		  struct gamma_curve_sector *sector_tbl, u32 num_sectors)
+{
+	struct drm_color_lut *lut;
+	u32 i, j, in, num = 0;
+
+	if (!lut_blob)
+		return;
+
+	lut = lut_blob->data;
+
+	for (i = 0; i < num_sectors; i++) {
+		for (j = 0; j < sector_tbl[i].num_of_segments; j++) {
+			in = sector_tbl[i].boundary_start +
+			     j * sector_tbl[i].segment_width;
+
+			coeffs[num++] = drm_color_lut_extract(lut[in].red,
+						KOMEDA_COLOR_PRECISION);
+		}
+	}
+
+	coeffs[num] = BIT(KOMEDA_COLOR_PRECISION);
+}
+
+void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
+{
+	drm_lut_to_coeffs(lut_blob, coeffs, sector_tbl, ARRAY_SIZE(sector_tbl));
+}
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
index a2df218f58e7..08ab69281648 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
@@ -11,7 +11,14 @@
 #include <drm/drm_color_mgmt.h>
 
 #define KOMEDA_N_YUV2RGB_COEFFS		12
+#define KOMEDA_N_RGB2YUV_COEFFS		12
+#define KOMEDA_COLOR_PRECISION		12
+#define KOMEDA_N_GAMMA_COEFFS		65
+#define KOMEDA_COLOR_LUT_SIZE		BIT(KOMEDA_COLOR_PRECISION)
+#define KOMEDA_N_CTM_COEFFS		9
+
+void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
 
 const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range);
 
-#endif
+#endif /*_KOMEDA_COLOR_MGMT_H_*/
-- 
2.20.1

[PATCH v5 3/4] drm/komeda: Add drm_ctm_to_coeffs()

[james qian wang (Arm Technology China)]

This function is for converting drm_color_ctm matrix to komeda hardware
required required Q2.12 2's complement CSC matrix.

v2:
  Move the fixpoint conversion function s31_32_to_q2_12() to drm core
  as a shared helper.

Signed-off-by: james qian wang (Arm Technology China) <james.qian.wang@arm.com>
Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 .../gpu/drm/arm/display/komeda/komeda_color_mgmt.c | 14 ++++++++++++++
 .../gpu/drm/arm/display/komeda/komeda_color_mgmt.h |  1 +
 2 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
index c180ce70c26c..d8e449e6ebda 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
@@ -117,3 +117,17 @@ void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
 {
 	drm_lut_to_coeffs(lut_blob, coeffs, sector_tbl, ARRAY_SIZE(sector_tbl));
 }
+
+void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs)
+{
+	struct drm_color_ctm *ctm;
+	u32 i;
+
+	if (!ctm_blob)
+		return;
+
+	ctm = ctm_blob->data;
+
+	for (i = 0; i < KOMEDA_N_CTM_COEFFS; i++)
+		coeffs[i] = drm_color_ctm_s31_32_to_qm_n(ctm->matrix[i], 3, 12);
+}
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
index 08ab69281648..2f4668466112 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
@@ -18,6 +18,7 @@
 #define KOMEDA_N_CTM_COEFFS		9
 
 void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
+void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs);
 
 const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range);
 
-- 
2.20.1

[PATCH v5 3/4] drm/komeda: Add drm_ctm_to_coeffs()

[james qian wang (Arm Technology China)]

This function is for converting drm_color_ctm matrix to komeda hardware
required required Q2.12 2's complement CSC matrix.

v2:
  Move the fixpoint conversion function s31_32_to_q2_12() to drm core
  as a shared helper.

Signed-off-by: james qian wang (Arm Technology China) <james.qian.wang@arm.com>
Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 .../gpu/drm/arm/display/komeda/komeda_color_mgmt.c | 14 ++++++++++++++
 .../gpu/drm/arm/display/komeda/komeda_color_mgmt.h |  1 +
 2 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
index c180ce70c26c..d8e449e6ebda 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
@@ -117,3 +117,17 @@ void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
 {
 	drm_lut_to_coeffs(lut_blob, coeffs, sector_tbl, ARRAY_SIZE(sector_tbl));
 }
+
+void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs)
+{
+	struct drm_color_ctm *ctm;
+	u32 i;
+
+	if (!ctm_blob)
+		return;
+
+	ctm = ctm_blob->data;
+
+	for (i = 0; i < KOMEDA_N_CTM_COEFFS; i++)
+		coeffs[i] = drm_color_ctm_s31_32_to_qm_n(ctm->matrix[i], 3, 12);
+}
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
index 08ab69281648..2f4668466112 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
@@ -18,6 +18,7 @@
 #define KOMEDA_N_CTM_COEFFS		9
 
 void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
+void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs);
 
 const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range);
 
-- 
2.20.1

[PATCH v5 4/4] drm/komeda: Adds gamma and color-transform support for DOU-IPS

[james qian wang (Arm Technology China)]

From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>

Adds gamma and color-transform support for DOU-IPS.
Adds two caps members fgamma_coeffs and ctm_coeffs to komeda_improc_state.
If color management changed, set gamma and color-transform accordingly.

v5: Rebase with drm-misc-next

Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
---
 .../arm/display/komeda/d71/d71_component.c    | 20 +++++++++++++++++++
 .../gpu/drm/arm/display/komeda/komeda_crtc.c  |  2 ++
 .../drm/arm/display/komeda/komeda_pipeline.h  |  3 +++
 .../display/komeda/komeda_pipeline_state.c    |  6 ++++++
 4 files changed, 31 insertions(+)

diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
index 6740b8422f11..63a1b6f9cbba 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
@@ -1032,7 +1032,9 @@ static int d71_merger_init(struct d71_dev *d71,
 static void d71_improc_update(struct komeda_component *c,
 			      struct komeda_component_state *state)
 {
+	struct drm_crtc_state *crtc_st = state->crtc->state;
 	struct komeda_improc_state *st = to_improc_st(state);
+	struct d71_pipeline *pipe = to_d71_pipeline(c->pipeline);
 	u32 __iomem *reg = c->reg;
 	u32 index, mask = 0, ctrl = 0;
 
@@ -1043,6 +1045,24 @@ static void d71_improc_update(struct komeda_component *c,
 	malidp_write32(reg, BLK_SIZE, HV_SIZE(st->hsize, st->vsize));
 	malidp_write32(reg, IPS_DEPTH, st->color_depth);
 
+	if (crtc_st->color_mgmt_changed) {
+		mask |= IPS_CTRL_FT | IPS_CTRL_RGB;
+
+		if (crtc_st->gamma_lut) {
+			malidp_write_group(pipe->dou_ft_coeff_addr, FT_COEFF0,
+					   KOMEDA_N_GAMMA_COEFFS,
+					   st->fgamma_coeffs);
+			ctrl |= IPS_CTRL_FT; /* enable gamma */
+		}
+
+		if (crtc_st->ctm) {
+			malidp_write_group(reg, IPS_RGB_RGB_COEFF0,
+					   KOMEDA_N_CTM_COEFFS,
+					   st->ctm_coeffs);
+			ctrl |= IPS_CTRL_RGB; /* enable gamut */
+		}
+	}
+
 	mask |= IPS_CTRL_YUV | IPS_CTRL_CHD422 | IPS_CTRL_CHD420;
 
 	/* config color format */
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 252015210fbc..1c452ea75999 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -617,6 +617,8 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
 
 	crtc->port = kcrtc->master->of_output_port;
 
+	drm_crtc_enable_color_mgmt(crtc, 0, true, KOMEDA_COLOR_LUT_SIZE);
+
 	return err;
 }
 
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
index bd6ca7c87037..ac8725e24853 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
@@ -11,6 +11,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include "malidp_utils.h"
+#include "komeda_color_mgmt.h"
 
 #define KOMEDA_MAX_PIPELINES		2
 #define KOMEDA_PIPELINE_MAX_LAYERS	4
@@ -327,6 +328,8 @@ struct komeda_improc_state {
 	struct komeda_component_state base;
 	u8 color_format, color_depth;
 	u16 hsize, vsize;
+	u32 fgamma_coeffs[KOMEDA_N_GAMMA_COEFFS];
+	u32 ctm_coeffs[KOMEDA_N_CTM_COEFFS];
 };
 
 /* display timing controller */
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index 42bdc63dcffa..0930234abb9d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -802,6 +802,12 @@ komeda_improc_validate(struct komeda_improc *improc,
 		st->color_format = BIT(__ffs(avail_formats));
 	}
 
+	if (kcrtc_st->base.color_mgmt_changed) {
+		drm_lut_to_fgamma_coeffs(kcrtc_st->base.gamma_lut,
+					 st->fgamma_coeffs);
+		drm_ctm_to_coeffs(kcrtc_st->base.ctm, st->ctm_coeffs);
+	}
+
 	komeda_component_add_input(&st->base, &dflow->input, 0);
 	komeda_component_set_output(&dflow->input, &improc->base, 0);
 
-- 
2.20.1

[PATCH v5 4/4] drm/komeda: Adds gamma and color-transform support for DOU-IPS

[james qian wang (Arm Technology China)]

From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>

Adds gamma and color-transform support for DOU-IPS.
Adds two caps members fgamma_coeffs and ctm_coeffs to komeda_improc_state.
If color management changed, set gamma and color-transform accordingly.

v5: Rebase with drm-misc-next

Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
---
 .../arm/display/komeda/d71/d71_component.c    | 20 +++++++++++++++++++
 .../gpu/drm/arm/display/komeda/komeda_crtc.c  |  2 ++
 .../drm/arm/display/komeda/komeda_pipeline.h  |  3 +++
 .../display/komeda/komeda_pipeline_state.c    |  6 ++++++
 4 files changed, 31 insertions(+)

diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
index 6740b8422f11..63a1b6f9cbba 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
@@ -1032,7 +1032,9 @@ static int d71_merger_init(struct d71_dev *d71,
 static void d71_improc_update(struct komeda_component *c,
 			      struct komeda_component_state *state)
 {
+	struct drm_crtc_state *crtc_st = state->crtc->state;
 	struct komeda_improc_state *st = to_improc_st(state);
+	struct d71_pipeline *pipe = to_d71_pipeline(c->pipeline);
 	u32 __iomem *reg = c->reg;
 	u32 index, mask = 0, ctrl = 0;
 
@@ -1043,6 +1045,24 @@ static void d71_improc_update(struct komeda_component *c,
 	malidp_write32(reg, BLK_SIZE, HV_SIZE(st->hsize, st->vsize));
 	malidp_write32(reg, IPS_DEPTH, st->color_depth);
 
+	if (crtc_st->color_mgmt_changed) {
+		mask |= IPS_CTRL_FT | IPS_CTRL_RGB;
+
+		if (crtc_st->gamma_lut) {
+			malidp_write_group(pipe->dou_ft_coeff_addr, FT_COEFF0,
+					   KOMEDA_N_GAMMA_COEFFS,
+					   st->fgamma_coeffs);
+			ctrl |= IPS_CTRL_FT; /* enable gamma */
+		}
+
+		if (crtc_st->ctm) {
+			malidp_write_group(reg, IPS_RGB_RGB_COEFF0,
+					   KOMEDA_N_CTM_COEFFS,
+					   st->ctm_coeffs);
+			ctrl |= IPS_CTRL_RGB; /* enable gamut */
+		}
+	}
+
 	mask |= IPS_CTRL_YUV | IPS_CTRL_CHD422 | IPS_CTRL_CHD420;
 
 	/* config color format */
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 252015210fbc..1c452ea75999 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -617,6 +617,8 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
 
 	crtc->port = kcrtc->master->of_output_port;
 
+	drm_crtc_enable_color_mgmt(crtc, 0, true, KOMEDA_COLOR_LUT_SIZE);
+
 	return err;
 }
 
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
index bd6ca7c87037..ac8725e24853 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
@@ -11,6 +11,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include "malidp_utils.h"
+#include "komeda_color_mgmt.h"
 
 #define KOMEDA_MAX_PIPELINES		2
 #define KOMEDA_PIPELINE_MAX_LAYERS	4
@@ -327,6 +328,8 @@ struct komeda_improc_state {
 	struct komeda_component_state base;
 	u8 color_format, color_depth;
 	u16 hsize, vsize;
+	u32 fgamma_coeffs[KOMEDA_N_GAMMA_COEFFS];
+	u32 ctm_coeffs[KOMEDA_N_CTM_COEFFS];
 };
 
 /* display timing controller */
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index 42bdc63dcffa..0930234abb9d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -802,6 +802,12 @@ komeda_improc_validate(struct komeda_improc *improc,
 		st->color_format = BIT(__ffs(avail_formats));
 	}
 
+	if (kcrtc_st->base.color_mgmt_changed) {
+		drm_lut_to_fgamma_coeffs(kcrtc_st->base.gamma_lut,
+					 st->fgamma_coeffs);
+		drm_ctm_to_coeffs(kcrtc_st->base.ctm, st->ctm_coeffs);
+	}
+
 	komeda_component_add_input(&st->base, &dflow->input, 0);
 	komeda_component_set_output(&dflow->input, &improc->base, 0);
 
-- 
2.20.1

Re: [PATCH v5 4/4] drm/komeda: Adds gamma and color-transform support for DOU-IPS

[Mihail Atanassov]

On Wednesday, 16 October 2019 11:34:30 BST james qian wang (Arm Technology China) wrote:
> From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>
> 
> Adds gamma and color-transform support for DOU-IPS.
> Adds two caps members fgamma_coeffs and ctm_coeffs to komeda_improc_state.
> If color management changed, set gamma and color-transform accordingly.
> 
> v5: Rebase with drm-misc-next
> 
> Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> ---
>  .../arm/display/komeda/d71/d71_component.c    | 20 +++++++++++++++++++
>  .../gpu/drm/arm/display/komeda/komeda_crtc.c  |  2 ++
>  .../drm/arm/display/komeda/komeda_pipeline.h  |  3 +++
>  .../display/komeda/komeda_pipeline_state.c    |  6 ++++++
>  4 files changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> index 6740b8422f11..63a1b6f9cbba 100644
> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> @@ -1032,7 +1032,9 @@ static int d71_merger_init(struct d71_dev *d71,
>  static void d71_improc_update(struct komeda_component *c,
>  			      struct komeda_component_state *state)
>  {
> +	struct drm_crtc_state *crtc_st = state->crtc->state;
>  	struct komeda_improc_state *st = to_improc_st(state);
> +	struct d71_pipeline *pipe = to_d71_pipeline(c->pipeline);
>  	u32 __iomem *reg = c->reg;
>  	u32 index, mask = 0, ctrl = 0;
>  
> @@ -1043,6 +1045,24 @@ static void d71_improc_update(struct komeda_component *c,
>  	malidp_write32(reg, BLK_SIZE, HV_SIZE(st->hsize, st->vsize));
>  	malidp_write32(reg, IPS_DEPTH, st->color_depth);
>  
> +	if (crtc_st->color_mgmt_changed) {
> +		mask |= IPS_CTRL_FT | IPS_CTRL_RGB;
> +
> +		if (crtc_st->gamma_lut) {
> +			malidp_write_group(pipe->dou_ft_coeff_addr, FT_COEFF0,
> +					   KOMEDA_N_GAMMA_COEFFS,
> +					   st->fgamma_coeffs);
> +			ctrl |= IPS_CTRL_FT; /* enable gamma */
> +		}
> +
> +		if (crtc_st->ctm) {
> +			malidp_write_group(reg, IPS_RGB_RGB_COEFF0,
> +					   KOMEDA_N_CTM_COEFFS,
> +					   st->ctm_coeffs);
> +			ctrl |= IPS_CTRL_RGB; /* enable gamut */
> +		}
> +	}
> +
>  	mask |= IPS_CTRL_YUV | IPS_CTRL_CHD422 | IPS_CTRL_CHD420;
>  
>  	/* config color format */
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index 252015210fbc..1c452ea75999 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -617,6 +617,8 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
>  
>  	crtc->port = kcrtc->master->of_output_port;
>  
> +	drm_crtc_enable_color_mgmt(crtc, 0, true, KOMEDA_COLOR_LUT_SIZE);
> +
>  	return err;
>  }
>  
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> index bd6ca7c87037..ac8725e24853 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> @@ -11,6 +11,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include "malidp_utils.h"
> +#include "komeda_color_mgmt.h"
>  
>  #define KOMEDA_MAX_PIPELINES		2
>  #define KOMEDA_PIPELINE_MAX_LAYERS	4
> @@ -327,6 +328,8 @@ struct komeda_improc_state {
>  	struct komeda_component_state base;
>  	u8 color_format, color_depth;
>  	u16 hsize, vsize;
> +	u32 fgamma_coeffs[KOMEDA_N_GAMMA_COEFFS];
> +	u32 ctm_coeffs[KOMEDA_N_CTM_COEFFS];
>  };
>  
>  /* display timing controller */
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> index 42bdc63dcffa..0930234abb9d 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> @@ -802,6 +802,12 @@ komeda_improc_validate(struct komeda_improc *improc,
>  		st->color_format = BIT(__ffs(avail_formats));
>  	}
>  
> +	if (kcrtc_st->base.color_mgmt_changed) {
> +		drm_lut_to_fgamma_coeffs(kcrtc_st->base.gamma_lut,
> +					 st->fgamma_coeffs);
> +		drm_ctm_to_coeffs(kcrtc_st->base.ctm, st->ctm_coeffs);
> +	}
> +
>  	komeda_component_add_input(&st->base, &dflow->input, 0);
>  	komeda_component_set_output(&dflow->input, &improc->base, 0);
>  
> 
You must've missed it, but I had a

Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>

(granted, it was deeper in a thread and quite informally written
as 'r-b me')

-- 
Mihail

Re: [PATCH v5 4/4] drm/komeda: Adds gamma and color-transform support for DOU-IPS

[Mihail Atanassov]

On Wednesday, 16 October 2019 11:34:30 BST james qian wang (Arm Technology China) wrote:
> From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>
> 
> Adds gamma and color-transform support for DOU-IPS.
> Adds two caps members fgamma_coeffs and ctm_coeffs to komeda_improc_state.
> If color management changed, set gamma and color-transform accordingly.
> 
> v5: Rebase with drm-misc-next
> 
> Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> ---
>  .../arm/display/komeda/d71/d71_component.c    | 20 +++++++++++++++++++
>  .../gpu/drm/arm/display/komeda/komeda_crtc.c  |  2 ++
>  .../drm/arm/display/komeda/komeda_pipeline.h  |  3 +++
>  .../display/komeda/komeda_pipeline_state.c    |  6 ++++++
>  4 files changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> index 6740b8422f11..63a1b6f9cbba 100644
> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> @@ -1032,7 +1032,9 @@ static int d71_merger_init(struct d71_dev *d71,
>  static void d71_improc_update(struct komeda_component *c,
>  			      struct komeda_component_state *state)
>  {
> +	struct drm_crtc_state *crtc_st = state->crtc->state;
>  	struct komeda_improc_state *st = to_improc_st(state);
> +	struct d71_pipeline *pipe = to_d71_pipeline(c->pipeline);
>  	u32 __iomem *reg = c->reg;
>  	u32 index, mask = 0, ctrl = 0;
>  
> @@ -1043,6 +1045,24 @@ static void d71_improc_update(struct komeda_component *c,
>  	malidp_write32(reg, BLK_SIZE, HV_SIZE(st->hsize, st->vsize));
>  	malidp_write32(reg, IPS_DEPTH, st->color_depth);
>  
> +	if (crtc_st->color_mgmt_changed) {
> +		mask |= IPS_CTRL_FT | IPS_CTRL_RGB;
> +
> +		if (crtc_st->gamma_lut) {
> +			malidp_write_group(pipe->dou_ft_coeff_addr, FT_COEFF0,
> +					   KOMEDA_N_GAMMA_COEFFS,
> +					   st->fgamma_coeffs);
> +			ctrl |= IPS_CTRL_FT; /* enable gamma */
> +		}
> +
> +		if (crtc_st->ctm) {
> +			malidp_write_group(reg, IPS_RGB_RGB_COEFF0,
> +					   KOMEDA_N_CTM_COEFFS,
> +					   st->ctm_coeffs);
> +			ctrl |= IPS_CTRL_RGB; /* enable gamut */
> +		}
> +	}
> +
>  	mask |= IPS_CTRL_YUV | IPS_CTRL_CHD422 | IPS_CTRL_CHD420;
>  
>  	/* config color format */
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index 252015210fbc..1c452ea75999 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -617,6 +617,8 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
>  
>  	crtc->port = kcrtc->master->of_output_port;
>  
> +	drm_crtc_enable_color_mgmt(crtc, 0, true, KOMEDA_COLOR_LUT_SIZE);
> +
>  	return err;
>  }
>  
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> index bd6ca7c87037..ac8725e24853 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> @@ -11,6 +11,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include "malidp_utils.h"
> +#include "komeda_color_mgmt.h"
>  
>  #define KOMEDA_MAX_PIPELINES		2
>  #define KOMEDA_PIPELINE_MAX_LAYERS	4
> @@ -327,6 +328,8 @@ struct komeda_improc_state {
>  	struct komeda_component_state base;
>  	u8 color_format, color_depth;
>  	u16 hsize, vsize;
> +	u32 fgamma_coeffs[KOMEDA_N_GAMMA_COEFFS];
> +	u32 ctm_coeffs[KOMEDA_N_CTM_COEFFS];
>  };
>  
>  /* display timing controller */
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> index 42bdc63dcffa..0930234abb9d 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> @@ -802,6 +802,12 @@ komeda_improc_validate(struct komeda_improc *improc,
>  		st->color_format = BIT(__ffs(avail_formats));
>  	}
>  
> +	if (kcrtc_st->base.color_mgmt_changed) {
> +		drm_lut_to_fgamma_coeffs(kcrtc_st->base.gamma_lut,
> +					 st->fgamma_coeffs);
> +		drm_ctm_to_coeffs(kcrtc_st->base.ctm, st->ctm_coeffs);
> +	}
> +
>  	komeda_component_add_input(&st->base, &dflow->input, 0);
>  	komeda_component_set_output(&dflow->input, &improc->base, 0);
>  
> 
You must've missed it, but I had a

Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>

(granted, it was deeper in a thread and quite informally written
as 'r-b me')

-- 
Mihail