[PATCH 0/4] drm/i915: Fix LUT rounding

[PATCH 0/4] drm/i915: Fix LUT rounding

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The current LUT rounding is generating weird results. Adjust
it to follow the OpenGL int<->float conversion rules.

Ville Syrjälä (4):
  drm: Fix color LUT rounding
  drm/i915: Adjust LUT rounding rules
  drm/i915: s/clamp()/min()/ in i965_lut_11p6_max_pack()
  drm/i915: Fix glk+ degamma LUT conversions

 drivers/gpu/drm/i915/display/intel_color.c | 70 +++++++++++-----------
 include/drm/drm_color_mgmt.h               | 18 +++---
 2 files changed, 42 insertions(+), 46 deletions(-)

-- 
2.41.0

[PATCH 1/4] drm: Fix color LUT rounding

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The current implementation of drm_color_lut_extract()
generates weird results. Eg. if we go through all the
values for 16->8bpc conversion we see the following pattern:

in            out (count)
   0 -   7f ->  0 (128)
  80 -  17f ->  1 (256)
 180 -  27f ->  2 (256)
 280 -  37f ->  3 (256)
...
fb80 - fc7f -> fc (256)
fc80 - fd7f -> fd (256)
fd80 - fe7f -> fe (256)
fe80 - ffff -> ff (384)

So less values map to 0 and more values map 0xff, which
doesn't seem particularly great.

To get just the same number of input values to map to
the same output values we'd just need to drop the rounding
entrirely. But perhaps a better idea would be to follow the
OpenGL int<->float conversion rules, in which case we get
the following results:

in            out (count)
   0 -   80 ->  0 (129)
  81 -  181 ->  1 (257)
 182 -  282 ->  2 (257)
 283 -  383 ->  3 (257)
...
fc7c - fd7c -> fc (257)
fd7d - fe7d -> fd (257)
fe7e - ff7e -> fe (257)
ff7f - ffff -> ff (129)

Note that since the divisor is constant the compiler
is able to optimize away the integer division in most
cases. The only exception is the _ULL() case on 32bit
architectures since that gets emitted as inline asm
via do_div() and thus the compiler doesn't get to
optimize it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 include/drm/drm_color_mgmt.h | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index 81c298488b0c..6be3cbe18944 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -36,20 +36,16 @@ struct drm_plane;
  *
  * Extract a degamma/gamma LUT value provided by user (in the form of
  * &drm_color_lut entries) and round it to the precision supported by the
- * hardware.
+ * hardware, following OpenGL int<->float conversion rules.
  */
 static inline u32 drm_color_lut_extract(u32 user_input, int bit_precision)
 {
-	u32 val = user_input;
-	u32 max = 0xffff >> (16 - bit_precision);
-
-	/* Round only if we're not using full precision. */
-	if (bit_precision < 16) {
-		val += 1UL << (16 - bit_precision - 1);
-		val >>= 16 - bit_precision;
-	}
-
-	return clamp_val(val, 0, max);
+	if (bit_precision > 16)
+		return DIV_ROUND_CLOSEST_ULL(mul_u32_u32(user_input, (1 << bit_precision) - 1),
+					     (1 << 16) - 1);
+	else
+		return DIV_ROUND_CLOSEST(user_input * ((1 << bit_precision) - 1),
+					 (1 << 16) - 1);
 }
 
 u64 drm_color_ctm_s31_32_to_qm_n(u64 user_input, u32 m, u32 n);
-- 
2.41.0

[PATCH 2/4] drm/i915: Adjust LUT rounding rules

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

drm_color_lut_extract() rounding was changed to follow the
OpenGL int<->float conversion rules. Adjust intel_color_lut_pack()
to match.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 2a2a163ea652..b01f463af861 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -785,14 +785,12 @@ static void chv_assign_csc(struct intel_crtc_state *crtc_state)
 /* convert hw value with given bit_precision to lut property val */
 static u32 intel_color_lut_pack(u32 val, int bit_precision)
 {
-	u32 max = 0xffff >> (16 - bit_precision);
-
-	val = clamp_val(val, 0, max);
-
-	if (bit_precision < 16)
-		val <<= 16 - bit_precision;
-
-	return val;
+	if (bit_precision > 16)
+		return DIV_ROUND_CLOSEST_ULL(mul_u32_u32(val, (1 << 16) - 1),
+					     (1 << bit_precision) - 1);
+	else
+		return DIV_ROUND_CLOSEST(val * ((1 << 16) - 1),
+					 (1 << bit_precision) - 1);
 }
 
 static u32 i9xx_lut_8(const struct drm_color_lut *color)
-- 
2.41.0

[PATCH 3/4] drm/i915: s/clamp()/min()/ in i965_lut_11p6_max_pack()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use min() instead of clamp() since the color values
involved are unsigned. No functional changes.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index b01f463af861..a4b30614bd63 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -909,7 +909,7 @@ static void i965_lut_10p6_pack(struct drm_color_lut *entry, u32 ldw, u32 udw)
 static u16 i965_lut_11p6_max_pack(u32 val)
 {
 	/* PIPEGCMAX is 11.6, clamp to 10.6 */
-	return clamp_val(val, 0, 0xffff);
+	return min(val, 0xffffu);
 }
 
 static u32 ilk_lut_10(const struct drm_color_lut *color)
-- 
2.41.0

[PATCH 4/4] drm/i915: Fix glk+ degamma LUT conversions

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The current implementation of change_lut_val_precision() is just
a convoluted way of shifting by 8. Implement the proper rounding
by just using drm_color_lut_extract() and intel_color_lut_pack()
like everyone else does.

And as the uapi can't handle >=1.0 values but the hardware
can we need to clamp the results appropriately in the readout
path.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 54 +++++++++++-----------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index a4b30614bd63..1cfbb3650304 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1526,14 +1526,27 @@ static int glk_degamma_lut_size(struct drm_i915_private *i915)
 		return 35;
 }
 
-/*
- * change_lut_val_precision: helper function to upscale or downscale lut values.
- * Parameters 'to' and 'from' needs to be less than 32. This should be sufficient
- * as currently there are no lut values exceeding 32 bit.
- */
-static u32 change_lut_val_precision(u32 lut_val, int to, int from)
+static u32 glk_degamma_lut(const struct drm_color_lut *color)
+{
+	return color->green;
+}
+
+static void glk_degamma_lut_pack(struct drm_color_lut *entry, u32 val)
+{
+	/* PRE_CSC_GAMC_DATA is 3.16, clamp to 0.16 */
+	entry->red = entry->green = entry->blue = min(val, 0xffffu);
+}
+
+static u32 mtl_degamma_lut(const struct drm_color_lut *color)
+{
+	return drm_color_lut_extract(color->green, 24);
+}
+
+static void mtl_degamma_lut_pack(struct drm_color_lut *entry, u32 val)
 {
-	return mul_u32_u32(lut_val, (1 << to)) / (1 << from);
+	/* PRE_CSC_GAMC_DATA is 3.24, clamp to 0.16 */
+	entry->red = entry->green = entry->blue =
+		intel_color_lut_pack(min(val, 0xffffffu), 24);
 }
 
 static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state,
@@ -1570,20 +1583,16 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state,
 		 * ToDo: Extend to max 7.0. Enable 32 bit input value
 		 * as compared to just 16 to achieve this.
 		 */
-		u32 lut_val;
-
-		if (DISPLAY_VER(i915) >= 14)
-			lut_val = change_lut_val_precision(lut[i].green, 24, 16);
-		else
-			lut_val = lut[i].green;
-
 		ilk_lut_write(crtc_state, PRE_CSC_GAMC_DATA(pipe),
-			      lut_val);
+			      DISPLAY_VER(i915) >= 14 ?
+			      mtl_degamma_lut(&lut[i]) : glk_degamma_lut(&lut[i]));
 	}
 
 	/* Clamp values > 1.0. */
 	while (i++ < glk_degamma_lut_size(i915))
-		ilk_lut_write(crtc_state, PRE_CSC_GAMC_DATA(pipe), 1 << 16);
+		ilk_lut_write(crtc_state, PRE_CSC_GAMC_DATA(pipe),
+			      DISPLAY_VER(i915) >= 14 ?
+			      1 << 24 : 1 << 16);
 
 	ilk_lut_write(crtc_state, PRE_CSC_GAMC_INDEX(pipe), 0);
 }
@@ -3573,17 +3582,10 @@ static struct drm_property_blob *glk_read_degamma_lut(struct intel_crtc *crtc)
 	for (i = 0; i < lut_size; i++) {
 		u32 val = intel_de_read_fw(dev_priv, PRE_CSC_GAMC_DATA(pipe));
 
-		/*
-		 * For MTL and beyond, convert back the 24 bit lut values
-		 * read from HW to 16 bit values to maintain parity with
-		 * userspace values
-		 */
 		if (DISPLAY_VER(dev_priv) >= 14)
-			val = change_lut_val_precision(val, 16, 24);
-
-		lut[i].red = val;
-		lut[i].green = val;
-		lut[i].blue = val;
+			mtl_degamma_lut_pack(&lut[i], val);
+		else
+			glk_degamma_lut_pack(&lut[i], val);
 	}
 
 	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
-- 
2.41.0

Re: [Intel-gfx] [PATCH 1/4] drm: Fix color LUT rounding

[Jani Nikula]

On Fri, 13 Oct 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> entrirely. But perhaps a better idea would be to follow the
> OpenGL int<->float conversion rules, in which case we get
> the following results:

Do you have a pointer to the rules handy, I couldn't find it. :(

Might also add the reference to the commit message and/or comment.

BR,
Jani.

-- 
Jani Nikula, Intel

Re: [Intel-gfx] [PATCH 1/4] drm: Fix color LUT rounding

[Ville Syrjälä]

On Tue, Oct 31, 2023 at 11:15:35AM +0200, Jani Nikula wrote:
> On Fri, 13 Oct 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > entrirely. But perhaps a better idea would be to follow the
> > OpenGL int<->float conversion rules, in which case we get
> > the following results:
> 
> Do you have a pointer to the rules handy, I couldn't find it. :(

Eg. '2.3.5 Fixed-Point Data Conversions' in GL 4.6 spec. The section
number probably changes depending on which version of the spec you
look at.

> 
> Might also add the reference to the commit message and/or comment.
> 
> BR,
> Jani.
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel

RE: [Intel-gfx] [PATCH 3/4] drm/i915: s/clamp()/min()/ in i965_lut_11p6_max_pack()

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Friday, October 13, 2023 6:44 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 3/4] drm/i915: s/clamp()/min()/ in
> i965_lut_11p6_max_pack()
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Use min() instead of clamp() since the color values involved are unsigned. No
> functional changes.
> 

LGTM.

Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index b01f463af861..a4b30614bd63 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -909,7 +909,7 @@ static void i965_lut_10p6_pack(struct drm_color_lut
> *entry, u32 ldw, u32 udw)  static u16 i965_lut_11p6_max_pack(u32 val)  {
>  	/* PIPEGCMAX is 11.6, clamp to 10.6 */
> -	return clamp_val(val, 0, 0xffff);
> +	return min(val, 0xffffu);
>  }
> 
>  static u32 ilk_lut_10(const struct drm_color_lut *color)
> --
> 2.41.0

Re: [Intel-gfx] [PATCH 0/4] drm/i915: Fix LUT rounding

[Jani Nikula]

On Fri, 13 Oct 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The current LUT rounding is generating weird results. Adjust
> it to follow the OpenGL int<->float conversion rules.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> Ville Syrjälä (4):
>   drm: Fix color LUT rounding
>   drm/i915: Adjust LUT rounding rules
>   drm/i915: s/clamp()/min()/ in i965_lut_11p6_max_pack()
>   drm/i915: Fix glk+ degamma LUT conversions
>
>  drivers/gpu/drm/i915/display/intel_color.c | 70 +++++++++++-----------
>  include/drm/drm_color_mgmt.h               | 18 +++---
>  2 files changed, 42 insertions(+), 46 deletions(-)

-- 
Jani Nikula, Intel

RE: [PATCH 2/4] drm/i915: Adjust LUT rounding rules

[Borah, Chaitanya Kumar]

Hello Ville,

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Friday, October 13, 2023 6:44 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Subject: [PATCH 2/4] drm/i915: Adjust LUT rounding rules
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> drm_color_lut_extract() rounding was changed to follow the OpenGL int<-
> >float conversion rules. Adjust intel_color_lut_pack() to match.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index 2a2a163ea652..b01f463af861 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -785,14 +785,12 @@ static void chv_assign_csc(struct intel_crtc_state
> *crtc_state)
>  /* convert hw value with given bit_precision to lut property val */  static u32
> intel_color_lut_pack(u32 val, int bit_precision)  {

Is this operation unique to Intel. Should there be a drm helper for this?

Regards

Chaitanya

> -	u32 max = 0xffff >> (16 - bit_precision);
> -
> -	val = clamp_val(val, 0, max);
> -
> -	if (bit_precision < 16)
> -		val <<= 16 - bit_precision;
> -
> -	return val;
> +	if (bit_precision > 16)
> +		return DIV_ROUND_CLOSEST_ULL(mul_u32_u32(val, (1 << 16)
> - 1),
> +					     (1 << bit_precision) - 1);
> +	else
> +		return DIV_ROUND_CLOSEST(val * ((1 << 16) - 1),
> +					 (1 << bit_precision) - 1);
>  }
> 
>  static u32 i9xx_lut_8(const struct drm_color_lut *color)
> --
> 2.41.0

RE: [PATCH 4/4] drm/i915: Fix glk+ degamma LUT conversions

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Friday, October 13, 2023 6:44 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Subject: [PATCH 4/4] drm/i915: Fix glk+ degamma LUT conversions
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The current implementation of change_lut_val_precision() is just a convoluted
> way of shifting by 8. Implement the proper rounding by just using
> drm_color_lut_extract() and intel_color_lut_pack() like everyone else does.
> 
> And as the uapi can't handle >=1.0 values but the hardware can we need to
> clamp the results appropriately in the readout path.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

LGTM.

Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>


> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 54 +++++++++++-----------
>  1 file changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index a4b30614bd63..1cfbb3650304 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1526,14 +1526,27 @@ static int glk_degamma_lut_size(struct
> drm_i915_private *i915)
>  		return 35;
>  }
> 
> -/*
> - * change_lut_val_precision: helper function to upscale or downscale lut
> values.
> - * Parameters 'to' and 'from' needs to be less than 32. This should be
> sufficient
> - * as currently there are no lut values exceeding 32 bit.
> - */
> -static u32 change_lut_val_precision(u32 lut_val, int to, int from)
> +static u32 glk_degamma_lut(const struct drm_color_lut *color) {
> +	return color->green;
> +}
> +
> +static void glk_degamma_lut_pack(struct drm_color_lut *entry, u32 val)
> +{
> +	/* PRE_CSC_GAMC_DATA is 3.16, clamp to 0.16 */
> +	entry->red = entry->green = entry->blue = min(val, 0xffffu); }
> +
> +static u32 mtl_degamma_lut(const struct drm_color_lut *color) {
> +	return drm_color_lut_extract(color->green, 24); }
> +
> +static void mtl_degamma_lut_pack(struct drm_color_lut *entry, u32 val)
>  {
> -	return mul_u32_u32(lut_val, (1 << to)) / (1 << from);
> +	/* PRE_CSC_GAMC_DATA is 3.24, clamp to 0.16 */
> +	entry->red = entry->green = entry->blue =
> +		intel_color_lut_pack(min(val, 0xffffffu), 24);
>  }
> 
>  static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state,
> @@ -1570,20 +1583,16 @@ static void glk_load_degamma_lut(const struct
> intel_crtc_state *crtc_state,
>  		 * ToDo: Extend to max 7.0. Enable 32 bit input value
>  		 * as compared to just 16 to achieve this.
>  		 */
> -		u32 lut_val;
> -
> -		if (DISPLAY_VER(i915) >= 14)
> -			lut_val = change_lut_val_precision(lut[i].green, 24,
> 16);
> -		else
> -			lut_val = lut[i].green;
> -
>  		ilk_lut_write(crtc_state, PRE_CSC_GAMC_DATA(pipe),
> -			      lut_val);
> +			      DISPLAY_VER(i915) >= 14 ?
> +			      mtl_degamma_lut(&lut[i]) :
> glk_degamma_lut(&lut[i]));
>  	}
> 
>  	/* Clamp values > 1.0. */
>  	while (i++ < glk_degamma_lut_size(i915))
> -		ilk_lut_write(crtc_state, PRE_CSC_GAMC_DATA(pipe), 1 <<
> 16);
> +		ilk_lut_write(crtc_state, PRE_CSC_GAMC_DATA(pipe),
> +			      DISPLAY_VER(i915) >= 14 ?
> +			      1 << 24 : 1 << 16);
> 
>  	ilk_lut_write(crtc_state, PRE_CSC_GAMC_INDEX(pipe), 0);  } @@ -
> 3573,17 +3582,10 @@ static struct drm_property_blob
> *glk_read_degamma_lut(struct intel_crtc *crtc)
>  	for (i = 0; i < lut_size; i++) {
>  		u32 val = intel_de_read_fw(dev_priv,
> PRE_CSC_GAMC_DATA(pipe));
> 
> -		/*
> -		 * For MTL and beyond, convert back the 24 bit lut values
> -		 * read from HW to 16 bit values to maintain parity with
> -		 * userspace values
> -		 */
>  		if (DISPLAY_VER(dev_priv) >= 14)
> -			val = change_lut_val_precision(val, 16, 24);
> -
> -		lut[i].red = val;
> -		lut[i].green = val;
> -		lut[i].blue = val;
> +			mtl_degamma_lut_pack(&lut[i], val);
> +		else
> +			glk_degamma_lut_pack(&lut[i], val);
>  	}
> 
>  	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> --
> 2.41.0

RE: [Intel-gfx] [PATCH 1/4] drm: Fix color LUT rounding

[Borah, Chaitanya Kumar]

Hello Ville,

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Ville
> Syrjälä
> Sent: Tuesday, October 31, 2023 9:37 PM
> To: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/4] drm: Fix color LUT rounding
> 
> On Tue, Oct 31, 2023 at 11:15:35AM +0200, Jani Nikula wrote:
> > On Fri, 13 Oct 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > > entrirely. But perhaps a better idea would be to follow the OpenGL
> > > int<->float conversion rules, in which case we get the following
> > > results:
> >
> > Do you have a pointer to the rules handy, I couldn't find it. :(
> 
> Eg. '2.3.5 Fixed-Point Data Conversions' in GL 4.6 spec. The section number
> probably changes depending on which version of the spec you look at.
> 

This section particularly talks about conversion of normalized fixed point  to floating point numbers and vice versa.
Pardon my limited knowledge on the topic but aren't we just doing a scaling factor conversion(Q0.16 -> Q0.8) in these patches?

I could not draw a direct relation between the formulas in the section[1] and what we are doing here.(but it could be just me!)

Regards

Chaitanya

[1] https://registry.khronos.org/OpenGL/specs/gl/glspec46.core.pdf '2.3.5 Fixed-Point Data Conversions'

> >
> > Might also add the reference to the commit message and/or comment.
> >
> > BR,
> > Jani.
> >
> > --
> > Jani Nikula, Intel
> 
> --
> Ville Syrjälä
> Intel

RE: [Intel-gfx] [PATCH 1/4] drm: Fix color LUT rounding

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: Borah, Chaitanya Kumar
> Sent: Monday, November 20, 2023 6:33 PM
> To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Jani
> Nikula <jani.nikula@linux.intel.com>
> Subject: RE: [Intel-gfx] [PATCH 1/4] drm: Fix color LUT rounding
> 
> Hello Ville,
> 
> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> > Ville Syrjälä
> > Sent: Tuesday, October 31, 2023 9:37 PM
> > To: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 1/4] drm: Fix color LUT rounding
> >
> > On Tue, Oct 31, 2023 at 11:15:35AM +0200, Jani Nikula wrote:
> > > On Fri, 13 Oct 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > > > entrirely. But perhaps a better idea would be to follow the OpenGL
> > > > int<->float conversion rules, in which case we get the following
> > > > results:
> > >
> > > Do you have a pointer to the rules handy, I couldn't find it. :(
> >
> > Eg. '2.3.5 Fixed-Point Data Conversions' in GL 4.6 spec. The section
> > number probably changes depending on which version of the spec you look
> at.
> >
> 
> This section particularly talks about conversion of normalized fixed point  to
> floating point numbers and vice versa.
> Pardon my limited knowledge on the topic but aren't we just doing a scaling
> factor conversion(Q0.16 -> Q0.8) in these patches?
> 
> I could not draw a direct relation between the formulas in the section[1] and
> what we are doing here.(but it could be just me!)

Scratch that! As I understand, in effect we are doing a Q0.16 Fixed Point -> Floating point -> Q0.8 Fixed Point conversion.
Correct me if I am wrong! Otherwise

LGTM.

Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>


> 
> Regards
> 
> Chaitanya
> 
> [1] https://registry.khronos.org/OpenGL/specs/gl/glspec46.core.pdf '2.3.5
> Fixed-Point Data Conversions'
> 
> > >
> > > Might also add the reference to the commit message and/or comment.
> > >
> > > BR,
> > > Jani.
> > >
> > > --
> > > Jani Nikula, Intel
> >
> > --
> > Ville Syrjälä
> > Intel

Re: [PATCH 2/4] drm/i915: Adjust LUT rounding rules

[Ville Syrjälä]

On Mon, Nov 20, 2023 at 06:08:57AM +0000, Borah, Chaitanya Kumar wrote:
> Hello Ville,
> 
> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Ville
> > Syrjala
> > Sent: Friday, October 13, 2023 6:44 PM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: dri-devel@lists.freedesktop.org
> > Subject: [PATCH 2/4] drm/i915: Adjust LUT rounding rules
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > drm_color_lut_extract() rounding was changed to follow the OpenGL int<-
> > >float conversion rules. Adjust intel_color_lut_pack() to match.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_color.c | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> > b/drivers/gpu/drm/i915/display/intel_color.c
> > index 2a2a163ea652..b01f463af861 100644
> > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > @@ -785,14 +785,12 @@ static void chv_assign_csc(struct intel_crtc_state
> > *crtc_state)
> >  /* convert hw value with given bit_precision to lut property val */  static u32
> > intel_color_lut_pack(u32 val, int bit_precision)  {
> 
> Is this operation unique to Intel. Should there be a drm helper for this?

If some other driver gains gamma readout support they
could probably use something like this. The other option
would be to rework the current helper to allow conversions
both ways.

> 
> Regards
> 
> Chaitanya
> 
> > -	u32 max = 0xffff >> (16 - bit_precision);
> > -
> > -	val = clamp_val(val, 0, max);
> > -
> > -	if (bit_precision < 16)
> > -		val <<= 16 - bit_precision;
> > -
> > -	return val;
> > +	if (bit_precision > 16)
> > +		return DIV_ROUND_CLOSEST_ULL(mul_u32_u32(val, (1 << 16)
> > - 1),
> > +					     (1 << bit_precision) - 1);
> > +	else
> > +		return DIV_ROUND_CLOSEST(val * ((1 << 16) - 1),
> > +					 (1 << bit_precision) - 1);
> >  }
> > 
> >  static u32 i9xx_lut_8(const struct drm_color_lut *color)
> > --
> > 2.41.0
> 

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 1/4] drm: Fix color LUT rounding

[Ville Syrjälä]

On Mon, Nov 20, 2023 at 01:17:05PM +0000, Borah, Chaitanya Kumar wrote:
> 
> 
> > -----Original Message-----
> > From: Borah, Chaitanya Kumar
> > Sent: Monday, November 20, 2023 6:33 PM
> > To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Jani
> > Nikula <jani.nikula@linux.intel.com>
> > Subject: RE: [Intel-gfx] [PATCH 1/4] drm: Fix color LUT rounding
> > 
> > Hello Ville,
> > 
> > > -----Original Message-----
> > > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> > > Ville Syrjälä
> > > Sent: Tuesday, October 31, 2023 9:37 PM
> > > To: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH 1/4] drm: Fix color LUT rounding
> > >
> > > On Tue, Oct 31, 2023 at 11:15:35AM +0200, Jani Nikula wrote:
> > > > On Fri, 13 Oct 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > > > > entrirely. But perhaps a better idea would be to follow the OpenGL
> > > > > int<->float conversion rules, in which case we get the following
> > > > > results:
> > > >
> > > > Do you have a pointer to the rules handy, I couldn't find it. :(
> > >
> > > Eg. '2.3.5 Fixed-Point Data Conversions' in GL 4.6 spec. The section
> > > number probably changes depending on which version of the spec you look
> > at.
> > >
> > 
> > This section particularly talks about conversion of normalized fixed point  to
> > floating point numbers and vice versa.
> > Pardon my limited knowledge on the topic but aren't we just doing a scaling
> > factor conversion(Q0.16 -> Q0.8) in these patches?
> > 
> > I could not draw a direct relation between the formulas in the section[1] and
> > what we are doing here.(but it could be just me!)
> 
> Scratch that! As I understand, in effect we are doing a Q0.16 Fixed Point -> Floating point -> Q0.8 Fixed Point conversion.

Yep, that's it.

> Correct me if I am wrong! Otherwise
> 
> LGTM.
> 
> Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> 
> 
> > 
> > Regards
> > 
> > Chaitanya
> > 
> > [1] https://registry.khronos.org/OpenGL/specs/gl/glspec46.core.pdf '2.3.5
> > Fixed-Point Data Conversions'
> > 
> > > >
> > > > Might also add the reference to the commit message and/or comment.
> > > >
> > > > BR,
> > > > Jani.
> > > >
> > > > --
> > > > Jani Nikula, Intel
> > >
> > > --
> > > Ville Syrjälä
> > > Intel

-- 
Ville Syrjälä
Intel

Re: [PATCH 0/4] drm/i915: Fix LUT rounding

[Ville Syrjälä]

On Fri, Oct 13, 2023 at 04:13:58PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The current LUT rounding is generating weird results. Adjust
> it to follow the OpenGL int<->float conversion rules.
> 
> Ville Syrjälä (4):
>   drm: Fix color LUT rounding
    ^
I'd like to merge this via drm-intel-next as needs to match
the rounding done in the readout path in i915.

Maarten,Maxime,Thomas can I get an ack for that?

>   drm/i915: Adjust LUT rounding rules
>   drm/i915: s/clamp()/min()/ in i965_lut_11p6_max_pack()
>   drm/i915: Fix glk+ degamma LUT conversions
> 
>  drivers/gpu/drm/i915/display/intel_color.c | 70 +++++++++++-----------
>  include/drm/drm_color_mgmt.h               | 18 +++---
>  2 files changed, 42 insertions(+), 46 deletions(-)
> 
> -- 
> 2.41.0

-- 
Ville Syrjälä
Intel

RE: [PATCH 2/4] drm/i915: Adjust LUT rounding rules

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Monday, November 20, 2023 7:57 PM
> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH 2/4] drm/i915: Adjust LUT rounding rules
> 
> On Mon, Nov 20, 2023 at 06:08:57AM +0000, Borah, Chaitanya Kumar wrote:
> > Hello Ville,
> >
> > > -----Original Message-----
> > > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf
> > > Of Ville Syrjala
> > > Sent: Friday, October 13, 2023 6:44 PM
> > > To: intel-gfx@lists.freedesktop.org
> > > Cc: dri-devel@lists.freedesktop.org
> > > Subject: [PATCH 2/4] drm/i915: Adjust LUT rounding rules
> > >
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > drm_color_lut_extract() rounding was changed to follow the OpenGL
> > > int<-
> > > >float conversion rules. Adjust intel_color_lut_pack() to match.
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_color.c | 14 ++++++--------
> > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> > > b/drivers/gpu/drm/i915/display/intel_color.c
> > > index 2a2a163ea652..b01f463af861 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > > @@ -785,14 +785,12 @@ static void chv_assign_csc(struct
> > > intel_crtc_state
> > > *crtc_state)
> > >  /* convert hw value with given bit_precision to lut property val */
> > > static u32
> > > intel_color_lut_pack(u32 val, int bit_precision)  {
> >
> > Is this operation unique to Intel. Should there be a drm helper for this?
> 
> If some other driver gains gamma readout support they could probably use
> something like this. The other option would be to rework the current helper
> to allow conversions both ways.
> 

The function name could be a minor inconvenience but anyway until that time arrives.

LGTM.

Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>

> >
> > Regards
> >
> > Chaitanya
> >
> > > -	u32 max = 0xffff >> (16 - bit_precision);
> > > -
> > > -	val = clamp_val(val, 0, max);
> > > -
> > > -	if (bit_precision < 16)
> > > -		val <<= 16 - bit_precision;
> > > -
> > > -	return val;
> > > +	if (bit_precision > 16)
> > > +		return DIV_ROUND_CLOSEST_ULL(mul_u32_u32(val, (1 << 16)
> > > - 1),
> > > +					     (1 << bit_precision) - 1);
> > > +	else
> > > +		return DIV_ROUND_CLOSEST(val * ((1 << 16) - 1),
> > > +					 (1 << bit_precision) - 1);
> > >  }
> > >
> > >  static u32 i9xx_lut_8(const struct drm_color_lut *color)
> > > --
> > > 2.41.0
> >
> 
> --
> Ville Syrjälä
> Intel

Re: [PATCH 0/4] drm/i915: Fix LUT rounding

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 619 bytes --]

On Mon, Nov 20, 2023 at 04:30:53PM +0200, Ville Syrjälä wrote:
> On Fri, Oct 13, 2023 at 04:13:58PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The current LUT rounding is generating weird results. Adjust
> > it to follow the OpenGL int<->float conversion rules.
> > 
> > Ville Syrjälä (4):
> >   drm: Fix color LUT rounding
>     ^
> I'd like to merge this via drm-intel-next as needs to match
> the rounding done in the readout path in i915.
> 
> Maarten,Maxime,Thomas can I get an ack for that?

Acked-by: Maxime Ripard <mripard@kernel.org>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 0/4] drm/i915: Fix LUT rounding

[Ville Syrjälä]

On Tue, Nov 21, 2023 at 11:51:10AM +0100, Maxime Ripard wrote:
> On Mon, Nov 20, 2023 at 04:30:53PM +0200, Ville Syrjälä wrote:
> > On Fri, Oct 13, 2023 at 04:13:58PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The current LUT rounding is generating weird results. Adjust
> > > it to follow the OpenGL int<->float conversion rules.
> > > 
> > > Ville Syrjälä (4):
> > >   drm: Fix color LUT rounding
> >     ^
> > I'd like to merge this via drm-intel-next as needs to match
> > the rounding done in the readout path in i915.
> > 
> > Maarten,Maxime,Thomas can I get an ack for that?
> 
> Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks. Series pushed to drm-intel-next.

-- 
Ville Syrjälä
Intel